[PATCH 0/6] bunch of zerocopy send changes

[PATCH 0/6] bunch of zerocopy send changes

[Pavel Begunkov]

4/6 adds some ordering guarantees for send vs notif CQEs.
5 and 6 save address (if any) when it goes async, so we're more
consistent and don't read it twice.

Pavel Begunkov (6):
  io_uring/net: fix must_hold annotation
  io_uring/net: fix zc send link failing
  io_uring/net: fix indention
  io_uring/notif: order notif vs send CQEs
  io_uring: conditional ->async_data allocation
  io_uring/net: save address for sendzc async execution

 io_uring/io_uring.c |  7 +++---
 io_uring/net.c      | 55 ++++++++++++++++++++++++++++++++++++++-------
 io_uring/net.h      |  1 +
 io_uring/notif.c    |  8 ++++---
 io_uring/opdef.c    |  4 +++-
 io_uring/opdef.h    |  2 ++
 6 files changed, 62 insertions(+), 15 deletions(-)

-- 
2.37.2

[PATCH 1/6] io_uring/net: fix must_hold annotation

[Pavel Begunkov]

Fix up the io_alloc_notif()'s __must_hold as we don't have a ctx
argument there but should get it from the slot instead.

Reported-by: Stefan Metzmacher <metze@samba.org>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/notif.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/notif.c b/io_uring/notif.c
index 977736e82c1a..568ff17dc552 100644
--- a/io_uring/notif.c
+++ b/io_uring/notif.c
@@ -73,7 +73,7 @@ struct io_kiocb *io_alloc_notif(struct io_ring_ctx *ctx,
 }
 
 void io_notif_slot_flush(struct io_notif_slot *slot)
-	__must_hold(&ctx->uring_lock)
+	__must_hold(&slot->notif->ctx->uring_lock)
 {
 	struct io_kiocb *notif = slot->notif;
 	struct io_notif_data *nd = io_notif_to_data(notif);
-- 
2.37.2

[PATCH 2/6] io_uring/net: fix zc send link failing

[Pavel Begunkov]

Failed requests should be marked with req_set_fail(), so links and cqe
skipping work correctly, which is missing in io_sendzc(). Note,
io_sendzc() return IOU_OK on failure, so the core code won't do the
cleanup for us.

Fixes: 06a5464be84e4 ("io_uring: wire send zc request type")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/net.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/io_uring/net.c b/io_uring/net.c
index f8cdf1dc3863..d6310c655a0f 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -1023,6 +1023,7 @@ int io_sendzc(struct io_kiocb *req, unsigned int issue_flags)
 		}
 		if (ret == -ERESTARTSYS)
 			ret = -EINTR;
+		req_set_fail(req);
 	} else if (zc->flags & IORING_RECVSEND_NOTIF_FLUSH) {
 		io_notif_slot_flush_submit(notif_slot, 0);
 	}
-- 
2.37.2

[PATCH 3/6] io_uring/net: fix indention

[Pavel Begunkov]

Fix up indention before we get complaints from tooling.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/net.c b/io_uring/net.c
index d6310c655a0f..3adcb09ae264 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -989,7 +989,7 @@ int io_sendzc(struct io_kiocb *req, unsigned int issue_flags)
 		ret = io_import_fixed(WRITE, &msg.msg_iter, req->imu,
 					(u64)(uintptr_t)zc->buf, zc->len);
 		if (unlikely(ret))
-				return ret;
+			return ret;
 	} else {
 		ret = import_single_range(WRITE, zc->buf, zc->len, &iov,
 					  &msg.msg_iter);
-- 
2.37.2

[PATCH 4/6] io_uring/notif: order notif vs send CQEs

[Pavel Begunkov]

Currently, there is no ordering between notification CQEs and
completions of the send flushing it, this quite complicates the
userspace, especially since we don't flush notification when the
send(+flush) request fails, i.e. there will be only one CQE. What we
can do is to make sure that notification completions come only after
sends.

The easiest way to achieve this is to not try to complete a notification
inline from io_sendzc() but defer it to task_work, considering that
io-wq sendzc is disallowed CQEs will be naturally ordered because
task_works will only be executed after we're done with submission and so
inline completion.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/notif.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/io_uring/notif.c b/io_uring/notif.c
index 568ff17dc552..96f076b175e0 100644
--- a/io_uring/notif.c
+++ b/io_uring/notif.c
@@ -81,8 +81,10 @@ void io_notif_slot_flush(struct io_notif_slot *slot)
 	slot->notif = NULL;
 
 	/* drop slot's master ref */
-	if (refcount_dec_and_test(&nd->uarg.refcnt))
-		io_notif_complete(notif);
+	if (refcount_dec_and_test(&nd->uarg.refcnt)) {
+		notif->io_task_work.func = __io_notif_complete_tw;
+		io_req_task_work_add(notif);
+	}
 }
 
 __cold int io_notif_unregister(struct io_ring_ctx *ctx)
