linux-kernel - Re: [PATCH v2 01/10] powerpc/smp: Cache node for reuse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87imegq9my.fsf@mpe.ellerman.id.au>
Date:   Wed, 22 Jul 2020 17:41:41 +1000
From:   Michael Ellerman <michaele@....ibm.com>
To:     Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:     Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Valentin Schneider <valentin.schneider@....com>,
        Nick Piggin <npiggin@....ibm.com>,
        Oliver OHalloran <oliveroh@....ibm.com>,
        Nathan Lynch <nathanl@...ux.ibm.com>,
        Michael Neuling <mikey@...ux.ibm.com>,
        Anton Blanchard <anton@....ibm.com>,
        Gautham R Shenoy <ego@...ux.vnet.ibm.com>,
        Vaidyanathan Srinivasan <svaidy@...ux.ibm.com>,
        Jordan Niethe <jniethe5@...il.com>
Subject: Re: [PATCH v2 01/10] powerpc/smp: Cache node for reuse

Srikar Dronamraju <srikar@...ux.vnet.ibm.com> writes:
> While cpu_to_node is inline function with access to per_cpu variable.
> However when using repeatedly, it may be cleaner to cache it in a local
> variable.

It's not clear what "cleaner" is supposed to mean. Are you saying it
makes the source clearer, or the generated code?

I'm not sure it will make any difference to the latter.

> Also fix a build error in a some weird config.
> "error: _numa_cpu_lookup_table_ undeclared"

Separate patch please.

> No functional change

The ifdef change means that's not true.

> Cc: linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>
> Cc: LKML <linux-kernel@...r.kernel.org>
> Cc: Michael Ellerman <michaele@....ibm.com>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Valentin Schneider <valentin.schneider@....com>
> Cc: Nick Piggin <npiggin@....ibm.com>
> Cc: Oliver OHalloran <oliveroh@....ibm.com>
> Cc: Nathan Lynch <nathanl@...ux.ibm.com>
> Cc: Michael Neuling <mikey@...ux.ibm.com>
> Cc: Anton Blanchard <anton@....ibm.com>
> Cc: Gautham R Shenoy <ego@...ux.vnet.ibm.com>
> Cc: Vaidyanathan Srinivasan <svaidy@...ux.ibm.com>
> Cc: Jordan Niethe <jniethe5@...il.com>
> Reviewed-by: Gautham R. Shenoy <ego@...ux.vnet.ibm.com>
> Signed-off-by: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/smp.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 73199470c265..680c0edcc59d 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -843,7 +843,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  
>  	DBG("smp_prepare_cpus\n");
>  
> -	/* 
> +	/*
>  	 * setup_cpu may need to be called on the boot cpu. We havent
>  	 * spun any cpus up but lets be paranoid.
>  	 */
> @@ -854,20 +854,24 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  	cpu_callin_map[boot_cpuid] = 1;
>  
>  	for_each_possible_cpu(cpu) {
> +		int node = cpu_to_node(cpu);
> +

Does cpu_to_node() even work here?

Doesn't look like it to me.

More fallout from 8c272261194d ("powerpc/numa: Enable USE_PERCPU_NUMA_NODE_ID") ?

>  		zalloc_cpumask_var_node(&per_cpu(cpu_sibling_map, cpu),
> -					GFP_KERNEL, cpu_to_node(cpu));
> +					GFP_KERNEL, node);
>  		zalloc_cpumask_var_node(&per_cpu(cpu_l2_cache_map, cpu),
> -					GFP_KERNEL, cpu_to_node(cpu));
> +					GFP_KERNEL, node);
>  		zalloc_cpumask_var_node(&per_cpu(cpu_core_map, cpu),
> -					GFP_KERNEL, cpu_to_node(cpu));
> +					GFP_KERNEL, node);
> +#ifdef CONFIG_NEED_MULTIPLE_NODES
>  		/*
>  		 * numa_node_id() works after this.
>  		 */
>  		if (cpu_present(cpu)) {
> -			set_cpu_numa_node(cpu, numa_cpu_lookup_table[cpu]);
> -			set_cpu_numa_mem(cpu,
> -				local_memory_node(numa_cpu_lookup_table[cpu]));
> +			node = numa_cpu_lookup_table[cpu];
> +			set_cpu_numa_node(cpu, node);
> +			set_cpu_numa_mem(cpu, local_memory_node(node));
>  		}
> +#endif
>  	}
>  
>  	/* Init the cpumasks so the boot CPU is related to itself */
> -- 
> 2.17.1

cheers