[1/2] net: adding memory barrier to the poll and receive callbacks

Adding memory barrier after the poll_wait function, paired with
receive callbacks. Adding fuctions sock_poll_wait and sock_has_sleeper
to wrap the memory barrier.

Without the memory barrier, following race can happen.
The race fires, when following code paths meet, and the tp->rcv_nxt 
and __add_wait_queue updates stay in CPU caches.


CPU1                         CPU2

sys_select                   receive packet
  ...                        ...
  __add_wait_queue           update tp->rcv_nxt
  ...                        ...
  tp->rcv_nxt check          sock_def_readable
  ...                        {
  schedule                      ...
                                if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
                                        wake_up_interruptible(sk->sk_sleep)
                                ...
                             }

If there was no cache the code would work ok, since the wait_queue and
rcv_nxt are opposit to each other.

Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already
passed the tp->rcv_nxt check and sleeps, or will get the new value for
tp->rcv_nxt and will return with new data mask.
In both cases the process (CPU1) is being added to the wait queue, so the
waitqueue_active (CPU2) call cannot miss and will wake up CPU1.

The bad case is 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 will then
endup calling schedule and sleep forever if there are no more data on the
socket.


Calls to poll_wait in following modules were ommited:
	net/bluetooth/af_bluetooth.c
	net/irda/af_irda.c
	net/irda/irnet/irnet_ppp.c
	net/mac80211/rc80211_pid_debugfs.c
	net/phonet/socket.c
	net/rds/af_rds.c
	net/rfkill/core.c
	net/sunrpc/cache.c
	net/sunrpc/rpc_pipe.c
	net/tipc/socket.c

wbr,
jirka


Signed-off-by: Jiri Olsa <jolsa@redhat.com>

---
 include/linux/poll.h |   11 +++++++++--
 include/net/sock.h   |   40 ++++++++++++++++++++++++++++++++++++++++
 net/atm/common.c     |    6 +++---
 net/core/datagram.c  |    2 +-
 net/core/sock.c      |    8 ++++----
 net/dccp/output.c    |    2 +-
 net/dccp/proto.c     |    2 +-
 net/ipv4/tcp.c       |    2 +-
 net/iucv/af_iucv.c   |    4 ++--
 net/rxrpc/af_rxrpc.c |    4 ++--
 net/unix/af_unix.c   |    8 ++++----
 11 files changed, 68 insertions(+), 21 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 29 Jun 2009, Jiri Olsa wrote:

> -static inline void poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
> +static inline void __poll_wait(struct file *filp,
> +				wait_queue_head_t *wait_address, poll_table *p)
> +{
> +	p->qproc(filp, wait_address, p);
> +}
> +
> +static inline void poll_wait(struct file *filp,
> +				wait_queue_head_t *wait_address, poll_table *p)
>  {
>  	if (p && wait_address)
> -		p->qproc(filp, wait_address, p);
> +		__poll_wait(filp, wait_address, p);
>  }

> +static inline void sock_poll_wait(struct file *filp, struct sock *sk,
> +		poll_table *p)
> +{
> +	if (p && sk->sk_sleep) {
> +		__poll_wait(filp, sk->sk_sleep, p);
> +		/*
> +		 * We need to be sure we are in sync with the
> +		 * socket flags modification.
> +		 *
> +		 * This memory barrier is paired in the sk_has_sleeper.
> +		*/
> +		smp_mb();
> +	}
> +}

I think Oleg already said this, but you can use directly poll_wait() 
without adding another abstraction, and the compiler will drop the double 
check for you:

extern void foo(int, int, int);
extern void mb(void);

static inline void cfoo(int a, int b, int c) {
	if (b && c)
		foo(a, b, c);
}

void xxx(int a, int b, int c) {
	if (b && c) {
		cfoo(a, b, c);
		mb();
	}
}
-----
xxx:
	subq	$8, %rsp
	testl	%esi, %esi
	je	.L3
	testl	%edx, %edx
	je	.L3
	call	foo
	addq	$8, %rsp
	jmp	mb
