Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 0442f14b authored by J. Bruce Fields's avatar J. Bruce Fields
Browse files

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.

Kosuke Tatsukawa found this issue while 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

).

Reported-by: default avatarKosuke Tatsukawa <tatsu@ab.jp.nec.com>
Signed-off-by: default avatarJ. Bruce Fields <bfields@redhat.com>
parent 7fc0564e

net/sunrpc/svcsock.c

+31 −6
Original line number Diff line number Diff line
@@ -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;

	/*

	 * 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);

}
@@ -1593,7 +1618,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);

}