knfsd: has_wspace per transport

net/sunrpc/svcsock.c +17 -12

@@ -268,21 +252,15 @@ svc_sock_enqueue(struct svc_sock *svsk)
 	BUG_ON(svsk->sk_pool != NULL);
 	svsk->sk_pool = pool;
 
-	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
-	if (((atomic_read(&svsk->sk_reserved) + serv->sv_max_mesg)*2
-	     > svc_sock_wspace(svsk))
+	if (svsk->sk_ops->sko_has_wspace
 	    && !test_bit(SK_CLOSE, &svsk->sk_flags)
 	    && !test_bit(SK_CONN, &svsk->sk_flags)) {
-		/* Don't enqueue while not enough space for reply */
-		dprintk("svc: socket %p  no space, %d*2 > %ld, not enqueued\n",
-			svsk->sk_sk, atomic_read(&svsk->sk_reserved)+serv->sv_max_mesg,
-			svc_sock_wspace(svsk));
-		svsk->sk_pool = NULL;
-		clear_bit(SK_BUSY, &svsk->sk_flags);
-		goto out_unlock;
+		if (!svsk->sk_ops->sko_has_wspace(svsk)) {
+			svsk->sk_pool = NULL;
+			clear_bit(SK_BUSY, &svsk->sk_flags);
+			goto out_unlock;
+		}
 	}
-	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
-
 
 	if (!list_empty(&pool->sp_threads)) {
 		rqstp = list_entry(pool->sp_threads.next,

@@ -904,13 +882,42 @@ svc_tcpip_prepare_reply(struct svc_rqst 
 	return 0;
 }
 
+/**
+ * svc_sock_has_write_space - Checks if there is enough space
+ * to send the reply on the socket.
+ * @svsk: the svc_sock to write on
+ * @wspace: the number of bytes available for writing
+ */
+static int svc_sock_has_write_space(struct svc_sock *svsk, int wspace)
+{
+	struct svc_serv	*serv = svsk->sk_server;
+	int required = atomic_read(&svsk->sk_reserved) + serv->sv_max_mesg;
+

net/sunrpc/svcsock.c svc_tcpip_prepare_reply(struct svc_rqst · line 905

Quoted 10 lines · Greg Banks

net/sunrpc/svcsock.c +10 -0

+	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	if (required*2 > wspace) {
+		/* Don't enqueue while not enough space for reply */
+		dprintk("svc: socket %p  no space, %d*2 > %d, not enqueued\n",
+			svsk->sk_sk, required, wspace);
+		return 0;
+	}
+	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	return 1;
+}

So, this is just my ignorance--why do the set and clear of SOCK_NOSPACE need to be ordered in the way they are? (Why not just set once inside the if clause?)

Quoted 15 lines · J. Bruce Fields

net/sunrpc/svcsock.c svc_tcpip_prepare_reply(struct svc_rqst · line 905

Quoted 10 lines · Greg Banks

net/sunrpc/svcsock.c +10 -0

+	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	if (required*2 > wspace) {
+		/* Don't enqueue while not enough space for reply */
+		dprintk("svc: socket %p  no space, %d*2 > %d, not enqueued\n",
+			svsk->sk_sk, required, wspace);
+		return 0;
+	}
+	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	return 1;
+}

So, this is just my ignorance--why do the set and clear of SOCK_NOSPACE need to be ordered in the way they are? (Why not just set once inside the if clause?)

I can't see a good reason for it, but I'm trying to minimise perturbations to the logic.

And I gather by the fact that you test "->sko_has_wspace" that RDMA doesn't have such a function? Do that mean that RDMA will never reject a write due to lack of space? That seems unlikely.

Quoted 3 lines · Neil Brown

And I gather by the fact that you test "->sko_has_wspace" that RDMA doesn't have such a function? Do that mean that RDMA will never reject a write due to lack of space? That seems unlikely.

RDMA uses credits, which are preallocated per-connection resources in the hardware. RDMA transports don't generally support flow control, that is left to the upper layer. As such, if the client is obeying the protocol, the send will succeed. And if it's not, then the client only loses since the failure will cause the connection to be lost.

Someday we might want to support this kind of test. There are shared resource schemes in RDMA, more for the receive side, but it's not out of the question for sends. As a default though, a NULL has_wspace makes sense.

Quoted 17 lines · On Thursday May 17, [email protected] wrote:

Quoted 15 lines · J. Bruce Fields

net/sunrpc/svcsock.c svc_tcpip_prepare_reply(struct svc_rqst · line 905

Quoted 10 lines · Greg Banks

net/sunrpc/svcsock.c +10 -0

+	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	if (required*2 > wspace) {
+		/* Don't enqueue while not enough space for reply */
+		dprintk("svc: socket %p  no space, %d*2 > %d, not enqueued\n",
+			svsk->sk_sk, required, wspace);
+		return 0;
+	}
+	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	return 1;
+}

So, this is just my ignorance--why do the set and clear of SOCK_NOSPACE need to be ordered in the way they are? (Why not just set once inside the if clause?)

Quoted 2 lines

I can't see a good reason for it, but I'm trying to minimise perturbations to the logic.

Unfortunately, you actually perturbed the important bit... Or at least, the bit that I thought was important when I wrote it.

Previously, sk_stream_wspace(), or sock_wspace() would be called *after* SOCK_NOSPACE was set. With your patch it is called *before*.

It is a fairly improbably race, but if the output queue flushed completely between calling XX_wspace and setting SOCK_NOSPACE, the sk_write_space callback might never get called.

And I gather by the fact that you test "->sko_has_wspace" that RDMA doesn't have such a function?

Do that mean that RDMA will never
reject a write due to lack of space?

That seems unlikely.
I would rather assume that every transport has a sko_has_wspace function...

Quoted 24 lines · Neil Brown

Quoted 17 lines · On Thursday May 17, [email protected] wrote:

Quoted 15 lines · J. Bruce Fields

net/sunrpc/svcsock.c svc_tcpip_prepare_reply(struct svc_rqst · line 905

Quoted 10 lines · Greg Banks

net/sunrpc/svcsock.c +10 -0

+	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	if (required*2 > wspace) {
+		/* Don't enqueue while not enough space for reply */
+		dprintk("svc: socket %p  no space, %d*2 > %d, not enqueued\n",
+			svsk->sk_sk, required, wspace);
+		return 0;
+	}
+	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	return 1;
+}

So, this is just my ignorance--why do the set and clear of SOCK_NOSPACE need to be ordered in the way they are? (Why not just set once inside the if clause?)

Quoted 2 lines

I can't see a good reason for it, but I'm trying to minimise perturbations to the logic.

Unfortunately, you actually perturbed the important bit... Or at least, the bit that I thought was important when I wrote it.

Quoted 6 lines

Previously, sk_stream_wspace(), or sock_wspace() would be called *after* SOCK_NOSPACE was set. With your patch it is called *before*.

It is a fairly improbably race, but if the output queue flushed completely between calling XX_wspace and setting SOCK_NOSPACE, the sk_write_space callback might never get called.

Woops. I'll fix that.

Quoted 2 lines

And I gather by the fact that you test "->sko_has_wspace" that RDMA doesn't have such a function?

You gather correctly.

Quoted 2 lines

Do that mean that RDMA will never
reject a write due to lack of space?

No, it means that the current RDMA send code will block waiting for space to become available. That's right, nfsd threads block on the network. Steel yourself, there's worse to come.

Quoted 3 lines

That seems unlikely.
I would rather assume that every transport has a sko_has_wspace function...

Ok, but for today the RDMA one will be

static int svc_rdma_has_wspace(struct svc_sock *svsk)

{
	return 1;
}

We might be able to pull the condition in the blocking logic out of svc_rdma_send() out to implement an sko_has_wspace, but there's something of an impedance mismatch. The RDMA queue limit is expressed in Work Requests not bytes, and the mapping between the two is not precisely visible at the point when has_wspace is called. I guess we'd have to use an upper bound.
Greg.

Quoted 37 lines · Greg Banks

Quoted 24 lines · Neil Brown

Quoted 17 lines · On Thursday May 17, [email protected] wrote:

Quoted 15 lines · J. Bruce Fields

net/sunrpc/svcsock.c svc_tcpip_prepare_reply(struct svc_rqst · line 905

Quoted 10 lines · Greg Banks

net/sunrpc/svcsock.c +10 -0

+	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	if (required*2 > wspace) {
+		/* Don't enqueue while not enough space for reply */
+		dprintk("svc: socket %p  no space, %d*2 > %d, not enqueued\n",
+			svsk->sk_sk, required, wspace);
+		return 0;
+	}
+	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	return 1;
+}

So, this is just my ignorance--why do the set and clear of SOCK_NOSPACE need to be ordered in the way they are? (Why not just set once inside the if clause?)

Quoted 2 lines

I can't see a good reason for it, but I'm trying to minimise perturbations to the logic.

Unfortunately, you actually perturbed the important bit... Or at least, the bit that I thought was important when I wrote it.

Quoted 6 lines

Previously, sk_stream_wspace(), or sock_wspace() would be called *after* SOCK_NOSPACE was set. With your patch it is called *before*.

It is a fairly improbably race, but if the output queue flushed completely between calling XX_wspace and setting SOCK_NOSPACE, the sk_write_space callback might never get called.

Woops. I'll fix that.

Quoted 2 lines

And I gather by the fact that you test "->sko_has_wspace" that RDMA doesn't have such a function?

Quoted 9 lines

You gather correctly.

Quoted 2 lines

Do that mean that RDMA will never
reject a write due to lack of space?

No, it means that the current RDMA send code will block waiting for space to become available. That's right, nfsd threads block on the network. Steel yourself, there's worse to come.

Uh... Not really. The queue depths are designed to match credits to worst case reply sizes. In the normal case, it should never have to wait. The wait is to catch the margins in the same way that a kmalloc will wait for memory to become available.

There's actually a stat kept by the transport that counts the number of times it waits.

There is a place that a wait is done in the "normal" case and that's for the completion of an RDMA_READ in the process of gathering the data for and RPC on receive. That wait happens _every_ time.

Quoted 11 lines

Quoted 3 lines

That seems unlikely.
I would rather assume that every transport has a sko_has_wspace function...

Ok, but for today the RDMA one will be

static int svc_rdma_has_wspace(struct svc_sock *svsk)

{
	return 1;
}

Quoted 7 lines

We might be able to pull the condition in the blocking logic out of svc_rdma_send() out to implement an sko_has_wspace, but there's something of an impedance mismatch. The RDMA queue limit is expressed in Work Requests not bytes, and the mapping between the two is not precisely visible at the point when has_wspace is called. I guess we'd have to use an upper bound.
Greg.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Quoted 8 lines · Greg Banks

Quoted 2 lines · Neil Brown

Do that mean that RDMA will never
reject a write due to lack of space?

No, it means that the current RDMA send code will block waiting for space to become available. That's right, nfsd threads block on the network. Steel yourself, there's worse to come.

Uh... Not really. The queue depths are designed to match credits to worst case reply sizes. In the normal case, it should never have to wait. The wait is to catch the margins in the same way that a kmalloc will wait for memory to become available.

There's actually a stat kept by the transport that counts the number of times it waits.

There is a place that a wait is done in the "normal" case and that's for the completion of an RDMA_READ in the process of gathering the data for and RPC on receive. That wait happens _every_ time.

Quoted 10 lines · Tom Tucker

Quoted 8 lines · Greg Banks

Quoted 2 lines · Neil Brown

Do that mean that RDMA will never
reject a write due to lack of space?

No, it means that the current RDMA send code will block waiting for space to become available. That's right, nfsd threads block on the network. Steel yourself, there's worse to come.

Quoted 4 lines

Uh... Not really. The queue depths are designed to match credits to worst case reply sizes. In the normal case, it should never have to wait. The wait is to catch the margins in the same way that a kmalloc will wait for memory to become available.

This is news to me, but then I just read that code not write it ;-)

I just poked around in your GIT tree and AFAICT the software queue depth limits are set by this logic:

    743 static int
    744 svc_rdma_accept(struct svc_rqst *rqstp)
    745 {
    ...
    775         ret = ib_query_device(newxprt->sc_cm_id->device, &devattr);
    ...
    788         newxprt->sc_max_requests = min((size_t)devattr.max_qp_wr,
    789                                    (size_t)svcrdma_max_requests);
    790         newxprt->sc_sq_depth = RPCRDMA_SQ_DEPTH_MULT * newxprt->sc_max_requests;


     54 unsigned int svcrdma_max_requests = RPCRDMA_MAX_REQUESTS;
     
    143 #define RPCRDMA_SQ_DEPTH_MULT   8
    145 #define RPCRDMA_MAX_REQUESTS    16

A brief experiment shows that devattr.max_qp_wr = 16384 on recent Mellanox cards, so the actual sc_sq_depth value will be ruled by svcrdma_max_requests, and be 128.

The worst case is a 4K page machine writing a 1MB reply to a READ rpc, which if I understand the code correctly uses a WR per page plus one for the reply header, or 1024/4+1 = 257 WRs. Now imagine all the nfsd threads(*) trying to do that to the same QP. SGI runs with 128 threads.

In this scenario, every call to svc_rdma_send() stands a good chance of blocking. The wait is not interruptible and has no timeout. If the client's HCA has a conniption under this load, the server will have some large fraction of the nfsds unkillably blocked until the server's HCA gives up and reports an error (I presume it does this?).

The wspace management code in knfsd is designed to avoid having nfsds ever block in the network stack; I don't see how the NFS/RDMA code achieves that. Or is there something I've missed?

* I would expect there to be a client-side limit on the number of outstanding READs so this doesn't normally happen, but part of the reason for knfsd's wspace management is to avoid DoS attacks from malicious or broken clients.

Quoted 2 lines

There's actually a stat kept by the transport that counts the number of times it waits.

Ah, that would be rdma_stat_sq_starve? It's been added since the November code drop so I hadn't noticed it. BTW is there a reason for emitting statistics to userspace as sysctls?

Quoted 4 lines

There is a place that a wait is done in the "normal" case and that's for the completion of an RDMA_READ in the process of gathering the data for and RPC on receive. That wait happens _every_ time.

Yes indeed, and this is the "worse" I was referring to. I have a crash dump in which 122 of the 128 nfsd threads are doing this:
0 schedule+0x249c [0xa00000010052799c]
1 schedule_timeout+0x1ac [0xa00000010052958c]
2 rdma_read_xdr+0xf2c [0xa0000002216bcd8c]
3 svc_rdma_recvfrom+0x1e5c [0xa0000002216bed7c]
4 svc_recv+0xc8c [0xa00000022169f0ac]
5 nfsd+0x1ec [0xa000000221d7504c]
6 __svc_create_thread_tramp+0x30c [0xa0000002216976ac] 7 kernel_thread_helper+0xcc [0xa00000010001290c]
8 start_kernel_thread+0x1c [0xa0000001000094bc]

That doesn't leave a lot of threads to do real work, like waiting for the filesystem.

And every now and again something goes awry in
IB land and each thread ends up waiting for 6 seconds or more.

Greg.

Signature · 3 lines

Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

Quoted 5 lines · Greg Banks

The wspace management code in knfsd is designed to avoid having nfsds ever block in the network stack; I don't see how the NFS/RDMA code achieves that. Or is there something I've missed?

You're dead on. We should implement wspace properly. I'll look into fixing this.

The bulk of the stats were exported in order to evaluate different I/O models. For example, do we poll some number of times on an SQ stall? How about an RQ stall, etc... I think many of these stats should be removed. I'd appreciate feedback on which ones we should retain.

What's happening here is that you're pounding the server with NFS WRITE and recvfrom is dutifully queuing another thread to process the next RPC in the RQ, which happens to require another RDMA READ and another wait and so on. As you point out, this is not productive and quickly consumes all your threads -- Ugh.

A simple optimization to the current design for this case is that if the RPC contains a read-list don't call svc_sock_enqueue until _after_ the RDMA READ completes.

The current thread will wait, but the remaining
threads (127 in your case) will be free to do other work for other mount points.

This only falls over in the limit of 128 mounts all pounding the server with writes. Comments?

Quoted 2 lines

And every now and again something goes awry in
IB land and each thread ends up waiting for 6 seconds or more.

Yes. Note that when this happens, the connection gets terminated.

Quoted 9 lines · Tom Tucker

Quoted 5 lines · Greg Banks

The wspace management code in knfsd is designed to avoid having nfsds ever block in the network stack; I don't see how the NFS/RDMA code achieves that. Or is there something I've missed?

You're dead on. We should implement wspace properly. I'll look into fixing this.

Great.

Quoted 4 lines

The bulk of the stats were exported in order to evaluate different I/O models. For example, do we poll some number of times on an SQ stall? How about an RQ stall, etc... I think many of these stats should be removed. I'd appreciate feedback on which ones we should retain.

Actually I quite like the stats, although the reap_[sr]q_at_* counters don't seem to be used. For rdma_stat_rq_prod, I would have had a counter increment where wc.status reports an error, thus futzing with the global cacheline only once in the normal success path.

But I think it would be better to report them through the existing /proc/net/rpc/nfsd mechanism, i.e. add an RDMA-specific call to svc_seq_show() and add your new counters to struct svc_stat.

Quoted 6 lines

What's happening here is that you're pounding the server with NFS WRITE and recvfrom is dutifully queuing another thread to process the next RPC in the RQ, which happens to require another RDMA READ and another wait and so on. As you point out, this is not productive and quickly consumes all your threads -- Ugh.

Exactly.

Quoted 3 lines

A simple optimization to the current design for this case is that if the RPC contains a read-list don't call svc_sock_enqueue until _after_ the RDMA READ completes.

I assume you meant "don't release the SK_BUSY bit", because svc_sock_enqueue will be called from the dto tasklet every time a new call arrives whether or not we've finished the RDMA READ.

Quoted 6 lines

The current thread will wait, but the remaining
threads (127 in your case) will be free to do other work for other mount points.

This only falls over in the limit of 128 mounts all pounding the server with writes. Comments?

A common deployment scenario I would expect to see would have at least twice as many clients as threads, and SGI have successfully tested scenarios with a 16:1 client:thread ratio. So I don't think we can solve the problem in any way which ties down a thread for up to 6 seconds per client, because there won't be enough threads to go around.

The other case that needs to work is a single fat client trying to throw as many bits as possible down the wire. For this case your proposal would limit the server to effectively serving WRITE RPCs serially,

... with what I presume would be a catastrophic effect on write performance. What you want is for knfsd to be able to throw as many threads at the single fat client as possible.

The knfsd design actually handles both these extreme cases quite well today with TCP sockets. The features which I think are key to providing this flexibility are:

* a pool of threads whose size is only loosely tied to the number of clients

* threads are not tied to clients, they compete for individual incoming RPCs

* threads take care to avoid blocking on the network

The problem we have achieving the last point for NFS/RDMA is that RDMA READs are fundamentally neither a per-svc_sock nor a per-thread concept. To achieve parallelism you want multiple sequences of RDMA READs proceeding per svc_sock. To avoid blocking threads you want a thread to be able to abandon a sequence of RDMA READs partway through and go back to it's main loop. Doing both implies a new object to hold the state for a single sequence of RDMA READs for a single WRITE RPC.

So I think the "right" way to handle RDMA READs is to define a new struct, for argument's sake "struct svc_rdma_read", containing some large fraction of the auto variables currently in the function rdma_read_xdr() and some from it's caller. When an RPC with a read list arrives, allocate a struct svc_rdma_read and add it to a list of such structs on the corresponding struct svcxprt_rdma. Issue some READ WRs, until doing so would block. When the completions for issued WRs arrive, mark the struct svc_rdma_read, instead of waking the nfsd as you do now, set SK_DATA on the svcxprt_rdma and enqueue it. When an nfsd later calls svc_rdma_recvfrom(), first check whether any of the struct svc_rdma_reads in flight have progressed, and depending on whether they're complete either issue some more READ WRs or finalise the struct svc_rdma_read by setting up the thread's page array.

In other words, invert the rdma_read_xdr() logic so nfsd return to the main loop instead of blocking.

Unfortunately it's kind of a major change. Thoughts?

Quoted 4 lines

Quoted 2 lines

And every now and again something goes awry in
IB land and each thread ends up waiting for 6 seconds or more.

Yes. Note that when this happens, the connection gets terminated.

Indeed. Meanwhile, for the last 6 seconds all the nfsds are blocked waiting for the one client, and none of the other clients are getting any service.

Greg.

Quoted 1 line · Greg Banks

On Tue, May 22, 2007 at 12:34:23PM -0500, Tom Tucker wrote:

I need to do a little clean up (e.g. atomic_inc()) and then I'll add them as suggested. If there are dissenters, please speak now...

Quoted 7 lines

Quoted 6 lines

The current thread will wait, but the remaining
threads (127 in your case) will be free to do other work for other mount points.

This only falls over in the limit of 128 mounts all pounding the server with writes. Comments?

Quoted 6 lines

A common deployment scenario I would expect to see would have at least twice as many clients as threads, and SGI have successfully tested scenarios with a 16:1 client:thread ratio. So I don't think we can solve the problem in any way which ties down a thread for up to 6 seconds per client, because there won't be enough threads to go around.

Agreed. But isn't that the 100 dead adapter scenario?

Someone has to wait.
Who?

Quoted 3 lines

In other words, invert the rdma_read_xdr() logic so nfsd return to the main loop instead of blocking.

Quoted 2 lines

Unfortunately it's kind of a major change. Thoughts?

Ok, I love it and I hate it :-) This is the consolidated waiter strategy that solves all the problems...except ...does this expose any read/write ordering issues at the client? Couldn't the client issue a write followed by a read and get the original data?

That's the hate it part. If we need to decide which requests are allowed to proceed on a QP with outstanding READ WR, things get messy quick.

Is there no such requirement?

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

And every now and again something goes awry in
IB land and each thread ends up waiting for 6 seconds or more.

Yes. Note that when this happens, the connection gets terminated.

Indeed. Meanwhile, for the last 6 seconds all the nfsds are blocked waiting for the one client, and none of the other clients are getting any service.

We need to think about this. A dead adapter causes havoc.

Quoted 7 lines · Tom Tucker

Quoted 1 line · Greg Banks

On Tue, May 22, 2007 at 12:34:23PM -0500, Tom Tucker wrote:

I need to do a little clean up (e.g. atomic_inc()) and then I'll add them as suggested. If there are dissenters, please speak now...

You should be able to get away with just ++. They're only stats, it's not like you have logic depending on them being exact.

Quoted 5 lines · Greg Banks

Quoted 4 lines · Tom Tucker

On 5/22/07 9:32 PM, "Greg Banks" <[email protected]> wrote:

Quoted 7 lines

Quoted 4 lines

Quoted 1 line · Tom Tucker

I need to do a little clean up (e.g. atomic_inc()) and then I'll add them as
suggested. If there are dissenters, please speak now...

You should be able to get away with just ++. They're only stats, it's not like you have logic depending on them being exact.

The usual way to do this is to allocate separate stat arrays for each CPU. All the per-CPU arrays are summed only when something wants to display stat totals.

That way you can use ++, be fairly sure you will get accurate statistics, and not have to be concerned about CPU cache effects or lock contention.

Quoted 10 lines · Chuck Lever

Quoted 9 lines · Greg Banks

Quoted 6 lines · Tom Tucker

Quoted 1 line · Greg Banks

On Tue, May 22, 2007 at 12:34:23PM -0500, Tom Tucker wrote:

I need to do a little clean up (e.g. atomic_inc()) and then I'll add them as suggested. If there are dissenters, please speak now...

You should be able to get away with just ++. They're only stats, it's not like you have logic depending on them being exact.

Quoted 3 lines

The usual way to do this is to allocate separate stat arrays for each CPU. All the per-CPU arrays are summed only when something wants to display stat totals.

That way you can use ++, be fairly sure you will get accurate statistics, and not have to be concerned about CPU cache effects or lock contention.

Here in SGI, home of the multiple-thousand CPU Linux boxes, making data structures per-cpu tends to be done...thoughtfully.

In any case, the remainder of svc_stat gets no such treatment today.

Quoted 2 lines

Do that mean that RDMA will never
reject a write due to lack of space?

No, it means that the current RDMA send code will block waiting for space to become available. That's right, nfsd threads block on the network. Steel yourself, there's worse to come.

Quoted 17 lines · On Thursday May 17, [email protected] wrote:

Quoted 15 lines · J. Bruce Fields

net/sunrpc/svcsock.c svc_tcpip_prepare_reply(struct svc_rqst · line 905

Quoted 10 lines · Greg Banks

net/sunrpc/svcsock.c +10 -0

+	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	if (required*2 > wspace) {
+		/* Don't enqueue while not enough space for reply */
+		dprintk("svc: socket %p  no space, %d*2 > %d, not enqueued\n",
+			svsk->sk_sk, required, wspace);
+		return 0;
+	}
+	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+	return 1;
+}

So, this is just my ignorance--why do the set and clear of SOCK_NOSPACE need to be ordered in the way they are? (Why not just set once inside the if clause?)

Quoted 2 lines

I can't see a good reason for it, but I'm trying to minimise perturbations to the logic.

Unfortunately, you actually perturbed the important bit... Or at least, the bit that I thought was important when I wrote it.

Previously, sk_stream_wspace(), or sock_wspace() would be called *after* SOCK_NOSPACE was set. With your patch it is called *before*.

Quoted 3 lines

Previously, sk_stream_wspace(), or sock_wspace() would be called *after* SOCK_NOSPACE was set. With your patch it is called *before*.

Latest patch does this: