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

[Juergen Gross <juergen.gross@ts.fujitsu.com>]

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@ts.fujitsu.com
>
> 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@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html