On Sun, Jan 28, 2018 at 10:33 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote: > > * 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. This is the feedback I can take action with, thank you. > >> 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. I reacted to your "for heaven's sake", I'll send a v6.
