Re: [Xen-devel] Re: [Patch] update cpumask handling for cpu pools in libxc and python

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Fri, 2010-09-17 at 10:20 +0100, Juergen Gross wrote:
> On 09/17/10 11:00, Ian Campbell wrote:
> > On Fri, 2010-09-17 at 06:51 +0100, Juergen Gross wrote:
> >> Signed-off-by: juergen.gross@xxxxxxxxxxxxxx
> >>
> >> To be able to support arbitrary numbers of physical cpus it was
> >> necessary to
> >> include the size of cpumaps in the xc-interfaces for cpu pools.
> >> These were:
> >>    definition of xc_cpupoolinfo_t
> >>    xc_cpupool_getinfo()
> >>    xc_cpupool_freeinfo()
> >>
> >> diff -r d978675f3d53 tools/libxc/xc_cpupool.c
> >> --- a/tools/libxc/xc_cpupool.c  Thu Sep 16 18:29:26 2010 +0100
> >> +++ b/tools/libxc/xc_cpupool.c  Fri Sep 17 07:42:30 2010 +0200
> >> @@ -34,6 +34,20 @@ static int do_sysctl_save(xc_interface *
> >>       return ret;
> >>   }
> >>
> >> +static int get_cpumap_size(xc_interface *xch)
> >> +{
> >> +    static int cpumap_size = 0;
> >> +    xc_physinfo_t physinfo;
> >> +
> >> +    if ( cpumap_size )
> >> +        return cpumap_size;
> >> +
> >> +    if ( !xc_physinfo(xch,&physinfo) )
> >> +        cpumap_size = (physinfo.max_cpu_id + 8) / 8;
> >
> > The + 8 / 8 thing still jumps out at me every time I look at it. Perhaps
> > the intention would be clearer if written as:
> >     int nr_cpus = physinfo.max_cpu_id + 1;
> >     cpumap_size = (nr+cpus + 7) / 8;
> 
> I modified it to make it more clear.

Thanks.

> 
> > If xc_physinfo fails (which seems like a reasonably fatal type of error)
> > then this function returns 0 but the caller does not seem to check for
> > this case.
> 
> Okay, test added.
> 
> >
> >> +xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
> >> +                       uint32_t poolid)
> >>   {
> >> [...]
> >> +    local_size = get_cpumap_size(xch);
> >> +    cpumap_size = (local_size + 7) / 8;
> >
> > 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?
> 
> >
> >> +    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.

>  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?

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel