Re: [PATCH v2] net: Generalise wq_has_sleeper helper
From: Herbert XuDate: Thu, 26 Nov 2015 13:55:39 +0800 > The memory barrier in the helper wq_has_sleeper is needed by just > about every user of waitqueue_active. This patch generalises it > by making it take a wait_queue_head_t directly. The existing > helper is renamed to skwq_has_sleeper. > > Signed-off-by: Herbert Xu Applied, thanks a lot Herbert. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: net: Generalise wq_has_sleeper helper
From: Peter ZijlstraDate: Wed, 25 Nov 2015 10:15:33 +0100 > On Tue, Nov 24, 2015 at 01:54:23PM +0800, Herbert Xu wrote: >> + * The race for tcp fires when the __add_wait_queue changes done by CPU1 >> stay >> + * in its cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 >> + * could then endup calling schedule and sleep forever if there are no more >> + * data on the socket. >> + * > > Would be easier to refer to the comment that now adorns > waitqueue_active(). Yeah, that might be a good idea. Herbert can you adjust this? >> + */ >> +static inline bool wq_has_sleeper(wait_queue_head_t *wq) >> +{ >> +/* We need to be sure we are in sync with the > > broken comment style. This is how we do it in the networking, so that's why it's formatted this way, but yes he will need to fix it up. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: net: Generalise wq_has_sleeper helper
On Tue, Nov 24, 2015 at 01:54:23PM +0800, Herbert Xu wrote: > diff --git a/include/linux/wait.h b/include/linux/wait.h > index 1e1bf9f..bd1157f 100644 > --- a/include/linux/wait.h > +++ b/include/linux/wait.h > @@ -107,6 +107,50 @@ static inline int waitqueue_active(wait_queue_head_t *q) > return !list_empty(>task_list); > } > > +/** > + * wq_has_sleeper - check if there are any waiting processes > + * @wq: wait queue head > + * > + * Returns true if wq has waiting processes > + * > + * The purpose of the wq_has_sleeper and sock_poll_wait is to wrap the memory > + * barrier call. They were added due to the race found within the tcp code. > + * > + * Consider following tcp code paths: > + * > + * CPU1 CPU2 > + * > + * sys_selectreceive packet > + * ... ... > + * __add_wait_queueupdate tp->rcv_nxt > + * ... ... > + * tp->rcv_nxt check sock_def_readable > + * ... { > + * schedule rcu_read_lock(); > + * wq = rcu_dereference(sk->sk_wq); > + * if (wq && waitqueue_active(>wait)) > + * wake_up_interruptible(>wait) > + * ... > + * } > + * > + * The race for tcp fires when the __add_wait_queue changes done by CPU1 stay > + * in its cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 > + * could then endup calling schedule and sleep forever if there are no more > + * data on the socket. > + * Would be easier to refer to the comment that now adorns waitqueue_active(). > + */ > +static inline bool wq_has_sleeper(wait_queue_head_t *wq) > +{ > + /* We need to be sure we are in sync with the broken comment style. > + * add_wait_queue modifications to the wait queue. > + * > + * This memory barrier should be paired with one on the > + * waiting side. > + */ > + smp_mb(); > + return waitqueue_active(wq); > +} > + > extern void add_wait_queue(wait_queue_head_t *q, wait_queue_t *wait); > extern void add_wait_queue_exclusive(wait_queue_head_t *q, wait_queue_t > *wait); > extern void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] net: Generalise wq_has_sleeper helper
On Wed, Nov 25, 2015 at 11:37:29AM -0500, David Miller wrote: > > > Would be easier to refer to the comment that now adorns > > waitqueue_active(). > > Yeah, that might be a good idea. Herbert can you adjust this? Sure, here is an updated patch. Note that this patch is based on net-next where there is actually no comment above waitqueue_active. But hopefully this will right itself once the trees are merged. Thanks, ---8<--- The memory barrier in the helper wq_has_sleeper is needed by just about every user of waitqueue_active. This patch generalises it by making it take a wait_queue_head_t directly. The existing helper is renamed to skwq_has_sleeper. Signed-off-by: Herbert Xudiff --git a/crypto/algif_aead.c b/crypto/algif_aead.c index 0aa6fdf..fb99f30 100644 --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -106,7 +106,7 @@ static void aead_wmem_wakeup(struct sock *sk) rcu_read_lock(); wq = rcu_dereference(sk->sk_wq); - if (wq_has_sleeper(wq)) + if (skwq_has_sleeper(wq)) wake_up_interruptible_sync_poll(>wait, POLLIN | POLLRDNORM | POLLRDBAND); @@ -157,7 +157,7 @@ static void aead_data_wakeup(struct sock *sk) rcu_read_lock(); wq = rcu_dereference(sk->sk_wq); - if (wq_has_sleeper(wq)) + if (skwq_has_sleeper(wq)) wake_up_interruptible_sync_poll(>wait, POLLOUT | POLLRDNORM | POLLRDBAND); diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c index af31a0e..0e6702e 100644 --- a/crypto/algif_skcipher.c +++ b/crypto/algif_skcipher.c @@ -238,7 +238,7 @@ static void skcipher_wmem_wakeup(struct sock *sk) rcu_read_lock(); wq = rcu_dereference(sk->sk_wq); - if (wq_has_sleeper(wq)) + if (skwq_has_sleeper(wq)) wake_up_interruptible_sync_poll(>wait, POLLIN | POLLRDNORM | POLLRDBAND); @@ -288,7 +288,7 @@ static void skcipher_data_wakeup(struct sock *sk) rcu_read_lock(); wq = rcu_dereference(sk->sk_wq); - if (wq_has_sleeper(wq)) + if (skwq_has_sleeper(wq)) wake_up_interruptible_sync_poll(>wait, POLLOUT | POLLRDNORM | POLLRDBAND); diff --git a/include/linux/wait.h b/include/linux/wait.h index 1e1bf9f..6aa09a8 100644 --- a/include/linux/wait.h +++ b/include/linux/wait.h @@ -107,6 +107,27 @@ static inline int waitqueue_active(wait_queue_head_t *q) return !list_empty(>task_list); } +/** + * wq_has_sleeper - check if there are any waiting processes + * @wq: wait queue head + * + * Returns true if wq has waiting processes + * + * Please refer to the comment for waitqueue_active. + */ +static inline bool wq_has_sleeper(wait_queue_head_t *wq) +{ + /* +* We need to be sure we are in sync with the +* add_wait_queue modifications to the wait queue. +* +* This memory barrier should be paired with one on the +* waiting side. +*/ + smp_mb(); + return waitqueue_active(wq); +} + extern void add_wait_queue(wait_queue_head_t *q, wait_queue_t *wait); extern void add_wait_queue_exclusive(wait_queue_head_t *q, wait_queue_t *wait); extern void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait); diff --git a/include/net/sock.h b/include/net/sock.h index bbf7c2c..4adbe00 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -58,6 +58,7 @@ #include #include #include +#include #include #include @@ -1879,12 +1880,12 @@ static inline bool sk_has_allocations(const struct sock *sk) } /** - * wq_has_sleeper - check if there are any waiting processes + * skwq_has_sleeper - check if there are any waiting processes * @wq: struct socket_wq * * Returns true if socket_wq has waiting processes * - * The purpose of the wq_has_sleeper and sock_poll_wait is to wrap the memory + * The purpose of the skwq_has_sleeper and sock_poll_wait is to wrap the memory * barrier call. They were added due to the race found within the tcp code. * * Consider following tcp code paths: @@ -1910,15 +1911,9 @@ static inline bool sk_has_allocations(const struct sock *sk) * data on the socket. * */ -static inline bool wq_has_sleeper(struct socket_wq *wq) +static inline bool skwq_has_sleeper(struct socket_wq *wq) { - /* We need to be sure we are in sync with the -* add_wait_queue modifications to the wait queue. -* -* This memory barrier is paired in the sock_poll_wait. -*/ - smp_mb(); - return wq &&
Re: net: Generalise wq_has_sleeper helper
On Tue, Nov 24, 2015 at 04:30:25PM -0500, David Miller wrote: > > I'm fine with wherever this patch goes. Herbert is there any > particular tree where it'll facilitate another user quickest? > > Or should I just toss it into net-next? > > Acked-by: David S. MillerNo Dave net-next is fine I think. This was prompted by Tatsukawa-san's patches to fix waitqueue users affected by this very race and they were all over the tree. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: net: Generalise wq_has_sleeper helper
From: Herbert XuDate: Tue, 24 Nov 2015 13:54:23 +0800 > On Wed, Nov 11, 2015 at 05:48:29PM +0800, Herbert Xu wrote: >> >> BTW, the networking folks found this years ago and even added >> helpers to deal with this. See for example wq_has_sleeper in >> include/net/sock.h. It would be good if we can move some of >> those helpers into wait.h instead. > > Here is a patch against net-next which makes the wq_has_sleeper > helper available to non-next users: > > ---8<--- > The memory barrier in the helper wq_has_sleeper is needed by just > about every user of waitqueue_active. This patch generalises it > by making it take a wait_queue_head_t directly. The existing > helper is renamed to skwq_has_sleeper. > > Signed-off-by: Herbert Xu I'm fine with wherever this patch goes. Herbert is there any particular tree where it'll facilitate another user quickest? Or should I just toss it into net-next? Acked-by: David S. Miller -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
net: Generalise wq_has_sleeper helper
On Wed, Nov 11, 2015 at 05:48:29PM +0800, Herbert Xu wrote: > > BTW, the networking folks found this years ago and even added > helpers to deal with this. See for example wq_has_sleeper in > include/net/sock.h. It would be good if we can move some of > those helpers into wait.h instead. Here is a patch against net-next which makes the wq_has_sleeper helper available to non-next users: ---8<--- The memory barrier in the helper wq_has_sleeper is needed by just about every user of waitqueue_active. This patch generalises it by making it take a wait_queue_head_t directly. The existing helper is renamed to skwq_has_sleeper. Signed-off-by: Herbert Xudiff --git a/crypto/algif_aead.c b/crypto/algif_aead.c index 0aa6fdf..fb99f30 100644 --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -106,7 +106,7 @@ static void aead_wmem_wakeup(struct sock *sk) rcu_read_lock(); wq = rcu_dereference(sk->sk_wq); - if (wq_has_sleeper(wq)) + if (skwq_has_sleeper(wq)) wake_up_interruptible_sync_poll(>wait, POLLIN | POLLRDNORM | POLLRDBAND); @@ -157,7 +157,7 @@ static void aead_data_wakeup(struct sock *sk) rcu_read_lock(); wq = rcu_dereference(sk->sk_wq); - if (wq_has_sleeper(wq)) + if (skwq_has_sleeper(wq)) wake_up_interruptible_sync_poll(>wait, POLLOUT | POLLRDNORM | POLLRDBAND); diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c index af31a0e..0e6702e 100644 --- a/crypto/algif_skcipher.c +++ b/crypto/algif_skcipher.c @@ -238,7 +238,7 @@ static void skcipher_wmem_wakeup(struct sock *sk) rcu_read_lock(); wq = rcu_dereference(sk->sk_wq); - if (wq_has_sleeper(wq)) + if (skwq_has_sleeper(wq)) wake_up_interruptible_sync_poll(>wait, POLLIN | POLLRDNORM | POLLRDBAND); @@ -288,7 +288,7 @@ static void skcipher_data_wakeup(struct sock *sk) rcu_read_lock(); wq = rcu_dereference(sk->sk_wq); - if (wq_has_sleeper(wq)) + if (skwq_has_sleeper(wq)) wake_up_interruptible_sync_poll(>wait, POLLOUT | POLLRDNORM | POLLRDBAND); diff --git a/include/linux/wait.h b/include/linux/wait.h index 1e1bf9f..bd1157f 100644 --- a/include/linux/wait.h +++ b/include/linux/wait.h @@ -107,6 +107,50 @@ static inline int waitqueue_active(wait_queue_head_t *q) return !list_empty(>task_list); } +/** + * wq_has_sleeper - check if there are any waiting processes + * @wq: wait queue head + * + * Returns true if wq has waiting processes + * + * The purpose of the wq_has_sleeper and sock_poll_wait is to wrap the memory + * barrier call. They were added due to the race found within the tcp code. + * + * Consider following tcp code paths: + * + * CPU1 CPU2 + * + * sys_selectreceive packet + * ... ... + * __add_wait_queueupdate tp->rcv_nxt + * ... ... + * tp->rcv_nxt check sock_def_readable + * ... { + * schedule rcu_read_lock(); + * wq = rcu_dereference(sk->sk_wq); + * if (wq && waitqueue_active(>wait)) + * wake_up_interruptible(>wait) + * ... + * } + * + * The race for tcp fires when the __add_wait_queue changes done by CPU1 stay + * in its cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 + * could then endup calling schedule and sleep forever if there are no more + * data on the socket. + * + */ +static inline bool wq_has_sleeper(wait_queue_head_t *wq) +{ + /* We need to be sure we are in sync with the +* add_wait_queue modifications to the wait queue. +* +* This memory barrier should be paired with one on the +* waiting side. +*/ + smp_mb(); + return waitqueue_active(wq); +} + extern void add_wait_queue(wait_queue_head_t *q, wait_queue_t *wait); extern void add_wait_queue_exclusive(wait_queue_head_t *q, wait_queue_t *wait); extern void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait); diff --git a/include/net/sock.h b/include/net/sock.h index bbf7c2c..4a6e9b6 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -58,6 +58,7 @@ #include #include #include +#include #include #include @@ -1879,46 +1880,14 @@ static inline bool sk_has_allocations(const struct sock *sk) } /** - * wq_has_sleeper - check if there are any waiting