2022-08-08 08:09:00

by Guo Ren

[permalink] [raw]
Subject: [RFC PATCH 1/4] vmstat: percpu: Rename HAVE_CMPXCHG_LOCAL to HAVE_CMPXCHG_PERCPU_BYTE

From: Guo Ren <[email protected]>

The name HAVE_CMPXCHG_LOCAL is confused with using cmpxchg_local, but
vmstat needs this_cpu_cmpxchg_1. Rename would clarify the meaning, and
maybe we could remove cmpxchg(64)_local API (Only drivers/iommu/intel
used) in the future.

Signed-off-by: Guo Ren <[email protected]>
Signed-off-by: Guo Ren <[email protected]>
---
.../features/locking/cmpxchg-local/arch-support.txt | 6 +++---
arch/Kconfig | 2 +-
arch/arm64/Kconfig | 2 +-
arch/s390/Kconfig | 2 +-
arch/x86/Kconfig | 2 +-
mm/vmstat.c | 4 ++--
6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/features/locking/cmpxchg-local/arch-support.txt b/Documentation/features/locking/cmpxchg-local/arch-support.txt
index 8b1a8d9e1c79..4d4c5c2fa66d 100644
--- a/Documentation/features/locking/cmpxchg-local/arch-support.txt
+++ b/Documentation/features/locking/cmpxchg-local/arch-support.txt
@@ -1,7 +1,7 @@
#
-# Feature name: cmpxchg-local
-# Kconfig: HAVE_CMPXCHG_LOCAL
-# description: arch supports the this_cpu_cmpxchg() API
+# Feature name: cmpxchg-percpu-byte
+# Kconfig: HAVE_CMPXCHG_PERCPU_BYTE
+# description: arch supports the this_cpu_cmpxchg_1() API
#
-----------------------
| arch |status|
diff --git a/arch/Kconfig b/arch/Kconfig
index f330410da63a..81800cdfe161 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -471,7 +471,7 @@ config HAVE_ALIGNED_STRUCT_PAGE
on a struct page for better performance. However selecting this
might increase the size of a struct page by a word.

-config HAVE_CMPXCHG_LOCAL
+config HAVE_CMPXCHG_PERCPU_BYTE
bool

config HAVE_CMPXCHG_DOUBLE
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 571cc234d0b3..24a82bdc766a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -175,7 +175,7 @@ config ARM64
select HAVE_EBPF_JIT
select HAVE_C_RECORDMCOUNT
select HAVE_CMPXCHG_DOUBLE
- select HAVE_CMPXCHG_LOCAL
+ select HAVE_CMPXCHG_PERCPU_BYTE
select HAVE_CONTEXT_TRACKING_USER
select HAVE_DEBUG_KMEMLEAK
select HAVE_DMA_CONTIGUOUS
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 318fce77601d..ac03af800bf7 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -151,7 +151,7 @@ config S390
select HAVE_ARCH_VMAP_STACK
select HAVE_ASM_MODVERSIONS
select HAVE_CMPXCHG_DOUBLE
- select HAVE_CMPXCHG_LOCAL
+ select HAVE_CMPXCHG_PERCPU_BYTE
select HAVE_DEBUG_KMEMLEAK
select HAVE_DMA_CONTIGUOUS
select HAVE_DYNAMIC_FTRACE
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..5f4f6df7b89f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -184,7 +184,7 @@ config X86
select HAVE_ARCH_WITHIN_STACK_FRAMES
select HAVE_ASM_MODVERSIONS
select HAVE_CMPXCHG_DOUBLE
- select HAVE_CMPXCHG_LOCAL
+ select HAVE_CMPXCHG_PERCPU_BYTE
select HAVE_CONTEXT_TRACKING_USER if X86_64
select HAVE_CONTEXT_TRACKING_USER_OFFSTACK if HAVE_CONTEXT_TRACKING_USER
select HAVE_C_RECORDMCOUNT
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 373d2730fcf2..b2fc6d28d3b2 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -554,9 +554,9 @@ void __dec_node_page_state(struct page *page, enum node_stat_item item)
}
EXPORT_SYMBOL(__dec_node_page_state);

