Messages in this thread | | | Date | Thu, 2 Mar 2023 11:51:17 +0100 | Subject | Re: [PATCH v2 02/11] this_cpu_cmpxchg: ARM64: switch this_cpu_cmpxchg to locked, add _local function | From | David Hildenbrand <> |
| |
On 02.03.23 11:42, David Hildenbrand wrote: > On 09.02.23 16:01, Marcelo Tosatti wrote: >> Goal is to have vmstat_shepherd to transfer from >> per-CPU counters to global counters remotely. For this, >> an atomic this_cpu_cmpxchg is necessary. >> >> Following the kernel convention for cmpxchg/cmpxchg_local, >> change ARM's this_cpu_cmpxchg_ helpers to be atomic, >> and add this_cpu_cmpxchg_local_ helpers which are not atomic. >> >> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> >> >> Index: linux-vmstat-remote/arch/arm64/include/asm/percpu.h >> =================================================================== >> --- linux-vmstat-remote.orig/arch/arm64/include/asm/percpu.h >> +++ linux-vmstat-remote/arch/arm64/include/asm/percpu.h >> @@ -232,13 +232,23 @@ PERCPU_RET_OP(add, add, ldadd) >> _pcp_protect_return(xchg_relaxed, pcp, val) >> >> #define this_cpu_cmpxchg_1(pcp, o, n) \ >> - _pcp_protect_return(cmpxchg_relaxed, pcp, o, n) >> + _pcp_protect_return(cmpxchg, pcp, o, n) >> #define this_cpu_cmpxchg_2(pcp, o, n) \ >> - _pcp_protect_return(cmpxchg_relaxed, pcp, o, n) >> + _pcp_protect_return(cmpxchg, pcp, o, n) >> #define this_cpu_cmpxchg_4(pcp, o, n) \ >> - _pcp_protect_return(cmpxchg_relaxed, pcp, o, n) >> + _pcp_protect_return(cmpxchg, pcp, o, n) >> #define this_cpu_cmpxchg_8(pcp, o, n) \ >> + _pcp_protect_return(cmpxchg, pcp, o, n) >> + >> +#define this_cpu_cmpxchg_local_1(pcp, o, n) \ >> _pcp_protect_return(cmpxchg_relaxed, pcp, o, n) >> +#define this_cpu_cmpxchg_local_2(pcp, o, n) \ >> + _pcp_protect_return(cmpxchg_relaxed, pcp, o, n) >> +#define this_cpu_cmpxchg_local_4(pcp, o, n) \ >> + _pcp_protect_return(cmpxchg_relaxed, pcp, o, n) >> +#define this_cpu_cmpxchg_local_8(pcp, o, n) \ >> + _pcp_protect_return(cmpxchg_relaxed, pcp, o, n) >> + > > Call me confused (not necessarily your fault :) ). > > We have cmpxchg_local, cmpxchg_relaxed and cmpxchg. > this_cpu_cmpxchg_local_* now calls ... *drumroll* ... cmpxchg_relaxed. > > IIUC, cmpxchg_local is only guaranteed to be atomic WRO the current CPU > (especially, protection against interrupts when the operation is > implemented using multiple instructions). We do have a generic > implementation that disables/enables interrupts. > > IIUC, cmpxchg_relaxed an atomic update without any memory ordering > guarantees (in contrast to cmpxchg, cmpxchg_acquire, cmpxchg_acquire). > We default to arch_cmpxchg if we don't have arch_cmpxchg_relaxed. > arch_cmpxchg defaults to arch_cmpxchg_local, if not supported. > > > Naturally I wonder: > > (a) Should these new variants be rather called > this_cpu_cmpxchg_relaxed_* ? > > (b) Should these new variants rather call the "_local" variant? > > > Shedding some light on this would be great.
Nevermind, looking at the other patches I realized that this is arch-specific. Other archs that have _local variants call the _local variants. So I assume we really want the name this_cpu_cmpxchg_local_*, and using _relaxed here is just the aarch64 way of implementing _local via _relaxed.
Confusing :)
-- Thanks,
David / dhildenb
| |