Re: [PATCH v2 05/10] percpu: Wire up cmpxchg128

From: "Arnd Bergmann" <arnd@arndb.de>
To: "Peter Zijlstra" <peterz@infradead.org>
Cc: "Linus Torvalds" <torvalds@linux-foundation.org>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Will Deacon" <will@kernel.org>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	dennis@kernel.org, "Tejun Heo" <tj@kernel.org>,
	"Christoph Lameter" <cl@linux.com>,
	"Heiko Carstens" <hca@linux.ibm.com>,
	gor@linux.ibm.com, "Alexander Gordeev" <agordeev@linux.ibm.com>,
	borntraeger@linux.ibm.com, "Sven Schnelle" <svens@linux.ibm.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Borislav Petkov" <bp@alien8.de>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	"Joerg Roedel" <joro@8bytes.org>,
	suravee.suthikulpanit@amd.com,
	"Robin Murphy" <robin.murphy@arm.com>,
	dwmw2@infradead.org, "Baolu Lu" <baolu.lu@linux.intel.com>,
	"Herbert Xu" <herbert@gondor.apana.org.au>,
	"David S . Miller" <davem@davemloft.net>,
	"Pekka Enberg" <penberg@kernel.org>,
	"David Rientjes" <rientjes@google.com>,
	"Joonsoo Kim" <iamjoonsoo.kim@lge.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Vlastimil Babka" <vbabka@suse.cz>,
	"Roman Gushchin" <roman.gushchin@linux.dev>,
	"Hyeonggon Yoo" <42.hyeyoo@gmail.com>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-s390@vger.kernel.org,
	iommu@lists.linux.dev, Linux-Arch <linux-arch@vger.kernel.org>,
	linux-crypto@vger.kernel.org
Subject: Re: [PATCH v2 05/10] percpu: Wire up cmpxchg128
Date: Mon, 06 Feb 2023 13:19:38 +0100	[thread overview]
Message-ID: <30e43e0b-4d16-433f-845d-dd026adfb252@app.fastmail.com> (raw)
In-Reply-To: <Y+DjULnIxcPU/rtp@hirez.programming.kicks-ass.net>

On Mon, Feb 6, 2023, at 12:24, Peter Zijlstra wrote:
> On Fri, Feb 03, 2023 at 06:25:04PM +0100, Arnd Bergmann wrote:

>> Unless I have misunderstood what you are doing, my concerns are
>> still the same:
>> 
>> >  #define this_cpu_cmpxchg(pcp, oval, nval) \
>> > -	__pcpu_size_call_return2(this_cpu_cmpxchg_, pcp, oval, nval)
>> > +	__pcpu_size16_call_return2(this_cpu_cmpxchg_, pcp, oval, nval)
>> >  #define this_cpu_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, 
>> > nval2) \
>> >  	__pcpu_double_call_return_bool(this_cpu_cmpxchg_double_, pcp1, pcp2, 
>> > oval1, oval2, nval1, nval2)
>> 
>> Having a variable-length this_cpu_cmpxchg() that turns into cmpxchg128()
>> and cmpxchg64() even on CPUs where this traps (!X86_FEATURE_CX16) seems
>> like a bad design to me.
>> 
>> I would much prefer fixed-length this_cpu_cmpxchg64()/this_cpu_cmpxchg128()
>> calls that never trap but fall back to the generic version on CPUs that
>> are lacking the atomics.
>
> You're thinking acidental usage etc..? Lemme see what I can do.

I wouldn't even call it accidental when the dependency is so subtle:

Having to call system_has_cmpxchg64() beforce calling cmpxchg64()
is already somewhat awkward but has some logic to it. Having to
call system_has_cmpxchg64()/system_has_cmpxchg128() before calling
this_cpu_cmpxchg() depending on the argument size on architectures
that sometimes have cmpxchg128 but not on architectures that always
have it or that never have it makes it useless as an abstraction.

     Arnd