Hi,
attached patch adds support of cpu pools in xl/libxl.
Ian, I didn''t split up this patch, because the updated libxl_cpumask
structure
requires changes to xl in any case.
BTW: I realized that libxlu_cfg_l.c seems to be in the repository.
Isn''t this
file generated by make? I think it should be removed there.
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
> On Fri, 2010-09-17 at 07:10 +0100, Juergen Gross wrote: > > attached patch adds support of cpu pools in xl/libxl. > Ian, I didn''t split up this patch, because the updated libxl_cpumask > structure requires changes to xl in any case.That''s fine.> BTW: I realized that libxlu_cfg_l.c seems to be in the repository. > Isn''t this file generated by make? I think it should be removed there.In an ideal world, yes. I think this was a workaround for buggy versions of flex observed in the field or something.> Signed-off-by: juergen.gross@ts.fujitsu.com > > Support of cpu pools in xl: > library functions > xl pool-create > xl pool-list > xl pool-destroy > xl pool-cpu-add > xl pool-cpu-remove > xl pool-migrate > > diff -r d978675f3d53 tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Thu Sep 16 18:29:26 2010 +0100 > +++ b/tools/libxl/libxl.c Fri Sep 17 07:42:30 2010 +0200 > @@ -609,9 +609,17 @@ 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, m; > + xc_cpupoolinfo_t *info; > + int size; > + uint32_t poolid; > + libxl_physinfo physinfo; > + > + if (libxl_get_physinfo(ctx, &physinfo) != 0) { > + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting phys info"); > + return NULL; > + } > + size = physinfo.max_cpu_id + 32;Where does the number 32 come from?> ptr = calloc(size, sizeof(libxl_poolinfo));> if (!ptr) { > @@ -619,16 +627,23 @@ libxl_poolinfo * libxl_list_pool(libxl_c > return NULL; > } > > - ret = xc_cpupool_getinfo(ctx->xch, 0, 256, info); > - if (ret<0) { > - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting cpupool info"); > - return NULL; > + poolid = 0; > + for (i = 0; i < size; i++) { > + info = xc_cpupool_getinfo(ctx->xch, poolid); > + if (info == NULL) > + break; > + ptr[i].poolid = info->cpupool_id; > + ptr[i].sched_id = info->sched_id; > + ptr[i].n_dom = info->n_dom; > + if (libxl_cpumap_alloc(&ptr[i].cpumap, physinfo.max_cpu_id + 1)) > + break; > + for (m = 0; m < ptr[i].cpumap.size / sizeof(*ptr[i].cpumap.map); m++) > + ptr[i].cpumap.map[m] = info->cpumap[m];I guess if "physinfo.max_cpu_id + 1" does not correspond to info->cpumap_size then that is a serious error but if it does happen then you may run over the end of either ptr[i].cpumap.map or info->cpumap. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Sep-17 11:41 UTC
Re: [Xen-devel] Re: [Patch] support of cpu pools in xl
On 09/17/10 11:46, Ian Campbell wrote:> >> On Fri, 2010-09-17 at 07:10 +0100, Juergen Gross wrote: >> >> attached patch adds support of cpu pools in xl/libxl. >> Ian, I didn''t split up this patch, because the updated libxl_cpumask >> structure requires changes to xl in any case. > > That''s fine. > >> BTW: I realized that libxlu_cfg_l.c seems to be in the repository. >> Isn''t this file generated by make? I think it should be removed there. > > In an ideal world, yes. I think this was a workaround for buggy versions > of flex observed in the field or something.How nice ;-)> >> Signed-off-by: juergen.gross@ts.fujitsu.com >> >> Support of cpu pools in xl: >> library functions >> xl pool-create >> xl pool-list >> xl pool-destroy >> xl pool-cpu-add >> xl pool-cpu-remove >> xl pool-migrate >> >> diff -r d978675f3d53 tools/libxl/libxl.c >> --- a/tools/libxl/libxl.c Thu Sep 16 18:29:26 2010 +0100 >> +++ b/tools/libxl/libxl.c Fri Sep 17 07:42:30 2010 +0200 >> @@ -609,9 +609,17 @@ 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, m; >> + xc_cpupoolinfo_t *info; >> + int size; >> + uint32_t poolid; >> + libxl_physinfo physinfo; >> + >> + if (libxl_get_physinfo(ctx,&physinfo) != 0) { >> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting phys info"); >> + return NULL; >> + } >> + size = physinfo.max_cpu_id + 32; > > Where does the number 32 come from?I just wanted to be able to support some (inactive) cpupools without any cpu allocated. It''s just a number which should normally be large enough.> >> ptr = calloc(size, sizeof(libxl_poolinfo)); > >> if (!ptr) { >> @@ -619,16 +627,23 @@ libxl_poolinfo * libxl_list_pool(libxl_c >> return NULL; >> } >> >> - ret = xc_cpupool_getinfo(ctx->xch, 0, 256, info); >> - if (ret<0) { >> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting cpupool info"); >> - return NULL; >> + poolid = 0; >> + for (i = 0; i< size; i++) { >> + info = xc_cpupool_getinfo(ctx->xch, poolid); >> + if (info == NULL) >> + break; >> + ptr[i].poolid = info->cpupool_id; >> + ptr[i].sched_id = info->sched_id; >> + ptr[i].n_dom = info->n_dom; >> + if (libxl_cpumap_alloc(&ptr[i].cpumap, physinfo.max_cpu_id + 1)) >> + break; >> + for (m = 0; m< ptr[i].cpumap.size / sizeof(*ptr[i].cpumap.map); m++) >> + ptr[i].cpumap.map[m] = info->cpumap[m]; > > I guess if "physinfo.max_cpu_id + 1" does not correspond to > info->cpumap_size then that is a serious error but if it does happen > then you may run over the end of either ptr[i].cpumap.map or > info->cpumap.Changed (introducing ''0'' when info->cpumap_size is too small). 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, 2010-09-17 at 12:41 +0100, Juergen Gross wrote:> On 09/17/10 11:46, Ian Campbell wrote: > > > >> On Fri, 2010-09-17 at 07:10 +0100, Juergen Gross wrote: > >> + size = physinfo.max_cpu_id + 32; > > > > Where does the number 32 come from? > > I just wanted to be able to support some (inactive) cpupools without any > cpu allocated. It''s just a number which should normally be large enough.What is the purpose of these inactive cpupools? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell writes ("Re: [Xen-devel] Re: [Patch] support of cpu pools in
xl"):> On Fri, 2010-09-17 at 12:41 +0100, Juergen Gross wrote:
> > I just wanted to be able to support some (inactive) cpupools without
any
> > cpu allocated. It''s just a number which should normally be
large enough.
>
> What is the purpose of these inactive cpupools?
Amongst other things, I would guess, the creation or removal of
cpupools !
I think we should have a #define rather than the magic number 32 though.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Juergen Gross writes ("Re: [Xen-devel] Re: [Patch] support of cpu pools in
xl"):> On 09/17/10 11:46, Ian Campbell wrote:
> > In an ideal world, yes. I think this was a workaround for buggy
versions
> > of flex observed in the field or something.
>
> How nice ;-)
I don''t think it''s really that bad an answer. Doing it this
way means
you can compile if you don''t have flex, and the flex output itself is
portable to any compiler/platform and not dependent on any part of the
configuration.
If you send patches which touch the .l files my build test will
rebuild the .c files so I don''t mind if you leave out the changes to
the .l in your patch.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On Fri, 2010-09-17 at 16:53 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] Re: [Patch] support of cpu pools in xl"): > > On Fri, 2010-09-17 at 12:41 +0100, Juergen Gross wrote: > > > I just wanted to be able to support some (inactive) cpupools without any > > > cpu allocated. It''s just a number which should normally be large enough. > > > > What is the purpose of these inactive cpupools? > > Amongst other things, I would guess, the creation or removal of > cpupools !I don''t think so, libxl_create_cpupool returns a new poolid for a newly created pool, so they are not needed for that. libxl_destroy_cpupool would only be used to delete a real cpupool, you don''t need dummy pools to remove pools. BTW I noticed that we have libxl_list_pool vs libxl_{create,destroy}_cpupool and libxl_cpupool_{cpuadd,cpuremove,movedomain}. I think the interface should use cpupool throughout and not just pool to make it clear what it is a pool of. IOW libxl_list_pool should be libxl_list_cpupool, the type should be called libxl_cpupool and functions such as libxl_name_to_poolid should instead be libxl_name_to_cpupoolid. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Sep-20 04:58 UTC
Re: [Xen-devel] Re: [Patch] support of cpu pools in xl
On 09/17/10 20:28, Ian Campbell wrote:> On Fri, 2010-09-17 at 16:53 +0100, Ian Jackson wrote: >> Ian Campbell writes ("Re: [Xen-devel] Re: [Patch] support of cpu pools in xl"): >>> On Fri, 2010-09-17 at 12:41 +0100, Juergen Gross wrote: >>>> I just wanted to be able to support some (inactive) cpupools without any >>>> cpu allocated. It''s just a number which should normally be large enough. >>> >>> What is the purpose of these inactive cpupools? >> >> Amongst other things, I would guess, the creation or removal of >> cpupools !"Inactive cpupools" were meant to be cpupools without any cpus and domains assigned to them. They can exist for a short time during creation and removal, but due to explicitly removing all cpus, too.> I don''t think so, libxl_create_cpupool returns a new poolid for a newly > created pool, so they are not needed for that.They have a poolid, but there might be more cpupools than cpus in the system. This was the reason for the "+ 32". But I agree, this should be done via a #define.> BTW I noticed that we have libxl_list_pool vs > libxl_{create,destroy}_cpupool and > libxl_cpupool_{cpuadd,cpuremove,movedomain}. I think the interface > should use cpupool throughout and not just pool to make it clear what it > is a pool of. IOW libxl_list_pool should be libxl_list_cpupool, the type > should be called libxl_cpupool and functions such as > libxl_name_to_poolid should instead be libxl_name_to_cpupoolid.Okay, I''ll change it. 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi, next version of cpupool support in xl. Changes since last version: rename "pool" to "cpupool" in interfaces, use #define instead of magic constant. Ian, I had to add functions attrib__uint64_t_ptr_set/get in python/xen/lowlevel/xl/xl.c to get a proper build. Could you please look if the dummy implementation is okay? I''m really not sure about it. 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
(I''m just back from vacation, sorry for the delay replying) On Mon, 2010-09-20 at 05:58 +0100, Juergen Gross wrote:> On 09/17/10 20:28, Ian Campbell wrote: > > On Fri, 2010-09-17 at 16:53 +0100, Ian Jackson wrote: > >> Ian Campbell writes ("Re: [Xen-devel] Re: [Patch] support of cpu pools in xl"): > >>> On Fri, 2010-09-17 at 12:41 +0100, Juergen Gross wrote: > >>>> I just wanted to be able to support some (inactive) cpupools without any > >>>> cpu allocated. It''s just a number which should normally be large enough. > >>> > >>> What is the purpose of these inactive cpupools? > >> > >> Amongst other things, I would guess, the creation or removal of > >> cpupools ! > > "Inactive cpupools" were meant to be cpupools without any cpus and domains > assigned to them. > They can exist for a short time during creation and removal, but due to > explicitly removing all cpus, too.That makes sense in itself but then why do you need to add a magic number? I think libxl_list_pool should look more like libxl_list_domain, which implies that the xc_cpupool_getinfo interface should not be changed as in your previous patch since the new interface seems to preclude this usage. You really need retain the first poolid + a max number of entries + return the actual number of entries used interface in order to have a usable interface when there is no way to query the maximum pool id. The problem with the interface you are trying to define is compounded by the fact that the returned array is sparse and so in fact you will run out of space at poolid == nr_cpus+32 rather than at number of pools =nr_cpus+32. (Note that in contrast libxl_list_domain returns a compact array so that you run out of space at 1024 domains total, not domid 1024). IMHO libxl_list_{pool,domain} should also go realloc the buffer and go around again in the case where the underlying xc call returned the maximum number of entries -- since there may be more to come. Perhaps this is less likely in the domain case (1024 domains is quite a lot, at least today) but it seem more plausible in the pool case? I think this is probably a separate issue though and getting the basic semantics of xc_cpupool_getinfo/libxl_list_pool is more important.> > I don''t think so, libxl_create_cpupool returns a new poolid for a newly > > created pool, so they are not needed for that. > > They have a poolid, but there might be more cpupools than cpus in the system. > This was the reason for the "+ 32". But I agree, this should be done via a > #define.I think it should be done by defining an interface which doesn''t need arbitrary magic numbers in the first place. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Oct-01 07:18 UTC
Re: [Xen-devel] Re: [Patch] support of cpu pools in xl
On 09/29/10 10:28, Ian Campbell wrote:> (I''m just back from vacation, sorry for the delay replying) > > On Mon, 2010-09-20 at 05:58 +0100, Juergen Gross wrote: >> On 09/17/10 20:28, Ian Campbell wrote: >>> On Fri, 2010-09-17 at 16:53 +0100, Ian Jackson wrote: >>>> Ian Campbell writes ("Re: [Xen-devel] Re: [Patch] support of cpu pools in xl"): >>>>> On Fri, 2010-09-17 at 12:41 +0100, Juergen Gross wrote: >>>>>> I just wanted to be able to support some (inactive) cpupools without any >>>>>> cpu allocated. It''s just a number which should normally be large enough. >>>>> >>>>> What is the purpose of these inactive cpupools? >>>> >>>> Amongst other things, I would guess, the creation or removal of >>>> cpupools ! >> >> "Inactive cpupools" were meant to be cpupools without any cpus and domains >> assigned to them. >> They can exist for a short time during creation and removal, but due to >> explicitly removing all cpus, too. > > That makes sense in itself but then why do you need to add a magic > number? > > I think libxl_list_pool should look more like libxl_list_domain, which > implies that the xc_cpupool_getinfo interface should not be changed as > in your previous patch since the new interface seems to preclude this > usage. You really need retain the first poolid + a max number of entries > + return the actual number of entries used interface in order to have a > usable interface when there is no way to query the maximum pool id. > > The problem with the interface you are trying to define is compounded by > the fact that the returned array is sparse and so in fact you will run > out of space at poolid == nr_cpus+32 rather than at number of pools => nr_cpus+32. (Note that in contrast libxl_list_domain returns a compact > array so that you run out of space at 1024 domains total, not domid > 1024).I think you misread the code. The returned array is NOT sparse. Please note that the hypervisor will return the info of the next cpu pool with poolid equal or larger as the requested one (that''s the reason why poolid is a vital return info).> > IMHO libxl_list_{pool,domain} should also go realloc the buffer and go > around again in the case where the underlying xc call returned the > maximum number of entries -- since there may be more to come. Perhaps > this is less likely in the domain case (1024 domains is quite a lot, at > least today) but it seem more plausible in the pool case? I think this > is probably a separate issue though and getting the basic semantics of > xc_cpupool_getinfo/libxl_list_pool is more important.I agree realloc-ing the buffer for the array in libxl_list_pool is a better solution (now easy to do as the cpumasks are allocated separately).> >>> I don''t think so, libxl_create_cpupool returns a new poolid for a newly >>> created pool, so they are not needed for that. >> >> They have a poolid, but there might be more cpupools than cpus in the system. >> This was the reason for the "+ 32". But I agree, this should be done via a >> #define. > > I think it should be done by defining an interface which doesn''t need > arbitrary magic numbers in the first place.I''ll resend modified patches. 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, 2010-10-01 at 08:18 +0100, Juergen Gross wrote:> > > The problem with the interface you are trying to define is > compounded by > > the fact that the returned array is sparse and so in fact you will > run > > out of space at poolid == nr_cpus+32 rather than at number of pools > => > nr_cpus+32. (Note that in contrast libxl_list_domain returns a > compact > > array so that you run out of space at 1024 domains total, not domid > > 1024). > > I think you misread the code. The returned array is NOT sparse. Please > note that the hypervisor will return the info of the next cpu pool > with poolid equal or larger as the requested one (that''s the reason > why poolid is a vital return info).Ah, I think that makes sense, thanks for taking the time to explain. I''ll take another look at the latest patch set. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel