[libvirt] [PATCH v2 1/3] libxl: implement NUMA capabilities reporting
Jim Fehlig
jfehlig at suse.com
Tue Jul 9 22:06:05 UTC 2013
Dario Faggioli wrote:
> On lun, 2013-07-08 at 18:45 -0600, Jim Fehlig wrote:
>
>> Dario Faggioli wrote:
>>
>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index e170357..7515995 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -161,6 +161,115 @@ libxlBuildCapabilities(virArch hostarch,
>>> }
>>>
>>> static virCapsPtr
>>> +libxlMakeNumaCapabilities(libxl_numainfo *numa_info,
>>> + int nr_nodes,
>>> + libxl_cputopology *cpu_topo,
>>> + int nr_cpus,
>>> + virCapsPtr caps)
>>>
>>>
>> I think this should return an int (0 success, -1 failure).
>>
>>
> Well, of course I can do that (but see below)
>
>
>> Noticed the following topology on an old 2 socket, quad core Intel
>> machine I'm using to test this patch
>>
>> <topology>
>> <cells num='1'>
>> <cell id='0'>
>> <memory unit='KiB'>9175040</memory>
>> <cpus num='8'>
>> <cpu id='0' socket_id='0' core_id='0' siblings='0,4'/>
>> <cpu id='1' socket_id='0' core_id='1' siblings='1,5'/>
>> <cpu id='2' socket_id='0' core_id='2' siblings='2,6'/>
>> <cpu id='3' socket_id='0' core_id='3' siblings='3,7'/>
>> <cpu id='4' socket_id='1' core_id='0' siblings='0,4'/>
>> <cpu id='5' socket_id='1' core_id='1' siblings='1,5'/>
>> <cpu id='6' socket_id='1' core_id='2' siblings='2,6'/>
>> <cpu id='7' socket_id='1' core_id='3' siblings='3,7'/>
>> </cpus>
>> </cell>
>> </cells>
>> </topology>
>>
>> I'm not sure how cpus 0 and 4 can be siblings when they are on different
>> sockets, but seems xl reports similar info
>>
>>
> Oh, I see, my bad, same coreID but different socketID means !siblings
> _even_ if on the same node. :-P
>
> That makes sense, what I was not thinking to was the possibility of
> having different sockets _within_ the same node, which seems like your
> case according to both libxl and numactl.
>
> Does that make sense? What machine is that? Do both the socket share the
> same memory bus? Again, it looks like so
>
Yep, makes sense. And yes, both sockets share the same memory bus. The
machine is an Intel DP server with Clovertown processors.
> Anyway, I will fix that.
>
>
>> cpu_topology :
>> cpu: core socket node
>> 0: 0 0 0
>> 1: 1 0 0
>> 2: 2 0 0
>> 3: 3 0 0
>> 4: 0 1 0
>> 5: 1 1 0
>> 6: 2 1 0
>> 7: 3 1 0
>> numa_info :
>> node: memsize memfree distances
>> 0: 8960 7116 10
>>
>> BTW, the qemu driver reports the following on the same machine
>>
>> <topology>
>> <cells num='1'>
>> <cell id='0'>
>> <memory unit='KiB'>8387124</memory>
>> <cpus num='8'>
>> <cpu id='0' socket_id='0' core_id='0' siblings='0'/>
>> <cpu id='1' socket_id='1' core_id='0' siblings='1'/>
>> <cpu id='2' socket_id='0' core_id='1' siblings='2'/>
>> <cpu id='3' socket_id='1' core_id='1' siblings='3'/>
>> <cpu id='4' socket_id='0' core_id='2' siblings='4'/>
>> <cpu id='5' socket_id='1' core_id='2' siblings='5'/>
>> <cpu id='6' socket_id='0' core_id='3' siblings='6'/>
>> <cpu id='7' socket_id='1' core_id='3' siblings='7'/>
>> </cpus>
>> </cell>
>> </cells>
>> </topology>
>>
>> which seems correct since hyperthreading is not supported.
>>
>>
> Yes, and it is basically the same, apart from the ordering, than what
> libxl says (notice that all cpus belongs to node 0!). Again, I will fix
> the siblings part in my patch, in order to take the case of "more
> sockets per node" into better account.
>
>
>>> +static virCapsPtr
>>> libxlMakeCapabilitiesInternal(virArch hostarch,
>>> libxl_physinfo *phy_info,
>>> char *capabilities)
>>> @@ -772,7 +881,11 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>>> {
>>> int err;
>>> libxl_physinfo phy_info;
>>> + libxl_numainfo *numa_info = NULL;
>>> + libxl_cputopology *cpu_topo = NULL;
>>> const libxl_version_info *ver_info;
>>> + int nr_nodes = 0, nr_cpus = 0;
>>> + virCapsPtr caps;
>>>
>>> err = regcomp(&xen_cap_rec, xen_cap_re, REG_EXTENDED);
>>> if (err != 0) {
>>> @@ -796,9 +909,35 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>>> return NULL;
>>> }
>>>
>>> - return libxlMakeCapabilitiesInternal(virArchFromHost(),
>>> + /* Let's try to fetch NUMA info, but it is not critical if we fail */
>>> + numa_info = libxl_get_numainfo(ctx, &nr_nodes);
>>> + if (numa_info == NULL)
>>> + VIR_WARN("libxl_get_numainfo failed to retrieve NUMA data");
>>> + else {
>>> + /* If the above failed, we'd have no NUMa caps anyway! */
>>> + cpu_topo = libxl_get_cpu_topology(ctx, &nr_cpus);
>>> + if (cpu_topo == NULL) {
>>> + VIR_WARN("libxl_get_cpu_topology failed to retrieve topology");
>>> + libxl_numainfo_list_free(numa_info, nr_nodes);
>>> + }
>>> + }
>>>
>>>
>> Move these after the call to libxlMakeCapabilitiesInternal, before
>> calling libxlMakeNumaCapabilities.
>>
>>
> Makes sense, will do.
>
>
>>> +
>>> + caps = libxlMakeCapabilitiesInternal(virArchFromHost(),
>>> &phy_info,
>>> ver_info->capabilities);
>>>
>>>
>> Check that caps != NULL ?
>>
>>
> Ok.
>
>
>>> +
>>> + /* Add topology information to caps, if available */
>>> + if (numa_info && cpu_topo)
>>> + caps = libxlMakeNumaCapabilities(numa_info,
>>> + nr_nodes,
>>> + cpu_topo,
>>> + nr_cpus,
>>> + caps);
>>>
>>>
>> Hmm, guess there is not much to check wrt errors at this point, so
>> libxlMakeNumaCapabilities could return void. I suppose returning
>> success or failure via int is more libvirt style.
>>
>>
> And here's the return 0 or -1 point. The point is we do not care much
> about errors as, if something bad happened during the construction of
> NUMA capabilities, we revert all we've done, with the effect of leaving
> caps unmodified, wrt the one libxlMakeNumaCapabilities() was given.
>
> That is why errors are reported and handled (as described above) inside
> the function itself, and that is therefore why I don't think it would be
> that useful to have it propagate such information to the caller in such
> an explicit manner as returning 0 or -1.
>
> Moreover, given as you said yourself it could well return void, I
> figured it was worthwhile to use the return value for something useful,
> not to mention that, yes, 0/-1 will make it more libvirt style, but this
> is an internal function that, at least according to me, should look more
> like libxlMakeCapabilitiesInternal() than like anything else (and
> libxlMakeCapabilitiesInternal() was returning a virCapsPtr before my
> patch :-) ).
>
Nod.
> So, in summary, I agree on the code motion above, but I think the
> signature and usage of libxlMakeNumaCapabilities() should stay as it is
> now.
Ok, no problem. As you said, it is an internal function and we can
change it if some future code motion warrants such a change.
Regards,
Jim
More information about the libvir-list
mailing list