Re: [PATCH 14/20] x86/barrier: Use alternative_io() in 32-bit barrier functions

[Ingo Molnar <mingo@kernel.org>]

* Josh Poimboeuf <jpoimboe@kernel.org> wrote:

> On Fri, Mar 14, 2025 at 01:49:48PM -1000, Linus Torvalds wrote:
> > So all of these patches look like good cleanups to me, but I do wonder
> > if we should
> > 
> >  (a) not use some naming *quite* as generic as 'ARG()'
> > 
> >  (b) make the asms use ARG_OUT/ARG_IN/ARG_CLOBBER() to clarify
> > 
> > because that ARG(), ARG(), ARGC() pattern looks odd to me.
> > 
> > Maybe it's just me.
> > 
> > Regardless, I do think the series looks like a nice improvement even
> > in the current form, even if that particular repeated pattern feels
> > strange.
> 
> So originally I had ASM_OUTPUT/ASM_INPUT/ASM_CLOBBER, but I ended up
> going with ARG() due to its nice vertical alignment and conciseness:
> 
> 
> 	__asm_call(qual,						\
> 		ALTERNATIVE("call this_cpu_cmpxchg8b_emu",		\
> 			    "cmpxchg8b " __percpu_arg([var]),		\
> 			    X86_FEATURE_CX8),				\
> 		ARG([var] "+m" (__my_cpu_var(_var)), "+a" (old__.low),	\
> 		    "+d" (old__.high)),					\
> 		ARG("b" (new__.low), "c" (new__.high), "S" (&(_var))),	\
> 		ARG("memory"));						\

Two nits:

1)

In justified cases we can align vertically just fine by using spaces:

  ASM_INPUT  ([var] "+m" (__my_cpu_var(_var)), "+a" (old__.low)...
  ASM_OUTPUT ("b" (new__.low), "c" (new__.high), "S" (&(_var))),
  ASM_CLOBBER("memory")

But I don't think the vertical alignment of visually disjoint, 
comma-separated arguments is an improvement in this specific case.

A *truly* advanced typographically aware syntactic construct would be 
something like:

		ALTERNATIVE("call this_cpu_cmpxchg8b_emu",		\
			    "cmpxchg8b " __percpu_arg([var]),		\
			    X86_FEATURE_CX8),				\
									\
		  ASM_INPUT( [var] "+m" (__my_cpu_var(_var)),		\
				   "+a" (old__.low),			\
				   "+d" (old__.high)),			\
									\
		  ASM_OUTPUT(       "b" (new__.low),			\
				    "c" (new__.high),			\
				    "S" (&(_var))),			\
									\
		  ASM_CLOBBER(	    "memory"));

Note how horizontal and vertical grouping improves readability by an 
order of magnitude and properly highlights the only named operand and 
makes it very easy to review this code, should it be a new submission 
(which it isn't).

And as Knuth said, the intersection of the sets of good coders and good 
typographers is necessarily a tiny percentage of humanity 
(paraphrased), but I digress ...

2)

If 'ARGS' is included in the naming then I'd like to insist on the 
plural 'ARGS', not 'ARG', because the common case for more complicated 
asm() statements is multiple asm template constraint arguments 
separated by commas.

But I don't think we need the 'ARGS':

> Though ASM_OUTPUT/ASM_INPUT/ASM_CLOBBER isn't so bad either:
> 
> 	__asm_call(qual,						\
> 		ALTERNATIVE("call this_cpu_cmpxchg8b_emu",		\
> 			    "cmpxchg8b " __percpu_arg([var]),		\
> 			    X86_FEATURE_CX8),				\
> 		ASM_OUTPUT([var] "+m" (__my_cpu_var(_var)),		\
> 			   "+a" (old__.low), "+d" (old__.high)),	\
> 		ASM_INPUT("b" (new__.low), "c" (new__.high),		\
> 			  "S" (&(_var))),				\
> 		ASM_CLOBBER("memory"));					\
> 
> 
> That has the nice benefit of being more self-documenting, albeit more
> verbose and less vertically aligned.
> 
> So I could go either way, really.

I'd vote on:

	ASM_INPUT(),
	ASM_OUTPUT(),
	ASM_CLOBBER()

... because the ASM_ prefix is already unique enough visually to reset 
any cross-pollination from other well-known namespaces, and because in 
coding shorter is better, all other things equal.

Thanks,

	Ingo