On 2/3/22 10:41 AM, Usama Arif wrote:
> @@ -1726,13 +1732,24 @@ static inline struct io_uring_cqe *io_get_cqe(struct io_ring_ctx *ctx)
> return &rings->cqes[tail & mask];
> }
>
> -static inline bool io_should_trigger_evfd(struct io_ring_ctx *ctx)
> +static void io_eventfd_signal(struct io_ring_ctx *ctx)
> {
> - if (likely(!ctx->cq_ev_fd))
> - return false;
> + struct io_ev_fd *ev_fd;
> +
> + rcu_read_lock();
> + /* rcu_dereference ctx->io_ev_fd once and use it for both for checking and eventfd_signal */
> + ev_fd = rcu_dereference(ctx->io_ev_fd);
> +
> + if (likely(!ev_fd))
> + goto out;
> if (READ_ONCE(ctx->rings->cq_flags) & IORING_CQ_EVENTFD_DISABLED)
> - return false;
> - return !ctx->eventfd_async || io_wq_current_is_worker();
> + goto out;
> +
> + if (!ctx->eventfd_async || io_wq_current_is_worker())
> + eventfd_signal(ev_fd->cq_ev_fd, 1);
> +
> +out:
> + rcu_read_unlock();
> }
Like Pavel pointed out, we still need the fast path (of not having an
event fd registered at all) to just do the cheap check and not need rcu
lock/unlock. Outside of that, I think this looks fine.
> static int io_eventfd_unregister(struct io_ring_ctx *ctx)
> {
> - if (ctx->cq_ev_fd) {
> - eventfd_ctx_put(ctx->cq_ev_fd);
> - ctx->cq_ev_fd = NULL;
> - return 0;
> + struct io_ev_fd *ev_fd;
> + int ret;
> +
> + mutex_lock(&ctx->ev_fd_lock);
> + ev_fd = rcu_dereference_protected(ctx->io_ev_fd, lockdep_is_held(&ctx->ev_fd_lock));
> + if (!ev_fd) {
> + ret = -ENXIO;
> + goto out;
> }
> + synchronize_rcu();
> + eventfd_ctx_put(ev_fd->cq_ev_fd);
> + kfree(ev_fd);
> + rcu_assign_pointer(ctx->io_ev_fd, NULL);
> + ret = 0;
>
> - return -ENXIO;
> +out:
> + mutex_unlock(&ctx->ev_fd_lock);
> + return ret;
> }
synchronize_rcu() can take a long time, and I think this is in the wrong
spot. It should be on the register side, IFF we need to expedite the
completion of a previous event fd unregistration. If we do it that way,
at least it'll only happen if it's necessary. What do you think?
--
Jens Axboe
