On 03/02/2022 18:49, Jens Axboe wrote:
On 2/3/22 11:24 AM, Usama Arif wrote:
-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();
}
This still needs what we discussed in v3, something ala:
/*
* This will potential race with eventfd registration, but that's
* always going to be the case if there is IO inflight while an eventfd
* descriptor is being registered.
*/
if (!rcu_dereference_raw(ctx->io_ev_fd))
return;
rcu_read_lock();
Hmm, so i am not so worried about the registeration, but actually
worried about unregisteration.
If after the check and before the rcu_read_lock, the eventfd is
unregistered won't we get a NULL pointer exception at
eventfd_signal(ev_fd->cq_ev_fd, 1)?
I guess checking for NULL twice would work, so something like this is ok
then?
static void io_eventfd_signal(struct io_ring_ctx *ctx)
{
struct io_ev_fd *ev_fd;
/* Return quickly if ctx->io_ev_fd doesn't exist */
if (likely(!rcu_dereference_raw(ctx->io_ev_fd)))
return;
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);
/*
* Check again if ev_fd exists incase an io_eventfd_unregister call
completed between
* the NULL check of ctx->io_ev_fd at the start of the function and
rcu_read_lock.
*/
if (unlikely(!ev_fd))
goto out;
if (READ_ONCE(ctx->rings->cq_flags) & IORING_CQ_EVENTFD_DISABLED)
goto out;
if (!ev_fd->eventfd_async || io_wq_current_is_worker())
eventfd_signal(ev_fd->cq_ev_fd, 1);
out:
rcu_read_unlock();
}
...
which I think is cheap enough and won't hit sparse complaints. The
@@ -9353,35 +9370,70 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx,
static int io_eventfd_register(struct io_ring_ctx *ctx, void __user *arg)
{
+ struct io_ev_fd *ev_fd;
__s32 __user *fds = arg;
- int fd;
+ int fd, ret;
- if (ctx->cq_ev_fd)
- return -EBUSY;
+ mutex_lock(&ctx->ev_fd_lock);
+ ret = -EBUSY;
+ if (rcu_dereference_protected(ctx->io_ev_fd, lockdep_is_held(&ctx->ev_fd_lock))) {
+ rcu_barrier();
+ if(rcu_dereference_protected(ctx->io_ev_fd, lockdep_is_held(&ctx->ev_fd_lock)))
+ goto out;
+ }
I wonder if we can get away with assigning ctx->io_ev_fd to NULL when we
do the call_rcu(). The struct itself will remain valid as long as we're
under rcu_read_lock() protection, so I think we'd be fine? If we do
that, then we don't need any rcu_barrier() or synchronize_rcu() calls,
as we can register a new one while the previous one is still being
killed.
Hmm?
We would have to remove the check that ctx->io_ev_fd != NULL. That we
would also result in 2 successive calls to io_eventfd_register without
any unregister in between being successful? Which i dont think is the
right behaviour?
I think the likelihood of hitting the rcu_barrier itself is quite low,
so probably the cost is low as well.
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) {
+ call_rcu(&ev_fd->rcu, io_eventfd_put);
+ ret = 0;
+ goto out;
}
+ ret = -ENXIO;
- return -ENXIO;
+out:
+ mutex_unlock(&ctx->ev_fd_lock);
+ return ret;
}
I also think that'd be cleaner without the goto:
{
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) {
call_rcu(&ev_fd->rcu, io_eventfd_put);
mutex_unlock(&ctx->ev_fd_lock);
return 0;
}
mutex_unlock(&ctx->ev_fd_lock);
return -ENXIO;
}
Thanks, will do that this in the next patchset with the above
io_eventfd_signal changes if those look ok as well?
