[PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Benjamin Coddington]

This is a third pass at the problem from [1] and [2].  It's the design
Neil described on [1], close to as he wrote it: a client object per peer,
a queue of ready transports per client, and a queue of ready clients per
pool.  A pool takes turns across clients instead of across transports.

I owe an explanation for leaving sparse-flow behind.  Two things:

The re-arm trigger Chuck asked about on [2] is a cliff wherever it's put,
and softening it means picking between watermarks and decaying credits
against real interactive workloads under load.  I ran some of that and
found the behavior depends on timing from several actors at once.  I
don't think I can characterize it well enough to defend a heuristic.

..and Neil's objection on [1] holds: the interactive frame only works
for clients with one or a few users.  A latency floor helps a client
while it's idle between requests.  What I have is a data mover with a
lot of connections sharing a server with clients that keep I/O in flight
themselves, a re-export gateway for one.  Sparse-flow puts those in the
same queue as the mover and they split the pool by connection count.
Neil asked then whether fairness between a client with one connection
and one with sixteen wasn't what I wanted -- but it is.

Approach
--------

Each transport belongs to a client, one per peer address and network
namespace.  A client has a queue of its ready transports, and the pool
has a queue of clients that have something ready.  A thread takes the
client at the head, dispatches one of its transports, and puts the client
back at the tail if it has more.  So every peer with work queued gets one
dispatch per round, however many connections it holds.

Enqueue is still lockless.  Dequeue takes one more lwq lock than before.
The client table is only touched when a connection is accepted and when a
client's last transport goes away, so there's no lookup on the dispatch
path and no RCU.  Listeners and UDP sockets share an anonymous client.
The one piece of Neil's description I left out is moving a transport
between clients.

It's always on and there's nothing to tune.

  patch 1  track clients by peer address, no change to dispatch
  patch 2  dispatch round-robin across clients

These go on top of the svc_clean_up_xprts() wake fix I sent separately
[3], on nfsd-testing.  That's the pre-existing issue Chuck asked me to
look at on [2].  I've only compiled them on nfsd-testing; the numbers
below are from v7.2.

What happened to the review items from [2]: there's no trigger and no
credit to tune, nothing is classified as batch or interactive, and there's
no priority tier, so nothing can be starved.  Control events take turns
like data.  A close queued behind k of its own client's transports gets
dispatched k rounds later, where today it waits behind every queued
transport in the pool.  I didn't add a separate queue for them.  I can if
that's wanted.

Results
-------

Same harness as [2], with one change: every load group now comes from its
own source address, so the server sees it as one client.  16 threads, 10ms
injected per op by the same test-only hook (not part of this series).  A
is v7.2 with the hook, and B adds the wake fix [3] and this series.
NFSv3 burst completion p50 in ms.  NFSv4.1 is within a few percent in
every cell.

Interactive burst of 32 against one busy client with K connections
(unobstructed floor 45.8ms):

    K       4      8     16     32
    A    82.8  249.0  439.9  838.9
    B    73.5   90.1   94.6   90.0

That's two clients taking turns, so about twice the floor, and it doesn't
move with K.  Against burst size N at K=16 it stays about twice the floor
too.  There's no knee any more:

    N        1      8     32     64     96    128
   floor  14.9   31.6   45.8   72.5   98.6  123.6
    A     31.6  125.3  440.0  865.8 1290.1 1705.3
    B     18.0   40.2   91.9  151.3  217.4  278.0

What it does scale with is the number of busy clients.  M clients with 4
connections each, burst of 32:

    M       1      2      4      8
    A    80.9  245.3  441.1  839.0
    B    75.0  112.5  158.3  252.3

That's the cost of dropping the priority tier.  A light client on
a server with a lot of busy peers waits its turn behind each of them.  It
still beats waiting behind each of their connections.

Share of the pool for a client with 2 connections against a client with K,
both backlogged:

    K        4      8     16     32
    A    46.3%  19.9%  11.1%   5.9%
    B    49.4%  47.6%  47.9%  48.4%

Six movers, against clients that are backlogged too.  Share for one
client with 4 connections, and for four such clients together:

                         movers at 4 conns    movers at 8 conns
                            A        B           A        B
    one client           14.3%    14.3%        7.7%    14.3%
    four clients         40.0%    40.0%       25.0%    40.0%
    one client, 1 conn    4.0%    13.0%        2.0%    13.0%

A client that already matches the movers' connection count sees no change.
What changes is that the movers can't buy more by opening more.

Chuck asked on [2] for something real rather than the synthetic victim.
A kernel v3 mount with one connection (noac, lookupcache=none) of a tmpfs
export, walking 2000 files with find | xargs stat (about 26k RPCs) and then
reading with four fio jobs, while the 16-connection aggressor runs from
another address:

                    alone (A / B)     loaded A    loaded B
    walk            1.53s / 2.08s      328.55s     107.74s
    fio 4k IOPS     21876 / 19776           80         777

Both loaded columns are worse than a real mix would be.  With every
request taking exactly 10ms the threads finish in batches, and a request
that shows up mid-batch waits for the next one.

Aggregate throughput is the same A and B in every saturated cell (about
1280 vs 1290 ops/s).  With every group on one source address B gives A's
numbers back, which is what I'd expect.

For the cost on the dispatch path I used fio over a loopback mount of a
tmpfs export, 4k O_DIRECT reads, nconnect=8, five 20s runs each: 95.5k
vs 95.8k IOPS at 16 jobs and 139.0k vs 139.2k at 64, A vs B.  I can't see
a difference.  That's one client on a KASAN kernel, so I wouldn't lean on
it too hard.

What this doesn't do
--------------------

Peers are told apart by address.  Everything behind one address shares
one client's turns: NAT, a gateway re-exporting for a lot of users, pods
behind a node address.  Chuck raised this on [1] and I don't have an
answer for v3.  For v4.1 the clientid could be the key, but the transport
would have to move between clients at session bind and I left that out.

Fairness is per pool.  With ten pools (pool_mode=percpu) a client with 2
connections against one with 16 got 26% where a single pool gives 49%.
v7.2 gives it 12% there.  Now that pools are per node this matters
more than it used to.

It equalizes dispatches, not thread time.  A client whose requests each
hold a thread for a long time still ends up holding most of the threads.

It's fair by host, not by class.  Six movers get six turns.

A single connection with 16 requests outstanding gets about a third
against a client with 8 connections in my synthetic harness, not half.
From tracing, its transport is dispatched within about 150us of becoming
ready and alternates with the other client, so I think that's my load
generator not keeping one connection supplied.  I haven't measured the
share a real single-connection client gets when it's backlogged.

Questions
---------

Is the anonymous client the right home for listeners?  A connection flood
gets one turn per round that way.

Does anyone want the pool_stats or a tracepoint to show clients?  I left
observability out to keep this small.

[1] https://lore.kernel.org/linux-nfs/cover.1780498019.git.bcodding@hammerspace.com/
[2] https://lore.kernel.org/linux-nfs/cover.1782314746.git.bcodding@hammerspace.com/
[3] https://lore.kernel.org/linux-nfs/5b162e1a03d59bd0d3cc479891965834d0d4f0c8.1790948398.git.bcodding@hammerspace.com/

Benjamin Coddington (2):
  SUNRPC: track service clients by peer address
  SUNRPC: dispatch ready transports round-robin across clients

 include/linux/sunrpc/svc.h      |  34 +++++-
 include/linux/sunrpc/svc_xprt.h |   1 +
 net/sunrpc/svc.c                |  36 +++++-
 net/sunrpc/svc_xprt.c           | 191 ++++++++++++++++++++++++++++----
 4 files changed, 240 insertions(+), 22 deletions(-)

-- 
2.53.0

[PATCH RFC 1/2] SUNRPC: track service clients by peer address

[Benjamin Coddington]

