Hi,
> -----Original Message-----
> From: Ricardo Biehl Pasquali [mailto:pasqual...@gmail.com]
> Sent: Thursday, October 18, 2018 11:27 AM
> To: netdev@vger.kernel.org
> Cc: Keller, Jacob E <jacob.e.kel...@intel.com>; Gomes, Vinicius
> <vinicius.go...@intel.com>; da...@davemloft.net
> Subject: Issues in error queue polling
>
> 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:
>
Right.
> 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.
>
Yes. The problem being that the application thread not being ready to handle
POLLPRI, so they want to avoid the application waking up to an event.
> 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.
>
Not entirely true but...
> 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 thread wakes up, but the application handling the events doesn't because
the thread goes right back to sleep.
> 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.
>
Yep, this is a better solution, and I wish it had been thought of before we
introduced SO_SELECT_ERR_QUEUE.
> 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).
Right. This is the goal for applications like ptp4l.
>
> 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).
Yes, this makes sense to me.
Thanks,
Jake
