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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3323 bytes --]

On mer, 2013-09-18 at 02:16 -0400, Elena Ufimtseva wrote:
> On Tue, Sep 17, 2013 at 10:10 AM, David Vrabel <david.vrabel@citrix.com> wrote:
>
> >> --- /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 think what David meant was to actually issue one/the hypercall,
perhaps with factitious and known to be wrong parameters, and check the
return value.

If that is EINVAL (or anything different than ENOSYS), then it means
that the hypercall is supported (i.e., we're running on a version of the
Xen hypervisor that has it implemented), and we can go ahead.

If that is ENOSYS, then it means there is no such hypercall, in which
case, the sooner we give up, the better. :-)

David, is this what you meant? If yes, Elena, does that make sense to
you? Do you think you can make the code look like this?

> >> +int __init xen_numa_init(void)
> >> +{
[snip]
> >> +     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?
> 
I was about to replay exactly the same (as Elena) but, on a second
thought, I think David has a point here.

After all, what's the point in calling stuff line acpi_numa_init(),
etc., we already know they're going to fail! So, yes, I think his idea
worth a try...

Also, bear in mind what Konrad said about a call to one of the functions
in x86_numa_init() blowing up if run as Dom0 on some AMD chips. If we
return 'success' here, that will never happen: we either setup a proper
vNUMA topology or go straight to dummy_*, no more room for weird stuff
to happen... It actually sounds pretty cool, doesn't it? :-P

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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