Re: POLLPRI/poll() behavior change since 2.6.31

[Leonardo Chiquitto <leonardo.lists@gmail.com>]

On Thu, Jan 6, 2011 at 2:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 06 janvier 2011 à 13:50 -0200, Leonardo Chiquitto a écrit :
>> Hello,
>>
>> Since 2.6.31, poll() no longer returns when waiting exclusively for a
>> POLLPRI event. If we wait for POLLPRI | POLLIN, though, it will
>> correctly return POLLPRI as a received event.
>>
>> The reproducer (code below) will print the following when running on
>> 2.6.30:
>>
>> $ ./pollpri-oob
>> main: starting
>> main: setup_pipe ok (sfd[0] = 5, sfd[1] = 4)
>> parent: child <pid 3790> started
>> child: polling...
>> parent: sending the message
>> parent: waiting for child to exit
>> child: poll(): some data <1,2> has arrived!
>> child: done
>> parent: done
>>
>> ... and will block when running on 2.6.37-rc7:
>>
>> $ ./pollpri-oob
>> main: starting
>> main: setup_pipe ok (sfd[0] = 5, sfd[1] = 4)
>> parent: child <pid 14148> started
>> child: polling...
>> parent: sending the message
>> parent: waiting for child to exit
>> [hangs here]
>>
>> I've bisected this behavior change to the following commit:
>>
>> commit 4938d7e0233a455f04507bac81d0886c71529537
>> Author: Eric Dumazet <dada1@cosmosbay.com>
>> Date:   Tue Jun 16 15:33:36 2009 -0700
>>
>>   poll: avoid extra wakeups in select/poll
>>
>>   After introduction of keyed wakeups Davide Libenzi did on epoll, we are
>>   able to avoid spurious wakeups in poll()/select() code too.
>>
>>   For example, typical use of poll()/select() is to wait for incoming
>>   network frames on many sockets.  But TX completion for UDP/TCP frames call
>>   sock_wfree() which in turn schedules thread.
>>
>>   When scheduled, thread does a full scan of all polled fds and can sleep
>>   again, because nothing is really available.  If number of fds is large,
>>   this cause significant load.
>>
>>   This patch makes select()/poll() aware of keyed wakeups and useless
>>   wakeups are avoided.  This reduces number of context switches by about 50%
>>   on some setups, and work performed by sofirq handlers.
>>
>>
>> I don't know if the new behavior is correct, but we've got one report of
>> an application that broke due to the change.
>
> Hi Leonardo
>
> Hmm, this is because    sock_def_readable() uses :
>
> wake_up_interruptible_sync_poll(&wq->wait, POLLIN | POLLRDNORM |
> POLLRDBAND);
>
> So POLLPRI bit is not signaled.
>
> I would just add POLLPRI flag in sock_def_readable()
>
> (Alternatively, define a tcp_def_readable() function to pass POLLPRI
> only if TCP_URG is set, but is it worth the pain for a seldom used
> feature ?)
>
> David, do you have an opinion on this ?
>
> Thanks
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index e5af8d5..7fd3541 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1907,7 +1907,7 @@ static void sock_def_readable(struct sock *sk, int len)
>        rcu_read_lock();
>        wq = rcu_dereference(sk->sk_wq);
>        if (wq_has_sleeper(wq))
> -               wake_up_interruptible_sync_poll(&wq->wait, POLLIN |
> +               wake_up_interruptible_sync_poll(&wq->wait, POLLIN | POLLPRI |
>                                                POLLRDNORM | POLLRDBAND);
>        sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
>        rcu_read_unlock();

Eric,

Thanks for the quick reply. I tested your patch and confirm that it resolves the
problem.

Regards,
Leonardo