[PATCH v2] x86: correct socket_cpumask allocation

[PATCH v2] x86: correct socket_cpumask allocation

[Chao Peng]

For booting cpu, the socket number is not needed to be 0 so
it needs to be computed by cpu number.

For secondary cpu, phys_proc_id is not valid in CPU_PREPARE
notifier(cpu_smpboot_alloc), so cpu_to_socket(cpu) can't be used.
Instead, pre-allocate secondary_cpu_mask in cpu_smpboot_alloc()
and later consume it in smp_store_cpu_info().

This patch also change socket_cpumask type from 'cpumask_var_t *'
to 'cpumask_t **' so that smaller NR_CPUS works.

Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
Changes in v2:
* Fix case that booting cpu is on the socket other than socket0.
* cpumask_var_t => cpumask_t * to make smaller NR_CPUS builds.
---
 xen/arch/x86/smpboot.c    | 27 ++++++++++++++++++++-------
 xen/include/asm-x86/smp.h |  2 +-
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index c73aa1b..86671a1 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -61,7 +61,8 @@ cpumask_t cpu_online_map __read_mostly;
 EXPORT_SYMBOL(cpu_online_map);
 
 unsigned int __read_mostly nr_sockets;
-cpumask_var_t *__read_mostly socket_cpumask;
+cpumask_t **__read_mostly socket_cpumask;
+static cpumask_t *secondary_socket_cpumask;
 
 struct cpuinfo_x86 cpu_data[NR_CPUS];
 
@@ -84,11 +85,21 @@ void *stack_base[NR_CPUS];
 static void smp_store_cpu_info(int id)
 {
     struct cpuinfo_x86 *c = cpu_data + id;
+    unsigned int socket;
 
     *c = boot_cpu_data;
     if ( id != 0 )
+    {
         identify_cpu(c);
 
+        socket = cpu_to_socket(id);
+        if ( !socket_cpumask[socket] )
+        {
+            socket_cpumask[socket] = secondary_socket_cpumask;
+            secondary_socket_cpumask = NULL;
+        }
+    }
+
     /*
      * Certain Athlons might work (for various values of 'work') in SMP
      * but they are not certified as MP capable.
@@ -658,7 +669,7 @@ static void cpu_smpboot_free(unsigned int cpu)
 
     if ( cpumask_empty(socket_cpumask[socket]) )
     {
-        free_cpumask_var(socket_cpumask[socket]);
+        xfree(socket_cpumask[socket]);
         socket_cpumask[socket] = NULL;
     }
 
@@ -705,7 +716,6 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     nodeid_t node = cpu_to_node(cpu);
     struct desc_struct *gdt;
     unsigned long stub_page;
-    unsigned int socket = cpu_to_socket(cpu);
 
     if ( node != NUMA_NO_NODE )
         memflags = MEMF_node(node);
@@ -748,8 +758,9 @@ static int cpu_smpboot_alloc(unsigned int cpu)
         goto oom;
     per_cpu(stubs.addr, cpu) = stub_page + STUB_BUF_CPU_OFFS(cpu);
 
-    if ( !socket_cpumask[socket] &&
-         !zalloc_cpumask_var(socket_cpumask + socket) )
+    if ( secondary_socket_cpumask == NULL &&
+         (secondary_socket_cpumask = _xzalloc(nr_cpumask_bits / 8,
+                                              sizeof(long))) == NULL )
         goto oom;
 
     if ( zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) &&
@@ -804,8 +815,10 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 
     set_nr_sockets();
 
-    socket_cpumask = xzalloc_array(cpumask_var_t, nr_sockets);
-    if ( !socket_cpumask || !zalloc_cpumask_var(socket_cpumask) )
+    socket_cpumask = xzalloc_array(cpumask_t *, nr_sockets);
+    if ( socket_cpumask == NULL ||
+         (socket_cpumask[cpu_to_socket(0)] = _xzalloc(nr_cpumask_bits / 8,
+                                                      sizeof(long))) == NULL )
         panic("No memory for socket CPU siblings map");
 
     if ( !zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, 0)) ||
diff --git a/xen/include/asm-x86/smp.h b/xen/include/asm-x86/smp.h
index e594062..ea07888 100644
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -67,7 +67,7 @@ extern unsigned int nr_sockets;
 void set_nr_sockets(void);
 
 /* Representing HT and core siblings in each socket. */
-extern cpumask_var_t *socket_cpumask;
+extern cpumask_t **socket_cpumask;
 
 #endif /* !__ASSEMBLY__ */
 
-- 
1.9.1

Re: [PATCH v2] x86: correct socket_cpumask allocation

[Dario Faggioli]

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

On Thu, 2015-07-09 at 16:26 +0800, Chao Peng wrote:
> For booting cpu, the socket number is not needed to be 0 so
> it needs to be computed by cpu number.
> 
> For secondary cpu, phys_proc_id is not valid in CPU_PREPARE
> notifier(cpu_smpboot_alloc), so cpu_to_socket(cpu) can't be used.
> Instead, pre-allocate secondary_cpu_mask in cpu_smpboot_alloc()
> and later consume it in smp_store_cpu_info().
> 
> This patch also change socket_cpumask type from 'cpumask_var_t *'
> to 'cpumask_t **' so that smaller NR_CPUS works.
> 
> Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
> Changes in v2:
> * Fix case that booting cpu is on the socket other than socket0.
>
As far as this goes:

Tested-by: Dario Faggioli <dario.faggioli@citrix.com>

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: 181 bytes --]

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

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

