lkml.org 
[lkml]   [2018]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 02/12] array_idx: sanitize speculative array de-references

* Dan Williams <dan.j.williams@intel.com> 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

\
 
 \ /
  Last update: 2018-01-28 19:33    [W:0.070 / U:3.128 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site