Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1469782 > unrolled thread

[PATCH] x86, cpu: Fix node state for whether it contains CPU

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86, cpu: Fix node state for whether it contains CPU Tim Chen <tim.c.chen@linux.intel.com> - 2016-08-25 01:30 +0200
    Re: [PATCH] x86, cpu: Fix node state for whether it contains CPU Peter Zijlstra <peterz@infradead.org> - 2016-08-29 15:40 +0200
      Re: [PATCH] x86, cpu: Fix node state for whether it contains CPU Tim Chen <tim.c.chen@linux.intel.com> - 2016-08-29 19:50 +0200

#1469782 — [PATCH] x86, cpu: Fix node state for whether it contains CPU

In current kernel code, we only call node_set_state(cpu_to_node(cpu),
N_CPU) when a cpu is hot plugged.  But we do not set the node state for
N_CPU when the cpus are brought online during boot.

So this could lead to failure when we check to see
if a node contains cpu with node_state(node_id, N_CPU).

One use case is in the node_reclaime function:

        /*
         * Only run node reclaim on the local node or on nodes that do
         * not
         * have associated processors. This will favor the local
         * processor
         * over remote processors and spread off node memory allocations
         * as wide as possible.
         */
        if (node_state(pgdat->node_id, N_CPU) && pgdat->node_id !=
		numa_node_id())
                return NODE_RECLAIM_NOSCAN;

I instrumented the kernel to call this function after boot and it
always returns 0 on a x86 desktop machine until I apply
the attached patch.

static int num_cpu_node(void)
{
       int i, nr_cpu_nodes = 0;

       for_each_node(i) {
               if (node_state(i, N_CPU))
                       ++ nr_cpu_nodes;
       }

       return nr_cpu_nodes;
}

I have not tested other architectues but they are likely
to have similar issue.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/kernel/smpboot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index d8f7d01..04c0574 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -259,6 +259,7 @@ static void notrace start_secondary(void *unused)
 	lock_vector_lock();
 	setup_vector_irq(smp_processor_id());
 	set_cpu_online(smp_processor_id(), true);
+	node_set_state(cpu_to_node(smp_processor_id()), N_CPU);
 	unlock_vector_lock();
 	cpu_set_state_online(smp_processor_id());
 	x86_platform.nmi_init();
-- 
2.5.5

[toc] | [next] | [standalone]

#1471852

On Wed, Aug 24, 2016 at 04:26:49PM -0700, Tim Chen wrote:
> In current kernel code, we only call node_set_state(cpu_to_node(cpu),
> N_CPU) when a cpu is hot plugged.  But we do not set the node state for
> N_CPU when the cpus are brought online during boot.
> 
> So this could lead to failure when we check to see
> if a node contains cpu with node_state(node_id, N_CPU).
> 
> One use case is in the node_reclaime function:
> 
>         /*
>          * Only run node reclaim on the local node or on nodes that do
>          * not
>          * have associated processors. This will favor the local
>          * processor
>          * over remote processors and spread off node memory allocations
>          * as wide as possible.
>          */
>         if (node_state(pgdat->node_id, N_CPU) && pgdat->node_id !=
> 		numa_node_id())
>                 return NODE_RECLAIM_NOSCAN;
> 
> I instrumented the kernel to call this function after boot and it
> always returns 0 on a x86 desktop machine until I apply
> the attached patch.
> 
> static int num_cpu_node(void)
> {
>        int i, nr_cpu_nodes = 0;
> 
>        for_each_node(i) {
>                if (node_state(i, N_CPU))
>                        ++ nr_cpu_nodes;
>        }
> 
>        return nr_cpu_nodes;
> }
> 
> I have not tested other architectues but they are likely
> to have similar issue.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
>  arch/x86/kernel/smpboot.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index d8f7d01..04c0574 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -259,6 +259,7 @@ static void notrace start_secondary(void *unused)
>  	lock_vector_lock();
>  	setup_vector_irq(smp_processor_id());
>  	set_cpu_online(smp_processor_id(), true);
> +	node_set_state(cpu_to_node(smp_processor_id()), N_CPU);
>  	unlock_vector_lock();
>  	cpu_set_state_online(smp_processor_id());
>  	x86_platform.nmi_init();

Would it not be easier to register the vmstat_notifier earlier, before
SMP bringup? Because with this change, we need to go fix all
architectures.

[toc] | [prev] | [next] | [standalone]

#1472008

On Mon, 2016-08-29 at 15:36 +0200, Peter Zijlstra wrote:
> On Wed, Aug 24, 2016 at 04:26:49PM -0700, Tim Chen wrote:
> > 
> > 
> > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> > ---
> >  arch/x86/kernel/smpboot.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index d8f7d01..04c0574 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -259,6 +259,7 @@ static void notrace start_secondary(void *unused)
> >  	lock_vector_lock();
> >  	setup_vector_irq(smp_processor_id());
> >  	set_cpu_online(smp_processor_id(), true);
> > +	node_set_state(cpu_to_node(smp_processor_id()), N_CPU);
> >  	unlock_vector_lock();
> >  	cpu_set_state_online(smp_processor_id());
> >  	x86_platform.nmi_init();
> Would it not be easier to register the vmstat_notifier earlier, before
> SMP bringup? Because with this change, we need to go fix all
> architectures.

I think checking all nodes for online cpus when we init vmstat
is probably the more straightforward way to go. We can do this after
SMP bring up. I'll update the patch for that.

Thanks.

Tim

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web