Re: [PATCH v2] x86: correct socket_cpumask allocation

[Andrew Cooper]

On 09/07/15 09:26, Chao Peng wrote:
> For booting cpu, the socket number is not needed to be 0 so
> it needs to be computed by cpu number.
>
> For secondary cpu, phys_proc_id is not valid in CPU_PREPARE
> notifier(cpu_smpboot_alloc), so cpu_to_socket(cpu) can't be used.
> Instead, pre-allocate secondary_cpu_mask in cpu_smpboot_alloc()
> and later consume it in smp_store_cpu_info().
>
> This patch also change socket_cpumask type from 'cpumask_var_t *'
> to 'cpumask_t **' so that smaller NR_CPUS works.
>
> Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>

I have thrown this patch into my XenServer upstream testing branch, to
get a wide coverage of servers.

(So far, on a random sample of 24 servers from our testing pool, not a
single one has successfully booted with the original bug present.)

~Andrew

Re: [PATCH v2] x86: correct socket_cpumask allocation

[Jan Beulich]

>>> On 09.07.15 at 10:26, <chao.p.peng@linux.intel.com> wrote:
> @@ -748,8 +758,9 @@ static int cpu_smpboot_alloc(unsigned int cpu)
>          goto oom;
>      per_cpu(stubs.addr, cpu) = stub_page + STUB_BUF_CPU_OFFS(cpu);
>  
> -    if ( !socket_cpumask[socket] &&
> -         !zalloc_cpumask_var(socket_cpumask + socket) )
> +    if ( secondary_socket_cpumask == NULL &&
> +         (secondary_socket_cpumask = _xzalloc(nr_cpumask_bits / 8,
> +                                              sizeof(long))) == NULL )

This is horrible since completely type-unsafe, and correct only
because _xmalloc() happens to allocate more space than requested
if the size isn't a multiple of MEM_ALIGN. And it makes me realize why
on IRC I first suggested xzalloc_array(): That would at least have
taken care of that latent bug. And remember that I did _not_
suggest _xzalloc(), but xzalloc().

Taken together I think we should stay with using zalloc_cpumask_var(),
and introduce zap_cpumask_var() (storing NULL in the big NR_CPUS
case and doing nothing in the small one). Should I be overlooking
something that still prevents this from building in both cases, the
above allocation should be changed to at least be type safe (and I
guess I'd rather waste a few bytes here than see you add fragile
casts or some such).

Jan

Re: [PATCH v2] x86: correct socket_cpumask allocation

[Chao Peng]

On Thu, Jul 09, 2015 at 10:41:55AM +0100, Jan Beulich wrote:
> >>> On 09.07.15 at 10:26, <chao.p.peng@linux.intel.com> wrote:
> > @@ -748,8 +758,9 @@ static int cpu_smpboot_alloc(unsigned int cpu)
> >          goto oom;
> >      per_cpu(stubs.addr, cpu) = stub_page + STUB_BUF_CPU_OFFS(cpu);
> >  
> > -    if ( !socket_cpumask[socket] &&
> > -         !zalloc_cpumask_var(socket_cpumask + socket) )
> > +    if ( secondary_socket_cpumask == NULL &&
> > +         (secondary_socket_cpumask = _xzalloc(nr_cpumask_bits / 8,
> > +                                              sizeof(long))) == NULL )
> 
> This is horrible since completely type-unsafe, and correct only
> because _xmalloc() happens to allocate more space than requested
> if the size isn't a multiple of MEM_ALIGN. And it makes me realize why
> on IRC I first suggested xzalloc_array(): That would at least have
> taken care of that latent bug. And remember that I did _not_
> suggest _xzalloc(), but xzalloc().
> 
> Taken together I think we should stay with using zalloc_cpumask_var(),
> and introduce zap_cpumask_var() (storing NULL in the big NR_CPUS
> case and doing nothing in the small one).

Apart from zap_cpumask_var() there is need to check if cpumask_vat_t is
NULL as well. While that is weird to satisfy compiler for small NR_CPUS case.

> Should I be overlooking
> something that still prevents this from building in both cases, the
> above allocation should be changed to at least be type safe (and I
> guess I'd rather waste a few bytes here than see you add fragile
> casts or some such).

So this solution is finally adopted. The new version is already sent out.

Chao