Re: [PATCH v3 1/2] mm: Optimize the ARCH_NEEDS_WEAK_PER_CPU logic for s390/alpha architectures

[David Hildenbrand <david@redhat.com>]

On 16.06.25 04:29, Hao Ge wrote:
> From: Hao Ge <gehao@kylinos.cn>

subject is misleading: we are not optimizing anything in this patch, do we?

It should probably be called

"mm/percpu: rename ARCH_NEEDS_WEAK_PER_CPU to MODULE_NEED_WEAK_PER_CPU" 
or sth. like that.

> 
> Recently discovered this entry while checking kallsyms on ARM64:
> ffff800083e509c0 D _shared_alloc_tag
> 
> If ARCH_NEEDS_WEAK_PER_CPU is not defined((it is only defined for

"... is not defined (it is only defined for

> s390 and alpha architectures),there's no need to statically define
> the percpu variable _shared_alloc_tag.
 > > Therefore,we need to implement isolation for this purpose.

In general, throw in a space after "." and "," to make this easier to parse.

> 
> However,currently ARCH_NEEDS_WEAK_PER_CPU is a #define and
> is enclosed within the #if defined(MODULE) conditional block.
> 
> When building the core kernel code for s390 or alpha architectures,
> ARCH_NEEDS_WEAK_PER_CPU remains undefined (as it is gated
> by #if defined(MODULE)).However,when building modules for these
> architectures,the macro is explicitly defined.
> 
> Therefore,we need to make ARCH_NEEDS_WEAK_PER_CPU a Kconfig option.
> And replace all instances of ARCH_NEEDS_WEAK_PER_CPU in the kernel
> code with MODULE_NEEDS_WEAK_PER_CPU,MODULE_NEEDS_WEAK_PER_CPU might
> be a more accurate description,because it was only needed for modules.
> Then,when defining the percpu variable _shared_alloc_tag,wrap it
> with the CONFIG_ARCH_NEEDS_WEAK_PER_CPU condition.
> 
> Therefore, this patch does the following things:

"this patch" should be avoided.

"Therefore, let's ..."

> Add the ARCH_NEEDS_WEAK_PER_CPU option to the mm Kconfig file
> and enable it for the s390 and alpha architectures.
> And replace all instances of ARCH_NEEDS_WEAK_PER_CPU
> in the kernel code with MODULE_NEEDS_WEAK_PER_CPU.

Most of the description here should likely go to patch #2. See below.

> 
> Suggested-by: Suren Baghdasaryan <surenb@google.com>
> Signed-off-by: Hao Ge <gehao@kylinos.cn>
> ---
>   arch/alpha/Kconfig              | 1 +
>   arch/alpha/include/asm/percpu.h | 2 +-
>   arch/s390/Kconfig               | 1 +
>   arch/s390/include/asm/percpu.h  | 2 +-
>   include/linux/alloc_tag.h       | 6 +++---
>   include/linux/percpu-defs.h     | 4 ++--
>   mm/Kconfig                      | 4 ++++
>   7 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
> index 109a4cddcd13..ce9bc842e384 100644
> --- a/arch/alpha/Kconfig
> +++ b/arch/alpha/Kconfig
> @@ -7,6 +7,7 @@ config ALPHA
>   	select ARCH_HAS_DMA_OPS if PCI
>   	select ARCH_MIGHT_HAVE_PC_PARPORT
>   	select ARCH_MIGHT_HAVE_PC_SERIO
> +	select ARCH_NEEDS_WEAK_PER_CPU
>   	select ARCH_NO_PREEMPT
>   	select ARCH_NO_SG_CHAIN
>   	select ARCH_USE_CMPXCHG_LOCKREF
> diff --git a/arch/alpha/include/asm/percpu.h b/arch/alpha/include/asm/percpu.h
> index 6923249f2d49..b164d3720e9e 100644
> --- a/arch/alpha/include/asm/percpu.h
> +++ b/arch/alpha/include/asm/percpu.h
> @@ -11,7 +11,7 @@
>    * Always use weak definitions for percpu variables in modules.
>    */
>   #if defined(MODULE) && defined(CONFIG_SMP)
> -#define ARCH_NEEDS_WEAK_PER_CPU
> +#define MODULE_NEEDS_WEAK_PER_CPU
>   #endif
>   
>   #include <asm-generic/percpu.h>
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 0c16dc443e2f..716031d22346 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -132,6 +132,7 @@ config S390
>   	select ARCH_INLINE_WRITE_UNLOCK_IRQ
>   	select ARCH_INLINE_WRITE_UNLOCK_IRQRESTORE
>   	select ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE
> +	select ARCH_NEEDS_WEAK_PER_CPU
>   	select ARCH_STACKWALK
>   	select ARCH_SUPPORTS_ATOMIC_RMW
>   	select ARCH_SUPPORTS_DEBUG_PAGEALLOC
> diff --git a/arch/s390/include/asm/percpu.h b/arch/s390/include/asm/percpu.h
> index 84f6b8357b45..0afc7ce9c26f 100644
> --- a/arch/s390/include/asm/percpu.h
> +++ b/arch/s390/include/asm/percpu.h
> @@ -18,7 +18,7 @@
>    * generate external references.
>    */
>   #if defined(MODULE)
> -#define ARCH_NEEDS_WEAK_PER_CPU
> +#define MODULE_NEEDS_WEAK_PER_CPU
>   #endif
>   
>   /*
> diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h
> index 8f7931eb7d16..f349cca0ebed 100644
> --- a/include/linux/alloc_tag.h
> +++ b/include/linux/alloc_tag.h
> @@ -88,7 +88,7 @@ static inline struct alloc_tag *ct_to_alloc_tag(struct codetag *ct)
>   	return container_of(ct, struct alloc_tag, ct);
>   }
>   
> -#ifdef ARCH_NEEDS_WEAK_PER_CPU
> +#ifdef MODULE_NEEDS_WEAK_PER_CPU
>   /*
>    * When percpu variables are required to be defined as weak, static percpu
>    * variables can't be used inside a function (see comments for DECLARE_PER_CPU_SECTION).
> @@ -102,7 +102,7 @@ DECLARE_PER_CPU(struct alloc_tag_counters, _shared_alloc_tag);
>   		.ct = CODE_TAG_INIT,						\
>   		.counters = &_shared_alloc_tag };
>   
> -#else /* ARCH_NEEDS_WEAK_PER_CPU */
> +#else /* MODULE_NEEDS_WEAK_PER_CPU */
>   
>   #ifdef MODULE
>   
> @@ -123,7 +123,7 @@ DECLARE_PER_CPU(struct alloc_tag_counters, _shared_alloc_tag);
>   
>   #endif /* MODULE */
>   
> -#endif /* ARCH_NEEDS_WEAK_PER_CPU */
> +#endif /* MODULE_NEEDS_WEAK_PER_CPU */
>   
>   DECLARE_STATIC_KEY_MAYBE(CONFIG_MEM_ALLOC_PROFILING_ENABLED_BY_DEFAULT,
>   			mem_alloc_profiling_key);
> diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h
> index 0aeb0e276a3e..b4ecfc3a7b2b 100644
> --- a/include/linux/percpu-defs.h
> +++ b/include/linux/percpu-defs.h
> @@ -64,13 +64,13 @@
>    * 2. Static percpu variables cannot be defined inside a function.
>    *
>    * Archs which need weak percpu definitions should define
> - * ARCH_NEEDS_WEAK_PER_CPU in asm/percpu.h when necessary.
> + * MODULE_NEEDS_WEAK_PER_CPU in asm/percpu.h when necessary.
>    *
>    * To ensure that the generic code observes the above two
>    * restrictions, if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is set weak
>    * definition is used for all cases.
>    */
> -#if defined(ARCH_NEEDS_WEAK_PER_CPU) || defined(CONFIG_DEBUG_FORCE_WEAK_PER_CPU)
> +#if defined(MODULE_NEEDS_WEAK_PER_CPU) || defined(CONFIG_DEBUG_FORCE_WEAK_PER_CPU)
>   /*
>    * __pcpu_scope_* dummy variable is used to enforce scope.  It
>    * receives the static modifier when it's used in front of
> diff --git a/mm/Kconfig b/mm/Kconfig
> index e113f713b493..2f55cc95cfcb 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -929,6 +929,10 @@ config ARCH_SUPPORTS_PUD_PFNMAP
>   	def_bool y
>   	depends on ARCH_SUPPORTS_HUGE_PFNMAP && HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
>   
> +# s390 and alpha should be enabled,see comments for DECLARE_PER_CPU_SECTION
> +config ARCH_NEEDS_WEAK_PER_CPU
> +       bool

I agree with Willy that this commit is not ideal -- in particular the 
"s390 and alpha should be enabled" can get stale easily.

If you want to add a comment, rather give guidance why any architecture 
would want to set this, like:

/* Architectures that XXX should set this. */

XXX to be defined.

But the bigger problem with this patch is that XXX cannot even be 
properly defined, because this patch does not make use of 
CONFIG_ARCH_NEEDS_WEAK_PER_CPU at all.

... that is done in the second patch.


So what you could do is move the actual introduction of 
CONFIG_ARCH_NEEDS_WEAK_PER_CPU to patch #2, where it is actually used, 
and limit this patch to the rename.

Similarly, teak the patch description to reflect only that.

-- 
Cheers,

David / dhildenb