Re: [Xen-devel] [Patch 1/2] support of cpupools in xl: update cpumask handling for cpu pools in libxc and python

[Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>]

On 10/01/10 11:35, Ian Campbell wrote:
> (I highly recommend the patchbomb extension ("hg email"), it makes
> sending series much simpler and threads the mails together in a
> convenient way)
Okay, I'll try hg email for the next round...

> On Fri, 2010-10-01 at 08:20 +0100, Juergen Gross wrote:
> > Signed-off-by: juergen.gross@xxxxxxxxxxxxxx
> This should come after the description.
Okay.

> > 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()
> Please also mention the change in xc_cpupool_getinfo semantics from
> caller allocated buffer to callee allocated+returned.
Okay.

> > @@ -64,50 +78,61 @@ int xc_cpupool_destroy(xc_interface *xch
> >       return do_sysctl_save(xch,&sysctl);
> >   }
> >
> > -int xc_cpupool_getinfo(xc_interface *xch,
> > -                       uint32_t first_poolid,
> > -                       uint32_t n_max,
> > -                       xc_cpupoolinfo_t *info)
> > +xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
> > +                       uint32_t poolid)
> [...]
> > -    memset(info, 0, n_max * sizeof(xc_cpupoolinfo_t));
> > +    local_size = get_cpumap_size(xch);
> > +    local = alloca(local_size);
> > +    if (!local_size)
> > +    {
> > +        PERROR("Could not get number of cpus");
> > +        return NULL;
> > +    }
> I imagine alloca(0) is most likely safe so long as you don't actually
> use the result, but the man page doesn't specifically say. Probably the
> check of !local_size should be before the alloca(local_size) to be on
> the safe side.
Yeah, you are right.

> > +    cpumap_size = (local_size + sizeof(*info->cpumap) - 1) / 
> > sizeof(*info->cpumap);
> xg_private.h defines a macro ROUNDUP, I wonder if that should be moved
> somewhere more generic and used to clarify code like this?
Doesn't fit really here (needs the number of bits, e.g. 6 in this case).
As this code will vanish with cpumask rework to be byte-based, I don't
change this now.

> > diff -r 8b7d253f0e17 tools/libxc/xenctrl.h
> > --- a/tools/libxc/xenctrl.h     Fri Oct 01 08:39:49 2010 +0100
> > +++ b/tools/libxc/xenctrl.h     Fri Oct 01 09:13:36 2010 +0100
> > @@ -564,15 +565,11 @@ int xc_cpupool_destroy(xc_interface *xch
> > [...]
> > + * @parm poolid lowest id for which info is returned
> > + * return cpupool info ptr (obtained by malloc)
> >    */
> > -int xc_cpupool_getinfo(xc_interface *xch,
> > -                       uint32_t first_poolid,
> > -                       uint32_t n_max,
> > -                       xc_cpupoolinfo_t *info);
> > +xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
> > +                       uint32_t poolid);
> >
> >   /**
> >    * Add cpu to a cpupool. cpu may be -1 indicating the first unassigned.
> > @@ -615,10 +612,12 @@ int xc_cpupool_movedomain(xc_interface *
> >    *
> >    * @parm xc_handle a handle to an open hypervisor interface
> >    * @parm cpumap pointer where to store the cpumap
> > + * @parm cpusize size of cpumap array in bytes
> >    * return 0 on success, -1 on failure
> >    */
> >   int xc_cpupool_freeinfo(xc_interface *xch,
> > -                        uint64_t *cpumap);
> > +                        uint64_t *cpumap,
> > +                        int cpusize);
> xc_cpupool_getinfo returns a callee allocated buffer and
> xc_cpupool_freeinfo expects to be given a caller allocated buffer? I
> think we should make this consistent one way of the other.
Agreed.

