first zcrx updates for 7.2

zcrx will need to have a pointer to an owning ctx to communicate different events. Reference the ctx while it's attached to zcrx, and rely on zcrx termination to drop the ctx to avoid circular ref deps.

Co-developed-by Vishwanath Seshagiri <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

io_uring/zcrx.c +19 -0

@@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
 	return allocated;
 }
 
+static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+	struct io_kiocb *req = tw_req.req;
+	struct io_ring_ctx *ctx = req->ctx;
+
+	io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
+	percpu_ref_put(&ctx->refs);
+	io_poison_req(req);
+	kmem_cache_free(req_cachep, req);
+}
+
+static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
+{
+	gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
+	u32 type_mask = 1 << type;
+	struct io_kiocb *req;
+
+	if (!(type_mask & ifq->allowed_notif_mask))
+		return;

Quoted 29 lines · Pavel Begunkov

io_uring/zcrx.c +25 -0

@@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
  	return allocated;
  }
 
+static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+	struct io_kiocb *req = tw_req.req;
+	struct io_ring_ctx *ctx = req->ctx;
+
+	io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
+	percpu_ref_put(&ctx->refs);
+	io_poison_req(req);
+	kmem_cache_free(req_cachep, req);
+}
+
+static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
+{
+	gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
+	u32 type_mask = 1 << type;
+	struct io_kiocb *req;
+
+	if (!(type_mask & ifq->allowed_notif_mask))
+		return;
+
+	guard(spinlock_bh)(&ifq->ctx_lock);
+	if (!ifq->master_ctx)
+		return;
+	if (type_mask & ifq->fired_notifs)
+		return;

io_uring/zcrx.c static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if · line 1173

Quoted 4 lines

io_uring/zcrx.c +4 -0

+
+	req = kmem_cache_alloc(req_cachep, gfp);
+	if (unlikely(!req))
+		return;

It'd be nice to avoid an allocation here inside ctx_lock and with bh's disabled, which looks like is also the only reason why GFP_ATOMIC is being used here.

Maybe opportunistically check ->fired_notifs early? Might also avoid the lock in the first place if we get back-to-back of these.

Quoted 29 lines · Jens Axboe

Quoted 28 lines · Pavel Begunkov

io_uring/zcrx.c +24 -0

@@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
       return allocated;
   }
   +static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+    struct io_kiocb *req = tw_req.req;
+    struct io_ring_ctx *ctx = req->ctx;
+
+    io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
+    percpu_ref_put(&ctx->refs);
+    io_poison_req(req);
+    kmem_cache_free(req_cachep, req);
+}
+
+static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
+{
+    gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
+    u32 type_mask = 1 << type;
+    struct io_kiocb *req;
+
+    if (!(type_mask & ifq->allowed_notif_mask))
+        return;
+
+    guard(spinlock_bh)(&ifq->ctx_lock);
+    if (!ifq->master_ctx)
+        return;
+    if (type_mask & ifq->fired_notifs)
+        return;

Quoted 8 lines

io_uring/zcrx.c static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if · line 1173

Quoted 4 lines

io_uring/zcrx.c +4 -0

+
+    req = kmem_cache_alloc(req_cachep, gfp);
+    if (unlikely(!req))
+        return;

It'd be nice to avoid an allocation here inside ctx_lock and with bh's disabled, which looks like is also the only reason why GFP_ATOMIC is being used here.

I thought about it, but it's already bh, it'd need to do pre allocations + caching to be reliable, but that's left out for now.

Quoted 2 lines

Maybe opportunistically check ->fired_notifs early? Might also avoid the lock in the first place if we get back-to-back of these.

Slow path, doesn't matter

Quoted 39 lines · Pavel Begunkov

Quoted 29 lines · Jens Axboe

Quoted 28 lines · Pavel Begunkov

io_uring/zcrx.c +24 -0

@@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
       return allocated;
   }
   +static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+    struct io_kiocb *req = tw_req.req;
+    struct io_ring_ctx *ctx = req->ctx;
+
+    io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
+    percpu_ref_put(&ctx->refs);
+    io_poison_req(req);
+    kmem_cache_free(req_cachep, req);
+}
+
+static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
+{
+    gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
+    u32 type_mask = 1 << type;
+    struct io_kiocb *req;
+
+    if (!(type_mask & ifq->allowed_notif_mask))
+        return;
+
+    guard(spinlock_bh)(&ifq->ctx_lock);
+    if (!ifq->master_ctx)
+        return;
+    if (type_mask & ifq->fired_notifs)
+        return;

Quoted 8 lines

io_uring/zcrx.c static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if · line 1173

Quoted 4 lines

io_uring/zcrx.c +4 -0

+
+    req = kmem_cache_alloc(req_cachep, gfp);
+    if (unlikely(!req))
+        return;

It'd be nice to avoid an allocation here inside ctx_lock and with bh's disabled, which looks like is also the only reason why GFP_ATOMIC is being used here.

Quoted 2 lines

I thought about it, but it's already bh, it'd need to do pre allocations + caching to be reliable, but that's left out for now.

Not sure I follow - GFP_KERNEL would be more reliable than GFP_ATOMIC. What's the contract in terms of the notification? If we fail the alloc, then userspace can't rely on the notification on the refill failure.

Are we under bh save already here, before doing it ourselves? If so, then how does the guard work?

Quoted 47 lines · Jens Axboe

Quoted 39 lines · Pavel Begunkov

Quoted 29 lines · Jens Axboe

Quoted 28 lines · Pavel Begunkov

io_uring/zcrx.c +24 -0

@@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
       return allocated;
   }
   +static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+    struct io_kiocb *req = tw_req.req;
+    struct io_ring_ctx *ctx = req->ctx;
+
+    io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
+    percpu_ref_put(&ctx->refs);
+    io_poison_req(req);
+    kmem_cache_free(req_cachep, req);
+}
+
+static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
+{
+    gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
+    u32 type_mask = 1 << type;
+    struct io_kiocb *req;
+
+    if (!(type_mask & ifq->allowed_notif_mask))
+        return;
+
+    guard(spinlock_bh)(&ifq->ctx_lock);
+    if (!ifq->master_ctx)
+        return;
+    if (type_mask & ifq->fired_notifs)
+        return;

Quoted 8 lines

io_uring/zcrx.c static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if · line 1173

Quoted 4 lines

io_uring/zcrx.c +4 -0

+
+    req = kmem_cache_alloc(req_cachep, gfp);
+    if (unlikely(!req))
+        return;

It'd be nice to avoid an allocation here inside ctx_lock and with bh's disabled, which looks like is also the only reason why GFP_ATOMIC is being used here.

Quoted 2 lines

I thought about it, but it's already bh, it'd need to do pre allocations + caching to be reliable, but that's left out for now.

Not sure I follow - GFP_KERNEL would be more reliable than GFP_ATOMIC. What's the contract in terms of the notification? If we fail the alloc, then userspace can't rely on the notification on the refill failure.

Quoted 2 lines

Are we under bh save already here, before doing it ourselves? If so, then how does the guard work?

In 99% of cases it's called from softirq, not sure what you mean by how it works.

Quoted 51 lines · Pavel Begunkov

Quoted 49 lines · Jens Axboe

Quoted 39 lines · Pavel Begunkov

Quoted 29 lines · Jens Axboe

Quoted 28 lines · Pavel Begunkov

io_uring/zcrx.c +24 -0

@@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
        return allocated;
    }
    +static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+    struct io_kiocb *req = tw_req.req;
+    struct io_ring_ctx *ctx = req->ctx;
+
+    io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
+    percpu_ref_put(&ctx->refs);
+    io_poison_req(req);
+    kmem_cache_free(req_cachep, req);
+}
+
+static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
+{
+    gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
+    u32 type_mask = 1 << type;
+    struct io_kiocb *req;
+
+    if (!(type_mask & ifq->allowed_notif_mask))
+        return;
+
+    guard(spinlock_bh)(&ifq->ctx_lock);
+    if (!ifq->master_ctx)
+        return;
+    if (type_mask & ifq->fired_notifs)
+        return;

Quoted 8 lines

io_uring/zcrx.c static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if · line 1173

Quoted 4 lines

io_uring/zcrx.c +4 -0

+
+    req = kmem_cache_alloc(req_cachep, gfp);
+    if (unlikely(!req))
+        return;

It'd be nice to avoid an allocation here inside ctx_lock and with bh's disabled, which looks like is also the only reason why GFP_ATOMIC is being used here.

Quoted 2 lines

I thought about it, but it's already bh, it'd need to do pre allocations + caching to be reliable, but that's left out for now.

Not sure I follow - GFP_KERNEL would be more reliable than GFP_ATOMIC. What's the contract in terms of the notification? If we fail the alloc, then userspace can't rely on the notification on the refill failure.

Are we under bh save already here, before doing it ourselves? If so, then how does the guard work?

Quoted 2 lines

In 99% of cases it's called from softirq, not sure what you mean by how it works.

Ah ok, I thought you meant it was already called with softirqs disabled. In which case the guard would seem broken, as we'd enable softirqs when exiting. But if we're just inside softirq yeah it's fine, and there's no point shuffling the allocation either.

Question on the contract still stands, in terms of missing a notification. I guess since it's a hint basically it doesn't really matter, just something that should be documented on the userspace side.

Do you have test cases for these?

Quoted 55 lines · Jens Axboe

Quoted 53 lines · Pavel Begunkov

Quoted 43 lines · Jens Axboe

Quoted 39 lines · Pavel Begunkov

Quoted 29 lines · Jens Axboe

Quoted 28 lines · Pavel Begunkov

(quoted diff) +24 -0

@@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
         return allocated;
     }
     +static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+    struct io_kiocb *req = tw_req.req;
+    struct io_ring_ctx *ctx = req->ctx;
+
+    io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
+    percpu_ref_put(&ctx->refs);
+    io_poison_req(req);
+    kmem_cache_free(req_cachep, req);
+}
+
+static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
+{
+    gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
+    u32 type_mask = 1 << type;
+    struct io_kiocb *req;
+
+    if (!(type_mask & ifq->allowed_notif_mask))
+        return;
+
+    guard(spinlock_bh)(&ifq->ctx_lock);
+    if (!ifq->master_ctx)
+        return;
+    if (type_mask & ifq->fired_notifs)
+        return;

Quoted 8 lines

Review static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if · line 1172

Quoted 4 lines

(quoted diff) +4 -0

+
+    req = kmem_cache_alloc(req_cachep, gfp);
+    if (unlikely(!req))
+        return;

It'd be nice to avoid an allocation here inside ctx_lock and with bh's disabled, which looks like is also the only reason why GFP_ATOMIC is being used here.

Quoted 2 lines

I thought about it, but it's already bh, it'd need to do pre allocations + caching to be reliable, but that's left out for now.

Quoted 6 lines

Not sure I follow - GFP_KERNEL would be more reliable than GFP_ATOMIC. What's the contract in terms of the notification? If we fail the alloc, then userspace can't rely on the notification on the refill failure.

Are we under bh save already here, before doing it ourselves? If so, then how does the guard work?

In 99% of cases it's called from softirq, not sure what you mean by how it works.

Quoted 4 lines

Ah ok, I thought you meant it was already called with softirqs disabled. In which case the guard would seem broken, as we'd enable softirqs when exiting. But if we're just inside softirq yeah it's fine, and there's no point shuffling the allocation either.

Softirqs are run with bh disabled, but bh_disable()/enable() are reenterable.

Quoted 3 lines

Question on the contract still stands, in terms of missing a notification. I guess since it's a hint basically it doesn't really matter, just something that should be documented on the userspace side.

Should rather be improved than documented, I'd say, but it's still better than not getting anything at all. And it's the only place where it can in theory be dropped, e.g. CQE overflow handling, though different GFP.

Quoted 1 line

Do you have test cases for these?

Clement needs to resend them. Actually, seems I forgot to CC Vish and Clement here, my bad.

Quoted 55 lines · Jens Axboe

Quoted 53 lines · Pavel Begunkov

Quoted 43 lines · Jens Axboe

Quoted 39 lines · Pavel Begunkov

Quoted 29 lines · Jens Axboe

Quoted 28 lines · Pavel Begunkov

(quoted diff) +24 -0

@@ -1126,6 +1142,48 @@ static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if
        return allocated;
    }
    +static void zcrx_notif_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+    struct io_kiocb *req = tw_req.req;
+    struct io_ring_ctx *ctx = req->ctx;
+
+    io_post_aux_cqe(ctx, req->cqe.user_data, req->cqe.res, 0);
+    percpu_ref_put(&ctx->refs);
+    io_poison_req(req);
+    kmem_cache_free(req_cachep, req);
+}
+
+static void zcrx_send_notif(struct io_zcrx_ifq *ifq, unsigned type)
+{
+    gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN | __GFP_ZERO;
+    u32 type_mask = 1 << type;
+    struct io_kiocb *req;
+
+    if (!(type_mask & ifq->allowed_notif_mask))
+        return;
+
+    guard(spinlock_bh)(&ifq->ctx_lock);
+    if (!ifq->master_ctx)
+        return;
+    if (type_mask & ifq->fired_notifs)
+        return;

Quoted 8 lines

Review static unsigned io_zcrx_refill_slow(struct page_pool *pp, struct io_zcrx_ifq *if · line 1172

Quoted 4 lines

(quoted diff) +4 -0

+
+    req = kmem_cache_alloc(req_cachep, gfp);
+    if (unlikely(!req))
+        return;

It'd be nice to avoid an allocation here inside ctx_lock and with bh's disabled, which looks like is also the only reason why GFP_ATOMIC is being used here.

Quoted 2 lines

I thought about it, but it's already bh, it'd need to do pre allocations + caching to be reliable, but that's left out for now.

Quoted 6 lines

Not sure I follow - GFP_KERNEL would be more reliable than GFP_ATOMIC. What's the contract in terms of the notification? If we fail the alloc, then userspace can't rely on the notification on the refill failure.

Are we under bh save already here, before doing it ourselves? If so, then how does the guard work?

In 99% of cases it's called from softirq, not sure what you mean by how it works.

Quoted 4 lines

Ah ok, I thought you meant it was already called with softirqs disabled. In which case the guard would seem broken, as we'd enable softirqs when exiting. But if we're just inside softirq yeah it's fine, and there's no point shuffling the allocation either.

Softirqs are run with bh disabled, but bh_disable()/enable() are reenterable.

Quoted 3 lines

Question on the contract still stands, in terms of missing a notification. I guess since it's a hint basically it doesn't really matter, just something that should be documented on the userspace side.

Should rather be improved than documented, I'd say, but it's still

better than not getting anything at all. And it's the only place where it can in theory be dropped, e.g. CQE overflow handling, though different GFP.

Quoted 1 line

Do you have test cases for these?

Clement needs to resend them. Actually, seems I forgot to CC Vish and Clement here, my bad.

First batch of zcrx updates for 7.2. The main part is patches 5-8, which add notifications from zcrx to userspace via asynchronous CQE posting about events like allocation failures and copying, and statistics. It's accompanied by relevant query updates. Patches 1-4 are general cleanups.

Bertie Tryner (1):
io_uring/zcrx: reorder fd allocation in zcrx_export()

[...]