On Fri, 2010-09-17 at 11:08 +0100, Juergen Gross wrote:
> Please ignore previous mail, I hit the send-button too early...
>
> On 09/17/10 11:44, Ian Campbell wrote:
> > On Fri, 2010-09-17 at 10:20 +0100, Juergen Gross wrote:
> >> On 09/17/10 11:00, Ian Campbell wrote:
> >>> local_size has already been rounded up in get_cpumap_size. Do we really
> >>> need to do it again?
> >>
> >> I've made it more clear that this is rounding to uint64.
> >
> > Wouldn't that be "(.. + 63) / 8" then?
>
> No, local_size is already bytes...
>
> >>
> >>>
> >>>> + size = sizeof(xc_cpupoolinfo_t) + cpumap_size * 8 + local_size;
> >>>
> >>> Why do we need both "cpumap_size * 8" and local_size additional bytes
> >>> here? Both contain the number of bytes necessary to contain a cpumap
> >>> bitmask and in fact I suspect they are both equal at this point (see
> >>> point about rounding above).
> >>
> >> The hypervisor returns a cpumask based on bytes, the tools use uint64-based
> >> cpumasks.
> >
> > Oh, I see, as well as xc_cpupool_info_t and the cpumap which it contains
> > being allocated together in a single buffer you are also including a
> > third buffer which is for local use in this function only but which is
> > included in the memory region returned to the caller (who doesn't know
> > about it). Seems a bit odd to me, why not just allocate it locally then
> > free it (or use alloca)?
> >
> > Actually, when I complete my hypercall buffer patch this memory will
> > need to be separately allocated any way since it needs to come from a
> > special pool. I'd prefer it if you just used explicit separate
> > allocation for this buffer from the start.
>
> Okay.
>
> >
> >> In practice this is equivalent as long as multiple of 8 bytes are
> >> written by the hypervisor and the system is running little endian.
> >> I prefer a clean interface mapping here.
> >
> > Using a single uint64 when there was a limit of 64 cpus made sense but
> > now that it is variable length why not just use bytes everywhere? It
> > would avoid a lot of confusion about what various size units are at
> > various points etc. You would avoid needing to translate between the
> > hypervisor and tools representations too, wouldn't you?
>
> This would suggest changing xc_vcpu_setaffinity() and xc_vcpu_getaffinity(),
> too.
Juergen told me privately that he will do this after this patchset,
which seems reasonable given the number of iterations we've been through
so far. So:
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home