Re: [Xen-devel] [PATCH v3 11/14] libxl: get and set soft affinity

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-11-20 at 13:00 +0100, Dario Faggioli wrote:

> > > > > +        return ERROR_FAIL;
> > > > > +    }
> > > > > +    libxl_cputopology_list_free(topology, nr_cpus);
> > > > 
> > > > Why are you retrieving this only to immediately throw it away?
> > > > 
> > > Because I need nr_cpus. :-)
> > 
> > Surely this is not the recommended way to get nr_cpus!
> > 
> > libxl_get_cpu_topology() itself calls libxl_get_max_cpus() which seems
> > like the obvious candidate.
> > 
> Well, it does indeed, but then it (most likely) returns something
> different from what libxl_get_max_cpus() says.
> 
> I fact, what it does is use the result of such call to size the arrays
> needed for calling xc_topologyinfo. Then, it takes what the call to
> xc_topologyinfo() returns in tinfo.max_cpu_index and returns that
> (increased by one, as that's the index rather than the number) to the
> caller.
> 
> I think the difference is libxl_get_max_cpus() returns the maximum
> possible number of supported cpus, while libxl_get_cputopology()
> --thanks to the fact that it goes through xc_topologyinfo(), not (only)
> xc_get_max_cpus(), like libxl_get_max_cpus() does-- returns the actual
> number of cpus.
> 
> What I need is the latter, and I'm looking again, but I'm again not
> finding anything easier (and, I agree, less ugly) than this to get it.
> I can go through libxl_get_physinfo(), rather than through
> _topologyinfo(), but the result is not going to be that different.
> 
> I'll keep looking but, in case I really don't fine anything, do you want
> me to:
>  - stick with this, at least for now;
>  - introduce a new libxl (and probably libxc too) interface for this

I don't understand why libxl_get_max_cpus, which is certainly at least
as big as you need, isn't sufficient here. Especially since in the same
function you call libxl_cpu_bitmap_alloc(...,..., 0) which uses
libxl_get_max_cpus.

You then use this with "libxl_bitmap_equal(cpumap, &ecpumap, nr_cpus)"
where surely the sizes of cpumap and ecpumap could be used and/or are
what actually matter? (and shouldn't you be checking that the sizes meet
some constraint relative to each other?)

> And I'm asking with this patch series in mind... I mean, I of course can
> add to my TODO list to do the latter, but do you think it's a
> prerequisite for accepting this patch?


> 
> Just let me know.
> 
> Regards,
> Dario
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel