Re: [PATCH RFC v2 1/2] linux/vnuma: vNUMA for PV domu guest

[David Vrabel <david.vrabel@citrix.com>]

On 18/09/13 07:16, Elena Ufimtseva wrote:
> On Tue, Sep 17, 2013 at 10:10 AM, David Vrabel <david.vrabel@citrix.com> wrote:
>> On 17/09/13 09:34, Elena Ufimtseva wrote:
>>> Requests NUMA topology info from Xen by issuing subop
>>> hypercall. Initializes NUMA nodes, sets number of CPUs,
>>> distance table and NUMA nodes memory ranges during boot.
>>> vNUMA topology defined by user in VM config file. Memory
>>> ranges are represented by structure vnuma_topology_info
>>> where start and end of memory area are defined in guests
>>> pfns numbers, constructed and aligned accordingly to
>>> e820 domain map.
>>> In case the received structure has errors, will fail to
>>> dummy numa init.
>>> Requires XEN with applied patches from vnuma patchset;
>>>
>>> Changes since v1:
>>> - moved the test for xen_pv_domain() into xen_numa_init;
>>> - replaced memory block search/allocation by single memblock_alloc;
>>> - moved xen_numa_init to vnuma.c from enlighten.c;
>>> - moved memblock structure to public interface memory.h;
>>> - specified signedness of vnuma topology structure members;
>>> - removed excessive debug output;
>>>
>>> TODO:
>>> - consider common interface for Dom0, HVM and PV guests to provide
>>> vNUMA topology;
>>> - dynamic numa balancing at the time of this patch (kernel 3.11
>>> 6e4664525b1db28f8c4e1130957f70a94c19213e with boot parameter
>>> numa_balancing=true that is such by default) crashes numa-enabled
>>> guest. Investigate further.
>>
>>> --- a/arch/x86/mm/numa.c
>>> +++ b/arch/x86/mm/numa.c
>>> @@ -19,6 +19,7 @@
>>>  #include <asm/amd_nb.h>
>>
>> #include <asm/xen/vnuma.h> here...
>>
>>>  #include "numa_internal.h"
>>> +#include "asm/xen/vnuma.h"
>>
>> ... not here.
>>
>>> --- /dev/null
>>> +++ b/arch/x86/xen/vnuma.c
>>> @@ -0,0 +1,92 @@
>>> +#include <linux/err.h>
>>> +#include <linux/memblock.h>
>>> +#include <xen/interface/xen.h>
>>> +#include <xen/interface/memory.h>
>>> +#include <asm/xen/interface.h>
>>> +#include <asm/xen/hypercall.h>
>>> +#include <asm/xen/vnuma.h>
>>> +#ifdef CONFIG_NUMA
>>> +/* Xen PV NUMA topology initialization */
>>> +static unsigned int xen_vnuma_init = 0;
>>> +int xen_vnuma_support()
>>> +{
>>> +     return xen_vnuma_init;
>>> +}
>>
>> I'm not sure how this and the usage in the next patch actually work.
>> xen_vnuma_init is only set after the test of numa_off prior to calling
>> xen_numa_init() which will set xen_vnuma_init.
> 
> David, its obscure and naming is not self explanatory.. Will fix it.
> But the idea was to make sure
> that NUMA can be safely turned on (for domu domain and if
> xen_numa_init call was sucessfull).

I understand what it's for, I just don't see how it works.

The code path looks like (I think):

xen_vnuma_init = 0;

if (!xen_vnuma_init)
    numa_off = 1

if (!numa_off)
    xen_numa_init()

However, if you go with the idea of calling dummy init in
xen_num_init()'s error path you don't need this.


>>> +     for (i = 0; i < numa_topo.nr_nodes; i++) {
>>> +             if (numa_add_memblk(i, varea[i].start, varea[i].end))
>>> +                     /* pass to numa_dummy_init */
>>> +                     goto vnumaout;
>>
>> If there's a failure here, numa may be partially setup.  Do you need to
>> undo any of the bits that have already setup?
> 
> Konrad asked me the same and I was under impression it is safe. But
> that was based on assumptions
> what I would rather avoid making.  I will add bits to unset numa in
> case of failure.

I looked at the other uses of this and none of them undo on failure so I
think it is fine as is.

>>> +     if (phys)
>>> +             memblock_free(__pa(phys), mem_size);
>>> +     if (physd)
>>> +             memblock_free(__pa(physd), dist_size);
>>> +     if (physc)
>>> +             memblock_free(__pa(physc), cpu_to_node_size);
>>> +     return rc;
>>
>> If you return an error, x86_numa_init() will try to call setup for other
>> NUMA system.  Consider calling numa_dummy_init() directly instead and
>> then returning success.
> 
> David, isnt it what x86_numa_init() supposed to do? try every
> *numa_init until one succeed?
> Will adding excplicit call to dummy numa from xen_init_numa brake this logic?

Yes, but if we know we're a PV guest we do not want to try any other
one, we want to fallback to the dummy init immediately.

David