A pool dispatches its ready transports in FIFO order, one RPC per turn,
so a peer's share of the service grows with the number of connections
it holds: a client with K connections is served K times as often as a
client with one.  Dispatching per peer instead needs the service to know
which transports belong to the same peer.

Add struct svc_client, one per peer address and network namespace of a
service, found through a small hash table under a spinlock.  The table
is touched only when a connection is accepted and when a client's last
transport is freed; dispatch will follow the transport's xpt_client
pointer, which is set before the transport can be enqueued (it is still
XPT_BUSY from svc_xprt_init()) and is kept alive by the transport's
reference.  Each client carries a per-pool lwq of its ready transports
and a node for the pool's queue of clients, unused as yet.

Listeners, UDP sockets, and accepted transports for which no client can
be allocated are bound to the service's anonymous client.

Dispatch is unchanged by this patch.

Suggested-by: NeilBrown <neil@brown.name>
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 include/linux/sunrpc/svc.h      | 32 +++++++++++
 include/linux/sunrpc/svc_xprt.h |  1 +
 net/sunrpc/svc.c                | 34 ++++++++++++
 net/sunrpc/svc_xprt.c           | 97 +++++++++++++++++++++++++++++++++
 4 files changed, 164 insertions(+)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index bdff8ccb92c7..496fd33da96f 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -58,6 +58,33 @@ enum {
 	SP_TASK_STARTING,	/* Task has started but not added to idle yet */
 };
 
+/*
+ * One peer of a service: the transports from one address in one network
+ * namespace.  Ready transports are queued per client and per pool, and a
+ * pool dispatches round-robin across its queued clients, so a peer's share
+ * of the service does not grow with its connection count.
+ */
+struct svc_client_pool {
+	struct lwq		cp_xprts;	/* ready transports */
+	struct lwq_node		cp_ready;	/* link in svc_pool.sp_clients */
+	unsigned long		cp_flags;
+	struct svc_client	*cp_client;
+};
+
+enum {
+	SVC_CP_QUEUED,	/* on sp_clients, or held by the thread that took it */
+};
+
+struct svc_client {
+	struct hlist_node	cl_hash;
+	refcount_t		cl_ref;
+	struct net		*cl_net;
+	struct sockaddr_storage	cl_addr;	/* peer address; port ignored */
+	struct svc_client_pool	cl_pool[];	/* one per pool */
+};
+
+#define SVC_CLIENT_HASH_BITS	8
+
 
 struct svc_rqst;
 
@@ -94,6 +121,10 @@ struct svc_serv {
 	struct svc_pool *	sv_pools;	/* array of thread pools */
 	int			(*sv_threadfn)(void *data);
 
+	spinlock_t		sv_client_lock;	/* protects sv_client_hash */
+	struct hlist_head	sv_client_hash[1 << SVC_CLIENT_HASH_BITS];
+	struct svc_client	*sv_anon_client; /* transports without a peer */
+
 #if defined(CONFIG_SUNRPC_BACKCHANNEL)
 	struct lwq		sv_cb_list;	/* queue for callback requests
 						 * that arrive over the same
@@ -500,6 +531,7 @@ void		   svc_reserve(struct svc_rqst *rqstp, int space);
 void		   svc_pool_wake_idle_thread(struct svc_pool *pool);
 struct svc_pool   *svc_pool_for_cpu(struct svc_serv *serv);
 unsigned int	   svc_serv_nrpools(const struct svc_serv *serv);
+struct svc_client *svc_client_alloc(unsigned int nrpools, gfp_t gfp);
 char *		   svc_print_addr(struct svc_rqst *, char *, size_t);
 const char *	   svc_proc_name(const struct svc_rqst *rqstp);
 int		   svc_encode_result_payload(struct svc_rqst *rqstp,
diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 7176c42f19d7..ee4dd0d3647c 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -63,6 +63,7 @@ struct svc_xprt {
 	unsigned long		xpt_flags;
 
 	struct svc_serv		*xpt_server;	/* service for transport */
+	struct svc_client	*xpt_client;	/* peer this transport belongs to */
 	atomic_t		xpt_reserved;	/* outq space rsvd, UDP only */
 	atomic_t		xpt_nr_rqsts;	/* Number of requests */
 	struct mutex		xpt_mutex;	/* to serialize sending data */
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 0a2c040ad096..cb2b7caaa627 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -387,6 +387,30 @@ static void svc_pool_destroy_counters(struct svc_pool *pool)
 	percpu_counter_destroy(&pool->sp_threads_woken);
 }
 
+/**
+ * svc_client_alloc - allocate a service client
+ * @nrpools: number of thread pools in the service
+ * @gfp: allocation flags
+ *
+ * Return: the client, holding one reference, or %NULL.
+ */
+struct svc_client *svc_client_alloc(unsigned int nrpools, gfp_t gfp)
+{
+	struct svc_client *cl;
+	unsigned int i;
+
+	cl = kzalloc(struct_size(cl, cl_pool, nrpools), gfp);
+	if (!cl)
+		return NULL;
+	refcount_set(&cl->cl_ref, 1);
+	INIT_HLIST_NODE(&cl->cl_hash);
+	for (i = 0; i < nrpools; i++) {
+		lwq_init(&cl->cl_pool[i].cp_xprts);
+		cl->cl_pool[i].cp_client = cl;
+	}
+	return cl;
+}
+
 /*
  * Create an RPC service
  */
@@ -432,8 +456,16 @@ __svc_create(struct svc_program *prog, int nprogs, struct svc_stat *stats,
 
 	__svc_init_bc(serv);
 
+	spin_lock_init(&serv->sv_client_lock);
+	serv->sv_anon_client = svc_client_alloc(npools, GFP_KERNEL);
+	if (!serv->sv_anon_client) {
+		kfree(serv);
+		return NULL;
+	}
+
 	serv->sv_pools = kzalloc_objs(struct svc_pool, npools);
 	if (!serv->sv_pools) {
+		kfree(serv->sv_anon_client);
 		kfree(serv);
 		return NULL;
 	}
@@ -459,6 +491,7 @@ __svc_create(struct svc_program *prog, int nprogs, struct svc_stat *stats,
 	while (i--)
 		svc_pool_destroy_counters(&serv->sv_pools[i]);
 	kfree(serv->sv_pools);
+	kfree(serv->sv_anon_client);
 	kfree(serv);
 	return NULL;
 }
@@ -545,6 +578,7 @@ svc_destroy(struct svc_serv **servp)
 	if (serv->sv_is_pooled)
 		svc_pool_map_put();
 
+	kfree(serv->sv_anon_client);
 	kfree(serv->sv_pools);
 	kfree(serv);
 }
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index c891532fb1f6..51753aeeb0ba 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -9,8 +9,11 @@
 #include <linux/sched/mm.h>
 #include <linux/errno.h>
 #include <linux/freezer.h>
+#include <linux/hash.h>
 #include <linux/slab.h>
 #include <net/sock.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <linux/sunrpc/addr.h>
 #include <linux/sunrpc/stats.h>
 #include <linux/sunrpc/svc_xprt.h>
@@ -167,6 +170,96 @@ void svc_xprt_deferred_close(struct svc_xprt *xprt)
 }
 EXPORT_SYMBOL_GPL(svc_xprt_deferred_close);
 