.L3:
	addq	$8, %rsp
	ret



- Davide


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 29, 2009 at 04:14:45PM +0200, Jiri Olsa wrote:
...
> +static inline void sock_poll_wait(struct file *filp, struct sock *sk,
...
> +	sock_poll_wait(file, sk->sk_sleep, wait);

-----------------------------^^^^^^^^^^^^
Is it something with my eyes or it wasn't compiled? ;-)

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 29, 2009 at 08:34:55AM -0700, Davide Libenzi wrote:
> On Mon, 29 Jun 2009, Jiri Olsa wrote:
> 
> > -static inline void poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
> > +static inline void __poll_wait(struct file *filp,
> > +				wait_queue_head_t *wait_address, poll_table *p)
> > +{
> > +	p->qproc(filp, wait_address, p);
> > +}
> > +
> > +static inline void poll_wait(struct file *filp,
> > +				wait_queue_head_t *wait_address, poll_table *p)
> >  {
> >  	if (p && wait_address)
> > -		p->qproc(filp, wait_address, p);
> > +		__poll_wait(filp, wait_address, p);
> >  }
> 
> > +static inline void sock_poll_wait(struct file *filp, struct sock *sk,
> > +		poll_table *p)
> > +{
> > +	if (p && sk->sk_sleep) {
> > +		__poll_wait(filp, sk->sk_sleep, p);
> > +		/*
> > +		 * We need to be sure we are in sync with the
> > +		 * socket flags modification.
> > +		 *
> > +		 * This memory barrier is paired in the sk_has_sleeper.
> > +		*/
> > +		smp_mb();
> > +	}
> > +}
> 
> I think Oleg already said this, but you can use directly poll_wait() 
> without adding another abstraction, and the compiler will drop the double 
> check for you:

I think Oleg told about cosmetics and let Jiri to choose. I'd only
add it's not mainly about optimization, but easy showing the main
difference, of course depending on taste.

Jarek P. 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 29 Jun 2009, Jarek Poplawski wrote:

> > I think Oleg already said this, but you can use directly poll_wait() 
> > without adding another abstraction, and the compiler will drop the double 
> > check for you:
> 
> I think Oleg told about cosmetics and let Jiri to choose. I'd only
> add it's not mainly about optimization, but easy showing the main
> difference, of course depending on taste.

We already have a universally used function to do that, and that's 
poll_wait().
That code (adding an extra __poll_wait()) was entirely about 
optimizations (otherwise why not use the existing poll_wait()?), so if 
the optimization does not actually take place, IMO it's better to not add 
an extra API.



- Davide


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 29, 2009 at 04:14:45PM +0200, Jiri Olsa wrote:
...
> +/**
> + * sk_has_sleeper - check if there are any waiting processes
> + * @sk: socket
> + *
> + * Returns true if socket has waiting processes
> + */
> +static inline int sk_has_sleeper(struct sock *sk)
> +{
> +	/*
> +	 * 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 sk->sk_sleep && waitqueue_active(sk->sk_sleep);
> +}
> +

Btw. I hope Jiri won't "listen" to me, but I can't stop to mention
sock_waitqueue_active() looks to me quite naturally better paired
with sock_poll_wait() than sk_has_sleeper(which otherwise is more
conspicuous, sorry Eric.)

Jarek P.

> +/**
> + * sock_poll_wait - place memory barrier behind the __poll_wait call.
> + * @filp: file
> + * @sk:   socket
> + * @p:    poll_table
> + */
> +static inline void sock_poll_wait(struct file *filp, struct sock *sk,
> +		poll_table *p)
> +{
> +	if (p && sk->sk_sleep) {
> +		__poll_wait(filp, sk->sk_sleep, p);
> +		/*
> +		 * We need to be sure we are in sync with the
> +		 * socket flags modification.
> +		 *
> +		 * This memory barrier is paired in the sk_has_sleeper.
> +		*/
> +		smp_mb();
> +	}
> +}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 29, 2009 at 10:36:30AM -0700, Davide Libenzi wrote:
> On Mon, 29 Jun 2009, Jarek Poplawski wrote:
> 
> > > I think Oleg already said this, but you can use directly poll_wait() 
> > > without adding another abstraction, and the compiler will drop the double 
> > > check for you:
> > 
> > I think Oleg told about cosmetics and let Jiri to choose. I'd only
> > add it's not mainly about optimization, but easy showing the main
> > difference, of course depending on taste.
> 
> We already have a universally used function to do that, and that's 
> poll_wait().
> That code (adding an extra __poll_wait()) was entirely about 
> optimizations (otherwise why not use the existing poll_wait()?), so if 
> the optimization does not actually take place, IMO it's better to not add 
> an extra API.

OK, you're right, it is about optimization! But IMHO mainly about
reading optimization... I simply guess me and probably Jiri too,
after reading Oleg's variant thought about compiler, instead of the
real difference.

Btw., maybe I miss something but I guess Oleg proposed something in
between: inlining __poll_wait(), which would save us 'extra API' and
compiler doubts. (But I still prefer Jiri's choice. ;-)

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 29, 2009 at 08:04:31PM +0200, Jarek Poplawski wrote:
...
> Btw., maybe I miss something but I guess Oleg proposed something in
> between: inlining __poll_wait(), which would save us 'extra API' and
> compiler doubts. (But I still prefer Jiri's choice. ;-)

After re-reading I guess Oleg didn't proposed anything in between yet.

Sorry,
Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 29, 2009 at 08:14:42PM +0200, Jarek Poplawski wrote:
...
> > (But I still prefer Jiri's choice. ;-)

...Even if Jiri decides to change his mind, because it really wasn't
intended for any persuasions.

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 29, 2009 at 07:19:26PM +0200, Jarek Poplawski wrote:
> On Mon, Jun 29, 2009 at 04:14:45PM +0200, Jiri Olsa wrote:
> ...
> > +static inline void sock_poll_wait(struct file *filp, struct sock *sk,
> ...
> > +	sock_poll_wait(file, sk->sk_sleep, wait);
> 
> -----------------------------^^^^^^^^^^^^
> Is it something with my eyes or it wasn't compiled? ;-)
> 
> Jarek P.

your eyes are great, my brain is screwed... sry about that, I'll resend

it was compiled actually.. must have ended up with warning I did not
noticed... thanks!

jirka
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 29, 2009 at 10:36:30AM -0700, Davide Libenzi wrote:
> On Mon, 29 Jun 2009, Jarek Poplawski wrote:
> 
> > > I think Oleg already said this, but you can use directly poll_wait() 
> > > without adding another abstraction, and the compiler will drop the double 
> > > check for you:
> > 
> > I think Oleg told about cosmetics and let Jiri to choose. I'd only
> > add it's not mainly about optimization, but easy showing the main
> > difference, of course depending on taste.
> 
> We already have a universally used function to do that, and that's 
> poll_wait().
> That code (adding an extra __poll_wait()) was entirely about 
> optimizations (otherwise why not use the existing poll_wait()?), so if 
> the optimization does not actually take place, IMO it's better to not add 
> an extra API.
> 
> 
> 
> - Davide
> 
>

my thinking was that both variants will endup in the same code anyway,
so it'd be probably better if the more readable (subjective) got in..

however I dont have any strong preffering feelings about either of those choices,
so I can convert easilly :)

jirka
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 29 Jun 2009, Jiri Olsa wrote:

> my thinking was that both variants will endup in the same code anyway,
> so it'd be probably better if the more readable (subjective) got in..
> 
> however I dont have any strong preffering feelings about either of those choices,
> so I can convert easilly :)

Please use the existing poll_wait() then, as there's no reason to add 
another API.


- Davide


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html