casaroli commented on code in PR #21477:
URL: https://github.com/apache/nuttx/pull/21477#discussion_r4498328611
##########
fs/vfs/fs_poll.c:
##########
@@ -306,6 +306,55 @@ void poll_notify(FAR struct pollfd_s **afds, int nfds,
pollevent_t eventset)
}
}
+/****************************************************************************
+ * Name: poll_setup_dispatch
+ *
+ * Description:
+ * Dispatch a poll setup to the underlying driver poll handler. When the
+ * pollfd is already registered (fds->priv != NULL), i.e. a persistent
+ * poll consumer such as epoll is re-querying readiness after a prior
+ * registration, a legacy driver that has not been migrated to the
+ * persistent/refresh model will refuse with -EBUSY because no free slot
+ * is found. In that case, rebuild the binding
+ * through the legacy teardown + set-up cycle so the semantics stay
+ * intact. The saved priv pointer is restored before tearing down
+ * because some legacy drivers clear it on a failed set-up.
+ *
+ * Input Parameters:
+ * poll - The driver poll handler
+ * filep - File structure instance
+ * fds - The structure describing the events to be monitored, OR NULL if
+ * this is a request to stop monitoring events.
+ *
+ * Returned Value:
+ * 0: Success; Negated errno on failure
+ *
+ ****************************************************************************/
+
+static int poll_setup_dispatch(FAR int (*poll)(FAR struct file *filep,
+ FAR struct pollfd_s *fds, bool setup),
+ FAR struct file *filep,
+ FAR struct pollfd_s *fds)
+{
+ FAR void *prev_priv = fds->priv;
+ int ret;
+
+ ret = poll(filep, fds, true);
Review Comment:
The refresh path is not reached: every `file_poll(..., true)` in fs_epoll.c
follows a `file_poll(..., false)` (lines 322, 423, 543), the teardowns set
`fds->priv` to NULL, and `poll()` clears it before the setup. No caller passes
a bound pollfd, so `prev_priv` is always NULL here, and the `fds->priv != NULL`
branches the second commit adds to 27 driver files and 9 socket types never run.
Fix: drop `poll_setup_dispatch()` and the second commit, or make
`epoll_refresh_node()` call the refresh without the teardown.
##########
fs/vfs/fs_epoll.c:
##########
@@ -282,119 +303,276 @@ static int epoll_do_create(int size, int flags)
}
/****************************************************************************
- * Name: epoll_setup
+ * Name: epoll_refresh_node
*
* Description:
- * Setup all the fd.
- *
- * Input Parameters:
- * eph - The epoll head pointer
- *
- * Returned Value:
- * Positive on success, negative on fail
+ * Re-query the current readiness of a level-triggered node. The driver
+ * poll is torn down before revents is cleared so an asynchronous
+ * poll_notify() cannot race with the core-side revents reset while the
+ * driver still owns this pollfd.
*
****************************************************************************/
-static int epoll_setup(FAR epoll_head_t *eph)
+static void epoll_refresh_node(FAR epoll_head_t *eph, FAR epoll_node_t *epn)
{
- FAR epoll_node_t *tepn;
- FAR epoll_node_t *epn;
- int ret;
+ irqstate_t flags;
- ret = nxmutex_lock(&eph->lock);
- if (ret < 0)
- {
- return ret;
- }
+ /* Drop the driver reference before clearing revents. */
- list_for_every_entry_safe(&eph->teardown, epn, tepn, 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.
- */
+ file_poll(epn->filep, &epn->pfd, false);
- epn->notified = false;
- epn->pfd.revents = 0;
- ret = file_poll(epn->filep, &epn->pfd, true);
- if (ret < 0)
- {
- ferr("epoll setup failed, filep=%p, events=%08" PRIx32 ", "
- "ret=%d\n", epn->filep, epn->pfd.events, ret);
- break;
- }
+ /* Clear the accumulated revents and ready state, ask the driver to
+ * report whether the state is still ready.
+ */
- list_delete(&epn->node);
- list_add_tail(&eph->setup, &epn->node);
- }
+ flags = spin_lock_irqsave(&eph->ready_lock);
+ epn->queued = false;
+ epn->pfd.revents = 0;
+ spin_unlock_irqrestore(&eph->ready_lock, flags);
- nxmutex_unlock(&eph->lock);
- return ret;
+ epn->suppress = true;
Review Comment:
A level-triggered fd with unread data is not reported again. The refresh
runs right after delivery, and `suppress` drops what the driver reports for the
data still in the buffer, so the node waits for a new driver event. Test on
qemu-armv8a:knsh: a pipe with 1 unread byte, the second `epoll_wait(ep, &ev, 1,
200)` returns 0 with this PR and 1 on master and on Linux; the same for an
eventfd that is not read, a TCP socket with 2 bytes after 1 is read, and a
listening AF_UNIX socket with 2 waiting connections after 1 `accept()`. The
same window also drops a real event from an interrupt or another CPU during the
setup.
Fix: remove `suppress`, and re-query the delivered level-triggered nodes
(clear `revents`, `file_poll(..., true)`) at the start of the next
`epoll_wait()`, after the caller has read, as the old `epoll_setup()` did; that
also keeps the eventfd case from reporting twice.
##########
fs/vfs/fs_epoll.c:
##########
@@ -282,119 +303,276 @@ static int epoll_do_create(int size, int flags)
}
/****************************************************************************
- * Name: epoll_setup
+ * Name: epoll_refresh_node
*
* Description:
- * Setup all the fd.
- *
- * Input Parameters:
- * eph - The epoll head pointer
- *
- * Returned Value:
- * Positive on success, negative on fail
+ * Re-query the current readiness of a level-triggered node. The driver
+ * poll is torn down before revents is cleared so an asynchronous
+ * poll_notify() cannot race with the core-side revents reset while the
+ * driver still owns this pollfd.
*
****************************************************************************/
-static int epoll_setup(FAR epoll_head_t *eph)
+static void epoll_refresh_node(FAR epoll_head_t *eph, FAR epoll_node_t *epn)
{
- FAR epoll_node_t *tepn;
- FAR epoll_node_t *epn;
- int ret;
+ irqstate_t flags;
- ret = nxmutex_lock(&eph->lock);
- if (ret < 0)
- {
- return ret;
- }
+ /* Drop the driver reference before clearing revents. */
- list_for_every_entry_safe(&eph->teardown, epn, tepn, 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.
- */
+ file_poll(epn->filep, &epn->pfd, false);
- epn->notified = false;
- epn->pfd.revents = 0;
- ret = file_poll(epn->filep, &epn->pfd, true);
- if (ret < 0)
- {
- ferr("epoll setup failed, filep=%p, events=%08" PRIx32 ", "
- "ret=%d\n", epn->filep, epn->pfd.events, ret);
- break;
- }
+ /* Clear the accumulated revents and ready state, ask the driver to
+ * report whether the state is still ready.
+ */
- list_delete(&epn->node);
- list_add_tail(&eph->setup, &epn->node);
- }
+ flags = spin_lock_irqsave(&eph->ready_lock);
+ epn->queued = false;
+ epn->pfd.revents = 0;
+ spin_unlock_irqrestore(&eph->ready_lock, flags);
- nxmutex_unlock(&eph->lock);
- return ret;
+ epn->suppress = true;
+ file_poll(epn->filep, &epn->pfd, true);
Review Comment:
The result of the setup is dropped. If it fails (no free slot, because
another `poll()` took it after the teardown on line 322), the node stays on
`eph->setup` with `pfd.priv` NULL: the fd is never reported again, and a later
`EPOLL_CTL_DEL` or close of the epoll fd calls the teardown with `priv` NULL,
which `timerfd_poll()` writes through (`*slot = NULL`, fs/vfs/fs_timerfd.c:375).
Fix: on failure, park the node on a list that `EPOLL_CTL_DEL` and close
handle without a teardown, and report `POLLERR` for it, as `epoll_wait()`
returned the error before.
--
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]
