Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Jeff Layton" <jlayton@kernel.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v3] nfsd: rework hashtable handling in nfsd_do_file_acquire
In-reply-to: <4FF85113-6F17-4F3C-AD31-E2472A988618@oracle.com>
References: <20221003113436.24161-1-jlayton@kernel.org>,
 <F4DF35B2-CE11-4BD9-8442-97852F57CE2E@oracle.com>,
 <166483484979.14457.9448463531121052564@noble.neil.brown.name>,
 <4FF85113-6F17-4F3C-AD31-E2472A988618@oracle.com>
Date:   Wed, 05 Oct 2022 10:06:07 +1100
Message-id: <166492476800.14457.10230243127842792324@noble.neil.brown.name>
Precedence: bulk

On Wed, 05 Oct 2022, Chuck Lever III wrote:
> 
> > On Oct 3, 2022, at 6:07 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> > On Tue, 04 Oct 2022, Chuck Lever III wrote:
> >> 
> >>> On Oct 3, 2022, at 7:34 AM, Jeff Layton <jlayton@kernel.org> 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