Hello Ricardo, My apologies for not attending to your earlier mails... Thanks for persisting. On Tue, 5 Feb 2019 at 20:08, Ricardo Biehl Pasquali <pasqualirb@xxxxxxxxx> wrote: > > Signed-off-by: Ricardo Biehl Pasquali <pasqualirb@xxxxxxxxx> > --- > man7/socket.7 | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > Note: I've already sent this patch two times 2018-10-21 and > 2018-12-20. Jacob E. Keller gave a feedback in the first > send. > > I've commented some issues of error queue polling in > network mailing list: > > https://www.spinics.net/lists/netdev/msg529722.html > > Ricardo Biehl Pasquali wrote (2018-10-18): > > The commit 7d4c04fc170087119727 ("net: add option to enable > > error queue packets waking select") (2013-03-28) introduced > > SO_SELECT_ERR_QUEUE, which masks POLLPRI with POLLERR event > > return in some socket poll callbacks. > > > > POLLERR event issued with sock_queue_err_skb() did not wake > > up a poll when POLLERR is the only requested event because > > sk_data_ready() (sock_def_readable()) was used and it > > doesn't mask POLLERR in poll wake up: > > > > wake_up_interruptible_sync_poll(&wq->wait, > > EPOLLIN | EPOLLPRI | > > EPOLLRDNORM | EPOLLRDBAND); > > > > If POLLIN or POLLPRI are requested, for example, poll does > > wake up. > > > > POLLERR wakeup by requesting POLLPRI is possible without > > set SO_SELECT_ERR_QUEUE. All the option does is masking > > POLLPRI as a returned event before poll returns. poll > > would return anyway because of POLLERR. > > > > Also, the sentence "[...] enable software to wait on error > > queue packets without waking up for regular data on the > > socket." from the above commit is not true. > > > > A POLLIN event issued via sock_def_readable() wakes up > > threads waiting for POLLPRI, and vice versa. However, > > poll() does not return, sleeping again, as the requested > > events do not match events. > > > > The commit 6e5d58fdc9bedd0255a8 ("skbuff: Fix not waking > > applications when errors are enqueued") (2018-03-14) make > > POLLERR alone wake up poll. It replaces sk_data_ready() > > (sock_def_readable()) with sk_error_report() > > (sock_def_error_report()). This makes "POLLERR wake up by > > requesting POLLPRI" obsolete. > > > > Rationale: > > > > POLLIN-only and POLLERR-only wake up are useful when there > > is a receiving thread, a sending thread, and a thread that > > get transmit timestamps. The thread polling on POLLERR will > > not wake up when regular data arrives (POLLIN). The thread > > polling on POLLIN will not wake up when tx timestamps are > > ready (POLLERR). > > > > One solution is adding an option that disable POLLERR as > > requested event. This is in the Virtual File System > > subsystem, not in the network, though. > > > > This solves the problem of waking up other threads that > > not interested in error queue. Thus allowing a separate > > thread take care of error queue (useful for receiving > > transmit timestamps). > > > > However, this may not be a good solution as it depends on > > polling behavior. Thoughts? > > > > By the way, as the kernel development shouldn't break user > > space, SO_SELECT_ERR_QUEUE can become a compatibility > > option. > > I have tried to explain the effect of the option and the > context in which it was introduced (more or less the reason > why it was introduced). The POLLIN-POLLPRI mutual wake up > issue was not commented. > > Is it clear? Should a NOTE be written about the other > issues? > > Cheers! > > pasquali > > diff --git a/man7/socket.7 b/man7/socket.7 > index 478fa364c..1ba308198 100644 > --- a/man7/socket.7 > +++ b/man7/socket.7 > @@ -49,10 +49,6 @@ > .\" commit a8fc92778080c845eaadc369a0ecf5699a03bef0 > .\" Author: Pavel Emelyanov <xemul@xxxxxxxxxxxxx> > .\" > -.\" SO_SELECT_ERR_QUEUE (3.10) > -.\" commit 7d4c04fc170087119727119074e72445f2bb192b > -.\" Author: Keller, Jacob E <jacob.e.keller@xxxxxxxxx> > -.\" > .\" SO_MAX_PACING_RATE (3.13) > .\" commit 62748f32d501f5d3712a7c372bbb92abc7c62bc7 > .\" Author: Eric Dumazet <edumazet@xxxxxxxxxx> > @@ -869,6 +865,29 @@ Indicates that an unsigned 32-bit value ancillary message (cmsg) > should be attached to received skbs indicating > the number of packets dropped by the socket since its creation. > .TP > +.BR SO_SELECT_ERR_QUEUE " (since Linux 3.10)" > +.\" commit 7d4c04fc170087119727119074e72445f2bb192b > +.\" Author: Keller, Jacob E <jacob.e.keller@xxxxxxxxx> > +Masks > +.B POLLPRI > +with > +.B POLLERR > +event return. What does "Masks... with" mean? It's not clear to me. > This was created when Does "This was created when" mean "This flag was added when"? > +.B POLLERR > +wake up depended on requesting > +.B POLLIN > +or > +.BR POLLPRI . > +After the commit 6e5d58fdc9bedd0255a8 ("skbuff: Fix not > +waking applications when errors are enqueued"), introduced > +in Linux 4.16, > +.B POLLERR > +wakes up normally. > +The option has no effect in wake up behavior, all it does > +is masking > +.B POLLPRI > +before poll returns. So, I'm not sure if I quite understand the text above. Do you mean that since Linux 4.16, SO_SELECT_ERR_QUEUE is no longere needed for its original purpose and that now its only effect is to "mask POLLPRI"? (And still, it's not clear to me what "masking POLLPRI" means.) Would you be able to answer the above questions, and then I could make some suggestions for a revised patch. Thanks, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/
