Re: [PATCH v2] sunrpc: fix waitqueue_active without memory barrier in sunrpc

J. Bruce Fields wrote:

On Fri, Oct 23, 2015 at 04:14:10AM +0000, Kosuke Tatsukawa wrote:

quoted

J. Bruce Fields wrote:

quoted

On Fri, Oct 16, 2015 at 02:28:10AM +0000, Kosuke Tatsukawa wrote:

quoted

Tatsukawa Kosuke wrote:

quoted

J. Bruce Fields wrote:

quoted

On Thu, Oct 15, 2015 at 11:44:20AM +0000, Kosuke Tatsukawa wrote:

quoted

Tatsukawa Kosuke wrote:

quoted

J. Bruce Fields wrote:

quoted

Thanks for the detailed investigation.

I think it would be worth adding a comment if that might help someone
having to reinvestigate this again some day.

It would be nice, but I find it difficult to write a comment in the
sunrpc layer why a memory barrier isn't necessary, using the knowledge
of how nfsd uses it, and the current implementation of the network code.

Personally, I would prefer removing the call to waitqueue_active() which
would make the memory barrier totally unnecessary at the cost of a
spin_lock + spin_unlock by unconditionally calling
wake_up_interruptible.

On second thought, the callbacks will be called frequently from the tcp
code, so it wouldn't be a good idea.

So, I was even considering documenting it like this, if it's not
overkill.

Hmm... but if this is right, then we may as well ask why we're doing the
wakeups at all.  Might be educational to test the code with them
removed.

sk_write_space will be called in sock_wfree() with UDP/IP each time
kfree_skb() is called.  With TCP/IP, sk_write_space is only called if
SOCK_NOSPACE has been set.

sk_data_ready will be called in both tcp_rcv_established() for TCP/IP
and in sock_queue_rcv_skb() for UDP/IP.  The latter lacks a memory
barrier with sk_data_ready called right after __skb_queue_tail().
I think this hasn't caused any problems because sk_data_ready wasn't
used.

Actually, svc_udp_data_ready() calls set_bit() which is an atomic
operation.  So there won't be a problem unless svsk is NULL.

So is it true that every caller of these socket callbacks has adequate
memory barriers between the time the change is made visible and the time
the callback is called?

If so, then there's nothing really specific about nfsd here.

In that case maybe it's the networking code that use some documentation,
if it doesn't already?  (Or maybe common helper functions for this

	if (waitqueue_active(wq))
		wake_up(wq)

pattern?)

Some of the other places defining these callback functions are using
  static inline bool wq_has_sleeper(struct socket_wq *wq)
defined in include/net/sock.h

The comment above the function explains that it was introduced for
exactly this purpose.

Even thought the argument variable uses the same name "wq", it has a
different type from the wq used in svcsock.c (struct socket_wq *
vs. wait_queue_head_t *).

OK, thanks.  So, I guess it still sounds like the code is OK as is, but
maybe my comment wasn't.  Here's another attempt.

Thank you.  By now the patch looks completely different from my original
patch, so I don't think I deserve to be mentioned in the Author line.

--b.

commit b805ca58a81a
Author: Kosuke Tatsukawa [off-list ref]
Date:   Fri Oct 9 01:44:07 2015 +0000

    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).

I should have used the stable link format to refer to the disucssion in
LKML instead of the lkml.org URL.  The stable link is

http://lkml.kernel.org/r/17EC94B0A072C34B8DCF0D30AD16044A02871D53-9lrffkYxhwTt6d3pZDjeaEtBU8KWyXPq@public.gmane.org

Having my name in the comment itself looks a little odd, since I don't
see other places in the kernel source code that mentions the reporter's
name.

+	 *
+	 * 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.)

The above line contains an extra "it".

Best regards.
---
Kosuke TATSUKAWA  | 3rd IT Platform Department
                  | IT Platform Division, NEC Corporation
                  | tatsu-zZGIbrA41Td8UrSeD/g0lQ@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html