> > diff -r 71f836615ea2 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c       Fri Sep 24 15:54:39 2010 +0100
> > +++ b/tools/libxl/libxl.c       Fri Oct 01 09:03:17 2010 +0200
> > @@ -610,26 +610,34 @@ libxl_poolinfo * libxl_list_pool(libxl_c
> >   libxl_poolinfo * libxl_list_pool(libxl_ctx *ctx, int *nb_pool)
> >   {
> >       libxl_poolinfo *ptr;
> > -    int i, ret;
> > -    xc_cpupoolinfo_t info[256];
> > -    int size = 256;
> > +    int i;
> > +    xc_cpupoolinfo_t *info;
> > +    uint32_t poolid;
> > +    libxl_physinfo physinfo;
> >
> > -    ptr = calloc(size, sizeof(libxl_poolinfo));
> > -    if (!ptr) {
> > -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
> > +    if (libxl_get_physinfo(ctx,&physinfo) != 0) {
> > +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting phys info");
> >           return NULL;
> >       }
> Am I missing where the contents of physinfo is subsequently used in this
> function?
Me too :-)
Should be in the second patch.

> > -    ret = xc_cpupool_getinfo(ctx->xch, 0, 256, info);
> > -    if (ret<0) {
> > -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting cpupool info");
> > -        return NULL;
> > +    ptr = NULL;
> > +
> > +    poolid = 0;
> > +    for (i = 0;; i++) {
> > +        info = xc_cpupool_getinfo(ctx->xch, poolid);
> > +        if (info == NULL)
> > +            break;
> > +        ptr = realloc(ptr, (i + 1) * sizeof(libxl_poolinfo));
> > +        if (!ptr) {
> > +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
> > +            return NULL;
> > +        }
> This will leak the previous value of ptr if realloc() fails. You need to
> do:
>         tmp = realloc(ptr, ....)
>         if (!tmp) {
>                 free(ptr);
>                 LIBXL__LOG_ERRNO(...);
>                 return NULL;
>         }
>         ptr = tmp;
Should be changed in other places, too:
libxc/xc_tmem.c
libxl/libxl.c (sometimes not even checked for error)

> > diff -r 71f836615ea2 tools/python/xen/lowlevel/xc/xc.c
> > --- a/tools/python/xen/lowlevel/xc/xc.c Fri Sep 24 15:54:39 2010 +0100
> > +++ b/tools/python/xen/lowlevel/xc/xc.c Fri Oct 01 09:03:17 2010 +0200
> > @@ -241,7 +241,7 @@ static PyObject *pyxc_vcpu_setaffinity(X
> >       if ( xc_physinfo(self->xc_handle,&info) != 0 )
> >           return pyxc_error_to_exception(self->xc_handle);
> >
> > -    nr_cpus = info.nr_cpus;
> > +    nr_cpus = info.max_cpu_id + 1;
> >
> >       size = (nr_cpus + cpumap_size * 8 - 1)/ (cpumap_size * 8);
> >       cpumap = malloc(cpumap_size * size);
> Is this (and the equivalent in getinfo) an independent bug fix for a
> pre-existing issue or does it somehow relate to the rest of the changes?
> I don't see any corresponding change to xc_vcpu_setaffinity is all.
It's an independent fix. OTOH it's cpumask related, so I put it in...
xc_vcpu_setaffinity is not touched as it takes the cpumask size as
parameter.

> Given the repeated uses of physinfo.max_cpu_id (here, in get_cpumap_size
> etc) might a xc_get_nr_cpus() function be worthwhile?
Okay. I'll call it xc_get_max_cpus().

> Presumably when you change these interfaces to use uint8_t instead of
> uint64_t this code becomes the same as the private get_cpumap_size you
> defined earlier so it might be worth exporting that functionality from
> libxc?
I'll merge these functions later.

> > @@ -1966,7 +1967,7 @@ static PyObject *pyxc_cpupool_getinfo(Xc
> >       PyObject *list, *info_dict;
> >
> >       uint32_t first_pool = 0;
> > -    int max_pools = 1024, nr_pools, i;
> > +    int max_pools = 1024, i;
> [...]
> > +    for (i = 0; i<  max_pools; i++)
> I don't think there is any 1024 pool limit inherent in the new libxc
> code, is there? You've removed the limit from libxl and I think the
> right thing to do is remove it here as well.
Correct.

> >       {
> > -        free(info);
> > -        return pyxc_error_to_exception(self->xc_handle);
> > -    }
> > -
> > -    list = PyList_New(nr_pools);
> > -    for ( i = 0 ; i<  nr_pools; i++ )
> > -    {
> > +        info = xc_cpupool_getinfo(self->xc_handle, first_pool);
> > +        if (info == NULL)
> > +            break;
> >           info_dict = Py_BuildValue(
> >               "{s:i,s:i,s:i,s:N}",
> > -            "cpupool",         (int)info[i].cpupool_id,
> > -            "sched",           info[i].sched_id,
> > -            "n_dom",           info[i].n_dom,
> > -            "cpulist",         cpumap_to_cpulist(info[i].cpumap));
> > +            "cpupool",         (int)info->cpupool_id,
> > +            "sched",           info->sched_id,
> > +            "n_dom",           info->n_dom,
> > +            "cpulist",         cpumap_to_cpulist(info->cpumap,
> > +                                                 info->cpumap_size));
> > +        first_pool = info->cpupool_id + 1;
> > +        free(info);
> > +
> >           if ( info_dict == NULL )
> >           {
> >               Py_DECREF(list);
> > -            if ( info_dict != NULL ) { Py_DECREF(info_dict); }
> > -            free(info);
> >               return NULL;
> >           }
> > -        PyList_SetItem(list, i, info_dict);
> > +
> > +        PyList_Append(list, info_dict);
> > +        Py_DECREF(info_dict);
> >       }
> > -
> > -    free(info);
> >
> >       return list;
> >   }
> > @@ -2072,12 +2066,28 @@ static PyObject *pyxc_cpupool_movedomain
> >
> >   static PyObject *pyxc_cpupool_freeinfo(XcObject *self)
> >   {
> > -    uint64_t cpumap;
> > +    uint64_t *cpumap;
> > +    xc_physinfo_t physinfo;
> > +    int ret;
> > +    PyObject *info = NULL;
> >
> > -    if (xc_cpupool_freeinfo(self->xc_handle,&cpumap) != 0)
> > +    if (xc_physinfo(self->xc_handle,&physinfo))
> >           return pyxc_error_to_exception(self->xc_handle);
> >
> > -    return cpumap_to_cpulist(cpumap);
> > +    cpumap = calloc((physinfo.max_cpu_id + 64) / 64, sizeof(uint64_t));
> Making xc_cpupool_freeinfo allocate the buffer, like xc_cpupool_getinfo
> does would remove the need for this sort of arithmetic in the users of
> libxc.
Yes.


Juergen

--
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@xxxxxxxxxxxxxx
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

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