From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753010Ab3KSO5K (ORCPT ); Tue, 19 Nov 2013 09:57:10 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:18104 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752354Ab3KSO5H (ORCPT ); Tue, 19 Nov 2013 09:57:07 -0500 X-IronPort-AV: E=Sophos;i="4.93,729,1378857600"; d="scan'208";a="73479759" Message-ID: <528B7C29.3050103@citrix.com> Date: Tue, 19 Nov 2013 14:56:41 +0000 From: David Vrabel User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.16) Gecko/20121215 Iceowl/1.0b1 Icedove/3.0.11 MIME-Version: 1.0 To: Konrad Rzeszutek Wilk CC: Elena Ufimtseva , , , , , , , , , , , , , Subject: Re: [PATCH RESEND v2 2/2] xen: enable vnuma for PV guest References: <1384811922-14642-1-git-send-email-ufimtseva@gmail.com> <1384811922-14642-3-git-send-email-ufimtseva@gmail.com> <528B5160.5010902@citrix.com> <20131119141620.GD5332@phenom.dumpdata.com> <528B774F.7090902@citrix.com> <20131119144630.GA5780@phenom.dumpdata.com> In-Reply-To: <20131119144630.GA5780@phenom.dumpdata.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.80.2.76] X-DLP: MIA1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> #include >>>>> #include >>>>> @@ -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