-#ifdef CONFIG_HAVE_CMPXCHG_LOCAL
+#ifdef CONFIG_HAVE_CMPXCHG_PERCPU_BYTE
/*
- * If we have cmpxchg_local support then we do not need to incur the overhead
+ * If we have this_cpu_cmpxchg_1 arch support then we do not need to incur the overhead
* that comes with local_irq_save/restore if we use this_cpu_cmpxchg.
*
* mod_state() modifies the zone counter state through atomic per cpu
--
2.36.1


2022-08-08 09:48:29

by Christoph Lameter

[permalink] [raw]
Subject: Re: [RFC PATCH 1/4] vmstat: percpu: Rename HAVE_CMPXCHG_LOCAL to HAVE_CMPXCHG_PERCPU_BYTE

On Mon, 8 Aug 2022, [email protected] wrote:

> The name HAVE_CMPXCHG_LOCAL is confused with using cmpxchg_local, but
> vmstat needs this_cpu_cmpxchg_1. Rename would clarify the meaning, and
> maybe we could remove cmpxchg(64)_local API (Only drivers/iommu/intel
> used) in the future.

HAVE_CMPXCHG_LOCAL indicates that cmpxchg_local() is available.

The term LOCAL is important because that has traditionally signified an
operation that has an atomic nature that only works on the local core.

cmpxchg local is used in slub too in the form of this_cpu_cmpxchg_double.

But there is the other naming using this_cpu.....

Maybe rename to

HAVE_THIS_CPU_CMPXCHG ?

and clean up all the other mentions of "local" in the source too?

There is also a local.h header around somewhere

2022-08-09 03:17:01

by Guo Ren

[permalink] [raw]
Subject: Re: [RFC PATCH 1/4] vmstat: percpu: Rename HAVE_CMPXCHG_LOCAL to HAVE_CMPXCHG_PERCPU_BYTE

On Mon, Aug 8, 2022 at 5:31 PM Christoph Lameter <[email protected]> wrote:
>
> On Mon, 8 Aug 2022, [email protected] wrote:
>
> > The name HAVE_CMPXCHG_LOCAL is confused with using cmpxchg_local, but
> > vmstat needs this_cpu_cmpxchg_1. Rename would clarify the meaning, and
> > maybe we could remove cmpxchg(64)_local API (Only drivers/iommu/intel
> > used) in the future.
>
> HAVE_CMPXCHG_LOCAL indicates that cmpxchg_local() is available.
>
> The term LOCAL is important because that has traditionally signified an
> operation that has an atomic nature that only works on the local core.
>
> cmpxchg local is used in slub too in the form of this_cpu_cmpxchg_double.
1. raw_cpu_generic_cmpxchg_double don't use cmpxchg(64)_local.
2. x86 and s390 implement this_cpu_cmpxchg_double with direct asm
code, no relationship to cmpxchg local.
3. Only arm64 using cmpxchg_double_local internal, but we could remove
the relationship from generic cmpxchg_double_local. It's a fake usage.
So maybe it's time to remove cmpxchg(64)_local in Linux and replace
them by this_cpu_cmpxchg & cmpxchg_relaxed.

>
> But there is the other naming using this_cpu.....
>
> Maybe rename to
>
> HAVE_THIS_CPU_CMPXCHG ?
I think we should keep 1/BYTE as a suffix because riscv only
implements 4bytes & 8bytes size cmpxchg. But vmstat needs 1Byte.

>
> and clean up all the other mentions of "local" in the source too?
Good point, I would try. How we deal with drivers/iommu/intel/iommu.c:
tmp = cmpxchg64_local(&pte->val, 0ULL, pteval);

Change "cmpxchg64_local -> cmpxchg64_relaxed" would make them happy? I
think they are cmpxchg_local & cmpxchg_sync users.


>
> There is also a local.h header around somewhere
Yes, thx for mentioning, I missed that. The alpha, loongarch, MIPS,
PowerPC and x86 make local_cmpxchg -> cmpxchg_local. Most of them are
copy-paste guys, not real users.


--
Best Regards
Guo Ren