+static unsigned int svc_client_hash(const struct sockaddr *sa,
+				    const struct net *net)
+{
+	u32 h = hash_ptr(net, 32);
+
+	switch (sa->sa_family) {
+	case AF_INET:
+		h = __ipv4_addr_hash(((const struct sockaddr_in *)sa)->sin_addr.s_addr, h);
+		break;
+	case AF_INET6:
+		h = __ipv6_addr_jhash(&((const struct sockaddr_in6 *)sa)->sin6_addr, h);
+		break;
+	}
+	return hash_32(h, SVC_CLIENT_HASH_BITS);
+}
+
+static struct svc_client *svc_client_get(struct svc_client *cl)
+{
+	refcount_inc(&cl->cl_ref);
+	return cl;
+}
+
+static void svc_client_put(struct svc_serv *serv, struct svc_client *cl)
+{
+	if (!refcount_dec_and_test(&cl->cl_ref))
+		return;
+	spin_lock_bh(&serv->sv_client_lock);
+	hlist_del(&cl->cl_hash);
+	spin_unlock_bh(&serv->sv_client_lock);
+	kfree(cl);
+}
+
+/* Caller holds sv_client_lock. */
+static struct svc_client *svc_client_find(struct hlist_head *head,
+					  const struct net *net,
+					  const struct sockaddr *sa)
+{
+	struct svc_client *cl;
+
+	hlist_for_each_entry(cl, head, cl_hash)
+		if (cl->cl_net == net &&
+		    rpc_cmp_addr((struct sockaddr *)&cl->cl_addr, sa) &&
+		    refcount_inc_not_zero(&cl->cl_ref))
+			return cl;
+	return NULL;
+}
+
+/*
+ * Bind @xprt to the client for its peer address.  Runs once per accepted
+ * transport, in process context, while the transport is still XPT_BUSY
+ * from svc_xprt_init(), so no enqueue can see the pointer change.  A
+ * transport without a usable peer address, or one for which no client can
+ * be allocated, stays on the service's anonymous client.
+ */
+static void svc_client_bind(struct svc_serv *serv, struct svc_xprt *xprt)
+{
+	struct sockaddr *sa = (struct sockaddr *)&xprt->xpt_remote;
+	struct svc_client *cl, *new = NULL;
+	struct net *net = xprt->xpt_net;
+	struct hlist_head *head;
+
+	if (sa->sa_family != AF_INET && sa->sa_family != AF_INET6)
+		return;
+	head = &serv->sv_client_hash[svc_client_hash(sa, net)];
+
+	spin_lock_bh(&serv->sv_client_lock);
+	cl = svc_client_find(head, net, sa);
+	spin_unlock_bh(&serv->sv_client_lock);
+	if (!cl) {
+		new = svc_client_alloc(svc_serv_nrpools(serv), GFP_KERNEL);
+		if (!new)
+			return;
+		new->cl_net = net;
+		memcpy(&new->cl_addr, sa, xprt->xpt_remotelen);
+
+		spin_lock_bh(&serv->sv_client_lock);
+		cl = svc_client_find(head, net, sa);
+		if (!cl) {
+			hlist_add_head(&new->cl_hash, head);
+			cl = new;
+			new = NULL;
+		}
+		spin_unlock_bh(&serv->sv_client_lock);
+		kfree(new);
+	}
+
+	svc_client_put(serv, xprt->xpt_client);
+	xprt->xpt_client = cl;
+}
+
 static void svc_xprt_free(struct kref *kref)
 {
 	struct svc_xprt *xprt =
@@ -176,6 +269,7 @@ static void svc_xprt_free(struct kref *kref)
 	trace_svc_xprt_free(xprt);
 	if (test_bit(XPT_CACHE_AUTH, &xprt->xpt_flags))
 		svcauth_unix_info_release(xprt);
+	svc_client_put(xprt->xpt_server, xprt->xpt_client);
 	put_cred(xprt->xpt_cred);
 	put_net_track(xprt->xpt_net, &xprt->ns_tracker);
 	/* See comment on corresponding get in xs_setup_bc_tcp(): */
@@ -213,6 +307,7 @@ void svc_xprt_init(struct net *net, struct svc_xprt_class *xcl,
 	xprt->xpt_ops = xcl->xcl_ops;
 	kref_init(&xprt->xpt_ref);
 	xprt->xpt_server = serv;
+	xprt->xpt_client = svc_client_get(serv->sv_anon_client);
 	INIT_LIST_HEAD(&xprt->xpt_list);
 	INIT_LIST_HEAD(&xprt->xpt_deferred);
 	INIT_LIST_HEAD(&xprt->xpt_users);
@@ -836,6 +931,8 @@ static bool svc_thread_wait_for_work(struct svc_rqst *rqstp, long timeo)
 
 static void svc_add_new_temp_xprt(struct svc_serv *serv, struct svc_xprt *newxpt)
 {
+	svc_client_bind(serv, newxpt);
+
 	spin_lock_bh(&serv->sv_lock);
 	set_bit(XPT_TEMP, &newxpt->xpt_flags);
 	list_add(&newxpt->xpt_list, &serv->sv_tempsocks);
-- 
2.53.0

[PATCH RFC 2/2] SUNRPC: dispatch ready transports round-robin across clients

[Benjamin Coddington]

A pool keeps its ready transports in one FIFO and serves one RPC per
turn, so a peer is served in proportion to how many of its connections
are backlogged.  A client with K connections, or a deep NFSv4.1 slot
table spread over nconnect transports, takes K turns for every one a
single-connection client gets, and the single-connection client's
latency grows with everyone else's connection count.

Queue ready transports on their client instead, and have the pool
dispatch round-robin across clients: take the client at the head of
the pool's client queue, dispatch one of its transports, and put the
client back at the tail if it has more.  Every peer with work queued
gets one dispatch per round however many connections it holds, and a
peer's wait is bounded by the number of busy peers rather than by the
number of busy connections.  Within a client, transports are still
served in FIFO order.

Enqueue stays lockless: the transport goes on the client's per-pool
lwq, and the client goes on the pool's lwq the first time one of its
transports becomes ready, guarded by a per-pool SVC_CP_QUEUED bit that
the dequeuing thread clears when it finds the client empty.  The bit is
cleared before the client's queue is re-tested, so an enqueue that saw
it set and did not queue the client is caught by the re-test.  Dequeue
takes one more lwq spinlock than before.  Control events (XPT_CONN,
XPT_CLOSE, XPT_HANDSHAKE) take turns like data: a close queued behind
k of its own client's transports is dispatched k rounds later, where
it used to wait behind every queued transport in the pool.

Network namespace teardown walks the pool's clients: those of the
namespace being destroyed are drained and their transports deleted; the
anonymous client, which may hold listeners from several namespaces, is
filtered per transport; the rest are re-queued and a thread is woken.

Peers are told apart by address, so everything behind one address
(NAT, several users on one host) shares one client's turns.

Suggested-by: NeilBrown <neil@brown.name>
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 include/linux/sunrpc/svc.h |  2 +-
 net/sunrpc/svc.c           |  2 +-
 net/sunrpc/svc_xprt.c      | 94 ++++++++++++++++++++++++++++++--------
 3 files changed, 76 insertions(+), 22 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 496fd33da96f..6b8d9292cfb8 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -38,7 +38,7 @@ struct svc_pool {
 	unsigned int		sp_nrthreads;	/* # of threads currently running in pool */
 	unsigned int		sp_nrthrmin;	/* Min number of threads to run per pool */
 	unsigned int		sp_nrthrmax;	/* Max requested number of threads in pool */
-	struct lwq		sp_xprts;	/* pending transports */
+	struct lwq		sp_clients;	/* clients with ready transports */
 	struct list_head	sp_all_threads;	/* all server threads */
 	struct llist_head	sp_idle_threads; /* idle server threads */
 
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index cb2b7caaa627..4adf113d067b 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -477,7 +477,7 @@ __svc_create(struct svc_program *prog, int nprogs, struct svc_stat *stats,
 				i, serv->sv_name);
 
 		pool->sp_id = i;
-		lwq_init(&pool->sp_xprts);
+		lwq_init(&pool->sp_clients);
 		INIT_LIST_HEAD(&pool->sp_all_threads);
 		init_llist_head(&pool->sp_idle_threads);
 
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 51753aeeb0ba..93cbbbc8b766 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -616,6 +616,7 @@ static bool svc_xprt_ready(struct svc_xprt *xprt)
  */
 void svc_xprt_enqueue(struct svc_xprt *xprt)
 {
+	struct svc_client_pool *cp;
 	struct svc_pool *pool;
 
 	if (!svc_xprt_ready(xprt))
@@ -630,25 +631,55 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 		return;
 
 	pool = svc_pool_for_cpu(xprt->xpt_server);
+	cp = &xprt->xpt_client->cl_pool[pool->sp_id];
 
 	percpu_counter_inc(&pool->sp_sockets_queued);
 	xprt->xpt_qtime = ktime_get();
-	lwq_enqueue(&xprt->xpt_ready, &pool->sp_xprts);
+	lwq_enqueue(&xprt->xpt_ready, &cp->cp_xprts);
+	if (!test_and_set_bit(SVC_CP_QUEUED, &cp->cp_flags))
+		lwq_enqueue(&cp->cp_ready, &pool->sp_clients);
 
 	svc_pool_wake_idle_thread(pool);
 }
 EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
 
 /*
- * Dequeue the first transport, if there is one.
+ * Put @cp back on the pool's client queue if it still has ready
+ * transports.  Otherwise clear SVC_CP_QUEUED and look once more: an
+ * enqueue that found the bit set has left the client for us to queue.
+ */
+static void svc_client_pool_requeue(struct svc_pool *pool,
+				    struct svc_client_pool *cp)
+{
+	if (!lwq_empty(&cp->cp_xprts)) {
+		lwq_enqueue(&cp->cp_ready, &pool->sp_clients);
+		return;
+	}
+	clear_bit(SVC_CP_QUEUED, &cp->cp_flags);
+	smp_mb__after_atomic();
+	if (!lwq_empty(&cp->cp_xprts) &&
+	    !test_and_set_bit(SVC_CP_QUEUED, &cp->cp_flags))
+		lwq_enqueue(&cp->cp_ready, &pool->sp_clients);
+}
+
+/*
+ * Dequeue the next transport: one from the client at the head of the
+ * pool's client queue, which then goes to the tail if it has more.
  */
 static struct svc_xprt *svc_xprt_dequeue(struct svc_pool *pool)
 {
-	struct svc_xprt	*xprt = NULL;
+	struct svc_client_pool *cp;
+	struct svc_xprt	*xprt;
 
-	xprt = lwq_dequeue(&pool->sp_xprts, struct svc_xprt, xpt_ready);
-	if (xprt)
-		svc_xprt_get(xprt);
+	do {
+		cp = lwq_dequeue(&pool->sp_clients, struct svc_client_pool,
+				 cp_ready);
+		if (!cp)
+			return NULL;
+		xprt = lwq_dequeue(&cp->cp_xprts, struct svc_xprt, xpt_ready);
+		svc_client_pool_requeue(pool, cp);
+	} while (!xprt);
+	svc_xprt_get(xprt);
 	return xprt;
 }
 
@@ -875,7 +906,7 @@ svc_thread_should_sleep(struct svc_rqst *rqstp)
 		return false;
 
 	/* was a socket queued? */
-	if (!lwq_empty(&pool->sp_xprts))
+	if (!lwq_empty(&pool->sp_clients))
 		return false;
 
 	/* are we shutting down? */
@@ -1325,26 +1356,49 @@ static int svc_close_list(struct svc_serv *serv, struct list_head *xprt_list, st
 	return ret;
 }
 
-static void svc_clean_up_xprts(struct svc_serv *serv, struct net *net)
+/*
+ * Delete the ready transports of @net that are queued on @cp; the rest go
+ * back in their order.
+ */
+static void svc_client_pool_clean(struct svc_client_pool *cp, struct net *net)
 {
+	struct llist_node *q, **t1, *t2;
 	struct svc_xprt *xprt;
+
+	q = lwq_dequeue_all(&cp->cp_xprts);
+	lwq_for_each_safe(xprt, t1, t2, &q, xpt_ready) {
+		if (xprt->xpt_net == net) {
+			set_bit(XPT_CLOSE, &xprt->xpt_flags);
+			svc_delete_xprt(xprt);
+			xprt = NULL;
+		}
+	}
+	if (q)
+		lwq_enqueue_batch(q, &cp->cp_xprts);
+}
+
+static void svc_clean_up_xprts(struct svc_serv *serv, struct net *net)
+{
 	int i;
 
 	for (i = 0; i < svc_serv_nrpools(serv); i++) {
 		struct svc_pool *pool = &serv->sv_pools[i];
-		struct llist_node *q, **t1, *t2;
-
-		q = lwq_dequeue_all(&pool->sp_xprts);
-		lwq_for_each_safe(xprt, t1, t2, &q, xpt_ready) {
-			if (xprt->xpt_net == net) {
-				set_bit(XPT_CLOSE, &xprt->xpt_flags);
-				svc_delete_xprt(xprt);
-				xprt = NULL;
-			}
+		struct svc_client_pool *cp;
+		struct svc_client *cl;
+		struct llist_node *q;
+
+		q = lwq_dequeue_all(&pool->sp_clients);
+		while (q) {
+			cp = container_of(q, struct svc_client_pool, cp_ready.node);
+			q = q->next;
+			/* Deleting the last transport frees the client. */
+			cl = svc_client_get(cp->cp_client);
+			if (cl->cl_net == net || cl == serv->sv_anon_client)
+				svc_client_pool_clean(cp, net);
+			svc_client_pool_requeue(pool, cp);
+			svc_client_put(serv, cl);
 		}
-
-		if (q && lwq_enqueue_batch(q, &pool->sp_xprts))
-			svc_pool_wake_idle_thread(pool);
+		svc_pool_wake_idle_thread(pool);
 	}
 }
 
-- 
2.53.0

Re: [PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Chuck Lever]

Hi Ben -

On Fri, Oct 2, 2026, at 12:42 PM, Benjamin Coddington wrote:
> This is a third pass at the problem from [1] and [2].  It's the design
> Neil described on [1], close to as he wrote it: a client object per peer,
> a queue of ready transports per client, and a queue of ready clients per
> pool.  A pool takes turns across clients instead of across transports.

I read through your patches and did not find a problem with the
SVC_CP_QUEUED handshake or with the client's lifetime. My concern
is with what the pool takes turns across, and with a few of the
results.


> Each transport belongs to a client, one per peer address and network
> namespace.

> The one piece of Neil's description I left out is moving a transport
> between clients.

I think that piece has to come back.

svc_client_find() matches on the full peer address, so each source
address gets its own turn. A host with N addresses is served N times
per round. That is the curve from your May numbers with the address
count in place of the connection count.


> It's fair by host, not by class.  Six movers get six turns.

It is fair by address. That covers more than six movers:

 - A multi-homed data mover gets a turn for each interface it
   connects from.

 - An NFSv4.1 client that trunks a session over N of its own
   addresses is one clientid and N svc_clients. It gets the larger
   share without asking for it.

 - An IPv6 host can connect from as many addresses in its prefix as
   it likes.

Meanwhile a NAT or a re-export gateway with hundreds of users behind
it gets one turn. A client with many users is held to one turn and a
client with many addresses is not. The multi-homed client case is
not something we can easily ignore.

For the next version, please add a case to the harness where one host
drives load from N source addresses against a single-address client.


> Chuck asked on [2] for something real rather than the synthetic victim.

>                     alone (A / B)     loaded A    loaded B
>     walk            1.53s / 2.08s      328.55s     107.74s
>     fio 4k IOPS     21876 / 19776           80         777

The "alone" column worries me more than the loaded ones. With no
aggressor, the walk takes 36% longer on B and fio loses about 10%.
A single client with one connection pays for one more lwq dequeue
and a few atomic operations per dispatch. That's a regression even
Linus will notice.


> For the cost on the dispatch path I used fio over a loopback mount of a
> tmpfs export, 4k O_DIRECT reads, nconnect=8, five 20s runs each: 95.5k
> vs 95.8k IOPS at 16 jobs and 139.0k vs 139.2k at 64, A vs B.  I can't see
> a difference.  That's one client on a KASAN kernel, so I wouldn't lean on
> it too hard.

That result and the "alone" column disagree. Can you repeat both on a
kernel without KASAN, with enough runs to show the variance? If the
unloaded slowdown is real, we need to know where it comes from.


> Fairness is per pool.  With ten pools (pool_mode=percpu) a client with 2
> connections against one with 16 got 26% where a single pool gives 49%.
> v7.2 gives it 12% there.  Now that pools are per node this matters
> more than it used to.

Any server with more than one NUMA node gets the diluted result by
default. I would like to see numbers for a two-node per node
configuration, and your thoughts on whether the client queue belongs
to the pool at all.


> A single connection with 16 requests outstanding gets about a third
> against a client with 8 connections in my synthetic harness, not half.
> From tracing, its transport is dispatched within about 150us of becoming
> ready and alternates with the other client, so I think that's my load
> generator not keeping one connection supplied.

XPT_BUSY might account for part of that. Once a thread dequeues
the client's only transport, the client is off sp_clients until
svc_xprt_received() enqueues that transport again. The 8-connection
client goes back on the tail immediately. A thread that comes free
in that window takes the other client. The window is one recvfrom,
so it matters only when several threads come free together. The
fixed 10ms service time makes them do that. Your first RFC's cover
letter attributed the same shortfall to XPT_BUSY.

Jittering the injected service time would show how much of the
shortfall comes from the harness. Counting consecutive dispatches
of the same client in the trace might show the rest.


> Control events take turns
> like data.  A close queued behind k of its own client's transports gets
> dispatched k rounds later, where today it waits behind every queued
> transport in the pool.  I didn't add a separate queue for them.  I can if
> that's wanted.

A round is as long as the number of ready clients. With k transports
queued ahead of it and M clients ready, the close waits about k * M
dispatches. For a client with many connections on a server with many
busy clients, that is longer than the wait in today's FIFO. Those are
the connections NFSD most wants to close promptly. Let's dispatch
XPT_CONN, XPT_CLOSE, and XPT_HANDSHAKE ahead of XPT_DATA within a
client.


> Is the anonymous client the right home for listeners?  A connection flood
> gets one turn per round that way.

I think so.


> Does anyone want the pool_stats or a tracepoint to show clients?  I left
> observability out to keep this small.

Yes, please. A tracepoint at dequeue that reports the client is
enough for now.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)

Re: [PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Daire Byrne]

Hi,

I can't give much critique of the approach, but I thought it might be
useful to add an "end user" story point.

We use NFS re-export servers/gateways heavily, and one of the big
issues we come up against time and again, is when the re-export
gateway starts doing bulky reads from the source server (which then
get cached to disk by FS-Cache).

When this happens, lots of the GETATTR and LOOKUP calls get a back
seat which then affects all the clients who are asking for data that
is already in the re-export server's page cache or FS-cache disk
backend. But they can't have the data served to them before first
checking the source file for changes (and we run with very long
actimeo=3600).

This is similar to your data movers versus filesystem walking fairness
tests. Except here it is knfsd thread processes on a single
client/server host.

We use nconnect, set tcp_slot_table_entries=256 and use
svc_rpc_per_connection_limit=4 in an attempt to allow for more
requests in flight (NFSv3) and limit greedy clients. But just a
handful of greedy clients (of the re-export server) can cause
starvation such that the re-export server doesn't service the metadata
lookups required to serve the already cached data to other clients in
a timely manner. It's annoying, the data is cached, ready and waiting,
but we can't serve it until we check for changes (there are none - we
rarely overwrite files).

Now I understand that these patches don't affect the re-export
server's connection to the upstream servers, but I am interested to
see what effect they have on the re-export server's clients. If we can
give a more equal share of requests to the clients does that in turn
reduce the re-export server bottleneck? Do those greedy readers reduce
their rops/s such that the re-export server doesn't flood the
connection to the backend server with bulky read requests?

I was going to test the patches but I got as far as they don't apply
cleanly to v7.2 and then got caught up in other things.

Anyway, I hope that's useful.

BTW - in case anyone is interested, AWS have put together a repo for
"re-export" servers: https://github.com/awslabs/knfsd-file-cache

Daire


On Fri, 2 Oct 2026 at 17:42, Benjamin Coddington
<ben.coddington@hammerspace.com> wrote:
>
> This is a third pass at the problem from [1] and [2].  It's the design
> Neil described on [1], close to as he wrote it: a client object per peer,
> a queue of ready transports per client, and a queue of ready clients per
> pool.  A pool takes turns across clients instead of across transports.
>
> I owe an explanation for leaving sparse-flow behind.  Two things:
>
> The re-arm trigger Chuck asked about on [2] is a cliff wherever it's put,
> and softening it means picking between watermarks and decaying credits
> against real interactive workloads under load.  I ran some of that and
> found the behavior depends on timing from several actors at once.  I
> don't think I can characterize it well enough to defend a heuristic.
>
> ..and Neil's objection on [1] holds: the interactive frame only works
> for clients with one or a few users.  A latency floor helps a client
> while it's idle between requests.  What I have is a data mover with a
> lot of connections sharing a server with clients that keep I/O in flight
> themselves, a re-export gateway for one.  Sparse-flow puts those in the
> same queue as the mover and they split the pool by connection count.
> Neil asked then whether fairness between a client with one connection
> and one with sixteen wasn't what I wanted -- but it is.
>
> Approach
> --------
>
> Each transport belongs to a client, one per peer address and network
> namespace.  A client has a queue of its ready transports, and the pool
> has a queue of clients that have something ready.  A thread takes the
> client at the head, dispatches one of its transports, and puts the client
> back at the tail if it has more.  So every peer with work queued gets one
> dispatch per round, however many connections it holds.
>
> Enqueue is still lockless.  Dequeue takes one more lwq lock than before.
> The client table is only touched when a connection is accepted and when a
> client's last transport goes away, so there's no lookup on the dispatch
> path and no RCU.  Listeners and UDP sockets share an anonymous client.
> The one piece of Neil's description I left out is moving a transport
> between clients.
>
> It's always on and there's nothing to tune.
>
>   patch 1  track clients by peer address, no change to dispatch
>   patch 2  dispatch round-robin across clients
>
> These go on top of the svc_clean_up_xprts() wake fix I sent separately
> [3], on nfsd-testing.  That's the pre-existing issue Chuck asked me to
> look at on [2].  I've only compiled them on nfsd-testing; the numbers
> below are from v7.2.
>
> What happened to the review items from [2]: there's no trigger and no
> credit to tune, nothing is classified as batch or interactive, and there's
> no priority tier, so nothing can be starved.  Control events take turns
> like data.  A close queued behind k of its own client's transports gets
> dispatched k rounds later, where today it waits behind every queued
> transport in the pool.  I didn't add a separate queue for them.  I can if
> that's wanted.
>
> Results
> -------
>
> Same harness as [2], with one change: every load group now comes from its
> own source address, so the server sees it as one client.  16 threads, 10ms
> injected per op by the same test-only hook (not part of this series).  A
> is v7.2 with the hook, and B adds the wake fix [3] and this series.
> NFSv3 burst completion p50 in ms.  NFSv4.1 is within a few percent in
> every cell.
>
> Interactive burst of 32 against one busy client with K connections
> (unobstructed floor 45.8ms):
>
>     K       4      8     16     32
>     A    82.8  249.0  439.9  838.9
>     B    73.5   90.1   94.6   90.0
>
> That's two clients taking turns, so about twice the floor, and it doesn't
> move with K.  Against burst size N at K=16 it stays about twice the floor
> too.  There's no knee any more:
>
>     N        1      8     32     64     96    128
>    floor  14.9   31.6   45.8   72.5   98.6  123.6
>     A     31.6  125.3  440.0  865.8 1290.1 1705.3
>     B     18.0   40.2   91.9  151.3  217.4  278.0
>
> What it does scale with is the number of busy clients.  M clients with 4
> connections each, burst of 32:
>
>     M       1      2      4      8
>     A    80.9  245.3  441.1  839.0
>     B    75.0  112.5  158.3  252.3
>
> That's the cost of dropping the priority tier.  A light client on
> a server with a lot of busy peers waits its turn behind each of them.  It
> still beats waiting behind each of their connections.
>
> Share of the pool for a client with 2 connections against a client with K,
> both backlogged:
>
>     K        4      8     16     32
>     A    46.3%  19.9%  11.1%   5.9%
>     B    49.4%  47.6%  47.9%  48.4%
>
> Six movers, against clients that are backlogged too.  Share for one
> client with 4 connections, and for four such clients together:
>
>                          movers at 4 conns    movers at 8 conns
>                             A        B           A        B
>     one client           14.3%    14.3%        7.7%    14.3%
>     four clients         40.0%    40.0%       25.0%    40.0%
>     one client, 1 conn    4.0%    13.0%        2.0%    13.0%
>
> A client that already matches the movers' connection count sees no change.
> What changes is that the movers can't buy more by opening more.
>
> Chuck asked on [2] for something real rather than the synthetic victim.
> A kernel v3 mount with one connection (noac, lookupcache=none) of a tmpfs
> export, walking 2000 files with find | xargs stat (about 26k RPCs) and then
> reading with four fio jobs, while the 16-connection aggressor runs from
> another address:
>
>                     alone (A / B)     loaded A    loaded B
>     walk            1.53s / 2.08s      328.55s     107.74s
>     fio 4k IOPS     21876 / 19776           80         777
>
> Both loaded columns are worse than a real mix would be.  With every
> request taking exactly 10ms the threads finish in batches, and a request
> that shows up mid-batch waits for the next one.
>
> Aggregate throughput is the same A and B in every saturated cell (about
> 1280 vs 1290 ops/s).  With every group on one source address B gives A's
> numbers back, which is what I'd expect.
>
> For the cost on the dispatch path I used fio over a loopback mount of a
> tmpfs export, 4k O_DIRECT reads, nconnect=8, five 20s runs each: 95.5k
> vs 95.8k IOPS at 16 jobs and 139.0k vs 139.2k at 64, A vs B.  I can't see
> a difference.  That's one client on a KASAN kernel, so I wouldn't lean on
> it too hard.
>
> What this doesn't do
> --------------------
>
> Peers are told apart by address.  Everything behind one address shares
> one client's turns: NAT, a gateway re-exporting for a lot of users, pods
> behind a node address.  Chuck raised this on [1] and I don't have an
> answer for v3.  For v4.1 the clientid could be the key, but the transport
> would have to move between clients at session bind and I left that out.
>
> Fairness is per pool.  With ten pools (pool_mode=percpu) a client with 2
> connections against one with 16 got 26% where a single pool gives 49%.
> v7.2 gives it 12% there.  Now that pools are per node this matters
> more than it used to.
>
> It equalizes dispatches, not thread time.  A client whose requests each
> hold a thread for a long time still ends up holding most of the threads.
>
> It's fair by host, not by class.  Six movers get six turns.
>
> A single connection with 16 requests outstanding gets about a third
> against a client with 8 connections in my synthetic harness, not half.
> From tracing, its transport is dispatched within about 150us of becoming
> ready and alternates with the other client, so I think that's my load
> generator not keeping one connection supplied.  I haven't measured the
> share a real single-connection client gets when it's backlogged.
>
> Questions
> ---------
>
> Is the anonymous client the right home for listeners?  A connection flood
> gets one turn per round that way.
>
> Does anyone want the pool_stats or a tracepoint to show clients?  I left
> observability out to keep this small.
>
> [1] https://lore.kernel.org/linux-nfs/cover.1780498019.git.bcodding@hammerspace.com/
> [2] https://lore.kernel.org/linux-nfs/cover.1782314746.git.bcodding@hammerspace.com/
> [3] https://lore.kernel.org/linux-nfs/5b162e1a03d59bd0d3cc479891965834d0d4f0c8.1790948398.git.bcodding@hammerspace.com/
>
> Benjamin Coddington (2):
>   SUNRPC: track service clients by peer address
>   SUNRPC: dispatch ready transports round-robin across clients
>
>  include/linux/sunrpc/svc.h      |  34 +++++-
>  include/linux/sunrpc/svc_xprt.h |   1 +
>  net/sunrpc/svc.c                |  36 +++++-
>  net/sunrpc/svc_xprt.c           | 191 ++++++++++++++++++++++++++++----
>  4 files changed, 240 insertions(+), 22 deletions(-)
>
> --
> 2.53.0
>

Re: [PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Benjamin Coddington]

On 5 Oct 2026, at 8:18, Daire Byrne wrote:
> We use nconnect, set tcp_slot_table_entries=256 and use
> svc_rpc_per_connection_limit=4 in an attempt to allow for more
> requests in flight (NFSv3) and limit greedy clients.

Hey Daire - that limit is per connection, so a client's nconnect multiplies
it.  I suspect what you really want is the limit per client.  That isn't in
these patches, but with the svc_client added here there's an object to hang
it on.

> Now I understand that these patches don't affect the re-export
> server's connection to the upstream servers, but I am interested to
> see what effect they have on the re-export server's clients. If we can
> give a more equal share of requests to the clients does that in turn
> reduce the re-export server bottleneck? Do those greedy readers reduce
> their rops/s such that the re-export server doesn't flood the
> connection to the backend server with bulky read requests?

I don't think it will, or not by much.  All this changes is which client
gets the next free nfsd thread.  It never leaves a thread idle to hold a
client back, so if the readers are the only ones with requests queued they
still get every thread.  And a READ that's waiting on the source server
keeps its thread the whole time.  The readers only give something up when
another client has a request waiting for a thread.

So it depends where your GETATTRs and LOOKUPs are stuck.  If they're
waiting for an nfsd thread on the re-export server, this should help.  If
they already have a thread and are waiting in the NFS client behind the
READs going to the source server, it won't, and that sounds more like what
you're describing.

You can tell which if you can catch it happening.  On the re-export server
the sunrpc:svc_xprt_dequeue tracepoint prints qtime-us, which is how long
the connection waited for a thread.  And mountstats on the mount of the
source server splits GETATTR and LOOKUP into backlog wait and RTT.  If
qtime is small while the clients are suffering then these patches aren't
going to help.

> I was going to test the patches but I got as far as they don't apply
> cleanly to v7.2 and then got caught up in other things.

Sorry, the posted ones are on Chuck's nfsd-testing.  Here they are on v7.2:

  https://github.com/bcodding/linux.git nfsd-clientq-7.2

It's three commits on top of v7.2: the svc_clean_up_xprts() wake fix and
the two from this RFC.  That's the tree the numbers in the cover letter
came from.

Ben

Re: [PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Benjamin Coddington]

On 4 Oct 2026, at 16:48, Chuck Lever wrote:
> I read through your patches and did not find a problem with the
> SVC_CP_QUEUED handshake or with the client's lifetime. My concern
> is with what the pool takes turns across, and with a few of the
> results.

Hey Chuck - thanks for going through it.  I'll take the results first and
come back to what it takes turns across, because I want to change
direction there.

> The "alone" column worries me more than the loaded ones. With no
> aggressor, the walk takes 36% longer on B and fio loses about 10%.
> ...
> That result and the "alone" column disagree. Can you repeat both on a
> kernel without KASAN, with enough runs to show the variance? If the
> unloaded slowdown is real, we need to know where it comes from.

Those are one run each, which I should have said.  It works out to about
20us per RPC in both the walk and the fio run.  That's a lot more than I
can explain with one more dequeue and a few atomics, so either it's noise
or there's something in there I don't understand yet.  I'll run them
properly without KASAN and find out which.

> Any server with more than one NUMA node gets the diluted result by
> default. I would like to see numbers for a two-node per node
> configuration, and your thoughts on whether the client queue belongs
> to the pool at all.

I'll get the two-node numbers.  I don't have a good answer for the second
part.  Getting it exactly fair across pools means a thread on one node
serving a transport that landed on the other, and I thought that's what
the pools are there to avoid.

> XPT_BUSY might account for part of that. Once a thread dequeues
> the client's only transport, the client is off sp_clients until
> svc_xprt_received() enqueues that transport again. The 8-connection
> client goes back on the tail immediately. A thread that comes free
> in that window takes the other client.

It accounts for nearly all of it.  I was wrong about the load generator.

I still had the trace, so I went back and looked at where the 8-connection
client's dequeues land.  The single connection was dispatched 1275 times
in 3 seconds and every one of those read a full RPC, so it always had a
request waiting.  The thread that took it had it back on the queue a
median 4us later (p90 79us).  Of the other client's 2656 dequeues, 1329
landed inside that window.  While the single connection was actually
queued, the other client got exactly one dequeue ahead of it 92.5% of the
time.  And the dequeues come in clumps, 69% of them within 50us of the one
before.

So the round-robin alternates like it should, and the single connection
loses a turn whenever threads come free faster than it can get back on the
queue.  I'll jitter the service time and see how much of it is left.

> A round is as long as the number of ready clients. With k transports
> queued ahead of it and M clients ready, the close waits about k * M
> dispatches.
> ...
> the connections NFSD most wants to close promptly. Let's dispatch
> XPT_CONN, XPT_CLOSE, and XPT_HANDSHAKE ahead of XPT_DATA within a
> client.

Yep, I compared rounds to transports.  Will do.

> Yes, please. A tracepoint at dequeue that reports the client is
> enough for now.

Ok.

> It is fair by address. That covers more than six movers:
> ...
> Meanwhile a NAT or a re-export gateway with hundreds of users behind
> it gets one turn. A client with many users is held to one turn and a
> client with many addresses is not. The multi-homed client case is
> not something we can easily ignore.

I agree with all of that.  My own mover is one of your examples, it's
NFSv3 and the hosts it runs on have more than one address.  I can bring
back moving the transport and key v4.1 on the clientid, but that does
nothing for v3, and a clientid is cheaper to come by than an address.

You said back in June that the scope of the identity is never going to
cover all the use cases, and I think three rounds of this have shown it.
You wanted a latency floor with no identity at all.  Neil wants it per
client.  Daire wants his workstations ahead of his render farm, and his
re-export servers are your gateway example.  I want all of my movers to
count as one.  I don't think there's an identity the kernel can pick
that's right for all of us, and every time I pick one I change behavior
for people who are happy with what they have today.

So here's what I'd like to try.  Keep the mechanism from this series, but
with nothing configured every transport stays on the anonymous client,
which dispatches in today's order (apart from the control events going
first).  That's the control run from the cover letter: with every load
group on one source address B gives A's numbers back.

Then let the admin say which peers share a turn.  A small table of address
prefixes, matched when the connection is accepted, where each entry either
names a class or says every address under it is its own client.  What I
posted becomes one line of that.  A multi-homed host is a list of its
addresses.  A v4.1 key could come later as another kind of match, and so
could a weight or a limit per class, which is what I think Daire actually
needs.

I know all three of you said no tunables in June.  I'd put this closer to
exports than to a tunable: it tells the server who is who.  Neil floated
an export flag for marking clients on the first thread.  I'd do it through
the nfsd netlink interface so it's per-net and not a module parameter.

It does raise the bar on the cost question.  If nothing is configured then
everyone pays for the two-level queue and gets nothing for it, so the
answer on your "alone" numbers has to be zero, not small.

Would you take something like that?  I'd rather ask before I build it.

Ben

Re: [PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Chuck Lever]

On Mon, Oct 5, 2026, at 10:46 AM, Benjamin Coddington wrote:
> I know all three of you said no tunables in June.  I'd put this closer to
> exports than to a tunable: it tells the server who is who.  Neil floated
> an export flag for marking clients on the first thread.  I'd do it through
> the nfsd netlink interface so it's per-net and not a module parameter.

I'm not sure how an admin would set such a tunable for a workload that
has to handle more than one of the fairness behaviors.

Scheduling, like getting old, is hard.


> It does raise the bar on the cost question.  If nothing is configured then
> everyone pays for the two-level queue and gets nothing for it, so the
> answer on your "alone" numbers has to be zero, not small.
>
> Would you take something like that?  I'd rather ask before I build it.

The "take/reject" question is answered by looking at patches and test
results, neither of which exist before a prototype is built. The best I
can do a priori is to say "that looks promising or that might suck."

I'll try to think about it more.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)

Re: [PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Benjamin Coddington]

On 4 Oct 2026, at 16:48, Chuck Lever wrote:

> Jittering the injected service time would show how much of the
> shortfall comes from the harness. Counting consecutive dispatches
> of the same client in the trace might show the rest.
> ...
> Any server with more than one NUMA node gets the diluted result by
> default. I would like to see numbers for a two-node per node
> configuration, and your thoughts on whether the client queue belongs
> to the pool at all.
> ...
> That result and the "alone" column disagree. Can you repeat both on a
> kernel without KASAN, with enough runs to show the variance? If the
> unloaded slowdown is real, we need to know where it comes from.

Here are the two-node numbers I promised, and the rest of what you asked
for, from real hardware this time.

Where the cover letter's numbers came from: a VM on my laptop, 10 vCPUs
and 6G, KASAN kernels, with the host busy with everything else I do in
a day.  The guest can't see any of that (no steal time, every vCPU looks
the same), so when the "alone" column came out 36% slower I had no way
to tell the kernel from the laptop.  Not the rig to measure a
microsecond with, and I should have said so.

I got a day on a two-socket box (2 x Xeon 6542Y, 96 threads, 1 TB), so
here's the review's list again from that.  Kernels are v7.2 with the
el9 config run through olddefconfig, no KASAN, A with the test hook and
B with the wake fix and the two patches on top.  Loopback unless said,
16 threads, 10ms injected.  Two things changed in the harness: the hook
sleeps with usleep_range(us, us) now instead of fsleep(), which was
giving every request the same 25% timer slack and releasing the threads
in batches, and the load generator jitters the requested service time
by 50%.  Each kernel was booted twice and every figure below came out
the same on both boots.

XPT_BUSY first.  With the service time jittered the single connection
gets 49.9% in all six runs against the 8-connection client, and 3 of the
other client's 2414 dequeues landed in its dequeue-to-requeue window,
which is 1us on this box.  So that whole shortfall was the harness, as
you said.

Two nodes.  The box has a 400G port on each node, so I put the client
side in a namespace on the node 0 port and the server on the node 1
port, through the switch.  Three pool setups, 16 threads:

                              one pool    per node,     per node,
                                          IRQs local    IRQs split
    total ops/s               1600/1600    797/797     1600/1600
    1x16 vs 8x4, share       11.1/49.9    11.1/50.0    8-22/25-28
    2x8 vs 16x4, share       11.1/50.0    11.1/50.0     7.7/50.0
    burst 32 vs K=16, p50     350/63       690/101      449/101
    1-conn client walk, s    37.7/2.6     75.4/4.9      4.9/5.2

(A/B in each cell.)  Two things in there.  With per node pools and the
NIC's queue IRQs where mlx5 put them, on the NIC's node, every request
lands in that node's pool and the other eight threads never run: half
the server, on both kernels.  With the IRQs spread over both nodes you
get what you described: a client's share is its share of each pool it's
in, so a single connection gets half of one pool, 25% here instead of
50%, and two connections get 50% or 25% depending on where RSS puts
them.  On A the same client got anywhere from 8% to 22% run to run on
the same luck, and that's also why the real client's walk comes out
even in the last column: A's victim happened to land in the light pool.
Those cells want a lot more runs than I gave them.

On whether the client queue belongs to the pool: making it exact across
pools would mean a thread on one node serving a transport that arrived
on the other, which is what the pools are there to avoid.  I'd rather
keep it per pool and say so.

The "alone" column doesn't reproduce.  Serial walk of 2000 files, 14k
RPCs, ten runs: 0.24-0.26s on both kernels, pinned to one node.  fio with
4 jobs on one connection: 126.6k IOPS on both.  What does show up is at
the top of the overhead test, 5 x 20s each:

                                   A        B
    nconnect=1, 16 jobs        152.8k   152.2k   -0.4%
    nconnect=8, 16 jobs        483.8k   475.5k   -1.7%
    nconnect=8, 64 jobs        584.2k   581.0k   -0.5%

That's about half a microsecond per dispatch, and all eight connections
there are one client, so every dequeue takes the pool's client lock and
then that client's lock.  My read is that it's the extra cache lines
moving between the enqueuing core and the dequeuing one (the client's
transport list and its queued bit, plus the pool's client list), not the
atomics themselves.  I haven't attributed it with perf yet.

The cover's tables again, NFSv3 burst completion p50 in ms, floor 37ms:

    K             4      8     16     32
    A            70    195    351    671
    B            56     62     61     63

    N (K=16)      1      8     32     64     96    128
    A            21     98    350    690   1035   1365
    B            11     31     62    103    144    179

    M hosts x 4   1      2      4      8
    A            70    192    351    670
    B            57     81    121    192

    S: 2x8 vs K, share   4      8     16     32
    A                 41.0   20.0   11.1    5.9
    B                 50.3   50.0   50.0   50.0

v4.1 is within 2% in every cell.  Totals are 1591-1604 ops/s in every
saturated cell on both.  The real v3 client walking 500 files against a
16-connection aggressor: 37.8s on A, 2.5s on B, 0.12s alone on both;
its fio 99 vs 1590 IOPS.

On the question in your second mail, an admin setting one table for
more than one fairness behavior: the posted kernel keys on source
address, so I can emulate "all movers are one class" today by giving
the movers one address.  Six movers against customers, share of
dispatches on B:

                                movers on six addrs   on one addr
    one customer, 1 conn x 16          14.3%              49.9%
    one customer, 4 x 4                14.3%              49.8%
    four customers, 4 x 4 each         40.0%              80.0%

Burst of 32 against M busy hosts, p50: hosts on their own addresses 57
81 121 192ms, hosts on one address 56 62 62 62ms.  And a mixed run of
six movers, two backlogged customers and one burst client: movers on
one address gives movers 28.9%, each customer 28.9%, burst p50 100ms;
on six addresses it's 68.9%, 11.5%, 11.5%, 192ms.  So one line in a
table does both at once: the movers held to a share, the customers
served per host, and the burst client's wait bounded by the number of
classes instead of hosts.  A is unchanged by the address either way.
What a table like that doesn't give is a third discipline on top; it's
still round-robin across whatever the keys are, and anything like a
weight or a cap would be an attribute on the class.

..and on the cost: if nothing is configured the old single queue could
stay in place, which would make that half a microsecond zero for anyone
who hasn't asked for anything.

Ben

Re: [PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Chuck Lever]

The extra good numbers redacted for brevity ...

Also, thanks Daire for your user stories, those details are interesting.


On Tue, Oct 6, 2026, at 2:07 PM, Benjamin Coddington wrote:
> On the question in your second mail, an admin setting one table for
> more than one fairness behavior: the posted kernel keys on source
> address, so I can emulate "all movers are one class" today by giving
> the movers one address.

OK, that helps me understand the motivation for your earlier question
regarding whether I would accept such an approach. You were referring,
in particular, to the proposal to introduce a new netlink API for
creating tables that configure server fairness, after Neil, Jeff, and
I all rejected tuneables in June.

The design requirements have evolved to need some kind of configuration,
because a single NFS server is frequently deployed in environments
where several different workloads are active at once. The requirement
to protect fairness between NFSv3, NFSv4, and presumably LOCALIO
client workloads makes this challenging.

I'm not terribly excited at adding this configuration to exports.
Today, netgroups are defined separately, for instance, and exportfs
is just a consumer of those groups. Making it more like iptables,
controlled via nfsdctl, resonates with me more.

We also have the problem of using raw IP addresses, which is a)
not user interface-friendly (hostnames might be nicer); and b)
not entirely secure.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)

