Re: [PATCH v5 02/12] array_idx: sanitize speculative array de-references

[Ingo Molnar]

* Dan Williams <dan.j.williams@xxxxxxxxx> wrote:

> Thomas, Peter, and Alexei wanted s/nospec_barrier/ifence/ and 

I just checked past discussions, and I cannot find that part, got any links or 
message-IDs?

PeterZ's feedback on Jan 8 was:

> On Sun, Jan 07, 2018 at 06:24:11PM -0800, Alexei Starovoitov wrote:
> > How about:
> > CONFIG_SPECTRE1_WORKAROUND_INDEX_MASK
> > CONFIG_SPECTRE1_WORKAROUND_LOAD_FENCE
>
> INSTRUCTION_FENCE if anything. LFENCE for Intel (and now also for AMD as
> per 0592b0bce169) is a misnomer, IFENCE would be a better name for it.

Which in that context clearly talked about the config space and how to name the 
instruction semantics in light of the confusion of LFENCE and MFENCE opcodes on 
Intel and AMD CPUs...

Also, those early reviews were fundamentally low level feedback related to the 
actual functionality of the barriers and their mapping to the hardware.

But the fact is, the current series introduces an inconsistent barrier namespace 
extension of:

	barrier()
	barrier_data()
	mb()
	rmb()
	wmb()
	store_mb()
	read_barrier_depends()
	...
+	ifence()
+	array_idx()
+	array_idx_mask()

This isn't bikeshed painting: _ALL_ existing barrier API names have 'barrier' or 
its abbreviation 'mb' (memory barrier) somewhere in their names, which makes it 
pretty easy to recognize them at a glance.

I'm giving you high level API naming feedback because we are now growing the size 
of the barrier API.

array_idx() on the other hand is pretty much close to a 'worst possible' name:

 - it's named in an overly generic, opaque fashion
 - doesn't indicate it at all that it's a barrier for something

... and since we want all kernel developers to use these facilities correctly, we 
want the names to be good and suggestive as well.

I'd accept pretty much anything else that adds 'barrier' or 'nospec' to the name: 
array_idx_barrier() or array_idx_nospec(). (I'm slightly leaning towards 'nospec' 
because it's more indicative of what is being done, and it also is what we do for 
get uaccess APIs.)

ifence() is a similar departure from existing barrier naming nomenclature, and I'd 
accept pretty much any other variant:

	barrier_nospec()
	ifence_nospec()

The kernel namespace cleanliness rules are clear and consistent, and there's 
nothing new about them:

 - the name of the API should unambiguously refer back to the API category. For
   barriers this common reference is 'barrier' or 'mb'.

 - use postfixes or prefixes consistently: pick one and don't mix them. If we go 
   with a _nospec() variant for the uaccess API names then we should use a similar
   naming for array_idx() and for the new barrier as well - no mixing.

> You can always follow on with a patch to fix up the names and placements to your 
> liking. While they'll pick on my name choices, they won't pick on yours, because 
> I simply can't be bothered to care about a bikeshed color at this point after 
> being bounced around for 5 revisions of this patch set.

Sorry, this kind of dismissive and condescending attitude won't cut it.

Thanks,

	Ingo