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;
Message-ID: <132490dda81e93b2f5145ee210167b4c3d0d2cd1.camel@redhat.com>
Subject: Re: [PATCH v3 0/9] SUNRPC service thread scheduler optimizations
From:   Jeff Layton <jlayton@redhat.com>
To:     Chuck Lever <cel@kernel.org>, linux-nfs@vger.kernel.org
Cc:     Chuck Lever <chuck.lever@oracle.com>, lorenzo@kernel.org,
        neilb@suse.de, david@fromorbit.com
Date:   Mon, 10 Jul 2023 14:29:41 -0400
In-Reply-To: <168900729243.7514.15141312295052254929.stgit@manet.1015granger.net>
References: <168900729243.7514.15141312295052254929.stgit@manet.1015granger.net>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-07-10 at 12:41 -0400, Chuck Lever wrote:
> Walking a linked list to find an idle thread is not CPU cache-
> friendly, and in fact I've noted palpable per-request latency
> impacts as the number of nfsd threads on the server increases.
>=20
> After discussing some possible improvements with Jeff at LSF/MM,
> I've been experimenting with the following series. I've measured an
> order of magnitude latency improvement in the thread lookup time,
> and have managed to keep the whole thing lockless.
>=20
> This version of the series addresses Neil's earlier comments and=20
> is robust under load. The first 7 patches in this series seem
> uncontroversial, so I'll push them to the=20
> "topic-sunrpc-thread-scheduling" branch of:=20
>=20
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
>=20
> The last two are still under discussion. They are posted as part
> of this series for comparison to other proposals, but are not yet=20
> included in the topic branch. But they are tested and working.
>=20
>=20
> Changes since v2:=20
> * Dropped the patch that converts sp_lock to a simple spinlock
> * Replaced explicit memory barriers in svc_get_next_xprt()
> * Select thread victims from the other end of the bitmap
> * Added a metric for wake-ups that find nothing on the transport queue
>=20
> Changes since RFC:
> * Add a counter for ingress RPC messages
> * Add a few documenting comments
> * Move the more controversial patches to the end of the series
> * Clarify the refactoring of svc_wake_up()=20
> * Increase the value of RPCSVC_MAXPOOLTHREADS to 4096 (and tested with th=
at many threads)
> * Optimize the loop in svc_pool_wake_idle_thread()
> * Optimize marking a thread "idle" in svc_get_next_xprt()
>=20
> ---
>=20
> Chuck Lever (9):
>       SUNRPC: Deduplicate thread wake-up code
>       SUNRPC: Report when no service thread is available.
>       SUNRPC: Split the svc_xprt_dequeue tracepoint
>       SUNRPC: Count ingress RPC messages per svc_pool
>       SUNRPC: Count pool threads that were awoken but found no work to do
>       SUNRPC: Clean up svc_set_num_threads
>       SUNRPC: Replace dprintk() call site in __svc_create()
>       SUNRPC: Replace sp_threads_all with an xarray
>       SUNRPC: Convert RQ_BUSY into a per-pool bitmap
>=20
>=20
>  fs/nfsd/nfssvc.c              |   3 +-
>  include/linux/sunrpc/svc.h    |  19 ++--
>  include/trace/events/sunrpc.h | 159 ++++++++++++++++++++++++++----
>  net/sunrpc/svc.c              | 177 ++++++++++++++++++++++------------
>  net/sunrpc/svc_xprt.c         |  99 ++++++++++---------
>  5 files changed, 323 insertions(+), 134 deletions(-)
>=20
> --
> Chuck Lever
>=20

You can add my Reviewed-by to the first 7 patches. The last two are not
quite there yet, I think, but I like how they're looking so far.

Cheers,
--=20
Jeff Layton <jlayton@redhat.com>