Elena Ufimtseva
2013-Nov-18 21:58 UTC
[PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
Enables numa if vnuma topology hypercall is supported and it is domU. Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com> --- arch/x86/xen/setup.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c index 68c054f..0aab799 100644 --- a/arch/x86/xen/setup.c +++ b/arch/x86/xen/setup.c @@ -20,6 +20,7 @@ #include <asm/numa.h> #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> +#include <asm/xen/vnuma.h> #include <xen/xen.h> #include <xen/page.h> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void) WARN_ON(xen_set_default_idle()); fiddle_vdso(); #ifdef CONFIG_NUMA - numa_off = 1; + if (!xen_initial_domain() && xen_vnuma_supported()) + numa_off = 0; + else + numa_off = 1; #endif } -- 1.7.10.4
David Vrabel
2013-Nov-19 11:54 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On 18/11/13 21:58, Elena Ufimtseva wrote:> Enables numa if vnuma topology hypercall is supported and it is domU.[...]> --- a/arch/x86/xen/setup.c > +++ b/arch/x86/xen/setup.c > @@ -20,6 +20,7 @@ > #include <asm/numa.h> > #include <asm/xen/hypervisor.h> > #include <asm/xen/hypercall.h> > +#include <asm/xen/vnuma.h> > > #include <xen/xen.h> > #include <xen/page.h> > @@ -598,6 +599,9 @@ void __init xen_arch_setup(void) > WARN_ON(xen_set_default_idle()); > fiddle_vdso(); > #ifdef CONFIG_NUMA > - numa_off = 1; > + if (!xen_initial_domain() && xen_vnuma_supported()) > + numa_off = 0; > + else > + numa_off = 1; > #endif > }I think this whole #ifdef CONFIG_NUMA can be removed and hence xen_vnuma_supported() can be removed as well. For any PV guest we can call the xen_numa_init() and it will do the right thing. For dom0, the hypercall will either: return something sensible (if in the future Xen sets something up), or it will error. If Xen does not have vnuma support, the hypercall will error. In both error cases, the dummy numa node is setup as required. If you do this, you can fold this change in with the previous patch. David
Konrad Rzeszutek Wilk
2013-Nov-19 14:16 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote:> On 18/11/13 21:58, Elena Ufimtseva wrote: > > Enables numa if vnuma topology hypercall is supported and it is domU. > [...] > > --- a/arch/x86/xen/setup.c > > +++ b/arch/x86/xen/setup.c > > @@ -20,6 +20,7 @@ > > #include <asm/numa.h> > > #include <asm/xen/hypervisor.h> > > #include <asm/xen/hypercall.h> > > +#include <asm/xen/vnuma.h> > > > > #include <xen/xen.h> > > #include <xen/page.h> > > @@ -598,6 +599,9 @@ void __init xen_arch_setup(void) > > WARN_ON(xen_set_default_idle()); > > fiddle_vdso(); > > #ifdef CONFIG_NUMA > > - numa_off = 1; > > + if (!xen_initial_domain() && xen_vnuma_supported()) > > + numa_off = 0; > > + else > > + numa_off = 1; > > #endif > > } > > I think this whole #ifdef CONFIG_NUMA can be removed and hence > xen_vnuma_supported() can be removed as well. > > For any PV guest we can call the xen_numa_init() and it will do the > right thing. > > For dom0, the hypercall will either: return something sensible (if in > the future Xen sets something up), or it will error. > > If Xen does not have vnuma support, the hypercall will error. > > In both error cases, the dummy numa node is setup as required.Incorrect. It will end up calling: if (!numa_init(amd_numa_init)) which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.") as that amd_numa_init is called before the dummy node init.> > If you do this, you can fold this change in with the previous patch. > > David
David Vrabel
2013-Nov-19 14:35 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote:> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote: >> On 18/11/13 21:58, Elena Ufimtseva wrote: >>> Enables numa if vnuma topology hypercall is supported and it is domU. >> [...] >>> --- a/arch/x86/xen/setup.c >>> +++ b/arch/x86/xen/setup.c >>> @@ -20,6 +20,7 @@ >>> #include <asm/numa.h> >>> #include <asm/xen/hypervisor.h> >>> #include <asm/xen/hypercall.h> >>> +#include <asm/xen/vnuma.h> >>> >>> #include <xen/xen.h> >>> #include <xen/page.h> >>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void) >>> WARN_ON(xen_set_default_idle()); >>> fiddle_vdso(); >>> #ifdef CONFIG_NUMA >>> - numa_off = 1; >>> + if (!xen_initial_domain() && xen_vnuma_supported()) >>> + numa_off = 0; >>> + else >>> + numa_off = 1; >>> #endif >>> } >> >> I think this whole #ifdef CONFIG_NUMA can be removed and hence >> xen_vnuma_supported() can be removed as well. >> >> For any PV guest we can call the xen_numa_init() and it will do the >> right thing. >> >> For dom0, the hypercall will either: return something sensible (if in >> the future Xen sets something up), or it will error. >> >> If Xen does not have vnuma support, the hypercall will error. >> >> In both error cases, the dummy numa node is setup as required. > > Incorrect. It will end up calling: > > if (!numa_init(amd_numa_init)) > > which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.") > as that amd_numa_init is called before the dummy node init.No it won''t. Any error path after the check for a PV guest will add the dummy node and return success, skipping any of the hardware-specific setup. David
Konrad Rzeszutek Wilk
2013-Nov-19 14:46 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote:> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote: > > On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote: > >> On 18/11/13 21:58, Elena Ufimtseva wrote: > >>> Enables numa if vnuma topology hypercall is supported and it is domU. > >> [...] > >>> --- a/arch/x86/xen/setup.c > >>> +++ b/arch/x86/xen/setup.c > >>> @@ -20,6 +20,7 @@ > >>> #include <asm/numa.h> > >>> #include <asm/xen/hypervisor.h> > >>> #include <asm/xen/hypercall.h> > >>> +#include <asm/xen/vnuma.h> > >>> > >>> #include <xen/xen.h> > >>> #include <xen/page.h> > >>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void) > >>> WARN_ON(xen_set_default_idle()); > >>> fiddle_vdso(); > >>> #ifdef CONFIG_NUMA > >>> - numa_off = 1; > >>> + if (!xen_initial_domain() && xen_vnuma_supported()) > >>> + numa_off = 0; > >>> + else > >>> + numa_off = 1; > >>> #endif > >>> } > >> > >> I think this whole #ifdef CONFIG_NUMA can be removed and hence > >> xen_vnuma_supported() can be removed as well. > >> > >> For any PV guest we can call the xen_numa_init() and it will do the > >> right thing. > >> > >> For dom0, the hypercall will either: return something sensible (if in > >> the future Xen sets something up), or it will error. > >> > >> If Xen does not have vnuma support, the hypercall will error. > >> > >> In both error cases, the dummy numa node is setup as required. > > > > Incorrect. It will end up calling: > > > > if (!numa_init(amd_numa_init)) > > > > which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.") > > as that amd_numa_init is called before the dummy node init. > > No it won''t. Any error path after the check for a PV guest will add the > dummy node and return success, skipping any of the hardware-specific setup.Duh! I totally missed ''return'' at the end of the check! However, even with that (so the return), that means this part won''t be called: 649 numa_init(dummy_numa_init); Which means there won''t be any dummy numa setup?
David Vrabel
2013-Nov-19 14:56 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On 19/11/13 14:46, Konrad Rzeszutek Wilk wrote:> On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote: >> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote: >>> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote: >>>> On 18/11/13 21:58, Elena Ufimtseva wrote: >>>>> Enables numa if vnuma topology hypercall is supported and it is domU. >>>> [...] >>>>> --- a/arch/x86/xen/setup.c >>>>> +++ b/arch/x86/xen/setup.c >>>>> @@ -20,6 +20,7 @@ >>>>> #include <asm/numa.h> >>>>> #include <asm/xen/hypervisor.h> >>>>> #include <asm/xen/hypercall.h> >>>>> +#include <asm/xen/vnuma.h> >>>>> >>>>> #include <xen/xen.h> >>>>> #include <xen/page.h> >>>>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void) >>>>> WARN_ON(xen_set_default_idle()); >>>>> fiddle_vdso(); >>>>> #ifdef CONFIG_NUMA >>>>> - numa_off = 1; >>>>> + if (!xen_initial_domain() && xen_vnuma_supported()) >>>>> + numa_off = 0; >>>>> + else >>>>> + numa_off = 1; >>>>> #endif >>>>> } >>>> >>>> I think this whole #ifdef CONFIG_NUMA can be removed and hence >>>> xen_vnuma_supported() can be removed as well. >>>> >>>> For any PV guest we can call the xen_numa_init() and it will do the >>>> right thing. >>>> >>>> For dom0, the hypercall will either: return something sensible (if in >>>> the future Xen sets something up), or it will error. >>>> >>>> If Xen does not have vnuma support, the hypercall will error. >>>> >>>> In both error cases, the dummy numa node is setup as required. >>> >>> Incorrect. It will end up calling: >>> >>> if (!numa_init(amd_numa_init)) >>> >>> which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.") >>> as that amd_numa_init is called before the dummy node init. >> >> No it won''t. Any error path after the check for a PV guest will add the >> dummy node and return success, skipping any of the hardware-specific setup. > > Duh! I totally missed ''return'' at the end of the check! > > However, even with that (so the return), that means > this part won''t be called: > > 649 numa_init(dummy_numa_init); > > Which means there won''t be any dummy numa setup?The relevant bits in dummy_numa_init are in the error path of xen_numa_init(). I do think this approach (using the provided API to setup the single (dummy) node), is preferable to calling dummy_numa_init(). If I thought the hypervisor ABI was finalized, I''d be happy with this series as-is -- the remaining issues are superficial. David
Konrad Rzeszutek Wilk
2013-Nov-19 15:19 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote:> On 19/11/13 14:46, Konrad Rzeszutek Wilk wrote: > > On Tue, Nov 19, 2013 at 02:35:59PM +0000, David Vrabel wrote: > >> On 19/11/13 14:16, Konrad Rzeszutek Wilk wrote: > >>> On Tue, Nov 19, 2013 at 11:54:08AM +0000, David Vrabel wrote: > >>>> On 18/11/13 21:58, Elena Ufimtseva wrote: > >>>>> Enables numa if vnuma topology hypercall is supported and it is domU. > >>>> [...] > >>>>> --- a/arch/x86/xen/setup.c > >>>>> +++ b/arch/x86/xen/setup.c > >>>>> @@ -20,6 +20,7 @@ > >>>>> #include <asm/numa.h> > >>>>> #include <asm/xen/hypervisor.h> > >>>>> #include <asm/xen/hypercall.h> > >>>>> +#include <asm/xen/vnuma.h> > >>>>> > >>>>> #include <xen/xen.h> > >>>>> #include <xen/page.h> > >>>>> @@ -598,6 +599,9 @@ void __init xen_arch_setup(void) > >>>>> WARN_ON(xen_set_default_idle()); > >>>>> fiddle_vdso(); > >>>>> #ifdef CONFIG_NUMA > >>>>> - numa_off = 1; > >>>>> + if (!xen_initial_domain() && xen_vnuma_supported()) > >>>>> + numa_off = 0; > >>>>> + else > >>>>> + numa_off = 1; > >>>>> #endif > >>>>> } > >>>> > >>>> I think this whole #ifdef CONFIG_NUMA can be removed and hence > >>>> xen_vnuma_supported() can be removed as well. > >>>> > >>>> For any PV guest we can call the xen_numa_init() and it will do the > >>>> right thing. > >>>> > >>>> For dom0, the hypercall will either: return something sensible (if in > >>>> the future Xen sets something up), or it will error. > >>>> > >>>> If Xen does not have vnuma support, the hypercall will error. > >>>> > >>>> In both error cases, the dummy numa node is setup as required. > >>> > >>> Incorrect. It will end up calling: > >>> > >>> if (!numa_init(amd_numa_init)) > >>> > >>> which will crash dom0 (see 8d54db795 "xen/boot: Disable NUMA for PV guests.") > >>> as that amd_numa_init is called before the dummy node init. > >> > >> No it won''t. Any error path after the check for a PV guest will add the > >> dummy node and return success, skipping any of the hardware-specific setup. > > > > Duh! I totally missed ''return'' at the end of the check! > > > > However, even with that (so the return), that means > > this part won''t be called: > > > > 649 numa_init(dummy_numa_init); > > > > Which means there won''t be any dummy numa setup? > > The relevant bits in dummy_numa_init are in the error path of > xen_numa_init().That seems the wrong place to do it. The top layer calls in each of the numa implementations and then falls back to the dummy. Calling from within the implementation on something that is eventually done on the upper level already is not right.> > I do think this approach (using the provided API to setup the single > (dummy) node), is preferable to calling dummy_numa_init().Doesn''t it do the same thing? And also what about if you the user provides fakenuma?> > If I thought the hypervisor ABI was finalized, I''d be happy with this > series as-is -- the remaining issues are superficial.That reads to me as an Ack, but I know you like to have it stated explicitly - so could you state the proper tag please?> > David
David Vrabel
2013-Nov-19 15:55 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On 19/11/13 15:19, Konrad Rzeszutek Wilk wrote:> On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote: >> The relevant bits in dummy_numa_init are in the error path of >> xen_numa_init(). > > That seems the wrong place to do it. The top layer calls > in each of the numa implementations and then falls back to > the dummy.Think of it as not the dummy, but Xen setting the NUMA configuration up with only a single node. The useful bits in dummy_numa_init() are two calls to standard functions for use by *_numa_init() calls so it just seems easier all round to just call then directly than add a dependancy on dummy_numa_init().> Calling from within the implementation on something that is eventually > done on the upper level already is not right.From the point of view of the caller, it does the right thing. NUMA is setup.>> I do think this approach (using the provided API to setup the single >> (dummy) node), is preferable to calling dummy_numa_init(). > > Doesn''t it do the same thing? And also what about if you the user > provides fakenuma?I don''t know what "fakenuma" is refering to.>> If I thought the hypervisor ABI was finalized, I''d be happy with this >> series as-is -- the remaining issues are superficial. > > That reads to me as an Ack, but I know you like to have it stated > explicitly - so could you state the proper tag please?"If I thought the hypervisor ABI was finalized..." is a pretty big "if" so I have deliberately /not/ given an ack or a reviewed tag but I''ve tried to be clear than I think the Linux side is now good enough (except for any changes for any updates to the hypervisor ABI). David
Konrad Rzeszutek Wilk
2013-Nov-19 16:20 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On Tue, Nov 19, 2013 at 03:55:06PM +0000, David Vrabel wrote:> On 19/11/13 15:19, Konrad Rzeszutek Wilk wrote: > > On Tue, Nov 19, 2013 at 02:56:41PM +0000, David Vrabel wrote: > >> The relevant bits in dummy_numa_init are in the error path of > >> xen_numa_init(). > > > > That seems the wrong place to do it. The top layer calls > > in each of the numa implementations and then falls back to > > the dummy. > > Think of it as not the dummy, but Xen setting the NUMA configuration up > with only a single node.Or with multiple nodes if numa=fake is used.> > The useful bits in dummy_numa_init() are two calls to standard functions > for use by *_numa_init() calls so it just seems easier all round to just > call then directly than add a dependancy on dummy_numa_init().My worry is more of the numa_init not being completly executed.> > > Calling from within the implementation on something that is eventually > > done on the upper level already is not right. > > >From the point of view of the caller, it does the right thing. NUMA is > setup. > > >> I do think this approach (using the provided API to setup the single > >> (dummy) node), is preferable to calling dummy_numa_init(). > > > > Doesn''t it do the same thing? And also what about if you the user > > provides fakenuma? > > I don''t know what "fakenuma" is refering to.numa=fake or see arch/x86/mm/numa_emulation.c Which gets executed if you end up calling: numa_init(X) and ''X'' returns a zero or positive value. Which it won''t as the first thing it the Xen PV NUMA code does is to see if the hypercall is supported. If not, it bails out - so we never get to do the ''numa=fake'' if a user wanted to.
Dario Faggioli
2013-Nov-23 16:48 UTC
Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest
On mar, 2013-11-19 at 14:56 +0000, David Vrabel wrote:> The relevant bits in dummy_numa_init are in the error path of > xen_numa_init(). > > I do think this approach (using the provided API to setup the single > (dummy) node), is preferable to calling dummy_numa_init(). > > If I thought the hypervisor ABI was finalized, I''d be happy with this > series as-is -- the remaining issues are superficial. >So, David, in this regard, what do you think about these (and the following messages in the thread? http://bugs.xenproject.org/xen/mid/%3C528B7D350200007800104840@nat28.tlf.novell.com%3E http://bugs.xenproject.org/xen/mid/%3C1384871712.19880.90.camel@Abyss%3E http://bugs.xenproject.org/xen/mid/%3C528B885702000078001048CF@nat28.tlf.novell.com%3E http://bugs.xenproject.org/xen/mid/%3C1384875772.15360.6.camel@Solace%3E http://bugs.xenproject.org/xen/mid/%3C528B97C502000078001049AE@nat28.tlf.novell.com%3E I think Jan has a point about how we are right now retrieving and treating the number of vnodes... Thanks and 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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel