svcrpc: document lack of some memory barriers

We're missing memory barriers in net/sunrpc/svcsock.c in some spots we'd
expect them.  But it doesn't appear they're necessary in our case, and
this is likely a hot path--for now just document the odd behavior.

I found this issue when I was looking through the linux source code
for places calling waitqueue_active() before wake_up*(), but without
preceding memory barriers, after sending a patch to fix a similar
issue in drivers/tty/n_tty.c  (Details about the original issue can be
found here: https://lkml.org/lkml/2015/9/28/849).

Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
[bfields@redhat.com,nfbrown@novell.com: document instead of adding barriers]
Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 48923730722d6093224faee5af900daae236326a..1f71eece04d3d5cfe912bcf93e54edd036faadc1 100644 (file)
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -399,6 +399,31 @@ static int svc_sock_secure_port(struct svc_rqst *rqstp)
        return svc_port_is_privileged(svc_addr(rqstp));
 }
 
+static bool sunrpc_waitqueue_active(wait_queue_head_t *wq)
+{
+       if (!wq)
+               return false;
+       /*
+        * Kosuke Tatsukawa points out there should normally be a memory
+        * barrier here--see wq_has_sleeper().
+        *
+        * It appears that isn't currently necessary, though, basically
+        * because callers all appear to have sufficient memory barriers
+        * between the time the relevant change is made and the
+        * time they call these callbacks.
+        *
+        * The nfsd code itself doesn't actually explicitly wait on
+        * these waitqueues, but it may wait on them for example in
+        * sendpage() or sendmsg() calls.  (And those may be the only
+        * places, since it it uses nonblocking reads.)
+        *
+        * Maybe we should add the memory barriers anyway, but these are
+        * hot paths so we'd need to be convinced there's no sigificant
+        * penalty.
+        */
+       return waitqueue_active(wq);
+}
+
 /*
  * INET callback when data has been received on the socket.
  */
@@ -414,7 +439,7 @@ static void svc_udp_data_ready(struct sock *sk)
                set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
                svc_xprt_enqueue(&svsk->sk_xprt);
        }
-       if (wq && waitqueue_active(wq))
+       if (sunrpc_waitqueue_active(wq))
                wake_up_interruptible(wq);
 }
 
@@ -432,7 +457,7 @@ static void svc_write_space(struct sock *sk)
                svc_xprt_enqueue(&svsk->sk_xprt);
        }
 
-       if (wq && waitqueue_active(wq)) {
+       if (sunrpc_waitqueue_active(wq)) {
                dprintk("RPC svc_write_space: someone sleeping on %p\n",
                       svsk);
                wake_up_interruptible(wq);
@@ -787,7 +812,7 @@ static void svc_tcp_listen_data_ready(struct sock *sk)
        }
 
        wq = sk_sleep(sk);
-       if (wq && waitqueue_active(wq))
+       if (sunrpc_waitqueue_active(wq))
                wake_up_interruptible_all(wq);
 }
 
@@ -808,7 +833,7 @@ static void svc_tcp_state_change(struct sock *sk)
                set_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags);
                svc_xprt_enqueue(&svsk->sk_xprt);
        }
-       if (wq && waitqueue_active(wq))
+       if (sunrpc_waitqueue_active(wq))
                wake_up_interruptible_all(wq);
 }
 
@@ -823,7 +848,7 @@ static void svc_tcp_data_ready(struct sock *sk)
                set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
                svc_xprt_enqueue(&svsk->sk_xprt);
        }
-       if (wq && waitqueue_active(wq))
+       if (sunrpc_waitqueue_active(wq))
                wake_up_interruptible(wq);
 }
 
@@ -1594,7 +1619,7 @@ static void svc_sock_detach(struct svc_xprt *xprt)
        sk->sk_write_space = svsk->sk_owspace;
 
        wq = sk_sleep(sk);
-       if (wq && waitqueue_active(wq))
+       if (sunrpc_waitqueue_active(wq))
                wake_up_interruptible(wq);
 }