Re: [PATCH RFC 0/2] SUNRPC: dispatch ready transports round-robin across clients

[Benjamin Coddington]

On 7 Oct 2026, at 10:15, Chuck Lever wrote:

> The extra good numbers redacted for brevity ...
>
> Also, thanks Daire for your user stories, those details are interesting.
>
>
> On Tue, Oct 6, 2026, at 2:07 PM, Benjamin Coddington wrote:
>> On the question in your second mail, an admin setting one table for
>> more than one fairness behavior: the posted kernel keys on source
>> address, so I can emulate "all movers are one class" today by giving
>> the movers one address.
>
> OK, that helps me understand the motivation for your earlier question
> regarding whether I would accept such an approach. You were referring,
> in particular, to the proposal to introduce a new netlink API for
> creating tables that configure server fairness, after Neil, Jeff, and
> I all rejected tuneables in June.
>
> The design requirements have evolved to need some kind of configuration,
> because a single NFS server is frequently deployed in environments
> where several different workloads are active at once. The requirement
> to protect fairness between NFSv3, NFSv4, and presumably LOCALIO
> client workloads makes this challenging.
>
> I'm not terribly excited at adding this configuration to exports.
> Today, netgroups are defined separately, for instance, and exportfs
> is just a consumer of those groups. Making it more like iptables,
> controlled via nfsdctl, resonates with me more.
>
> We also have the problem of using raw IP addresses, which is a)
> not user interface-friendly (hostnames might be nicer); and b)
> not entirely secure.

