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