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

[Elena Ufimtseva <ufimtseva@xxxxxxxxx>]

On Wed, Sep 18, 2013 at 3:17 AM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx> 
>> 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?

Sure, in my todo list )

>
>> >> +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

I agree and it does it make sense. I did not know how far I can go
with modifying
original linux logic.

>
> 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)
>



-- 
Elena

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