[PATCH] [pv-ops domU] support MAXSMP

[PATCH] [pv-ops domU] support MAXSMP

[Andrew Jones]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

The MAXSMP config option requires CPUMASK_OFFSTACK, which in turn
requires we init the memory for the maps while we bringing up the cpus.
MAXSMP also increases NR_CPUS to 4096. This increase in size exposed an
issue in the argument construction for mulitcalls from
xen_flush_tlb_others. The args should only need space for the actual
number of cpus, which with xen is currently only up to 32.

Andrew

[-- Attachment #2: xen-pv-support-maxsmp --]
[-- Type: text/plain, Size: 1623 bytes --]


The MAXSMP config option requires CPUMASK_OFFSTACK, which in turn
requires we init the memory for the maps while we bringing up the cpus.
MAXSMP also increases NR_CPUS to 4096. This increase in size exposed an
issue in the argument construction for mulitcalls from
xen_flush_tlb_others. The args should only need space for the actual
number of cpus, which with xen is currently only up to 32.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/x86/xen/mmu.c |    2 +-
 arch/x86/xen/smp.c |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 3bf7b1d..0cf4d7c 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -1293,7 +1293,7 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
 {
 	struct {
 		struct mmuext_op op;
-		DECLARE_BITMAP(mask, NR_CPUS);
+		DECLARE_BITMAP(mask, num_processors);
 	} *args;
 	struct multicall_space mcs;
 
diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index fe03eee..da5aec1 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -178,11 +178,18 @@ static void __init xen_smp_prepare_boot_cpu(void)
 static void __init xen_smp_prepare_cpus(unsigned int max_cpus)
 {
 	unsigned cpu;
+	unsigned int i;
 
 	xen_init_lock_cpu(0);
 
 	smp_store_cpu_info(0);
 	cpu_data(0).x86_max_cores = 1;
+
+	for_each_possible_cpu(i) {
+		zalloc_cpumask_var(&per_cpu(cpu_sibling_map, i), GFP_KERNEL);
+		zalloc_cpumask_var(&per_cpu(cpu_core_map, i), GFP_KERNEL);
+		zalloc_cpumask_var(&cpu_data(i).llc_shared_map, GFP_KERNEL);
+	}
 	set_cpu_sibling_map(0);
 
 	if (xen_smp_intr_init(0))
-- 
1.6.5.2


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] [pv-ops domU] support MAXSMP

[Jan Beulich]

>>> Andrew Jones <drjones@redhat.com> 18.12.09 10:31 >>>
>The MAXSMP config option requires CPUMASK_OFFSTACK, which in turn
>requires we init the memory for the maps while we bringing up the cpus.
>MAXSMP also increases NR_CPUS to 4096. This increase in size exposed an
>issue in the argument construction for mulitcalls from
>xen_flush_tlb_others. The args should only need space for the actual
>number of cpus, which with xen is currently only up to 32.

I don't think new code should be making assumptions like this anymore,
since Xen already supports higher numbers (it's merely the tools which
so far don't). You're basically trading a compile time detectable large
value on stack for one that can grow large dynamically (and hence
require quite a bit more effort to debug, should it ever overrun the
stack).

Jan

Re: [PATCH] [pv-ops domU] support MAXSMP

[Andrew Jones]

On 12/18/2009 11:07 AM, Jan Beulich wrote:
>>>> Andrew Jones<drjones@redhat.com>  18.12.09 10:31>>>
>> The MAXSMP config option requires CPUMASK_OFFSTACK, which in turn
>> requires we init the memory for the maps while we bringing up the cpus.
>> MAXSMP also increases NR_CPUS to 4096. This increase in size exposed an
>> issue in the argument construction for mulitcalls from
>> xen_flush_tlb_others. The args should only need space for the actual
>> number of cpus, which with xen is currently only up to 32.
>
> I don't think new code should be making assumptions like this anymore,
> since Xen already supports higher numbers (it's merely the tools which
> so far don't). You're basically trading a compile time detectable large
> value on stack for one that can grow large dynamically (and hence
> require quite a bit more effort to debug, should it ever overrun the
> stack).

I say 32 cpus in my description to point out the large difference 
between NR_CPUS and the actual number used. However, the code shouldn't 
exceed the limits in multicall until something around 2000 cpus.

Keeping it compile-time is good for the stack analysis, but overly 
wasteful when using values like 4096, since the expected case is 
thousands less.  If we want to keep it static then we need to change 
MC_ARGS to also be dependent in some way on NR_CPUS.

Andrew

Re: [PATCH] [pv-ops domU] support MAXSMP

[Andrew Jones]

On 12/18/2009 11:24 AM, Andrew Jones wrote:
> On 12/18/2009 11:07 AM, Jan Beulich wrote:
>>>>> Andrew Jones<drjones@redhat.com> 18.12.09 10:31>>>
>>> The MAXSMP config option requires CPUMASK_OFFSTACK, which in turn
>>> requires we init the memory for the maps while we bringing up the cpus.
>>> MAXSMP also increases NR_CPUS to 4096. This increase in size exposed an
>>> issue in the argument construction for mulitcalls from
>>> xen_flush_tlb_others. The args should only need space for the actual
>>> number of cpus, which with xen is currently only up to 32.
>>
>> I don't think new code should be making assumptions like this anymore,
>> since Xen already supports higher numbers (it's merely the tools which
>> so far don't). You're basically trading a compile time detectable large
>> value on stack for one that can grow large dynamically (and hence
>> require quite a bit more effort to debug, should it ever overrun the
>> stack).
>
> I say 32 cpus in my description to point out the large difference
> between NR_CPUS and the actual number used. However, the code shouldn't
> exceed the limits in multicall until something around 2000 cpus.
>
> Keeping it compile-time is good for the stack analysis, but overly
> wasteful when using values like 4096, since the expected case is
> thousands less. If we want to keep it static then we need to change
> MC_ARGS to also be dependent in some way on NR_CPUS.
>

Another note here is that the amount of stack allocation is the same 
regardless of the value of num_processors. We're just creating a pointer 
to this structure on the stack. sizeof is smart enough to pass the 
appropriate dynamic size on to the mc call for validation though. So I 
think all should be good with this patch.

Andrew

Re: [PATCH] [pv-ops domU] support MAXSMP

[Jan Beulich]

>>> Andrew Jones <drjones@redhat.com> 18.12.09 14:23 >>>
>Another note here is that the amount of stack allocation is the same 
>regardless of the value of num_processors. We're just creating a pointer 
>to this structure on the stack. sizeof is smart enough to pass the 
>appropriate dynamic size on to the mc call for validation though. So I 
>think all should be good with this patch.

Ah, okay. At least it'll be a clear BUG_ON() if an overflow happens. And
that's probably what you got before your patch?

Jan

Re: [PATCH] [pv-ops domU] support MAXSMP

[Andrew Jones]

On 12/18/2009 02:41 PM, Jan Beulich wrote:
>>>> Andrew Jones<drjones@redhat.com>  18.12.09 14:23>>>
>> Another note here is that the amount of stack allocation is the same
>> regardless of the value of num_processors. We're just creating a pointer
>> to this structure on the stack. sizeof is smart enough to pass the
>> appropriate dynamic size on to the mc call for validation though. So I
>> think all should be good with this patch.
>
> Ah, okay. At least it'll be a clear BUG_ON() if an overflow happens. And
> that's probably what you got before your patch?
>

That's right; "kernel BUG at arch/x86/xen/multicalls.c:209!"

Andrew

Re: [PATCH] [pv-ops domU] support MAXSMP

[Konrad Rzeszutek Wilk]

On Fri, Dec 18, 2009 at 02:23:00PM +0100, Andrew Jones wrote:
> On 12/18/2009 11:24 AM, Andrew Jones wrote:
> >On 12/18/2009 11:07 AM, Jan Beulich wrote:
> >>>>>Andrew Jones<drjones@redhat.com> 18.12.09 10:31>>>
> >>>The MAXSMP config option requires CPUMASK_OFFSTACK, which in turn
> >>>requires we init the memory for the maps while we bringing up the cpus.
> >>>MAXSMP also increases NR_CPUS to 4096. This increase in size exposed an
> >>>issue in the argument construction for mulitcalls from
> >>>xen_flush_tlb_others. The args should only need space for the actual
> >>>number of cpus, which with xen is currently only up to 32.
> >>
> >>I don't think new code should be making assumptions like this anymore,
> >>since Xen already supports higher numbers (it's merely the tools which
> >>so far don't). You're basically trading a compile time detectable large
> >>value on stack for one that can grow large dynamically (and hence
> >>require quite a bit more effort to debug, should it ever overrun the
> >>stack).
> >
> >I say 32 cpus in my description to point out the large difference
> >between NR_CPUS and the actual number used. However, the code shouldn't
> >exceed the limits in multicall until something around 2000 cpus.
> >
> >Keeping it compile-time is good for the stack analysis, but overly
> >wasteful when using values like 4096, since the expected case is
> >thousands less. If we want to keep it static then we need to change
> >MC_ARGS to also be dependent in some way on NR_CPUS.
> >
> 
> Another note here is that the amount of stack allocation is the same
> regardless of the value of num_processors. We're just creating a
> pointer to this structure on the stack. sizeof is smart enough to
> pass the appropriate dynamic size on to the mc call for validation
> though. So I think all should be good with this patch.

Andrew,

Are you carrying this patch in Fedora? Is there a newer version of
this patch that you have?

Re: [PATCH] [pv-ops domU] support MAXSMP

[Andrew Jones]

----- Original Message -----
> On Fri, Dec 18, 2009 at 02:23:00PM +0100, Andrew Jones wrote:
> > On 12/18/2009 11:24 AM, Andrew Jones wrote:
> > >On 12/18/2009 11:07 AM, Jan Beulich wrote:
> > >>>>>Andrew Jones<drjones@redhat.com> 18.12.09 10:31>>>
> > >>>The MAXSMP config option requires CPUMASK_OFFSTACK, which in turn
> > >>>requires we init the memory for the maps while we bringing up the
> > >>>cpus.
> > >>>MAXSMP also increases NR_CPUS to 4096. This increase in size
> > >>>exposed an
> > >>>issue in the argument construction for mulitcalls from
> > >>>xen_flush_tlb_others. The args should only need space for the
> > >>>actual
> > >>>number of cpus, which with xen is currently only up to 32.
> > >>
> > >>I don't think new code should be making assumptions like this
> > >>anymore,
> > >>since Xen already supports higher numbers (it's merely the tools
> > >>which
> > >>so far don't). You're basically trading a compile time detectable
> > >>large
> > >>value on stack for one that can grow large dynamically (and hence
> > >>require quite a bit more effort to debug, should it ever overrun
> > >>the
> > >>stack).
> > >
> > >I say 32 cpus in my description to point out the large difference
> > >between NR_CPUS and the actual number used. However, the code
> > >shouldn't
> > >exceed the limits in multicall until something around 2000 cpus.
> > >
> > >Keeping it compile-time is good for the stack analysis, but overly
> > >wasteful when using values like 4096, since the expected case is
> > >thousands less. If we want to keep it static then we need to change
> > >MC_ARGS to also be dependent in some way on NR_CPUS.
> > >
> >
> > Another note here is that the amount of stack allocation is the same
> > regardless of the value of num_processors. We're just creating a
> > pointer to this structure on the stack. sizeof is smart enough to
> > pass the appropriate dynamic size on to the mc call for validation
> > though. So I think all should be good with this patch.
> 
> Andrew,
> 
> Are you carrying this patch in Fedora? Is there a newer version of
> this patch that you have?

Not in Fedora, it's only in RHEL. From a quick look I don't believe the
patch would need to be changed for the current kernel, but I haven't tried
it.

Drew

Re: [PATCH] [pv-ops domU] support MAXSMP

[Konrad Rzeszutek Wilk]

> > Andrew,
> > 
> > Are you carrying this patch in Fedora? Is there a newer version of
> > this patch that you have?
> 
> Not in Fedora, it's only in RHEL. From a quick look I don't believe the
> patch would need to be changed for the current kernel, but I haven't tried
> it.

OK, I tried it and had to modify it a bit but so far it seems to work
fine. Do you have some other patches that never go upstreamed?

Re: [PATCH] [pv-ops domU] support MAXSMP

[Andrew Jones]

----- Original Message -----
> > > Andrew,
> > >
> > > Are you carrying this patch in Fedora? Is there a newer version of
> > > this patch that you have?
> >
> > Not in Fedora, it's only in RHEL. From a quick look I don't believe
> > the
> > patch would need to be changed for the current kernel, but I haven't
> > tried
> > it.
> 
> OK, I tried it and had to modify it a bit but so far it seems to work
> fine. Do you have some other patches that never go upstreamed?

Not that I know of. We really haven't needed to write too many
xen-related patches for the RHEL6 kernel. Thanks for hunting this
maxsmp one down.

Drew