I've been working through all the various ways I could think of to allow
admins to classify; heres a list with the reasons why I ruled them out
(except for the last):

  - tc / nftables packet marks: tc classes exist only on egress, and the
    only tag that reaches the accepted socket is the mark.  RDMA has
    neither packets nor a socket.

  - Listener or local server address: the client chooses which address it
    mounts, so the client would choose its own class.

  - DSCP / traffic class: on RDMA it's whatever the client declared, and
    the NFS client has no knob to set it.

  - IPv4 route realms: no IPv6 equivalent, and the realm code has seen
    only fixes since 2015.  Clients on the server's own subnet would also
    need tag-only routes.

  - nft sets or ipset as the address table: the kernel API tests a
    packet, not an address, so it would need new netfilter API.

  - /etc/exports and the mountd upcall: export lines get duplicated, which
    is complicated and brittle.  The class is also only known after the
    first request, which needs the transport move.

  - Module parameter: too global, as you said in June.

  - Address-prefix table through nfsd netlink (what I floated on 10-05):
    workable, but it needs a new kernel table and matching code, a new
    netlink interface, and nfs-utils work.

  - Classify-only BPF struct_ops at accept: It runs once per connection on
    the accept path shared by TCP and RDMA, and covers IPv6.  With no
    program loaded, dispatch is unchanged.  The prefix table becomes an LPM
    map the admin updates live, with no new kernel table or netlink.

So I now have a pending RFC v2 which, while complicated, has the distinct
advantage that it has near-zero performance overhead if unconfigured, and
successfully punts the classification problem to a sysadmin that cares.  I
hope to post it today, but it will need review over in bpf@vger as well.

Ben