On Wed, 05 Oct 2022, Chuck Lever III wrote:
>
> > On Oct 3, 2022, at 6:07 PM, NeilBrown <neilb@xxxxxxx> wrote:
> >
> > On Tue, 04 Oct 2022, Chuck Lever III wrote:
> >>
> >>> On Oct 3, 2022, at 7:34 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >>>
> >>> nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
> >>> to get a reference after finding it in the hash. Take the
> >>> rcu_read_lock() and call rhashtable_lookup directly.
> >>>
> >>> Switch to using rhashtable_lookup_insert_key as well, and use the usual
> >>> retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
> >>> target as well.
> >>
> >> The insert_err goto is there to remove a very rare case from
> >> the hot path. I'd kinda like to keep that feature of this code.
> >
> > ????
> > The fast path in the new code looks quite clean - what concerns you?
> > Maybe a "likely()" annotation can be used to encourage the compiler to
> > optimise for the non-error path so the error-handling gets moved
> > out-of-line (assuming it isn't already), but don't think the new code
> > needs that goto.
>
> It's an instruction cache footprint issue.
>
> A CPU populates its instruction cache by reading instructions from
> memory in bulk (some multiple of the size of the cacheline). I
> would like to keep the instructions involved with very rare cases
> (like this one) out-of-line so they do not clutter the CPU's
> instruction cache.
>
> Unfortunately the compiler on my system has decided to place this
> snippet of code right before the out_status: label, which defeats
> my intention.
Don't you hate that!!!!
On the unpatched code, if I put a "likely" annotation on the assumed
common case:
if (likely(!nf)) {
nf = new;
goto open_file;
}
then the open_file code is placed immediately after the
rhashtable_lookup_get_insert_key().
This supports my suggestion that likely/unlikely annotations are better
at controlling code placement than source-code placement.
I've thought for a while that those annotations should be
optimise_for() and optimise_against() or similar. That is what is being
requested.
NeilBrown