-- 
2.37.2

[PATCH 5/6] io_uring: conditional ->async_data allocation

[Pavel Begunkov]

There are opcodes that need ->async_data only in some cases and
allocation it unconditionally may hurt performance. Add an option to
opdef to make move the allocation part from the core io_uring to opcode
specific code.
Note, we can't just set opdef->async_size to zero because there are
other helpers that rely on it, e.g. io_alloc_async_data().

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 7 ++++---
 io_uring/opdef.h    | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index ebfdb2212ec2..77616279000b 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1450,9 +1450,10 @@ int io_req_prep_async(struct io_kiocb *req)
 		return 0;
 	if (WARN_ON_ONCE(req_has_async_data(req)))
 		return -EFAULT;
-	if (io_alloc_async_data(req))
-		return -EAGAIN;
-
+	if (!io_op_defs[req->opcode].manual_alloc) {
+		if (io_alloc_async_data(req))
+			return -EAGAIN;
+	}
 	return def->prep_async(req);
 }
 
diff --git a/io_uring/opdef.h b/io_uring/opdef.h
index ece8ed4f96c4..763c6e54e2ee 100644
--- a/io_uring/opdef.h
+++ b/io_uring/opdef.h
@@ -25,6 +25,8 @@ struct io_op_def {
 	unsigned		ioprio : 1;
 	/* supports iopoll */
 	unsigned		iopoll : 1;
+	/* opcode specific path will handle ->async_data allocation if needed */
+	unsigned		manual_alloc : 1;
 	/* size of async data needed, if any */
 	unsigned short		async_size;
 
-- 
2.37.2

[PATCH 6/6] io_uring/net: save address for sendzc async execution

[Pavel Begunkov]

We usually copy all bits that a request needs from the userspace for
async execution, so the userspace can keep them on the stack. However,
send zerocopy violates this pattern for addresses and may reloads it
e.g. from io-wq. Save the address if any in ->async_data as usual.

Reported-by: Stefan Metzmacher <metze@samba.org>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/net.c   | 52 +++++++++++++++++++++++++++++++++++++++++-------
 io_uring/net.h   |  1 +
 io_uring/opdef.c |  4 +++-
 3 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/io_uring/net.c b/io_uring/net.c
index 3adcb09ae264..4eaeb805e720 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -182,6 +182,37 @@ static int io_sendmsg_copy_hdr(struct io_kiocb *req,
 					&iomsg->free_iov);
 }
 
