[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/3] libxl: implement NUMA capabilities reporting



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]