casaroli commented on code in PR #21477:
URL: https://github.com/apache/nuttx/pull/21477#discussion_r4515635339
##########
fs/vfs/fs_epoll.c:
##########
@@ -282,119 +320,290 @@ static int epoll_do_create(int size, int flags)
}
/****************************************************************************
- * Name: epoll_setup
+ * Name: epoll_refresh
*
* Description:
- * Setup all the fd.
- *
- * Input Parameters:
- * eph - The epoll head pointer
- *
- * Returned Value:
- * Positive on success, negative on fail
+ * Re-query level-triggered nodes that were delivered by the previous
+ * epoll_wait(). They are refreshed at the beginning of the next wait so
+ * fd state that remains ready (pipe/eventfd/socket/listener data still
+ * pending) is reported again, without suppressing real notifications that
+ * arrive between waits.
*
****************************************************************************/
-static int epoll_setup(FAR epoll_head_t *eph)
+static void epoll_refresh(FAR epoll_head_t *eph)
{
- FAR epoll_node_t *tepn;
FAR epoll_node_t *epn;
+ FAR epoll_node_t *tmp;
+ irqstate_t flags;
int ret;
- ret = nxmutex_lock(&eph->lock);
- if (ret < 0)
- {
- return ret;
- }
+ nxmutex_lock(&eph->lock);
- list_for_every_entry_safe(&eph->teardown, epn, tepn, epoll_node_t, node)
+ list_for_every_entry_safe(&eph->refresh, epn, tmp, epoll_node_t, node)
{
- /* Setup again to check the notified pollfd last epoll_wait() to
- * cover the situation several poll event pending on one fd.
- */
-
- epn->notified = false;
- epn->pfd.revents = 0;
- ret = file_poll(epn->filep, &epn->pfd, true);
- if (ret < 0)
+ if (!epn->queued)
{
- ferr("epoll setup failed, filep=%p, events=%08" PRIx32 ", "
- "ret=%d\n", epn->filep, epn->pfd.events, ret);
- break;
+ flags = spin_lock_irqsave(&eph->ready_lock);
+ epn->pfd.revents = 0;
+ spin_unlock_irqrestore(&eph->ready_lock, flags);
+
+ ret = file_poll(epn->filep, &epn->pfd, true);
+ if (ret < 0)
+ {
+ ferr("epoll refresh failed, filep=%p, events=%08" PRIx32 ", "
+ "ret=%d\n", epn->filep, epn->pfd.events, ret);
+
+ epn->armed = epn->pfd.priv != NULL;
+ epn->pfd.revents = POLLERR;
+ epoll_node_ready(eph, epn);
+ }
}
list_delete(&epn->node);
list_add_tail(&eph->setup, &epn->node);
}
nxmutex_unlock(&eph->lock);
- return ret;
}
/****************************************************************************
- * Name: epoll_teardown
+ * Name: epoll_collect
*
* Description:
- * Teardown all the notified fd and check the notified fd's event with user
- * expected event.
+ * Deliver all the events currently pending in the ready list. For each
+ * EPOLLONESHOT node, drop its driver poll registration and park it into
+ * the oneshot list until it is re-armed by epoll_ctl(). For every other
+ * (level-triggered) node, keep the registration and park it into the
+ * refresh list so it can be re-queried by the next epoll_wait().
*
* Input Parameters:
* eph - The epoll head pointer
* evs - The epoll events array
* maxevents - The epoll events array size
*
* Returned Value:
- * Return the number of fd that notified and the events is also user
- * expected.
+ * Return the number of fd that delivered to the user.
*
****************************************************************************/
-static int epoll_teardown(FAR epoll_head_t *eph, FAR struct epoll_event *evs,
- int maxevents)
+static int epoll_collect(FAR epoll_head_t *eph, FAR struct epoll_event *evs,
+ int maxevents)
{
- FAR epoll_node_t *tepn;
FAR epoll_node_t *epn;
- int i = 0;
+ struct list_node deliver;
+ irqstate_t flags;
+ int i = 0;
+
+ list_initialize(&deliver);
nxmutex_lock(&eph->lock);
- list_for_every_entry_safe(&eph->setup, epn, tepn, epoll_node_t, node)
+ /* Phase 1: atomically move at most maxevents ready nodes out of the
+ * ready list into a private queue, so nodes not delivered to userspace
+ * stay queued for the next epoll_wait().
+ *
+ * Keep epn->queued true while a node is on the private deliver queue.
+ * The same rnode link is still in use there, so a concurrent
+ * driver notification must not enqueue it into eph->ready again. The
+ * epoll mutex also prevents epoll_ctl() from trying to dequeue a node
+ * from this private queue.
+ */
+
+ flags = spin_lock_irqsave(&eph->ready_lock);
+ while (i < maxevents && !list_is_empty(&eph->ready))
+ {
+ epn = container_of(list_remove_head(&eph->ready), epoll_node_t, rnode);
+ list_add_tail(&deliver, &epn->rnode);
+ i++;
+ }
+
+ if (!list_is_empty(&eph->ready))
{
- /* Only check the notified fd */
+ int semcount;
- if (!epn->notified)
+ nxsem_get_value(&eph->sem, &semcount);
+ if (semcount < 1)
{
- continue;
+ nxsem_post(&eph->sem);
}
+ }
- /* Teradown all the notified fd */
+ spin_unlock_irqrestore(&eph->ready_lock, flags);
+ i = 0;
- file_poll(epn->filep, &epn->pfd, false);
- list_delete(&epn->node);
+ /* Phase 2: classify and deliver. */
+
+ while (!list_is_empty(&deliver))
+ {
+ epn = container_of(list_remove_head(&deliver), epoll_node_t, rnode);
if (epn->pfd.revents != 0 && i < maxevents)
{
- evs[i].data = epn->data;
+ evs[i].data = epn->data;
evs[i++].events = epn->pfd.revents;
- if ((epn->pfd.events & EPOLLONESHOT) != 0)
- {
- list_add_tail(&eph->oneshot, &epn->node);
- }
- else
+ }
+
+ if ((epn->pfd.events & EPOLLONESHOT) != 0)
+ {
+ /* EPOLLONESHOT: tear down the driver poll and park the node until
+ * it is re-armed by epoll_ctl(EPOLL_CTL_MOD).
+ */
+
+ if (epn->armed)
{
- list_add_tail(&eph->teardown, &epn->node);
+ file_poll(epn->filep, &epn->pfd, false);
+ epn->armed = false;
}
+
+ flags = spin_lock_irqsave(&eph->ready_lock);
+ epn->queued = false;
+ spin_unlock_irqrestore(&eph->ready_lock, flags);
+
+ list_delete(&epn->node);
+ list_add_tail(&eph->oneshot, &epn->node);
}
else
{
- list_add_tail(&eph->teardown, &epn->node);
+ /* Level-triggered: keep ownership stable, allow new asynchronous
+ * notifications to queue again, and re-query readiness at the
+ * beginning of the next epoll_wait().
+ */
+
+ flags = spin_lock_irqsave(&eph->ready_lock);
Review Comment:
A delivered level-triggered node keeps its old `revents` until the next
`epoll_refresh()`, and `epoll_refresh()` skips the node when it is queued (line
345). Because `pfd.events` has `POLLALWAYS`, `poll_notify()` calls
`epoll_default_cb()` for any event, and the old `POLLIN` queues the node: the
`POLLOUT` that `pipecommon_read()` (drivers/pipes/pipe_common.c:499) and
`eventfd_do_read()` (fs/vfs/fs_eventfd.c:333) send after a read is enough. Test
on qemu-armv8a:knsh: a pipe with 1 byte, `epoll_wait()`, `read()` the byte,
`epoll_wait(ep, &ev, 1, 300)` returns 1 (`EPOLLIN`) with this PR, 0 on master
and on Linux; the same for an eventfd. A blocking `read()` after this report
waits for new data.
Fix: clear `pfd.revents` under `ready_lock` when a delivered level-triggered
node moves to the refresh list in `epoll_collect()`.
##########
fs/vfs/fs_epoll.c:
##########
@@ -282,119 +320,290 @@ static int epoll_do_create(int size, int flags)
}
/****************************************************************************
- * Name: epoll_setup
+ * Name: epoll_refresh
*
* Description:
- * Setup all the fd.
- *
- * Input Parameters:
- * eph - The epoll head pointer
- *
- * Returned Value:
- * Positive on success, negative on fail
+ * Re-query level-triggered nodes that were delivered by the previous
+ * epoll_wait(). They are refreshed at the beginning of the next wait so
+ * fd state that remains ready (pipe/eventfd/socket/listener data still
+ * pending) is reported again, without suppressing real notifications that
+ * arrive between waits.
*
****************************************************************************/
-static int epoll_setup(FAR epoll_head_t *eph)
+static void epoll_refresh(FAR epoll_head_t *eph)
{
- FAR epoll_node_t *tepn;
FAR epoll_node_t *epn;
+ FAR epoll_node_t *tmp;
+ irqstate_t flags;
int ret;
- ret = nxmutex_lock(&eph->lock);
- if (ret < 0)
- {
- return ret;
- }
+ nxmutex_lock(&eph->lock);
- list_for_every_entry_safe(&eph->teardown, epn, tepn, epoll_node_t, node)
+ list_for_every_entry_safe(&eph->refresh, epn, tmp, epoll_node_t, node)
{
- /* Setup again to check the notified pollfd last epoll_wait() to
- * cover the situation several poll event pending on one fd.
- */
-
- epn->notified = false;
- epn->pfd.revents = 0;
- ret = file_poll(epn->filep, &epn->pfd, true);
- if (ret < 0)
+ if (!epn->queued)
{
- ferr("epoll setup failed, filep=%p, events=%08" PRIx32 ", "
- "ret=%d\n", epn->filep, epn->pfd.events, ret);
- break;
+ flags = spin_lock_irqsave(&eph->ready_lock);
+ epn->pfd.revents = 0;
+ spin_unlock_irqrestore(&eph->ready_lock, flags);
+
+ ret = file_poll(epn->filep, &epn->pfd, true);
Review Comment:
`epoll_refresh()` sets up a pollfd that is still bound, and this commit
removes the -EBUSY fallback of `file_poll()`. The socket types return -EBUSY
for a bound pollfd (commit 2, e.g. `tcp_pollsetup()` `if (fds->priv != NULL)
return -EBUSY;`), so lines 357-359 queue `POLLERR` on every wait after the
first event. Test on qemu-armv8a:knsh, this PR merged into master: for TCP, a
TCP listener, UDP and AF_UNIX stream sockets, the first `epoll_wait()` returns
`EPOLLIN`, every later one returns `EPOLLERR`, also after all data is read
(master and Linux: `EPOLLIN`, then nothing). The drivers that commit 2 does not
change get the same second setup: `can_poll()` (drivers/can/can.c:1255) and
`touch_poll()` (drivers/input/touchscreen_upper.c:357) return -EBUSY,
`adc_poll()` (drivers/analog/adc.c:637) takes one more slot on each wait.
Fix: keep the teardown + setup fallback for a driver that returns -EBUSY on
a bound pollfd (as `poll_setup_dispatch()` did), or tear down the delivered
node before the refresh setup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