+int io_sendzc_prep_async(struct io_kiocb *req)
+{
+	struct io_sendzc *zc = io_kiocb_to_cmd(req, struct io_sendzc);
+	struct io_async_msghdr *io;
+	int ret;
+
+	if (!zc->addr || req_has_async_data(req))
+		return 0;
+	if (io_alloc_async_data(req))
+		return -ENOMEM;
+
+	io = req->async_data;
+	ret = move_addr_to_kernel(zc->addr, zc->addr_len, &io->addr);
+	return ret;
+}
+
+static int io_setup_async_addr(struct io_kiocb *req,
+			      struct sockaddr_storage *addr,
+			      unsigned int issue_flags)
+{
+	struct io_async_msghdr *io;
+
+	if (!addr || req_has_async_data(req))
+		return -EAGAIN;
+	if (io_alloc_async_data(req))
+		return -ENOMEM;
+	io = req->async_data;
+	memcpy(&io->addr, addr, sizeof(io->addr));
+	return -EAGAIN;
+}
+
 int io_sendmsg_prep_async(struct io_kiocb *req)
 {
 	int ret;
@@ -944,7 +975,7 @@ static int io_sg_from_iter(struct sock *sk, struct sk_buff *skb,
 
 int io_sendzc(struct io_kiocb *req, unsigned int issue_flags)
 {
-	struct sockaddr_storage address;
+	struct sockaddr_storage __address, *addr;
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_sendzc *zc = io_kiocb_to_cmd(req, struct io_sendzc);
 	struct io_notif_slot *notif_slot;
@@ -978,10 +1009,16 @@ int io_sendzc(struct io_kiocb *req, unsigned int issue_flags)
 	msg.msg_namelen = 0;
 
 	if (zc->addr) {
-		ret = move_addr_to_kernel(zc->addr, zc->addr_len, &address);
-		if (unlikely(ret < 0))
-			return ret;
-		msg.msg_name = (struct sockaddr *)&address;
+		if (req_has_async_data(req)) {
+			struct io_async_msghdr *io = req->async_data;
+
+			msg.msg_name = &io->addr;
+		} else {
+			ret = move_addr_to_kernel(zc->addr, zc->addr_len, &__address);
+			if (unlikely(ret < 0))
+				return ret;
+			msg.msg_name = (struct sockaddr *)&__address;
+		}
 		msg.msg_namelen = zc->addr_len;
 	}
 
@@ -1013,13 +1050,14 @@ int io_sendzc(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (unlikely(ret < min_ret)) {
 		if (ret == -EAGAIN && (issue_flags & IO_URING_F_NONBLOCK))
-			return -EAGAIN;
+			return io_setup_async_addr(req, addr, issue_flags);
+
 		if (ret > 0 && io_net_retry(sock, msg.msg_flags)) {
 			zc->len -= ret;
 			zc->buf += ret;
 			zc->done_io += ret;
 			req->flags |= REQ_F_PARTIAL_IO;
-			return -EAGAIN;
+			return io_setup_async_addr(req, addr, issue_flags);
 		}
 		if (ret == -ERESTARTSYS)
 			ret = -EINTR;
diff --git a/io_uring/net.h b/io_uring/net.h
index 7c438d39c089..f91f56c6eeac 100644
--- a/io_uring/net.h
+++ b/io_uring/net.h
@@ -31,6 +31,7 @@ struct io_async_connect {
 int io_shutdown_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
 int io_shutdown(struct io_kiocb *req, unsigned int issue_flags);
 
+int io_sendzc_prep_async(struct io_kiocb *req);
 int io_sendmsg_prep_async(struct io_kiocb *req);
 void io_sendmsg_recvmsg_cleanup(struct io_kiocb *req);
 int io_sendmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index 72dd2b2d8a9d..41410126c1c6 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -478,13 +478,15 @@ const struct io_op_def io_op_defs[] = {
 		.pollout		= 1,
 		.audit_skip		= 1,
 		.ioprio			= 1,
+		.manual_alloc		= 1,
 #if defined(CONFIG_NET)
+		.async_size		= sizeof(struct io_async_msghdr),
 		.prep			= io_sendzc_prep,
 		.issue			= io_sendzc,
+		.prep_async		= io_sendzc_prep_async,
 #else
 		.prep			= io_eopnotsupp_prep,
 #endif
-
 	},
 };
 
-- 
2.37.2

Re: [PATCH 0/6] bunch of zerocopy send changes

[Jens Axboe]

On Wed, 24 Aug 2022 13:07:37 +0100, Pavel Begunkov wrote:
> 4/6 adds some ordering guarantees for send vs notif CQEs.
> 5 and 6 save address (if any) when it goes async, so we're more
> consistent and don't read it twice.
> 
> Pavel Begunkov (6):
>   io_uring/net: fix must_hold annotation
>   io_uring/net: fix zc send link failing
>   io_uring/net: fix indention
>   io_uring/notif: order notif vs send CQEs
>   io_uring: conditional ->async_data allocation
>   io_uring/net: save address for sendzc async execution
> 
> [...]

Applied, thanks!

[1/6] io_uring/net: fix must_hold annotation
      commit: 2cacedc873ab5f5945d8f1b71804b0bcea0383ff
[2/6] io_uring/net: fix zc send link failing
      commit: 5a848b7c9e5e4d94390fbc391ccb81d40f3ccfb5
[3/6] io_uring/net: fix indention
      commit: 986e263def32eec89153babf469859d837507d34
[4/6] io_uring/notif: order notif vs send CQEs
      commit: 53bdc88aac9a21aae937452724fa4738cd843795
[5/6] io_uring: conditional ->async_data allocation
      commit: 5916943943d19a854238d50d1fe2047467cbeb3c
[6/6] io_uring/net: save address for sendzc async execution
      commit: 0596fa5ef9aff29219021fa6f0117b604ff83d09

Best regards,
-- 
Jens Axboe