[RFC PATCH 0/9] io_uring: support sqe group and provide group kbuf

[RFC PATCH 0/9] io_uring: support sqe group and provide group kbuf

[Ming Lei]

Hello,

This patch adds sqe user ext flags, generic sqe group usage, and
provide group kbuf based on sqe group. sqe group provides one efficient
way to share resource among one group of sqes, such as, it can be for
implementing multiple copying(copy data from single source to multiple
destinations) via single syscall.

Finally implements provide group kbuf for uring command, and ublk use this
for supporting zero copy, and actually this feature can be used to support
generic device zero copy.

The last liburing patch adds helpers for using sqe group, also adds
tests for sqe group. 

ublksrv userspace implements zero copy by sqe group & provide group
kbuf:

	https://github.com/ublk-org/ublksrv/commits/group-provide-buf/
	git clone https://github.com/ublk-org/ublksrv.git -b group-provide-buf

	make test T=loop/009:nbd/061:nbd/062	#ublk zc tests

Any comments are welcome!

Ming Lei (9):
  io_uring: net: don't check sqe->__pad2[0] for send zc
  io_uring: support user sqe ext flags
  io_uring: add helper for filling cqes in
    __io_submit_flush_completions()
  io_uring: add one output argument to io_submit_sqe
  io_uring: support SQE group
  io_uring: support providing sqe group buffer
  io_uring/uring_cmd: support provide group kernel buffer
  ublk: support provide io buffer
  liburing: support sqe ext_flags & sqe group

 drivers/block/ublk_drv.c       | 156 ++++++++++++++++++-
 include/linux/io_uring/cmd.h   |   9 ++
 include/linux/io_uring_types.h |  46 +++++-
 include/uapi/linux/io_uring.h  |  24 ++-
 include/uapi/linux/ublk_cmd.h  |   7 +-
 io_uring/filetable.h           |   2 +-
 io_uring/io_uring.c            | 268 +++++++++++++++++++++++++++++----
 io_uring/io_uring.h            |  17 ++-
 io_uring/kbuf.c                |  62 ++++++++
 io_uring/kbuf.h                |  12 ++
 io_uring/net.c                 |  31 +++-
 io_uring/opdef.c               |   5 +
 io_uring/opdef.h               |   2 +
 io_uring/rw.c                  |  20 ++-
 io_uring/uring_cmd.c           |  25 ++-
 15 files changed, 640 insertions(+), 46 deletions(-)

-- 
2.42.0

[PATCH 1/9] io_uring: net: don't check sqe->__pad2[0] for send zc

[Ming Lei]

send_zc never uses any byte in sqe->__pad2[], and the check is
unnecessary, so remove it and prepare for using the last byte
as sqe extended flags.

Signed-off-by: Ming Lei <ming.lei@redhat.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 97d815d13b6a..9c0567892945 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -997,7 +997,7 @@ int io_send_zc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 
 	zc->done_io = 0;
 
-	if (unlikely(READ_ONCE(sqe->__pad2[0]) || READ_ONCE(sqe->addr3)))
+	if (unlikely(READ_ONCE(sqe->addr3)))
 		return -EINVAL;
 	/* we don't support IOSQE_CQE_SKIP_SUCCESS just yet */
 	if (req->flags & REQ_F_CQE_SKIP)
-- 
2.42.0

[PATCH 2/9] io_uring: support user sqe ext flags

[Ming Lei]

sqe->flags is u8, and now we have used 7 bits, so take the last one for
extending purpose.

If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
IORING_OP_URING_CMD.

io_slot_flags() return value is converted to `ULL` because the affected bits
are beyond 32bit now.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/io_uring_types.h |  6 ++++--
 include/uapi/linux/io_uring.h  | 13 +++++++++++++
 io_uring/filetable.h           |  2 +-
 io_uring/io_uring.c            | 14 +++++++++++++-
 io_uring/uring_cmd.c           |  3 ++-
 5 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index 3e72fa52f1e3..67347e5d06ec 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -435,6 +435,7 @@ struct io_tw_state {
 };
 
 enum {
+	/* 1st byte is from sqe->flags, and 2nd is from sqe ext_flags */
 	REQ_F_FIXED_FILE_BIT	= IOSQE_FIXED_FILE_BIT,
 	REQ_F_IO_DRAIN_BIT	= IOSQE_IO_DRAIN_BIT,
 	REQ_F_LINK_BIT		= IOSQE_IO_LINK_BIT,
@@ -442,9 +443,10 @@ enum {
 	REQ_F_FORCE_ASYNC_BIT	= IOSQE_ASYNC_BIT,
 	REQ_F_BUFFER_SELECT_BIT	= IOSQE_BUFFER_SELECT_BIT,
 	REQ_F_CQE_SKIP_BIT	= IOSQE_CQE_SKIP_SUCCESS_BIT,
+	REQ_F_SQE_EXT_FLAGS_BIT	= IOSQE_HAS_EXT_FLAGS_BIT,
 
-	/* first byte is taken by user flags, shift it to not overlap */
-	REQ_F_FAIL_BIT		= 8,
+	/* first 2 bytes are taken by user flags, shift it to not overlap */
+	REQ_F_FAIL_BIT		= 16,
 	REQ_F_INFLIGHT_BIT,
 	REQ_F_CUR_POS_BIT,
 	REQ_F_NOWAIT_BIT,
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index a7f847543a7f..4847d7cf1ac9 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -98,6 +98,10 @@ struct io_uring_sqe {
 			__u64	__pad2[1];
 		};
 		__u64	optval;
+		struct {
+			__u8	__pad4[15];
+			__u8	ext_flags;
+		};
 		/*
 		 * If the ring is initialized with IORING_SETUP_SQE128, then
 		 * this field is used for 80 bytes of arbitrary command data
@@ -123,6 +127,7 @@ enum io_uring_sqe_flags_bit {
 	IOSQE_ASYNC_BIT,
 	IOSQE_BUFFER_SELECT_BIT,
 	IOSQE_CQE_SKIP_SUCCESS_BIT,
+	IOSQE_HAS_EXT_FLAGS_BIT,
 };
 
 /*
@@ -142,6 +147,11 @@ enum io_uring_sqe_flags_bit {
 #define IOSQE_BUFFER_SELECT	(1U << IOSQE_BUFFER_SELECT_BIT)
 /* don't post CQE if request succeeded */
 #define IOSQE_CQE_SKIP_SUCCESS	(1U << IOSQE_CQE_SKIP_SUCCESS_BIT)
+/*
+ * sqe ext flags carried in the last byte, or bit23~bit16 of
+ * sqe->uring_cmd_flags for IORING_URING_CMD.
+ */
+#define IOSQE_HAS_EXT_FLAGS	(1U << IOSQE_HAS_EXT_FLAGS_BIT)
 
 /*
  * io_uring_setup() flags
@@ -263,11 +273,14 @@ enum io_uring_op {
 
 /*
  * sqe->uring_cmd_flags		top 8bits aren't available for userspace
+ * bit31 ~ bit24		kernel internal usage
+ * bit23 ~ bit16		sqe ext flags
  * IORING_URING_CMD_FIXED	use registered buffer; pass this flag
  *				along with setting sqe->buf_index.
  */
 #define IORING_URING_CMD_FIXED	(1U << 0)
 #define IORING_URING_CMD_MASK	IORING_URING_CMD_FIXED
+#define IORING_URING_CMD_EXT_MASK	0x00ff0000
 
 
 /*
diff --git a/io_uring/filetable.h b/io_uring/filetable.h
index b2435c4dca1f..d25247c9b9f5 100644
--- a/io_uring/filetable.h
+++ b/io_uring/filetable.h
@@ -43,7 +43,7 @@ io_fixed_file_slot(struct io_file_table *table, unsigned i)
 #define FFS_ISREG		0x2UL
 #define FFS_MASK		~(FFS_NOWAIT|FFS_ISREG)
 
-static inline unsigned int io_slot_flags(struct io_fixed_file *slot)
+static inline unsigned long io_slot_flags(struct io_fixed_file *slot)
 {
 	return (slot->file_ptr & ~FFS_MASK) << REQ_F_SUPPORT_NOWAIT_BIT;
 }
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 8df9ad010803..6d4def11aebf 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -109,7 +109,8 @@
 			  IOSQE_IO_HARDLINK | IOSQE_ASYNC)
 
 #define SQE_VALID_FLAGS	(SQE_COMMON_FLAGS | IOSQE_BUFFER_SELECT | \
-			IOSQE_IO_DRAIN | IOSQE_CQE_SKIP_SUCCESS)
+			IOSQE_IO_DRAIN | IOSQE_CQE_SKIP_SUCCESS | \
+			IOSQE_HAS_EXT_FLAGS)
 
 #define IO_REQ_CLEAN_FLAGS (REQ_F_BUFFER_SELECTED | REQ_F_NEED_CLEANUP | \
 				REQ_F_POLLED | REQ_F_INFLIGHT | REQ_F_CREDS | \
@@ -2080,6 +2081,17 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 		/* enforce forwards compatibility on users */
 		if (sqe_flags & ~SQE_VALID_FLAGS)
 			return io_init_fail_req(req, -EINVAL);
+		if (sqe_flags & IOSQE_HAS_EXT_FLAGS) {
+			u32 sqe_ext_flags;
+
+			if (opcode != IORING_OP_URING_CMD)
+				sqe_ext_flags = READ_ONCE(sqe->ext_flags);
+			else
+				sqe_ext_flags = (READ_ONCE(sqe->uring_cmd_flags)
+					& IORING_URING_CMD_EXT_MASK) >> 16;
+			req->flags |= sqe_ext_flags << 8;
+		}
+
 		if (sqe_flags & IOSQE_BUFFER_SELECT) {
 			if (!def->buffer_select)
 				return io_init_fail_req(req, -EOPNOTSUPP);
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 334d31dd6628..43b71f29e7b3 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -202,7 +202,8 @@ int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (sqe->__pad1)
 		return -EINVAL;
 
-	ioucmd->flags = READ_ONCE(sqe->uring_cmd_flags);
+	ioucmd->flags = READ_ONCE(sqe->uring_cmd_flags) &
+		~IORING_URING_CMD_EXT_MASK;
 	if (ioucmd->flags & ~IORING_URING_CMD_MASK)
 		return -EINVAL;
 
-- 
2.42.0

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Jens Axboe]

On 4/7/24 7:03 PM, Ming Lei wrote:
> sqe->flags is u8, and now we have used 7 bits, so take the last one for
> extending purpose.
> 
> If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
> from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
> IORING_OP_URING_CMD.
> 
> io_slot_flags() return value is converted to `ULL` because the affected bits
> are beyond 32bit now.

If we're extending flags, which is something we arguably need to do at
some point, I think we should have them be generic and not spread out.
If uring_cmd needs specific flags and don't have them, then we should
add it just for that.

-- 
Jens Axboe

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Ming Lei]

On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
> On 4/7/24 7:03 PM, Ming Lei wrote:
> > sqe->flags is u8, and now we have used 7 bits, so take the last one for
> > extending purpose.
> > 
> > If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
> > from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
> > IORING_OP_URING_CMD.
> > 
> > io_slot_flags() return value is converted to `ULL` because the affected bits
> > are beyond 32bit now.
> 
> If we're extending flags, which is something we arguably need to do at
> some point, I think we should have them be generic and not spread out.

Sorry, maybe I don't get your idea, and the ext_flag itself is always
initialized in io_init_req(), like normal sqe->flags, same with its
usage.

> If uring_cmd needs specific flags and don't have them, then we should
> add it just for that.

The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
and can't be reused for generic flag. If we want to use byte48~63, it has
to be overlapped with uring_cmd's payload, and it is one generic sqe
flag, which is applied on uring_cmd too.

That is the only way I thought of, or any other suggestion for extending sqe
flags generically?


Thanks,
Ming

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Pavel Begunkov]

On 4/23/24 14:57, Ming Lei wrote:
> On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
>> On 4/7/24 7:03 PM, Ming Lei wrote:
>>> sqe->flags is u8, and now we have used 7 bits, so take the last one for
>>> extending purpose.
>>>
>>> If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
>>> from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
>>> IORING_OP_URING_CMD.
>>>
>>> io_slot_flags() return value is converted to `ULL` because the affected bits
>>> are beyond 32bit now.
>>
>> If we're extending flags, which is something we arguably need to do at
>> some point, I think we should have them be generic and not spread out.
> 
> Sorry, maybe I don't get your idea, and the ext_flag itself is always
> initialized in io_init_req(), like normal sqe->flags, same with its
> usage.
> 
>> If uring_cmd needs specific flags and don't have them, then we should
>> add it just for that.
> 
> The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
> borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
> and can't be reused for generic flag. If we want to use byte48~63, it has
> to be overlapped with uring_cmd's payload, and it is one generic sqe
> flag, which is applied on uring_cmd too.

Which is exactly the mess nobody would want to see. And I'd also
argue 8 extra bits is not enough anyway, otherwise the history will
repeat itself pretty soon

> That is the only way I thought of, or any other suggestion for extending sqe
> flags generically?

idea 1: just use the last bit. When we need another one it'd be time
to think about a long overdue SQE layout v2, this way we can try
to make flags u32 and clean up other problems.

idea 2: the group assembling flag can move into cmds. Very roughly:

io_cmd_init() {
	ublk_cmd_init();
}

ublk_cmd_init() {
	io_uring_start_grouping(ctx, cmd);
}

io_uring_start_grouping(ctx, cmd) {
	ctx->grouping = true;
	ctx->group_head = cmd->req;
}

submit_sqe() {
	if (ctx->grouping) {
		link_to_group(req, ctx->group_head);
		if (!(req->flags & REQ_F_LINK))
			ctx->grouping = false;
	}
}

-- 
Pavel Begunkov

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Ming Lei]

On Mon, Apr 29, 2024 at 04:24:54PM +0100, Pavel Begunkov wrote:
> On 4/23/24 14:57, Ming Lei wrote:
> > On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
> > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > sqe->flags is u8, and now we have used 7 bits, so take the last one for
> > > > extending purpose.
> > > > 
> > > > If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
> > > > from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
> > > > IORING_OP_URING_CMD.
> > > > 
> > > > io_slot_flags() return value is converted to `ULL` because the affected bits
> > > > are beyond 32bit now.
> > > 
> > > If we're extending flags, which is something we arguably need to do at
> > > some point, I think we should have them be generic and not spread out.
> > 
> > Sorry, maybe I don't get your idea, and the ext_flag itself is always
> > initialized in io_init_req(), like normal sqe->flags, same with its
> > usage.
> > 
> > > If uring_cmd needs specific flags and don't have them, then we should
> > > add it just for that.
> > 
> > The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
> > borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
> > and can't be reused for generic flag. If we want to use byte48~63, it has
> > to be overlapped with uring_cmd's payload, and it is one generic sqe
> > flag, which is applied on uring_cmd too.
> 
> Which is exactly the mess nobody would want to see. And I'd also

The trouble is introduced by supporting uring_cmd, and solving it by setting
ext flags for uring_cmd specially by liburing helper is still reasonable or
understandable, IMO.

> argue 8 extra bits is not enough anyway, otherwise the history will
> repeat itself pretty soon

It is started with 8 bits, now doubled when io_uring is basically
mature, even though history might repeat, it will take much longer time

> 
> > That is the only way I thought of, or any other suggestion for extending sqe
> > flags generically?
> 
> idea 1: just use the last bit. When we need another one it'd be time
> to think about a long overdue SQE layout v2, this way we can try
> to make flags u32 and clean up other problems.

It looks over-kill to invent SQE v2 just for solving the trouble in
uring_cmd, and supporting two layouts can be new trouble for io_uring.

Also I doubt the problem can be solved in layout v2:

- 64 byte is small enough to support everything, same for v2

- uring_cmd has only 16 bytes payload, taking any byte from
the payload may cause trouble for drivers

- the only possible change could still be to suppress bytes for OP
specific flags, but it might cause trouble for some OPs, such as
network.

> 
> idea 2: the group assembling flag can move into cmds. Very roughly:
> 
> io_cmd_init() {
> 	ublk_cmd_init();
> }
> 
> ublk_cmd_init() {
> 	io_uring_start_grouping(ctx, cmd);
> }
> 
> io_uring_start_grouping(ctx, cmd) {
> 	ctx->grouping = true;
> 	ctx->group_head = cmd->req;
> }

How can you know one group is starting without any flag? Or you still
suggest the approach taken in fused command?

> 
> submit_sqe() {
> 	if (ctx->grouping) {
> 		link_to_group(req, ctx->group_head);
> 		if (!(req->flags & REQ_F_LINK))
> 			ctx->grouping = false;
> 	}
> }

The group needs to be linked to existed link chain, so reusing REQ_F_LINK may
not doable.


Thanks,
Ming

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Pavel Begunkov]

On 4/30/24 04:43, Ming Lei wrote:
> On Mon, Apr 29, 2024 at 04:24:54PM +0100, Pavel Begunkov wrote:
>> On 4/23/24 14:57, Ming Lei wrote:
>>> On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
>>>> On 4/7/24 7:03 PM, Ming Lei wrote:
>>>>> sqe->flags is u8, and now we have used 7 bits, so take the last one for
>>>>> extending purpose.
>>>>>
>>>>> If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
>>>>> from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
>>>>> IORING_OP_URING_CMD.
>>>>>
>>>>> io_slot_flags() return value is converted to `ULL` because the affected bits
>>>>> are beyond 32bit now.
>>>>
>>>> If we're extending flags, which is something we arguably need to do at
>>>> some point, I think we should have them be generic and not spread out.
>>>
>>> Sorry, maybe I don't get your idea, and the ext_flag itself is always
>>> initialized in io_init_req(), like normal sqe->flags, same with its
>>> usage.
>>>
>>>> If uring_cmd needs specific flags and don't have them, then we should
>>>> add it just for that.
>>>
>>> The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
>>> borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
>>> and can't be reused for generic flag. If we want to use byte48~63, it has
>>> to be overlapped with uring_cmd's payload, and it is one generic sqe
>>> flag, which is applied on uring_cmd too.
>>
>> Which is exactly the mess nobody would want to see. And I'd also
> 
> The trouble is introduced by supporting uring_cmd, and solving it by setting
> ext flags for uring_cmd specially by liburing helper is still reasonable or
> understandable, IMO.
> 
>> argue 8 extra bits is not enough anyway, otherwise the history will
>> repeat itself pretty soon
> 
> It is started with 8 bits, now doubled when io_uring is basically
> mature, even though history might repeat, it will take much longer time

You're mistaken, only 7 bits are taken not because there haven't been
ideas and need to use them, but because we're out of space and we've
been saving it for something that might be absolutely necessary.

POLL_FIRST IMHO should've been a generic feature, but it worked around
being a send/recv specific flag, same goes for the use of registered
buffers, not to mention ideas for which we haven't had enough flag space.

>>> That is the only way I thought of, or any other suggestion for extending sqe
>>> flags generically?
>>
>> idea 1: just use the last bit. When we need another one it'd be time
>> to think about a long overdue SQE layout v2, this way we can try
>> to make flags u32 and clean up other problems.
> 
> It looks over-kill to invent SQE v2 just for solving the trouble in
> uring_cmd, and supporting two layouts can be new trouble for io_uring.

Sounds too uring_cmd centric, it's not specifically for uring_cmd, it's
just one of reasons. As for overkill, that's why I'm not telling you
to change the layour, but suggesting to take the last bit for the
group flag and leave future problems for the future.


> Also I doubt the problem can be solved in layout v2:
> 
> - 64 byte is small enough to support everything, same for v2
> 
> - uring_cmd has only 16 bytes payload, taking any byte from
> the payload may cause trouble for drivers
> 
> - the only possible change could still be to suppress bytes for OP
> specific flags, but it might cause trouble for some OPs, such as
> network.

Look up sqe's __pad1, for example


>> idea 2: the group assembling flag can move into cmds. Very roughly:
>>
>> io_cmd_init() {
>> 	ublk_cmd_init();
>> }
>>
>> ublk_cmd_init() {
>> 	io_uring_start_grouping(ctx, cmd);
>> }
>>
>> io_uring_start_grouping(ctx, cmd) {
>> 	ctx->grouping = true;
>> 	ctx->group_head = cmd->req;
>> }
> 
> How can you know one group is starting without any flag? Or you still
> suggest the approach taken in fused command?

That would be ublk's business, e.g. ublk or cmds specific flag


>> submit_sqe() {
>> 	if (ctx->grouping) {
>> 		link_to_group(req, ctx->group_head);
>> 		if (!(req->flags & REQ_F_LINK))
>> 			ctx->grouping = false;
>> 	}
>> }
> 
> The group needs to be linked to existed link chain, so reusing REQ_F_LINK may
> not doable.

Would it break zero copy feature if you cant?

-- 
Pavel Begunkov

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Ming Lei]

On Tue, Apr 30, 2024 at 01:00:30PM +0100, Pavel Begunkov wrote:
> On 4/30/24 04:43, Ming Lei wrote:
> > On Mon, Apr 29, 2024 at 04:24:54PM +0100, Pavel Begunkov wrote:
> > > On 4/23/24 14:57, Ming Lei wrote:
> > > > On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
> > > > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > > > sqe->flags is u8, and now we have used 7 bits, so take the last one for
> > > > > > extending purpose.
> > > > > > 
> > > > > > If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
> > > > > > from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
> > > > > > IORING_OP_URING_CMD.
> > > > > > 
> > > > > > io_slot_flags() return value is converted to `ULL` because the affected bits
> > > > > > are beyond 32bit now.
> > > > > 
> > > > > If we're extending flags, which is something we arguably need to do at
> > > > > some point, I think we should have them be generic and not spread out.
> > > > 
> > > > Sorry, maybe I don't get your idea, and the ext_flag itself is always
> > > > initialized in io_init_req(), like normal sqe->flags, same with its
> > > > usage.
> > > > 
> > > > > If uring_cmd needs specific flags and don't have them, then we should
> > > > > add it just for that.
> > > > 
> > > > The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
> > > > borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
> > > > and can't be reused for generic flag. If we want to use byte48~63, it has
> > > > to be overlapped with uring_cmd's payload, and it is one generic sqe
> > > > flag, which is applied on uring_cmd too.
> > > 
> > > Which is exactly the mess nobody would want to see. And I'd also
> > 
> > The trouble is introduced by supporting uring_cmd, and solving it by setting
> > ext flags for uring_cmd specially by liburing helper is still reasonable or
> > understandable, IMO.
> > 
> > > argue 8 extra bits is not enough anyway, otherwise the history will
> > > repeat itself pretty soon
> > 
> > It is started with 8 bits, now doubled when io_uring is basically
> > mature, even though history might repeat, it will take much longer time
> 
> You're mistaken, only 7 bits are taken not because there haven't been
> ideas and need to use them, but because we're out of space and we've
> been saving it for something that might be absolutely necessary.
> 
> POLL_FIRST IMHO should've been a generic feature, but it worked around
> being a send/recv specific flag, same goes for the use of registered
> buffers, not to mention ideas for which we haven't had enough flag space.

OK, but I am wondering why not extend flags a bit so that io_uring can
become extendable, just like this patch.

> 
> > > > That is the only way I thought of, or any other suggestion for extending sqe
> > > > flags generically?
> > > 
> > > idea 1: just use the last bit. When we need another one it'd be time
> > > to think about a long overdue SQE layout v2, this way we can try
> > > to make flags u32 and clean up other problems.
> > 
> > It looks over-kill to invent SQE v2 just for solving the trouble in
> > uring_cmd, and supporting two layouts can be new trouble for io_uring.
> 
> Sounds too uring_cmd centric, it's not specifically for uring_cmd, it's
> just one of reasons. As for overkill, that's why I'm not telling you
> to change the layour, but suggesting to take the last bit for the
> group flag and leave future problems for the future.

You mentioned 8bit flag is designed from beginning just for saving
space, so SQE V2 may not help us at all.

If the last bit can be reserved for extend flag, it is still possible
to extend sqe flags a bit, such as this patch. Otherwise, we just lose
chance to extend sqe flags in future.

Jens, can you share your idea/option wrt. extending sqe flags?

> 
> 
> > Also I doubt the problem can be solved in layout v2:
> > 
> > - 64 byte is small enough to support everything, same for v2
> > 
> > - uring_cmd has only 16 bytes payload, taking any byte from
> > the payload may cause trouble for drivers
> > 
> > - the only possible change could still be to suppress bytes for OP
> > specific flags, but it might cause trouble for some OPs, such as
> > network.
> 
> Look up sqe's __pad1, for example

Suppose it is just for uring_cmd, '__pad1' is shared with cmd_op, which is aligned
with ioctl cmd and is supposed to be 32bit.

Same with 'off' which is used in rw at least, if sqe group is to be
generic flag.

> 
> 
> > > idea 2: the group assembling flag can move into cmds. Very roughly:
> > > 
> > > io_cmd_init() {
> > > 	ublk_cmd_init();
> > > }
> > > 
> > > ublk_cmd_init() {
> > > 	io_uring_start_grouping(ctx, cmd);
> > > }
> > > 
> > > io_uring_start_grouping(ctx, cmd) {
> > > 	ctx->grouping = true;
> > > 	ctx->group_head = cmd->req;
> > > }
> > 
> > How can you know one group is starting without any flag? Or you still
> > suggest the approach taken in fused command?
> 
> That would be ublk's business, e.g. ublk or cmds specific flag

Then it becomes dedicated fused command actually, and last year's main
concern is that the approach isn't generic.

> 
> 
> > > submit_sqe() {
> > > 	if (ctx->grouping) {
> > > 		link_to_group(req, ctx->group_head);
> > > 		if (!(req->flags & REQ_F_LINK))
> > > 			ctx->grouping = false;
> > > 	}
> > > }
> > 
> > The group needs to be linked to existed link chain, so reusing REQ_F_LINK may
> > not doable.
> 
> Would it break zero copy feature if you cant?

The whole sqe group needs to be linked to existed link chain, so we
can't reuse REQ_F_LINK here.

Thanks,
Ming

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Pavel Begunkov]

On 4/30/24 13:56, Ming Lei wrote:
> On Tue, Apr 30, 2024 at 01:00:30PM +0100, Pavel Begunkov wrote:
>> On 4/30/24 04:43, Ming Lei wrote:
>>> On Mon, Apr 29, 2024 at 04:24:54PM +0100, Pavel Begunkov wrote:
>>>> On 4/23/24 14:57, Ming Lei wrote:
>>>>> On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
>>>>>> On 4/7/24 7:03 PM, Ming Lei wrote:
>>>>>>> sqe->flags is u8, and now we have used 7 bits, so take the last one for
>>>>>>> extending purpose.
>>>>>>>
>>>>>>> If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
>>>>>>> from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
>>>>>>> IORING_OP_URING_CMD.
>>>>>>>
>>>>>>> io_slot_flags() return value is converted to `ULL` because the affected bits
>>>>>>> are beyond 32bit now.
>>>>>>
>>>>>> If we're extending flags, which is something we arguably need to do at
>>>>>> some point, I think we should have them be generic and not spread out.
>>>>>
>>>>> Sorry, maybe I don't get your idea, and the ext_flag itself is always
>>>>> initialized in io_init_req(), like normal sqe->flags, same with its
>>>>> usage.
>>>>>
>>>>>> If uring_cmd needs specific flags and don't have them, then we should
>>>>>> add it just for that.
>>>>>
>>>>> The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
>>>>> borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
>>>>> and can't be reused for generic flag. If we want to use byte48~63, it has
>>>>> to be overlapped with uring_cmd's payload, and it is one generic sqe
>>>>> flag, which is applied on uring_cmd too.
>>>>
>>>> Which is exactly the mess nobody would want to see. And I'd also
>>>
>>> The trouble is introduced by supporting uring_cmd, and solving it by setting
>>> ext flags for uring_cmd specially by liburing helper is still reasonable or
>>> understandable, IMO.
>>>
>>>> argue 8 extra bits is not enough anyway, otherwise the history will
>>>> repeat itself pretty soon
>>>
>>> It is started with 8 bits, now doubled when io_uring is basically
>>> mature, even though history might repeat, it will take much longer time
>>
>> You're mistaken, only 7 bits are taken not because there haven't been
>> ideas and need to use them, but because we're out of space and we've
>> been saving it for something that might be absolutely necessary.
>>
>> POLL_FIRST IMHO should've been a generic feature, but it worked around
>> being a send/recv specific flag, same goes for the use of registered
>> buffers, not to mention ideas for which we haven't had enough flag space.
> 
> OK, but I am wondering why not extend flags a bit so that io_uring can
> become extendable, just like this patch.

That would be great if can be done cleanly. Even having it
non contig with the first 8bits is fine, but not conditional
depending on opcode is too much.


>>>>> That is the only way I thought of, or any other suggestion for extending sqe
>>>>> flags generically?
>>>>
>>>> idea 1: just use the last bit. When we need another one it'd be time
>>>> to think about a long overdue SQE layout v2, this way we can try
>>>> to make flags u32 and clean up other problems.
>>>
>>> It looks over-kill to invent SQE v2 just for solving the trouble in
>>> uring_cmd, and supporting two layouts can be new trouble for io_uring.
>>
>> Sounds too uring_cmd centric, it's not specifically for uring_cmd, it's
>> just one of reasons. As for overkill, that's why I'm not telling you
>> to change the layour, but suggesting to take the last bit for the
>> group flag and leave future problems for the future.
> 
> You mentioned 8bit flag is designed from beginning just for saving
> space, so SQE V2 may not help us at all.

Not sure what you mean. Retrospectively speaking, u8 for flags was
an oversight


> If the last bit can be reserved for extend flag, it is still possible
> to extend sqe flags a bit, such as this patch. Otherwise, we just lose
> chance to extend sqe flags in future.

That's why I mentioned SQE layout v2, i.e. a ctx flag which reshuffles
sqe fields in a better way. Surely there will be a lot of headache with
such a migration, but you can make flags a u32 then if you find space
and wouldn't even need and extending flag.


> Jens, can you share your idea/option wrt. extending sqe flags?
> 
>>
>>
>>> Also I doubt the problem can be solved in layout v2:
>>>
>>> - 64 byte is small enough to support everything, same for v2
>>>
>>> - uring_cmd has only 16 bytes payload, taking any byte from
>>> the payload may cause trouble for drivers
>>>
>>> - the only possible change could still be to suppress bytes for OP
>>> specific flags, but it might cause trouble for some OPs, such as
>>> network.
>>
>> Look up sqe's __pad1, for example
> 
> Suppose it is just for uring_cmd, '__pad1' is shared with cmd_op, which is aligned
> with ioctl cmd and is supposed to be 32bit.

It's not shared with cmd_op, it's in a struct with it, unless you
use a u32 part of ->addr2/off, it's just that, a completely
unnecessary created padding. There was also another field left,
at least in case for nvme.

> Same with 'off' which is used in rw at least, if sqe group is to be
> generic flag.
> 
>>
>>
>>>> idea 2: the group assembling flag can move into cmds. Very roughly:
>>>>
>>>> io_cmd_init() {
>>>> 	ublk_cmd_init();
>>>> }
>>>>
>>>> ublk_cmd_init() {
>>>> 	io_uring_start_grouping(ctx, cmd);
>>>> }
>>>>
>>>> io_uring_start_grouping(ctx, cmd) {
>>>> 	ctx->grouping = true;
>>>> 	ctx->group_head = cmd->req;
>>>> }
>>>
>>> How can you know one group is starting without any flag? Or you still
>>> suggest the approach taken in fused command?
>>
>> That would be ublk's business, e.g. ublk or cmds specific flag
> 
> Then it becomes dedicated fused command actually, and last year's main
> concern is that the approach isn't generic.

My concern is anything leaking into hot paths, even if it's a
generic feature (and I wouldn't call it that). The question is
rather at what degree. I wouldn't call groups in isolation
without zc exciting, and making it to look like a generic feature
just for the sake of it might even be worse than having it opcode
specific.

Regardless, this approach doesn't forbid some other opcode from
doing ctx->grouping = true based on some other opcode specific
flag, doesn't necessarily binds it to cmds/ublk.

>>>> submit_sqe() {
>>>> 	if (ctx->grouping) {
>>>> 		link_to_group(req, ctx->group_head);
>>>> 		if (!(req->flags & REQ_F_LINK))
>>>> 			ctx->grouping = false;
>>>> 	}
>>>> }
>>>
>>> The group needs to be linked to existed link chain, so reusing REQ_F_LINK may
>>> not doable.
>>
>> Would it break zero copy feature if you cant?
> 
> The whole sqe group needs to be linked to existed link chain, so we
> can't reuse REQ_F_LINK here.

Why though? You're passing a buffer from the head to all group-linked
requests, how do normal links come into the picture?

-- 
Pavel Begunkov

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Ming Lei]

On Tue, Apr 30, 2024 at 03:10:01PM +0100, Pavel Begunkov wrote:
> On 4/30/24 13:56, Ming Lei wrote:
> > On Tue, Apr 30, 2024 at 01:00:30PM +0100, Pavel Begunkov wrote:
> > > On 4/30/24 04:43, Ming Lei wrote:
> > > > On Mon, Apr 29, 2024 at 04:24:54PM +0100, Pavel Begunkov wrote:
> > > > > On 4/23/24 14:57, Ming Lei wrote:
> > > > > > On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
> > > > > > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > > > > > sqe->flags is u8, and now we have used 7 bits, so take the last one for
> > > > > > > > extending purpose.
> > > > > > > > 
> > > > > > > > If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
> > > > > > > > from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
> > > > > > > > IORING_OP_URING_CMD.
> > > > > > > > 
> > > > > > > > io_slot_flags() return value is converted to `ULL` because the affected bits
> > > > > > > > are beyond 32bit now.
> > > > > > > 
> > > > > > > If we're extending flags, which is something we arguably need to do at
> > > > > > > some point, I think we should have them be generic and not spread out.
> > > > > > 
> > > > > > Sorry, maybe I don't get your idea, and the ext_flag itself is always
> > > > > > initialized in io_init_req(), like normal sqe->flags, same with its
> > > > > > usage.
> > > > > > 
> > > > > > > If uring_cmd needs specific flags and don't have them, then we should
> > > > > > > add it just for that.
> > > > > > 
> > > > > > The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
> > > > > > borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
> > > > > > and can't be reused for generic flag. If we want to use byte48~63, it has
> > > > > > to be overlapped with uring_cmd's payload, and it is one generic sqe
> > > > > > flag, which is applied on uring_cmd too.
> > > > > 
> > > > > Which is exactly the mess nobody would want to see. And I'd also
> > > > 
> > > > The trouble is introduced by supporting uring_cmd, and solving it by setting
> > > > ext flags for uring_cmd specially by liburing helper is still reasonable or
> > > > understandable, IMO.
> > > > 
> > > > > argue 8 extra bits is not enough anyway, otherwise the history will
> > > > > repeat itself pretty soon
> > > > 
> > > > It is started with 8 bits, now doubled when io_uring is basically
> > > > mature, even though history might repeat, it will take much longer time
> > > 
> > > You're mistaken, only 7 bits are taken not because there haven't been
> > > ideas and need to use them, but because we're out of space and we've
> > > been saving it for something that might be absolutely necessary.
> > > 
> > > POLL_FIRST IMHO should've been a generic feature, but it worked around
> > > being a send/recv specific flag, same goes for the use of registered
> > > buffers, not to mention ideas for which we haven't had enough flag space.
> > 
> > OK, but I am wondering why not extend flags a bit so that io_uring can
> > become extendable, just like this patch.
> 
> That would be great if can be done cleanly. Even having it
> non contig with the first 8bits is fine, but not conditional
> depending on opcode is too much.

byte56~63 is used for uring_cmd payload, and it can't be done without
depending on uring_cmd op.

The patch is simple, and this usage can be well-documented. In
userspace, just one special helper is needed for setting uring_cmd
ext_flags only.

Except for this simple way, I don't see other approaches to extend sqe flags.

> 
> 
> > > > > > That is the only way I thought of, or any other suggestion for extending sqe
> > > > > > flags generically?
> > > > > 
> > > > > idea 1: just use the last bit. When we need another one it'd be time
> > > > > to think about a long overdue SQE layout v2, this way we can try
> > > > > to make flags u32 and clean up other problems.
> > > > 
> > > > It looks over-kill to invent SQE v2 just for solving the trouble in
> > > > uring_cmd, and supporting two layouts can be new trouble for io_uring.
> > > 
> > > Sounds too uring_cmd centric, it's not specifically for uring_cmd, it's
> > > just one of reasons. As for overkill, that's why I'm not telling you
> > > to change the layour, but suggesting to take the last bit for the
> > > group flag and leave future problems for the future.
> > 
> > You mentioned 8bit flag is designed from beginning just for saving
> > space, so SQE V2 may not help us at all.
> 
> Not sure what you mean. Retrospectively speaking, u8 for flags was
> an oversight

You mentioned that:

	You're mistaken, only 7 bits are taken not because there haven't been
	ideas and need to use them, but because we're out of space and we've
	been saving it for something that might be absolutely necessary.

Nothing is changed since then, so where to find more free space from
64 bytes for sqe flags now?

> 
> 
> > If the last bit can be reserved for extend flag, it is still possible
> > to extend sqe flags a bit, such as this patch. Otherwise, we just lose
> > chance to extend sqe flags in future.
> 
> That's why I mentioned SQE layout v2, i.e. a ctx flag which reshuffles
> sqe fields in a better way. Surely there will be a lot of headache with
> such a migration, but you can make flags a u32 then if you find space
> and wouldn't even need and extending flag.

It is one hard problem, and it may not be answered in short time, cause all
use cases need to be covered, meantime 3 extra bytes are saved from the
reshuffling, with alignment respected meantime.

Also it isn't worth of layout v2 just for extending sqe flags.

> 
> 
> > Jens, can you share your idea/option wrt. extending sqe flags?
> > 
> > > 
> > > 
> > > > Also I doubt the problem can be solved in layout v2:
> > > > 
> > > > - 64 byte is small enough to support everything, same for v2
> > > > 
> > > > - uring_cmd has only 16 bytes payload, taking any byte from
> > > > the payload may cause trouble for drivers
> > > > 
> > > > - the only possible change could still be to suppress bytes for OP
> > > > specific flags, but it might cause trouble for some OPs, such as
> > > > network.
> > > 
> > > Look up sqe's __pad1, for example
> > 
> > Suppose it is just for uring_cmd, '__pad1' is shared with cmd_op, which is aligned
> > with ioctl cmd and is supposed to be 32bit.
> 
> It's not shared with cmd_op, it's in a struct with it, unless you
> use a u32 part of ->addr2/off, it's just that, a completely
> unnecessary created padding. There was also another field left,
> at least in case for nvme.

OK, __pad1 is available for uring_cmd, and it could be better to use
__pad1 for uring_cmd ext flags, but it still depends on uring_cmd, and
now ext_flags can be u16 or more, :-)

Thanks for sharing this point.

> 
> > Same with 'off' which is used in rw at least, if sqe group is to be
> > generic flag.
> > 
> > > 
> > > 
> > > > > idea 2: the group assembling flag can move into cmds. Very roughly:
> > > > > 
> > > > > io_cmd_init() {
> > > > > 	ublk_cmd_init();
> > > > > }
> > > > > 
> > > > > ublk_cmd_init() {
> > > > > 	io_uring_start_grouping(ctx, cmd);
> > > > > }
> > > > > 
> > > > > io_uring_start_grouping(ctx, cmd) {
> > > > > 	ctx->grouping = true;
> > > > > 	ctx->group_head = cmd->req;
> > > > > }
> > > > 
> > > > How can you know one group is starting without any flag? Or you still
> > > > suggest the approach taken in fused command?
> > > 
> > > That would be ublk's business, e.g. ublk or cmds specific flag
> > 
> > Then it becomes dedicated fused command actually, and last year's main
> > concern is that the approach isn't generic.
> 
> My concern is anything leaking into hot paths, even if it's a
> generic feature (and I wouldn't call it that). The question is
> rather at what degree. I wouldn't call groups in isolation
> without zc exciting, and making it to look like a generic feature
> just for the sake of it might even be worse than having it opcode
> specific.
> 
> Regardless, this approach doesn't forbid some other opcode from
> doing ctx->grouping = true based on some other opcode specific
> flag, doesn't necessarily binds it to cmds/ublk.

Yes.

> 
> > > > > submit_sqe() {
> > > > > 	if (ctx->grouping) {
> > > > > 		link_to_group(req, ctx->group_head);
> > > > > 		if (!(req->flags & REQ_F_LINK))
> > > > > 			ctx->grouping = false;
> > > > > 	}
> > > > > }
> > > > 
> > > > The group needs to be linked to existed link chain, so reusing REQ_F_LINK may
> > > > not doable.
> > > 
> > > Would it break zero copy feature if you cant?
> > 
> > The whole sqe group needs to be linked to existed link chain, so we
> > can't reuse REQ_F_LINK here.
> 
> Why though? You're passing a buffer from the head to all group-linked
> requests, how do normal links come into the picture?

For example of ublk-nbd, tcp send requests have to be linked, and each
send request belongs to one group in case of zero copy.

Meantime linking is one generic feature, and it can be used everywhere, so
not good to disallow it in application

Thanks,
Ming

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Pavel Begunkov]

On 4/30/24 16:46, Ming Lei wrote:
> On Tue, Apr 30, 2024 at 03:10:01PM +0100, Pavel Begunkov wrote:
>> On 4/30/24 13:56, Ming Lei wrote:
>>> On Tue, Apr 30, 2024 at 01:00:30PM +0100, Pavel Begunkov wrote:
>>>> On 4/30/24 04:43, Ming Lei wrote:
>>>>> On Mon, Apr 29, 2024 at 04:24:54PM +0100, Pavel Begunkov wrote:
>>>>>> On 4/23/24 14:57, Ming Lei wrote:
>>>>>>> On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
>>>>>>>> On 4/7/24 7:03 PM, Ming Lei wrote:
>>>>>>>>> sqe->flags is u8, and now we have used 7 bits, so take the last one for
>>>>>>>>> extending purpose.
>>>>>>>>>
>>>>>>>>> If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
>>>>>>>>> from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
>>>>>>>>> IORING_OP_URING_CMD.
>>>>>>>>>
>>>>>>>>> io_slot_flags() return value is converted to `ULL` because the affected bits
>>>>>>>>> are beyond 32bit now.
>>>>>>>>
>>>>>>>> If we're extending flags, which is something we arguably need to do at
>>>>>>>> some point, I think we should have them be generic and not spread out.
>>>>>>>
>>>>>>> Sorry, maybe I don't get your idea, and the ext_flag itself is always
>>>>>>> initialized in io_init_req(), like normal sqe->flags, same with its
>>>>>>> usage.
>>>>>>>
>>>>>>>> If uring_cmd needs specific flags and don't have them, then we should
>>>>>>>> add it just for that.
>>>>>>>
>>>>>>> The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
>>>>>>> borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
>>>>>>> and can't be reused for generic flag. If we want to use byte48~63, it has
>>>>>>> to be overlapped with uring_cmd's payload, and it is one generic sqe
>>>>>>> flag, which is applied on uring_cmd too.
>>>>>>
>>>>>> Which is exactly the mess nobody would want to see. And I'd also
>>>>>
>>>>> The trouble is introduced by supporting uring_cmd, and solving it by setting
>>>>> ext flags for uring_cmd specially by liburing helper is still reasonable or
>>>>> understandable, IMO.
>>>>>
>>>>>> argue 8 extra bits is not enough anyway, otherwise the history will
>>>>>> repeat itself pretty soon
>>>>>
>>>>> It is started with 8 bits, now doubled when io_uring is basically
>>>>> mature, even though history might repeat, it will take much longer time
>>>>
>>>> You're mistaken, only 7 bits are taken not because there haven't been
>>>> ideas and need to use them, but because we're out of space and we've
>>>> been saving it for something that might be absolutely necessary.
>>>>
>>>> POLL_FIRST IMHO should've been a generic feature, but it worked around
>>>> being a send/recv specific flag, same goes for the use of registered
>>>> buffers, not to mention ideas for which we haven't had enough flag space.
>>>
>>> OK, but I am wondering why not extend flags a bit so that io_uring can
>>> become extendable, just like this patch.
>>
>> That would be great if can be done cleanly. Even having it
>> non contig with the first 8bits is fine, but not conditional
>> depending on opcode is too much.
> 
> byte56~63 is used for uring_cmd payload, and it can't be done without
> depending on uring_cmd op.
> 
> The patch is simple, and this usage can be well-documented. In
> userspace, just one special helper is needed for setting uring_cmd
> ext_flags only.

One simple helper here, one simple helper there, one line in man
in some other place, in the end it'll turn to be a horrible mess.

It's not even a question when we'd see people asking "I used
set_ext_flags but why it doesn't work" from people missing a
separate cmd flag. Or just to be sure they would call both,
such things happen even with more straightforward APIs, and it's
just one problem.

> Except for this simple way, I don't see other approaches to extend sqe flags.

Well, that's why I described below how exactly it can be done
cleanly in a long run.

>>>>>>> That is the only way I thought of, or any other suggestion for extending sqe
>>>>>>> flags generically?
>>>>>>
>>>>>> idea 1: just use the last bit. When we need another one it'd be time
>>>>>> to think about a long overdue SQE layout v2, this way we can try
>>>>>> to make flags u32 and clean up other problems.
>>>>>
>>>>> It looks over-kill to invent SQE v2 just for solving the trouble in
>>>>> uring_cmd, and supporting two layouts can be new trouble for io_uring.
>>>>
>>>> Sounds too uring_cmd centric, it's not specifically for uring_cmd, it's
>>>> just one of reasons. As for overkill, that's why I'm not telling you
>>>> to change the layour, but suggesting to take the last bit for the
>>>> group flag and leave future problems for the future.
>>>
>>> You mentioned 8bit flag is designed from beginning just for saving
>>> space, so SQE V2 may not help us at all.
>>
>> Not sure what you mean. Retrospectively speaking, u8 for flags was
>> an oversight
> 
> You mentioned that:
> 
> 	You're mistaken, only 7 bits are taken not because there haven't been
> 	ideas and need to use them, but because we're out of space and we've
> 	been saving it for something that might be absolutely necessary.
> 
> Nothing is changed since then, so where to find more free space from
> 64 bytes for sqe flags now?

ditto

>>> If the last bit can be reserved for extend flag, it is still possible
>>> to extend sqe flags a bit, such as this patch. Otherwise, we just lose
>>> chance to extend sqe flags in future.
>>
>> That's why I mentioned SQE layout v2, i.e. a ctx flag which reshuffles
>> sqe fields in a better way. Surely there will be a lot of headache with
>> such a migration, but you can make flags a u32 then if you find space
>> and wouldn't even need and extending flag.
> 
> It is one hard problem, and it may not be answered in short time, cause all
> use cases need to be covered, meantime 3 extra bytes are saved from the
> reshuffling, with alignment respected meantime.
> 
> Also it isn't worth of layout v2 just for extending sqe flags.

Not just, by opting to the pain of migration to a new SQE layout
we can revise more API decisions. It's a separate topic for
discussion, and the latter it's done the better because we'd
collect more design mistakes that can be fixed.

Reiterating again, you're not blocked by it.

>>> Jens, can you share your idea/option wrt. extending sqe flags?
>>>
>>>>
>>>>
>>>>> Also I doubt the problem can be solved in layout v2:
>>>>>
>>>>> - 64 byte is small enough to support everything, same for v2
>>>>>
>>>>> - uring_cmd has only 16 bytes payload, taking any byte from
>>>>> the payload may cause trouble for drivers
>>>>>
>>>>> - the only possible change could still be to suppress bytes for OP
>>>>> specific flags, but it might cause trouble for some OPs, such as
>>>>> network.
>>>>
>>>> Look up sqe's __pad1, for example
>>>
>>> Suppose it is just for uring_cmd, '__pad1' is shared with cmd_op, which is aligned
>>> with ioctl cmd and is supposed to be 32bit.
>>
>> It's not shared with cmd_op, it's in a struct with it, unless you
>> use a u32 part of ->addr2/off, it's just that, a completely
>> unnecessary created padding. There was also another field left,
>> at least in case for nvme.
> 
> OK, __pad1 is available for uring_cmd, and it could be better to use
> __pad1 for uring_cmd ext flags, but it still depends on uring_cmd, and
> now ext_flags can be u16 or more, :-)
> 
> Thanks for sharing this point.
> 
>>
>>> Same with 'off' which is used in rw at least, if sqe group is to be
>>> generic flag.
>>>
>>>>
>>>>
>>>>>> idea 2: the group assembling flag can move into cmds. Very roughly:
>>>>>>
>>>>>> io_cmd_init() {
>>>>>> 	ublk_cmd_init();
>>>>>> }
>>>>>>
>>>>>> ublk_cmd_init() {
>>>>>> 	io_uring_start_grouping(ctx, cmd);
>>>>>> }
>>>>>>
>>>>>> io_uring_start_grouping(ctx, cmd) {
>>>>>> 	ctx->grouping = true;
>>>>>> 	ctx->group_head = cmd->req;
>>>>>> }
>>>>>
>>>>> How can you know one group is starting without any flag? Or you still
>>>>> suggest the approach taken in fused command?
>>>>
>>>> That would be ublk's business, e.g. ublk or cmds specific flag
>>>
>>> Then it becomes dedicated fused command actually, and last year's main
>>> concern is that the approach isn't generic.
>>
>> My concern is anything leaking into hot paths, even if it's a
>> generic feature (and I wouldn't call it that). The question is
>> rather at what degree. I wouldn't call groups in isolation
>> without zc exciting, and making it to look like a generic feature
>> just for the sake of it might even be worse than having it opcode
>> specific.
>>
>> Regardless, this approach doesn't forbid some other opcode from
>> doing ctx->grouping = true based on some other opcode specific
>> flag, doesn't necessarily binds it to cmds/ublk.
> 
> Yes.
> 
>>
>>>>>> submit_sqe() {
>>>>>> 	if (ctx->grouping) {
>>>>>> 		link_to_group(req, ctx->group_head);
>>>>>> 		if (!(req->flags & REQ_F_LINK))
>>>>>> 			ctx->grouping = false;
>>>>>> 	}
>>>>>> }
>>>>>
>>>>> The group needs to be linked to existed link chain, so reusing REQ_F_LINK may
>>>>> not doable.
>>>>
>>>> Would it break zero copy feature if you cant?
>>>
>>> The whole sqe group needs to be linked to existed link chain, so we
>>> can't reuse REQ_F_LINK here.
>>
>> Why though? You're passing a buffer from the head to all group-linked
>> requests, how do normal links come into the picture?
> 
> For example of ublk-nbd, tcp send requests have to be linked, and each
> send request belongs to one group in case of zero copy.

"Linked" like in "add to a group"? Or in terms of traditional
IOSQE_IO_LINK links? Because if it's about groups you don't need
IOSQE_IO_LINK. And if it's IOSQE_IO_LINK linking, then same
is supposedly can be done in userspace.

> Meantime linking is one generic feature, and it can be used everywhere, so
> not good to disallow it in application

-- 
Pavel Begunkov

Re: [PATCH 2/9] io_uring: support user sqe ext flags

[Ming Lei]

On Thu, May 02, 2024 at 03:22:10PM +0100, Pavel Begunkov wrote:
> On 4/30/24 16:46, Ming Lei wrote:
> > On Tue, Apr 30, 2024 at 03:10:01PM +0100, Pavel Begunkov wrote:
> > > On 4/30/24 13:56, Ming Lei wrote:
> > > > On Tue, Apr 30, 2024 at 01:00:30PM +0100, Pavel Begunkov wrote:
> > > > > On 4/30/24 04:43, Ming Lei wrote:
> > > > > > On Mon, Apr 29, 2024 at 04:24:54PM +0100, Pavel Begunkov wrote:
> > > > > > > On 4/23/24 14:57, Ming Lei wrote:
> > > > > > > > On Mon, Apr 22, 2024 at 12:16:12PM -0600, Jens Axboe wrote:
> > > > > > > > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > > > > > > > sqe->flags is u8, and now we have used 7 bits, so take the last one for
> > > > > > > > > > extending purpose.
> > > > > > > > > > 
> > > > > > > > > > If bit7(IOSQE_HAS_EXT_FLAGS_BIT) is 1, it means this sqe carries ext flags
> > > > > > > > > > from the last byte(.ext_flags), or bit23~bit16 of sqe->uring_cmd_flags for
> > > > > > > > > > IORING_OP_URING_CMD.
> > > > > > > > > > 
> > > > > > > > > > io_slot_flags() return value is converted to `ULL` because the affected bits
> > > > > > > > > > are beyond 32bit now.
> > > > > > > > > 
> > > > > > > > > If we're extending flags, which is something we arguably need to do at
> > > > > > > > > some point, I think we should have them be generic and not spread out.
> > > > > > > > 
> > > > > > > > Sorry, maybe I don't get your idea, and the ext_flag itself is always
> > > > > > > > initialized in io_init_req(), like normal sqe->flags, same with its
> > > > > > > > usage.
> > > > > > > > 
> > > > > > > > > If uring_cmd needs specific flags and don't have them, then we should
> > > > > > > > > add it just for that.
> > > > > > > > 
> > > > > > > > The only difference is that bit23~bit16 of sqe->uring_cmd_flags is
> > > > > > > > borrowed for uring_cmd's ext flags, because sqe byte0~47 have been taken,
> > > > > > > > and can't be reused for generic flag. If we want to use byte48~63, it has
> > > > > > > > to be overlapped with uring_cmd's payload, and it is one generic sqe
> > > > > > > > flag, which is applied on uring_cmd too.
> > > > > > > 
> > > > > > > Which is exactly the mess nobody would want to see. And I'd also
> > > > > > 
> > > > > > The trouble is introduced by supporting uring_cmd, and solving it by setting
> > > > > > ext flags for uring_cmd specially by liburing helper is still reasonable or
> > > > > > understandable, IMO.
> > > > > > 
> > > > > > > argue 8 extra bits is not enough anyway, otherwise the history will
> > > > > > > repeat itself pretty soon
> > > > > > 
> > > > > > It is started with 8 bits, now doubled when io_uring is basically
> > > > > > mature, even though history might repeat, it will take much longer time
> > > > > 
> > > > > You're mistaken, only 7 bits are taken not because there haven't been
> > > > > ideas and need to use them, but because we're out of space and we've
> > > > > been saving it for something that might be absolutely necessary.
> > > > > 
> > > > > POLL_FIRST IMHO should've been a generic feature, but it worked around
> > > > > being a send/recv specific flag, same goes for the use of registered
> > > > > buffers, not to mention ideas for which we haven't had enough flag space.
> > > > 
> > > > OK, but I am wondering why not extend flags a bit so that io_uring can
> > > > become extendable, just like this patch.
> > > 
> > > That would be great if can be done cleanly. Even having it
> > > non contig with the first 8bits is fine, but not conditional
> > > depending on opcode is too much.
> > 
> > byte56~63 is used for uring_cmd payload, and it can't be done without
> > depending on uring_cmd op.
> > 
> > The patch is simple, and this usage can be well-documented. In
> > userspace, just one special helper is needed for setting uring_cmd
> > ext_flags only.
> 
> One simple helper here, one simple helper there, one line in man
> in some other place, in the end it'll turn to be a horrible mess.
> 
> It's not even a question when we'd see people asking "I used
> set_ext_flags but why it doesn't work" from people missing a
> separate cmd flag. Or just to be sure they would call both,
> such things happen even with more straightforward APIs, and it's
> just one problem.
> 
> > Except for this simple way, I don't see other approaches to extend sqe flags.
> 
> Well, that's why I described below how exactly it can be done
> cleanly in a long run.

OK, then looks io_uring isn't extendable wrt. sqe->flags in long run.

> 
> > > > > > > > That is the only way I thought of, or any other suggestion for extending sqe
> > > > > > > > flags generically?
> > > > > > > 
> > > > > > > idea 1: just use the last bit. When we need another one it'd be time
> > > > > > > to think about a long overdue SQE layout v2, this way we can try
> > > > > > > to make flags u32 and clean up other problems.
> > > > > > 
> > > > > > It looks over-kill to invent SQE v2 just for solving the trouble in
> > > > > > uring_cmd, and supporting two layouts can be new trouble for io_uring.
> > > > > 
> > > > > Sounds too uring_cmd centric, it's not specifically for uring_cmd, it's
> > > > > just one of reasons. As for overkill, that's why I'm not telling you
> > > > > to change the layour, but suggesting to take the last bit for the
> > > > > group flag and leave future problems for the future.
> > > > 
> > > > You mentioned 8bit flag is designed from beginning just for saving
> > > > space, so SQE V2 may not help us at all.
> > > 
> > > Not sure what you mean. Retrospectively speaking, u8 for flags was
> > > an oversight
> > 
> > You mentioned that:
> > 
> > 	You're mistaken, only 7 bits are taken not because there haven't been
> > 	ideas and need to use them, but because we're out of space and we've
> > 	been saving it for something that might be absolutely necessary.
> > 
> > Nothing is changed since then, so where to find more free space from
> > 64 bytes for sqe flags now?
> 
> ditto
> 
> > > > If the last bit can be reserved for extend flag, it is still possible
> > > > to extend sqe flags a bit, such as this patch. Otherwise, we just lose
> > > > chance to extend sqe flags in future.
> > > 
> > > That's why I mentioned SQE layout v2, i.e. a ctx flag which reshuffles
> > > sqe fields in a better way. Surely there will be a lot of headache with
> > > such a migration, but you can make flags a u32 then if you find space
> > > and wouldn't even need and extending flag.
> > 
> > It is one hard problem, and it may not be answered in short time, cause all
> > use cases need to be covered, meantime 3 extra bytes are saved from the
> > reshuffling, with alignment respected meantime.
> > 
> > Also it isn't worth of layout v2 just for extending sqe flags.
> 
> Not just, by opting to the pain of migration to a new SQE layout
> we can revise more API decisions. It's a separate topic for
> discussion, and the latter it's done the better because we'd
> collect more design mistakes that can be fixed.
> 
> Reiterating again, you're not blocked by it.
> 
> > > > Jens, can you share your idea/option wrt. extending sqe flags?
> > > > 
> > > > > 
> > > > > 
> > > > > > Also I doubt the problem can be solved in layout v2:
> > > > > > 
> > > > > > - 64 byte is small enough to support everything, same for v2
> > > > > > 
> > > > > > - uring_cmd has only 16 bytes payload, taking any byte from
> > > > > > the payload may cause trouble for drivers
> > > > > > 
> > > > > > - the only possible change could still be to suppress bytes for OP
> > > > > > specific flags, but it might cause trouble for some OPs, such as
> > > > > > network.
> > > > > 
> > > > > Look up sqe's __pad1, for example
> > > > 
> > > > Suppose it is just for uring_cmd, '__pad1' is shared with cmd_op, which is aligned
> > > > with ioctl cmd and is supposed to be 32bit.
> > > 
> > > It's not shared with cmd_op, it's in a struct with it, unless you
> > > use a u32 part of ->addr2/off, it's just that, a completely
> > > unnecessary created padding. There was also another field left,
> > > at least in case for nvme.
> > 
> > OK, __pad1 is available for uring_cmd, and it could be better to use
> > __pad1 for uring_cmd ext flags, but it still depends on uring_cmd, and
> > now ext_flags can be u16 or more, :-)
> > 
> > Thanks for sharing this point.
> > 
> > > 
> > > > Same with 'off' which is used in rw at least, if sqe group is to be
> > > > generic flag.
> > > > 
> > > > > 
> > > > > 
> > > > > > > idea 2: the group assembling flag can move into cmds. Very roughly:
> > > > > > > 
> > > > > > > io_cmd_init() {
> > > > > > > 	ublk_cmd_init();
> > > > > > > }
> > > > > > > 
> > > > > > > ublk_cmd_init() {
> > > > > > > 	io_uring_start_grouping(ctx, cmd);
> > > > > > > }
> > > > > > > 
> > > > > > > io_uring_start_grouping(ctx, cmd) {
> > > > > > > 	ctx->grouping = true;
> > > > > > > 	ctx->group_head = cmd->req;
> > > > > > > }
> > > > > > 
> > > > > > How can you know one group is starting without any flag? Or you still
> > > > > > suggest the approach taken in fused command?
> > > > > 
> > > > > That would be ublk's business, e.g. ublk or cmds specific flag
> > > > 
> > > > Then it becomes dedicated fused command actually, and last year's main
> > > > concern is that the approach isn't generic.
> > > 
> > > My concern is anything leaking into hot paths, even if it's a
> > > generic feature (and I wouldn't call it that). The question is
> > > rather at what degree. I wouldn't call groups in isolation
> > > without zc exciting, and making it to look like a generic feature
> > > just for the sake of it might even be worse than having it opcode
> > > specific.
> > > 
> > > Regardless, this approach doesn't forbid some other opcode from
> > > doing ctx->grouping = true based on some other opcode specific
> > > flag, doesn't necessarily binds it to cmds/ublk.
> > 
> > Yes.
> > 
> > > 
> > > > > > > submit_sqe() {
> > > > > > > 	if (ctx->grouping) {
> > > > > > > 		link_to_group(req, ctx->group_head);
> > > > > > > 		if (!(req->flags & REQ_F_LINK))
> > > > > > > 			ctx->grouping = false;
> > > > > > > 	}
> > > > > > > }
> > > > > > 
> > > > > > The group needs to be linked to existed link chain, so reusing REQ_F_LINK may
> > > > > > not doable.
> > > > > 
> > > > > Would it break zero copy feature if you cant?
> > > > 
> > > > The whole sqe group needs to be linked to existed link chain, so we
> > > > can't reuse REQ_F_LINK here.
> > > 
> > > Why though? You're passing a buffer from the head to all group-linked
> > > requests, how do normal links come into the picture?
> > 
> > For example of ublk-nbd, tcp send requests have to be linked, and each
> > send request belongs to one group in case of zero copy.
> 
> "Linked" like in "add to a group"? Or in terms of traditional
> IOSQE_IO_LINK links? Because if it's about groups you don't need

It is IOSQE_IO_LINK.

Here one group is just for handling zero copy IO, such as handling one
ublk block request, but there can be lots of such groups, each group
needs to be linked with another group, because it is tcp send.

There are more such examples, such as normal IO depending on meta IO by
IOSQE_IO_LINK.

You are suggesting to reuse IOSQE_IO_LINK, then this inter-group linking
or meta IO linking with normal group is simply non working.

> IOSQE_IO_LINK. And if it's IOSQE_IO_LINK linking, then same
> is supposedly can be done in userspace.

Yes, it can work by forcing to replace IOSQE_IO_LINK with one extra
io_uring_enter() in userspace, together with extra overhead, and more complexity.

That is why I think disallowing generic feature of IOSQE_IO_LINK isn't one
wise option. Is there any other OP or example which doesn't support IOSQE_IO_LINK?

Thanks,
Ming

[PATCH 3/9] io_uring: add helper for filling cqes in __io_submit_flush_completions()

[Ming Lei]

No functional change, and prepare for supporting SQE group.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 io_uring/io_uring.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 6d4def11aebf..c73819c04c0b 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1458,16 +1458,14 @@ static void io_free_batch_list(struct io_ring_ctx *ctx,
 	} while (node);
 }
 
-void __io_submit_flush_completions(struct io_ring_ctx *ctx)
-	__must_hold(&ctx->uring_lock)
+static inline void io_fill_cqe_lists(struct io_ring_ctx *ctx,
+				     struct io_wq_work_list *list)
 {
-	struct io_submit_state *state = &ctx->submit_state;
 	struct io_wq_work_node *node;
 
-	__io_cq_lock(ctx);
-	__wq_list_for_each(node, &state->compl_reqs) {
+	__wq_list_for_each(node, list) {
 		struct io_kiocb *req = container_of(node, struct io_kiocb,
-					    comp_list);
+						    comp_list);
 
 		if (!(req->flags & REQ_F_CQE_SKIP) &&
 		    unlikely(!io_fill_cqe_req(ctx, req))) {
@@ -1480,6 +1478,15 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx)
 			}
 		}
 	}
+}
+
+void __io_submit_flush_completions(struct io_ring_ctx *ctx)
+	__must_hold(&ctx->uring_lock)
+{
+	struct io_submit_state *state = &ctx->submit_state;
+
+	__io_cq_lock(ctx);
+	io_fill_cqe_lists(ctx, &state->compl_reqs);
 	__io_cq_unlock_post(ctx);
 
 	if (!wq_list_empty(&ctx->submit_state.compl_reqs)) {
-- 
2.42.0

[PATCH 4/9] io_uring: add one output argument to io_submit_sqe

[Ming Lei]

Add one output argument to io_submit_sqe() for returning how many SQEs
handled in this function.

Prepare for supporting SQE group, which can include multiple member SQEs
to handle in io_submit_sqe().

No functional change.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 io_uring/io_uring.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index c73819c04c0b..4969d21ea2f8 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2204,12 +2204,13 @@ static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
 }
 
 static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
-			 const struct io_uring_sqe *sqe)
+			 const struct io_uring_sqe *sqe, unsigned int *nr)
 	__must_hold(&ctx->uring_lock)
 {
 	struct io_submit_link *link = &ctx->submit_state.link;
 	int ret;
 
+	*nr = 1;
 	ret = io_init_req(ctx, req, sqe);
 	if (unlikely(ret))
 		return io_submit_fail_init(sqe, req, ret);
@@ -2351,6 +2352,7 @@ int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr)
 	do {
 		const struct io_uring_sqe *sqe;
 		struct io_kiocb *req;
+		unsigned int done;
 
 		if (unlikely(!io_alloc_req(ctx, &req)))
 			break;
@@ -2363,12 +2365,13 @@ int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr)
 		 * Continue submitting even for sqe failure if the
 		 * ring was setup with IORING_SETUP_SUBMIT_ALL
 		 */
-		if (unlikely(io_submit_sqe(ctx, req, sqe)) &&
+		if (unlikely(io_submit_sqe(ctx, req, sqe, &done)) &&
 		    !(ctx->flags & IORING_SETUP_SUBMIT_ALL)) {
-			left--;
+			left -= done;
 			break;
 		}
-	} while (--left);
+		left -= done;
+	} while (left);
 
 	if (unlikely(left)) {
 		ret -= left;
-- 
2.42.0

[PATCH 5/9] io_uring: support SQE group

[Ming Lei]

SQE group is defined as one chain of SQEs starting with the first sqe that
has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
doesn't have it set, and it is similar with chain of linked sqes.

The 1st SQE is group leader, and the other SQEs are group member. The group
leader is always freed after all members are completed. Group members
aren't submitted until the group leader is completed, and there isn't any
dependency among group members, and IOSQE_IO_LINK can't be set for group
members, same with IOSQE_IO_DRAIN.

Typically the group leader provides or makes resource, and the other members
consume the resource, such as scenario of multiple backup, the 1st SQE is to
read data from source file into fixed buffer, the other SQEs write data from
the same buffer into other destination files. SQE group provides very
efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
submitted in single syscall, no need to submit fs read SQE first, and wait
until read SQE is completed, 2) no need to link all write SQEs together, then
write SQEs can be submitted to files concurrently. Meantime application is
simplified a lot in this way.

Another use case is to for supporting generic device zero copy:

- the lead SQE is for providing device buffer, which is owned by device or
  kernel, can't be cross userspace, otherwise easy to cause leak for devil
  application or panic

- member SQEs reads or writes concurrently against the buffer provided by lead
  SQE

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/io_uring_types.h |  10 ++
 include/uapi/linux/io_uring.h  |   4 +
 io_uring/io_uring.c            | 218 ++++++++++++++++++++++++++++++---
 io_uring/io_uring.h            |  17 ++-
 4 files changed, 231 insertions(+), 18 deletions(-)

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index 67347e5d06ec..7ce4a2d4a8b8 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -444,6 +444,7 @@ enum {
 	REQ_F_BUFFER_SELECT_BIT	= IOSQE_BUFFER_SELECT_BIT,
 	REQ_F_CQE_SKIP_BIT	= IOSQE_CQE_SKIP_SUCCESS_BIT,
 	REQ_F_SQE_EXT_FLAGS_BIT	= IOSQE_HAS_EXT_FLAGS_BIT,
+	REQ_F_SQE_GROUP_BIT	= 8 + IOSQE_EXT_SQE_GROUP_BIT,
 
 	/* first 2 bytes are taken by user flags, shift it to not overlap */
 	REQ_F_FAIL_BIT		= 16,
@@ -474,6 +475,7 @@ enum {
 	REQ_F_CAN_POLL_BIT,
 	REQ_F_BL_EMPTY_BIT,
 	REQ_F_BL_NO_RECYCLE_BIT,
+	REQ_F_SQE_GROUP_LEAD_BIT,
 
 	/* not a real bit, just to check we're not overflowing the space */
 	__REQ_F_LAST_BIT,
@@ -497,6 +499,8 @@ enum {
 	REQ_F_BUFFER_SELECT	= IO_REQ_FLAG(REQ_F_BUFFER_SELECT_BIT),
 	/* IOSQE_CQE_SKIP_SUCCESS */
 	REQ_F_CQE_SKIP		= IO_REQ_FLAG(REQ_F_CQE_SKIP_BIT),
+	/* IOSQE_EXT_SQE_GROUP */
+	REQ_F_SQE_GROUP		= IO_REQ_FLAG(REQ_F_SQE_GROUP_BIT),
 
 	/* fail rest of links */
 	REQ_F_FAIL		= IO_REQ_FLAG(REQ_F_FAIL_BIT),
@@ -552,6 +556,8 @@ enum {
 	REQ_F_BL_EMPTY		= IO_REQ_FLAG(REQ_F_BL_EMPTY_BIT),
 	/* don't recycle provided buffers for this request */
 	REQ_F_BL_NO_RECYCLE	= IO_REQ_FLAG(REQ_F_BL_NO_RECYCLE_BIT),
+	/* sqe group lead */
+	REQ_F_SQE_GROUP_LEAD	= IO_REQ_FLAG(REQ_F_SQE_GROUP_LEAD_BIT),
 };
 
 typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts);
@@ -665,6 +671,10 @@ struct io_kiocb {
 		u64			extra1;
 		u64			extra2;
 	} big_cqe;
+
+	/* all SQE group members linked here for group lead */
+	struct io_kiocb			*grp_link;
+	atomic_t			grp_refs;
 };
 
 struct io_overflow_cqe {
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 4847d7cf1ac9..c0d34f2a2c17 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -128,6 +128,8 @@ enum io_uring_sqe_flags_bit {
 	IOSQE_BUFFER_SELECT_BIT,
 	IOSQE_CQE_SKIP_SUCCESS_BIT,
 	IOSQE_HAS_EXT_FLAGS_BIT,
+
+	IOSQE_EXT_SQE_GROUP_BIT = 0,
 };
 
 /*
@@ -152,6 +154,8 @@ enum io_uring_sqe_flags_bit {
  * sqe->uring_cmd_flags for IORING_URING_CMD.
  */
 #define IOSQE_HAS_EXT_FLAGS	(1U << IOSQE_HAS_EXT_FLAGS_BIT)
+/* defines sqe group */
+#define IOSQE_EXT_SQE_GROUP	(1U << IOSQE_EXT_SQE_GROUP_BIT)
 
 /*
  * io_uring_setup() flags
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 4969d21ea2f8..0f41b26723a8 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -147,6 +147,10 @@ static bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx,
 					 bool cancel_all);
 
 static void io_queue_sqe(struct io_kiocb *req);
+static bool io_get_sqe(struct io_ring_ctx *ctx,
+		const struct io_uring_sqe **sqe);
+static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
+		       const struct io_uring_sqe *sqe);
 
 struct kmem_cache *req_cachep;
 static struct workqueue_struct *iou_wq __ro_after_init;
@@ -925,10 +929,189 @@ bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags)
 	return posted;
 }
 
+static void __io_req_failed(struct io_kiocb *req, s32 res, bool skip_cqe)
+{
+	const struct io_cold_def *def = &io_cold_defs[req->opcode];
+
+	lockdep_assert_held(&req->ctx->uring_lock);
+
+	if (skip_cqe)
+		req->flags |= REQ_F_FAIL | REQ_F_CQE_SKIP;
+	else
+		req_set_fail(req);
+	io_req_set_res(req, res, io_put_kbuf(req, IO_URING_F_UNLOCKED));
+	if (def->fail)
+		def->fail(req);
+}
+
+void io_req_defer_failed(struct io_kiocb *req, s32 res)
+	__must_hold(&ctx->uring_lock)
+{
+	__io_req_failed(req, res, false);
+	io_req_complete_defer(req);
+}
+
+static void io_req_defer_failed_sliently(struct io_kiocb *req, s32 res)
+	__must_hold(&ctx->uring_lock)
+{
+	__io_req_failed(req, res, true);
+	io_req_complete_defer(req);
+}
+
+/*
+ * Called after member req is completed, and return the lead request for
+ * caller to fill cqe & free it really
+ */
+static inline struct io_kiocb *io_complete_group_member(struct io_kiocb *req)
+{
+	struct io_kiocb *lead = req->grp_link;
+
+	req->grp_link = NULL;
+	if (lead && atomic_dec_and_test(&lead->grp_refs)) {
+		req->grp_link = NULL;
+		lead->flags &= ~REQ_F_SQE_GROUP_LEAD;
+		return lead;
+	}
+
+	return NULL;
+}
+
+/*
+ * Called after lead req is completed and before posting cqe and freeing,
+ * for issuing member requests
+ */
+void io_complete_group_lead(struct io_kiocb *req, unsigned issue_flags)
+{
+	struct io_kiocb *member = req->grp_link;
+
+	if (!member)
+		return;
+
+	while (member) {
+		struct io_kiocb *next = member->grp_link;
+
+		member->grp_link = req;
+		if (unlikely(req->flags & REQ_F_FAIL)) {
+			/*
+			 * Now group lead is failed, so simply fail members
+			 * with -EIO, and the application can figure out
+			 * the reason from lead's cqe->res
+			 */
+			__io_req_failed(member, -EIO, false);
+
+			if (issue_flags & IO_URING_F_COMPLETE_DEFER)
+				io_req_complete_defer(member);
+			else {
+				member->io_task_work.func = io_req_task_complete;
+				io_req_task_work_add(member);
+			}
+		} else {
+			trace_io_uring_submit_req(member);
+			if ((issue_flags & IO_URING_F_COMPLETE_DEFER) &&
+					!(member->flags & REQ_F_FORCE_ASYNC))
+				io_queue_sqe(member);
+			else
+				io_req_task_queue(member);
+		}
+		member = next;
+	}
+	req->grp_link = NULL;
+}
+
+static bool sqe_is_group_member(const struct io_uring_sqe *sqe)
+{
+	return (READ_ONCE(sqe->flags) & IOSQE_HAS_EXT_FLAGS) &&
+		(READ_ONCE(sqe->ext_flags) & IOSQE_EXT_SQE_GROUP);
+}
+
+/*
+ * Initialize the whole SQE group.
+ *
+ * Walk every member in this group even though failure happens. If the lead
+ * request is failed, CQE is only posted for lead, otherwise, CQE is posted
+ * for every member request. Member requests aren't issued until the lead
+ * is completed, and the lead request won't be freed until all member
+ * requests are completed.
+ *
+ * The whole group shares the link flag in group lead, and other members
+ * aren't allowed to set any LINK flag. And only the lead request may
+ * appear in the submission link list.
+ */
+static int io_init_req_group(struct io_ring_ctx *ctx, struct io_kiocb *lead,
+		int *nr, int lead_ret)
+	__must_hold(&ctx->uring_lock)
+{
+	bool more = true;
+	int cnt = 0;
+
+	lead->grp_link = NULL;
+	do {
+		const struct io_uring_sqe *sqe;
+		struct io_kiocb *req = NULL;
+		int ret = -ENOMEM;
+
+		io_alloc_req(ctx, &req);
+
+		if (unlikely(!io_get_sqe(ctx, &sqe))) {
+			if (req)
+				io_req_add_to_cache(req, ctx);
+			break;
+		}
+
+		/* one group ends with !IOSQE_EXT_SQE_GROUP */
+		if (!sqe_is_group_member(sqe))
+			more = false;
+
+		if (req) {
+			ret = io_init_req(ctx, req, sqe);
+			/*
+			 * Both IO_LINK and IO_DRAIN aren't allowed for
+			 * group member, and the boundary has to be in
+			 * the lead sqe, so the whole group shares
+			 * same IO_LINK and IO_DRAIN.
+			 */
+			if (!ret && (req->flags & (IO_REQ_LINK_FLAGS |
+							REQ_F_IO_DRAIN)))
+				ret = -EINVAL;
+			if (!more)
+				req->flags |= REQ_F_SQE_GROUP;
+			if (unlikely(ret)) {
+				/*
+				 * The lead will be failed, so don't post
+				 * CQE for any member
+				 */
+				io_req_defer_failed_sliently(req, ret);
+			} else {
+				req->grp_link = lead->grp_link;
+				lead->grp_link = req;
+			}
+		}
+		cnt += 1;
+		if (ret)
+			lead_ret = ret;
+	} while (more);
+
+	/* Mark lead if we get members, otherwise degrade it to normal req */
+	if (cnt > 0) {
+		lead->flags |= REQ_F_SQE_GROUP_LEAD;
+		atomic_set(&lead->grp_refs, cnt);
+		*nr += cnt;
+	} else {
+		lead->flags &= ~REQ_F_SQE_GROUP;
+	}
+
+	return lead_ret;
+}
+
 static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
+	if (req_is_group_lead(req)) {
+		io_complete_group_lead(req, issue_flags);
+		return;
+	}
+
 	/*
 	 * All execution paths but io-wq use the deferred completions by
 	 * passing IO_URING_F_COMPLETE_DEFER and thus should not end up here.
@@ -960,20 +1143,6 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
 	req_ref_put(req);
 }
 
-void io_req_defer_failed(struct io_kiocb *req, s32 res)
-	__must_hold(&ctx->uring_lock)
-{
-	const struct io_cold_def *def = &io_cold_defs[req->opcode];
-
-	lockdep_assert_held(&req->ctx->uring_lock);
-
-	req_set_fail(req);
-	io_req_set_res(req, res, io_put_kbuf(req, IO_URING_F_UNLOCKED));
-	if (def->fail)
-		def->fail(req);
-	io_req_complete_defer(req);
-}
-
 /*
  * Don't initialise the fields below on every allocation, but do that in
  * advance and keep them valid across allocations.
@@ -1459,7 +1628,8 @@ static void io_free_batch_list(struct io_ring_ctx *ctx,
 }
 
 static inline void io_fill_cqe_lists(struct io_ring_ctx *ctx,
-				     struct io_wq_work_list *list)
+				     struct io_wq_work_list *list,
+				     struct io_wq_work_list *grp)
 {
 	struct io_wq_work_node *node;
 
@@ -1477,6 +1647,13 @@ static inline void io_fill_cqe_lists(struct io_ring_ctx *ctx,
 				io_req_cqe_overflow(req);
 			}
 		}
+
+		if (grp && req_is_group_member(req)) {
+			struct io_kiocb *lead = io_complete_group_member(req);
+
+			if (lead)
+				wq_list_add_head(&lead->comp_list, grp);
+		}
 	}
 }
 
@@ -1484,14 +1661,19 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx)
 	__must_hold(&ctx->uring_lock)
 {
 	struct io_submit_state *state = &ctx->submit_state;
+	struct io_wq_work_list list = {NULL, NULL};
 
 	__io_cq_lock(ctx);
-	io_fill_cqe_lists(ctx, &state->compl_reqs);
+	io_fill_cqe_lists(ctx, &state->compl_reqs, &list);
+	if (!wq_list_empty(&list))
+		io_fill_cqe_lists(ctx, &list, NULL);
 	__io_cq_unlock_post(ctx);
 
-	if (!wq_list_empty(&ctx->submit_state.compl_reqs)) {
+	if (!wq_list_empty(&state->compl_reqs)) {
 		io_free_batch_list(ctx, state->compl_reqs.first);
 		INIT_WQ_LIST(&state->compl_reqs);
+		if (!wq_list_empty(&list))
+			io_free_batch_list(ctx, list.first);
 	}
 	ctx->submit_state.cq_flush = false;
 }
@@ -2212,6 +2394,8 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 
 	*nr = 1;
 	ret = io_init_req(ctx, req, sqe);
+	if (req->flags & REQ_F_SQE_GROUP)
+		ret = io_init_req_group(ctx, req, nr, ret);
 	if (unlikely(ret))
 		return io_submit_fail_init(sqe, req, ret);
 
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 1eb65324792a..99eeb4eee219 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -104,6 +104,7 @@ bool __io_alloc_req_refill(struct io_ring_ctx *ctx);
 
 bool io_match_task_safe(struct io_kiocb *head, struct task_struct *task,
 			bool cancel_all);
+void io_complete_group_lead(struct io_kiocb *req, unsigned int issue_flags);
 
 enum {
 	IO_EVENTFD_OP_SIGNAL_BIT,
@@ -113,6 +114,16 @@ enum {
 void io_eventfd_ops(struct rcu_head *rcu);
 void io_activate_pollwq(struct io_ring_ctx *ctx);
 
+static inline bool req_is_group_lead(struct io_kiocb *req)
+{
+	return req->flags & REQ_F_SQE_GROUP_LEAD;
+}
+
+static inline bool req_is_group_member(struct io_kiocb *req)
+{
+	return !req_is_group_lead(req) && (req->flags & REQ_F_SQE_GROUP);
+}
+
 static inline void io_lockdep_assert_cq_locked(struct io_ring_ctx *ctx)
 {
 #if defined(CONFIG_PROVE_LOCKING)
@@ -355,7 +366,11 @@ static inline void io_req_complete_defer(struct io_kiocb *req)
 
 	lockdep_assert_held(&req->ctx->uring_lock);
 
-	wq_list_add_tail(&req->comp_list, &state->compl_reqs);
+	if (unlikely(req_is_group_lead(req)))
+		io_complete_group_lead(req, IO_URING_F_COMPLETE_DEFER |
+				IO_URING_F_NONBLOCK);
+	else
+		wq_list_add_tail(&req->comp_list, &state->compl_reqs);
 }
 
 static inline void io_commit_cqring_flush(struct io_ring_ctx *ctx)
-- 
2.42.0

Re: [PATCH 5/9] io_uring: support SQE group

[Jens Axboe]

On 4/7/24 7:03 PM, Ming Lei wrote:
> SQE group is defined as one chain of SQEs starting with the first sqe that
> has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> doesn't have it set, and it is similar with chain of linked sqes.
> 
> The 1st SQE is group leader, and the other SQEs are group member. The group
> leader is always freed after all members are completed. Group members
> aren't submitted until the group leader is completed, and there isn't any
> dependency among group members, and IOSQE_IO_LINK can't be set for group
> members, same with IOSQE_IO_DRAIN.
> 
> Typically the group leader provides or makes resource, and the other members
> consume the resource, such as scenario of multiple backup, the 1st SQE is to
> read data from source file into fixed buffer, the other SQEs write data from
> the same buffer into other destination files. SQE group provides very
> efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> submitted in single syscall, no need to submit fs read SQE first, and wait
> until read SQE is completed, 2) no need to link all write SQEs together, then
> write SQEs can be submitted to files concurrently. Meantime application is
> simplified a lot in this way.
> 
> Another use case is to for supporting generic device zero copy:
> 
> - the lead SQE is for providing device buffer, which is owned by device or
>   kernel, can't be cross userspace, otherwise easy to cause leak for devil
>   application or panic
> 
> - member SQEs reads or writes concurrently against the buffer provided by lead
>   SQE

In concept, this looks very similar to "sqe bundles" that I played with
in the past:

https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle

Didn't look too closely yet at the implementation, but in spirit it's
about the same in that the first entry is processed first, and there's
no ordering implied between the test of the members of the bundle /
group.

I do think that's a flexible thing to support, particularly if:

1) We can do it more efficiently than links, which are pretty horrible.
2) It enables new worthwhile use cases
3) It's done cleanly 
4) It's easily understandable and easy to document, so that users will
   actually understand what this is and what use cases it enable. Part
   of that is actually naming, it should be readily apparent what a
   group is, what the lead is, and what the members are. Using your
   terminology here, definitely worth spending some time on that to get
   it just right and self evident.

-- 
Jens Axboe

Re: [PATCH 5/9] io_uring: support SQE group

[Kevin Wolf]

Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
> On 4/7/24 7:03 PM, Ming Lei wrote:
> > SQE group is defined as one chain of SQEs starting with the first sqe that
> > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > doesn't have it set, and it is similar with chain of linked sqes.
> > 
> > The 1st SQE is group leader, and the other SQEs are group member. The group
> > leader is always freed after all members are completed. Group members
> > aren't submitted until the group leader is completed, and there isn't any
> > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > members, same with IOSQE_IO_DRAIN.
> > 
> > Typically the group leader provides or makes resource, and the other members
> > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > read data from source file into fixed buffer, the other SQEs write data from
> > the same buffer into other destination files. SQE group provides very
> > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > submitted in single syscall, no need to submit fs read SQE first, and wait
> > until read SQE is completed, 2) no need to link all write SQEs together, then
> > write SQEs can be submitted to files concurrently. Meantime application is
> > simplified a lot in this way.
> > 
> > Another use case is to for supporting generic device zero copy:
> > 
> > - the lead SQE is for providing device buffer, which is owned by device or
> >   kernel, can't be cross userspace, otherwise easy to cause leak for devil
> >   application or panic
> > 
> > - member SQEs reads or writes concurrently against the buffer provided by lead
> >   SQE
> 
> In concept, this looks very similar to "sqe bundles" that I played with
> in the past:
> 
> https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
> 
> Didn't look too closely yet at the implementation, but in spirit it's
> about the same in that the first entry is processed first, and there's
> no ordering implied between the test of the members of the bundle /
> group.

When I first read this patch, I wondered if it wouldn't make sense to
allow linking a group with subsequent requests, e.g. first having a few
requests that run in parallel and once all of them have completed
continue with the next linked one sequentially.

For SQE bundles, you reused the LINK flag, which doesn't easily allow
this. Ming's patch uses a new flag for groups, so the interface would be
more obvious, you simply set the LINK flag on the last member of the
group (or on the leader, doesn't really matter). Of course, this doesn't
mean it has to be implemented now, but there is a clear way forward if
it's wanted.

The part that looks a bit arbitrary in Ming's patch is that the group
leader is always completed before the rest starts. It makes perfect
sense in the context that this series is really after (enabling zero
copy for ublk), but it doesn't really allow the case you mention in the
SQE bundle commit message, running everything in parallel and getting a
single CQE for the whole group.

I suppose you could hack around the sequential nature of the first
request by using an extra NOP as the group leader - which isn't any
worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
the group completion would still be missing. (Of course, removing the
sequential first operation would mean that ublk wouldn't have the buffer
ready any more when the other requests try to use it, so that would
defeat the purpose of the series...)

I wonder if we can still combine both approaches and create some
generally useful infrastructure and not something where it's visible
that it was designed mostly for ublk's special case and other use cases
just happened to be enabled as a side effect.

Kevin

Re: [PATCH 5/9] io_uring: support SQE group

[Ming Lei]

On Tue, Apr 23, 2024 at 03:08:55PM +0200, Kevin Wolf wrote:
> Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
> > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > SQE group is defined as one chain of SQEs starting with the first sqe that
> > > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > > doesn't have it set, and it is similar with chain of linked sqes.
> > > 
> > > The 1st SQE is group leader, and the other SQEs are group member. The group
> > > leader is always freed after all members are completed. Group members
> > > aren't submitted until the group leader is completed, and there isn't any
> > > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > > members, same with IOSQE_IO_DRAIN.
> > > 
> > > Typically the group leader provides or makes resource, and the other members
> > > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > > read data from source file into fixed buffer, the other SQEs write data from
> > > the same buffer into other destination files. SQE group provides very
> > > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > > submitted in single syscall, no need to submit fs read SQE first, and wait
> > > until read SQE is completed, 2) no need to link all write SQEs together, then
> > > write SQEs can be submitted to files concurrently. Meantime application is
> > > simplified a lot in this way.
> > > 
> > > Another use case is to for supporting generic device zero copy:
> > > 
> > > - the lead SQE is for providing device buffer, which is owned by device or
> > >   kernel, can't be cross userspace, otherwise easy to cause leak for devil
> > >   application or panic
> > > 
> > > - member SQEs reads or writes concurrently against the buffer provided by lead
> > >   SQE
> > 
> > In concept, this looks very similar to "sqe bundles" that I played with
> > in the past:
> > 
> > https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
> > 
> > Didn't look too closely yet at the implementation, but in spirit it's
> > about the same in that the first entry is processed first, and there's
> > no ordering implied between the test of the members of the bundle /
> > group.
> 
> When I first read this patch, I wondered if it wouldn't make sense to
> allow linking a group with subsequent requests, e.g. first having a few
> requests that run in parallel and once all of them have completed
> continue with the next linked one sequentially.
> 
> For SQE bundles, you reused the LINK flag, which doesn't easily allow
> this. Ming's patch uses a new flag for groups, so the interface would be
> more obvious, you simply set the LINK flag on the last member of the
> group (or on the leader, doesn't really matter). Of course, this doesn't
> mean it has to be implemented now, but there is a clear way forward if
> it's wanted.

Reusing LINK for bundle breaks existed link chains(BUNDLE linked to existed
link chain), so I think it may not work.

The link rule is explicit for sqe group:

- only group leader can set link flag, which is applied on the whole
group: the next sqe in the link chain won't be started until the
previous linked sqe group is completed

- link flag can't be set for group members

Also sqe group doesn't limit async for both group leader and member.

sqe group vs link & async is covered in the last liburing test code.

> 
> The part that looks a bit arbitrary in Ming's patch is that the group
> leader is always completed before the rest starts. It makes perfect
> sense in the context that this series is really after (enabling zero
> copy for ublk), but it doesn't really allow the case you mention in the
> SQE bundle commit message, running everything in parallel and getting a
> single CQE for the whole group.

I think it should be easy to cover bundle in this way, such as add one new
op IORING_OP_BUNDLE as Jens did, and implement the single CQE for whole group/bundle.

> 
> I suppose you could hack around the sequential nature of the first
> request by using an extra NOP as the group leader - which isn't any
> worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
> the group completion would still be missing. (Of course, removing the
> sequential first operation would mean that ublk wouldn't have the buffer
> ready any more when the other requests try to use it, so that would
> defeat the purpose of the series...)
> 
> I wonder if we can still combine both approaches and create some
> generally useful infrastructure and not something where it's visible
> that it was designed mostly for ublk's special case and other use cases
> just happened to be enabled as a side effect.

sqe group is actually one generic interface, please see the multiple copy(
copy one file to multiple destinations in single syscall for one range) example
in the last patch, and it can support generic device zero copy: any device internal
buffer can be linked with io_uring operations in this way, which can't
be done by traditional splice/pipe.

I guess it can be used in network Rx zero copy too, but may depend on actual
network Rx use case.



Thanks,
Ming

Re: [PATCH 5/9] io_uring: support SQE group

[Kevin Wolf]

Am 24.04.2024 um 03:39 hat Ming Lei geschrieben:
> On Tue, Apr 23, 2024 at 03:08:55PM +0200, Kevin Wolf wrote:
> > Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
> > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > SQE group is defined as one chain of SQEs starting with the first sqe that
> > > > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > > > doesn't have it set, and it is similar with chain of linked sqes.
> > > > 
> > > > The 1st SQE is group leader, and the other SQEs are group member. The group
> > > > leader is always freed after all members are completed. Group members
> > > > aren't submitted until the group leader is completed, and there isn't any
> > > > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > > > members, same with IOSQE_IO_DRAIN.
> > > > 
> > > > Typically the group leader provides or makes resource, and the other members
> > > > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > > > read data from source file into fixed buffer, the other SQEs write data from
> > > > the same buffer into other destination files. SQE group provides very
> > > > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > > > submitted in single syscall, no need to submit fs read SQE first, and wait
> > > > until read SQE is completed, 2) no need to link all write SQEs together, then
> > > > write SQEs can be submitted to files concurrently. Meantime application is
> > > > simplified a lot in this way.
> > > > 
> > > > Another use case is to for supporting generic device zero copy:
> > > > 
> > > > - the lead SQE is for providing device buffer, which is owned by device or
> > > >   kernel, can't be cross userspace, otherwise easy to cause leak for devil
> > > >   application or panic
> > > > 
> > > > - member SQEs reads or writes concurrently against the buffer provided by lead
> > > >   SQE
> > > 
> > > In concept, this looks very similar to "sqe bundles" that I played with
> > > in the past:
> > > 
> > > https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
> > > 
> > > Didn't look too closely yet at the implementation, but in spirit it's
> > > about the same in that the first entry is processed first, and there's
> > > no ordering implied between the test of the members of the bundle /
> > > group.
> > 
> > When I first read this patch, I wondered if it wouldn't make sense to
> > allow linking a group with subsequent requests, e.g. first having a few
> > requests that run in parallel and once all of them have completed
> > continue with the next linked one sequentially.
> > 
> > For SQE bundles, you reused the LINK flag, which doesn't easily allow
> > this. Ming's patch uses a new flag for groups, so the interface would be
> > more obvious, you simply set the LINK flag on the last member of the
> > group (or on the leader, doesn't really matter). Of course, this doesn't
> > mean it has to be implemented now, but there is a clear way forward if
> > it's wanted.
> 
> Reusing LINK for bundle breaks existed link chains (BUNDLE linked to
> existed link chain), so I think it may not work.

You can always extend things *somehow*, but it wouldn't fit very
naturally. That's why I feel your approach on this detail is a little
better.

> The link rule is explicit for sqe group:
> 
> - only group leader can set link flag, which is applied on the whole
> group: the next sqe in the link chain won't be started until the
> previous linked sqe group is completed
> 
> - link flag can't be set for group members
> 
> Also sqe group doesn't limit async for both group leader and member.
> 
> sqe group vs link & async is covered in the last liburing test code.

Oh right, I didn't actually notice that you already implement what I
proposed!

I was expecting the flag on the last SQE and I saw in the code that this
isn't allowed, but I completely missed your comment that explicitly
states that it's the group leader that gets the link flag. Of course,
this is just as good.

> > The part that looks a bit arbitrary in Ming's patch is that the group
> > leader is always completed before the rest starts. It makes perfect
> > sense in the context that this series is really after (enabling zero
> > copy for ublk), but it doesn't really allow the case you mention in the
> > SQE bundle commit message, running everything in parallel and getting a
> > single CQE for the whole group.
> 
> I think it should be easy to cover bundle in this way, such as add one
> new op IORING_OP_BUNDLE as Jens did, and implement the single CQE for
> whole group/bundle.

This requires an extra SQE compared to just creating the group with
flags, but I suppose this is not a big problem. An alternative might be
sending the CQE for the group leader only after the whole group has
completed if we're okay with userspace never knowing when the leader
itself completed.

However, assuming an IORING_OP_BUNDLE command, if this command only
completes after the whole group, doesn't that conflict with the
principle that all other commands are only started after the first one
has completed?

Maybe we shouldn't wait for the whole group leader request to complete,
but just give the group leader a chance to prepare the group before all
requests in the group (including the leader itself) are run in parallel.
Maybe io_issue_sqe() could just start the rest of the group somewhere
after calling def->issue() for the leader. Then you can't prepare the
group buffer asynchronously, but I don't think this is needed, right?

Your example with one read followed by multiple writes would then have
to be written slightly differently: First the read outside of the group,
linked to a group of writes. I honestly think this makes more sense as
an interface, too, because then links are for sequential things and
groups are (only) for parallel things. This feels clearer than having
both a sequential and a parallel element in groups.

> > I suppose you could hack around the sequential nature of the first
> > request by using an extra NOP as the group leader - which isn't any
> > worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
> > the group completion would still be missing. (Of course, removing the
> > sequential first operation would mean that ublk wouldn't have the buffer
> > ready any more when the other requests try to use it, so that would
> > defeat the purpose of the series...)
> > 
> > I wonder if we can still combine both approaches and create some
> > generally useful infrastructure and not something where it's visible
> > that it was designed mostly for ublk's special case and other use cases
> > just happened to be enabled as a side effect.
> 
> sqe group is actually one generic interface, please see the multiple
> copy( copy one file to multiple destinations in single syscall for one
> range) example in the last patch

Yes, that's an example that happens to work well with the model that you
derived from ublk.

If you have the opposite case, reading a buffer that is spread across
multiple files and then writing it to one target (i.e. first step
parallel, second step sequential), you can't represent this well
currently. You could work around it by having a NOP leader, but that's
not very elegant.

This asymmetry suggests that it's not the perfect interface yet.

If the whole group runs in parallel instead, including the leader, then
both examples become symmetrical. You have a group for the parallel I/O
and a linked single request for the other operation.

Or if both steps are parallel, you can just have two linked groups.

> and it can support generic device zero copy: any device internal
> buffer can be linked with io_uring operations in this way, which can't
> be done by traditional splice/pipe.

Is this actually implemented or is it just a potential direction for the
future?

> I guess it can be used in network Rx zero copy too, but may depend on
> actual network Rx use case.

Kevin

Re: [PATCH 5/9] io_uring: support SQE group

[Ming Lei]

On Thu, Apr 25, 2024 at 11:27:32AM +0200, Kevin Wolf wrote:
> Am 24.04.2024 um 03:39 hat Ming Lei geschrieben:
> > On Tue, Apr 23, 2024 at 03:08:55PM +0200, Kevin Wolf wrote:
> > > Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
> > > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > > SQE group is defined as one chain of SQEs starting with the first sqe that
> > > > > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > > > > doesn't have it set, and it is similar with chain of linked sqes.
> > > > > 
> > > > > The 1st SQE is group leader, and the other SQEs are group member. The group
> > > > > leader is always freed after all members are completed. Group members
> > > > > aren't submitted until the group leader is completed, and there isn't any
> > > > > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > > > > members, same with IOSQE_IO_DRAIN.
> > > > > 
> > > > > Typically the group leader provides or makes resource, and the other members
> > > > > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > > > > read data from source file into fixed buffer, the other SQEs write data from
> > > > > the same buffer into other destination files. SQE group provides very
> > > > > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > > > > submitted in single syscall, no need to submit fs read SQE first, and wait
> > > > > until read SQE is completed, 2) no need to link all write SQEs together, then
> > > > > write SQEs can be submitted to files concurrently. Meantime application is
> > > > > simplified a lot in this way.
> > > > > 
> > > > > Another use case is to for supporting generic device zero copy:
> > > > > 
> > > > > - the lead SQE is for providing device buffer, which is owned by device or
> > > > >   kernel, can't be cross userspace, otherwise easy to cause leak for devil
> > > > >   application or panic
> > > > > 
> > > > > - member SQEs reads or writes concurrently against the buffer provided by lead
> > > > >   SQE
> > > > 
> > > > In concept, this looks very similar to "sqe bundles" that I played with
> > > > in the past:
> > > > 
> > > > https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
> > > > 
> > > > Didn't look too closely yet at the implementation, but in spirit it's
> > > > about the same in that the first entry is processed first, and there's
> > > > no ordering implied between the test of the members of the bundle /
> > > > group.
> > > 
> > > When I first read this patch, I wondered if it wouldn't make sense to
> > > allow linking a group with subsequent requests, e.g. first having a few
> > > requests that run in parallel and once all of them have completed
> > > continue with the next linked one sequentially.
> > > 
> > > For SQE bundles, you reused the LINK flag, which doesn't easily allow
> > > this. Ming's patch uses a new flag for groups, so the interface would be
> > > more obvious, you simply set the LINK flag on the last member of the
> > > group (or on the leader, doesn't really matter). Of course, this doesn't
> > > mean it has to be implemented now, but there is a clear way forward if
> > > it's wanted.
> > 
> > Reusing LINK for bundle breaks existed link chains (BUNDLE linked to
> > existed link chain), so I think it may not work.
> 
> You can always extend things *somehow*, but it wouldn't fit very
> naturally. That's why I feel your approach on this detail is a little
> better.

Linking group in traditionally way is real use case, please see
ublk-nbd's zero copy implementation.

https://github.com/ublk-org/ublksrv/blob/group-provide-buf/nbd/tgt_nbd.cpp

> 
> > The link rule is explicit for sqe group:
> > 
> > - only group leader can set link flag, which is applied on the whole
> > group: the next sqe in the link chain won't be started until the
> > previous linked sqe group is completed
> > 
> > - link flag can't be set for group members
> > 
> > Also sqe group doesn't limit async for both group leader and member.
> > 
> > sqe group vs link & async is covered in the last liburing test code.
> 
> Oh right, I didn't actually notice that you already implement what I
> proposed!
> 
> I was expecting the flag on the last SQE and I saw in the code that this
> isn't allowed, but I completely missed your comment that explicitly
> states that it's the group leader that gets the link flag. Of course,
> this is just as good.
> 
> > > The part that looks a bit arbitrary in Ming's patch is that the group
> > > leader is always completed before the rest starts. It makes perfect
> > > sense in the context that this series is really after (enabling zero
> > > copy for ublk), but it doesn't really allow the case you mention in the
> > > SQE bundle commit message, running everything in parallel and getting a
> > > single CQE for the whole group.
> > 
> > I think it should be easy to cover bundle in this way, such as add one
> > new op IORING_OP_BUNDLE as Jens did, and implement the single CQE for
> > whole group/bundle.
> 
> This requires an extra SQE compared to just creating the group with
> flags, but I suppose this is not a big problem. An alternative might be
> sending the CQE for the group leader only after the whole group has
> completed if we're okay with userspace never knowing when the leader
> itself completed.
> 
> However, assuming an IORING_OP_BUNDLE command, if this command only
> completes after the whole group, doesn't that conflict with the

Here the completion means committing CQE to userspace ring.

> principle that all other commands are only started after the first one
> has completed?

I meant IORING_OP_BUNDLE is the group leader, and the first one is the
the leader.

The member requests won't be started until the leader is completed, and
here the completion means that the request is completed from subsystem
(FS, netowork, ...), so there isn't conflict, but yes, we need to
describe the whole ideas/terms more carefully.

> 
> Maybe we shouldn't wait for the whole group leader request to complete,
> but just give the group leader a chance to prepare the group before all
> requests in the group (including the leader itself) are run in parallel.
> Maybe io_issue_sqe() could just start the rest of the group somewhere
> after calling def->issue() for the leader. Then you can't prepare the
> group buffer asynchronously, but I don't think this is needed, right?

That isn't true, if leader request is one network RX, we need to wait
until the recv is done, then the following member requests can be
started for consuming the received data.

Same example with the multiple copy one in last patch.

> 
> Your example with one read followed by multiple writes would then have
> to be written slightly differently: First the read outside of the group,
> linked to a group of writes. I honestly think this makes more sense as
> an interface, too, because then links are for sequential things and
> groups are (only) for parallel things. This feels clearer than having
> both a sequential and a parallel element in groups.

Group also implements 1:N dependency, in which N members depends on single
group leader, meantime there isn't any dependency among each members. That
is something the current io_uring is missing.

> 
> > > I suppose you could hack around the sequential nature of the first
> > > request by using an extra NOP as the group leader - which isn't any
> > > worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
> > > the group completion would still be missing. (Of course, removing the
> > > sequential first operation would mean that ublk wouldn't have the buffer
> > > ready any more when the other requests try to use it, so that would
> > > defeat the purpose of the series...)
> > > 
> > > I wonder if we can still combine both approaches and create some
> > > generally useful infrastructure and not something where it's visible
> > > that it was designed mostly for ublk's special case and other use cases
> > > just happened to be enabled as a side effect.
> > 
> > sqe group is actually one generic interface, please see the multiple
> > copy( copy one file to multiple destinations in single syscall for one
> > range) example in the last patch
> 
> Yes, that's an example that happens to work well with the model that you
> derived from ublk.

Not only for ublk and device zero copy, it also have the multiple copy example.

> 
> If you have the opposite case, reading a buffer that is spread across
> multiple files and then writing it to one target (i.e. first step
> parallel, second step sequential), you can't represent this well
> currently. You could work around it by having a NOP leader, but that's
> not very elegant.

Yeah, linking the group(nop & reads) with the following write does
work for the above copy case, :-)

> 
> This asymmetry suggests that it's not the perfect interface yet.

1:N dependency requires the asymmetry, an nothing in this world is perfect, :-)
But we can try to make it better.

> 
> If the whole group runs in parallel instead, including the leader, then
> both examples become symmetrical. You have a group for the parallel I/O
> and a linked single request for the other operation.
> 
> Or if both steps are parallel, you can just have two linked groups.

I think sqe group can be extended to this way easily by one new flag if
there is such real use case. We still can use leader's link flag for
same purpose, an only advance the linking chain until the whole group
is done. 

Then sqe group just degrades to single link group without 1:N dependency
covered and leader is just for providing group link flag, looks it can be
well defined and documented, and could be useful too, IMO.

> 
> > and it can support generic device zero copy: any device internal
> > buffer can be linked with io_uring operations in this way, which can't
> > be done by traditional splice/pipe.
> 
> Is this actually implemented or is it just a potential direction for the
> future?

It is potential direction since sqe group & provide buffer provides one
generic framework to export device internal buffer for further consuming
in zero copy & non-mmap way.


Thanks,
Ming

Re: [PATCH 5/9] io_uring: support SQE group

[Kevin Wolf]

Am 26.04.2024 um 09:53 hat Ming Lei geschrieben:
> On Thu, Apr 25, 2024 at 11:27:32AM +0200, Kevin Wolf wrote:
> > Am 24.04.2024 um 03:39 hat Ming Lei geschrieben:
> > > On Tue, Apr 23, 2024 at 03:08:55PM +0200, Kevin Wolf wrote:
> > > > When I first read this patch, I wondered if it wouldn't make sense to
> > > > allow linking a group with subsequent requests, e.g. first having a few
> > > > requests that run in parallel and once all of them have completed
> > > > continue with the next linked one sequentially.
> > > > 
> > > > For SQE bundles, you reused the LINK flag, which doesn't easily allow
> > > > this. Ming's patch uses a new flag for groups, so the interface would be
> > > > more obvious, you simply set the LINK flag on the last member of the
> > > > group (or on the leader, doesn't really matter). Of course, this doesn't
> > > > mean it has to be implemented now, but there is a clear way forward if
> > > > it's wanted.
> > > 
> > > Reusing LINK for bundle breaks existed link chains (BUNDLE linked to
> > > existed link chain), so I think it may not work.
> > 
> > You can always extend things *somehow*, but it wouldn't fit very
> > naturally. That's why I feel your approach on this detail is a little
> > better.
> 
> Linking group in traditionally way is real use case, please see
> ublk-nbd's zero copy implementation.
> 
> https://github.com/ublk-org/ublksrv/blob/group-provide-buf/nbd/tgt_nbd.cpp

I'm not sure what you're trying to argue, I agreed with you twice?

I don't think Jens's bundles break existing users of links because the
special meaning is only triggered with IORING_OP_BUNDLE. But obviously
they don't allow linking a bundle with something else after it, which
feels more limiting than necessary.

> > > The link rule is explicit for sqe group:
> > > 
> > > - only group leader can set link flag, which is applied on the whole
> > > group: the next sqe in the link chain won't be started until the
> > > previous linked sqe group is completed
> > > 
> > > - link flag can't be set for group members
> > > 
> > > Also sqe group doesn't limit async for both group leader and member.
> > > 
> > > sqe group vs link & async is covered in the last liburing test code.
> > 
> > Oh right, I didn't actually notice that you already implement what I
> > proposed!
> > 
> > I was expecting the flag on the last SQE and I saw in the code that this
> > isn't allowed, but I completely missed your comment that explicitly
> > states that it's the group leader that gets the link flag. Of course,
> > this is just as good.
> > 
> > > > The part that looks a bit arbitrary in Ming's patch is that the group
> > > > leader is always completed before the rest starts. It makes perfect
> > > > sense in the context that this series is really after (enabling zero
> > > > copy for ublk), but it doesn't really allow the case you mention in the
> > > > SQE bundle commit message, running everything in parallel and getting a
> > > > single CQE for the whole group.
> > > 
> > > I think it should be easy to cover bundle in this way, such as add one
> > > new op IORING_OP_BUNDLE as Jens did, and implement the single CQE for
> > > whole group/bundle.
> > 
> > This requires an extra SQE compared to just creating the group with
> > flags, but I suppose this is not a big problem. An alternative might be
> > sending the CQE for the group leader only after the whole group has
> > completed if we're okay with userspace never knowing when the leader
> > itself completed.
> > 
> > However, assuming an IORING_OP_BUNDLE command, if this command only
> > completes after the whole group, doesn't that conflict with the
> 
> Here the completion means committing CQE to userspace ring.
> 
> > principle that all other commands are only started after the first one
> > has completed?
> 
> I meant IORING_OP_BUNDLE is the group leader, and the first one is the
> the leader.
> 
> The member requests won't be started until the leader is completed, and
> here the completion means that the request is completed from subsystem
> (FS, netowork, ...), so there isn't conflict, but yes, we need to
> describe the whole ideas/terms more carefully.

Is there precedence for requests that are completed, but don't result in
a CQE immediately? But yes, it's the same as I had in mind above when I
was talking about completing the leader only after the whole group has
completed.

> > Maybe we shouldn't wait for the whole group leader request to complete,
> > but just give the group leader a chance to prepare the group before all
> > requests in the group (including the leader itself) are run in parallel.
> > Maybe io_issue_sqe() could just start the rest of the group somewhere
> > after calling def->issue() for the leader. Then you can't prepare the
> > group buffer asynchronously, but I don't think this is needed, right?
> 
> That isn't true, if leader request is one network RX, we need to wait
> until the recv is done, then the following member requests can be
> started for consuming the received data.
> 
> Same example with the multiple copy one in last patch.

I don't see a group kernel buffer in the last patch at all? It seems to
use userspace buffers. In which case the read doesn't have to be part of
the group at all: You can have a read and link that with a group of
writes. Then you have the clear semantics of link = sequential,
group = parallel again.

But let's assume that the read actually did provide a group buffer.

What this example showed me is that grouping requests for parallel
submission is logically independent from grouping requests for sharing a
buffer. For full flexibility, they would probably have to be separate
concepts. You could then have the same setup as before (read linked to a
group of writes), but still share a group kernel buffer for the whole
sequence.

However, it's not clear if that the full flexibility is needed, and it
would probably complicate the implementation a bit.

> > Your example with one read followed by multiple writes would then have
> > to be written slightly differently: First the read outside of the group,
> > linked to a group of writes. I honestly think this makes more sense as
> > an interface, too, because then links are for sequential things and
> > groups are (only) for parallel things. This feels clearer than having
> > both a sequential and a parallel element in groups.
> 
> Group also implements 1:N dependency, in which N members depends on
> single group leader, meantime there isn't any dependency among each
> members. That is something the current io_uring is missing.

Dependencies are currently expressed with links, which is why I felt
that it would be good to use them in this case, too. Groups that only
include parallel requests and can be part of a link chain even provide
N:M dependencies, so are even more powerful than the fixed 1:N of your
groups.

The only thing that doesn't work as nicely then is sharing the buffer as
long as it's not treated as a separate concept.

> > > > I suppose you could hack around the sequential nature of the first
> > > > request by using an extra NOP as the group leader - which isn't any
> > > > worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
> > > > the group completion would still be missing. (Of course, removing the
> > > > sequential first operation would mean that ublk wouldn't have the buffer
> > > > ready any more when the other requests try to use it, so that would
> > > > defeat the purpose of the series...)
> > > > 
> > > > I wonder if we can still combine both approaches and create some
> > > > generally useful infrastructure and not something where it's visible
> > > > that it was designed mostly for ublk's special case and other use cases
> > > > just happened to be enabled as a side effect.
> > > 
> > > sqe group is actually one generic interface, please see the multiple
> > > copy( copy one file to multiple destinations in single syscall for one
> > > range) example in the last patch
> > 
> > Yes, that's an example that happens to work well with the model that you
> > derived from ublk.
> 
> Not only for ublk and device zero copy, it also have the multiple copy example.

This is what I replied to. Yes, it's an example where the model works
fine. This is not evidence that the model is as generic as it could be,
just that it's an example that fits it.

> > If you have the opposite case, reading a buffer that is spread across
> > multiple files and then writing it to one target (i.e. first step
> > parallel, second step sequential), you can't represent this well
> > currently. You could work around it by having a NOP leader, but that's
> > not very elegant.
> 
> Yeah, linking the group(nop & reads) with the following write does
> work for the above copy case, :-)
> 
> > 
> > This asymmetry suggests that it's not the perfect interface yet.
> 
> 1:N dependency requires the asymmetry, an nothing in this world is perfect, :-)
> But we can try to make it better.

The asymmetry doesn't contribute anything to the 1:N dependency. As
discussed above, normal links combined with fully parallel (and
therefore symmetrical) groups provide this functionality, too.

The only real reason I see for justifying it is the group kernel buffer.

> > If the whole group runs in parallel instead, including the leader, then
> > both examples become symmetrical. You have a group for the parallel I/O
> > and a linked single request for the other operation.
> > 
> > Or if both steps are parallel, you can just have two linked groups.
> 
> I think sqe group can be extended to this way easily by one new flag if
> there is such real use case. We still can use leader's link flag for
> same purpose, an only advance the linking chain until the whole group
> is done. 
> 
> Then sqe group just degrades to single link group without 1:N dependency
> covered and leader is just for providing group link flag, looks it can be
> well defined and documented, and could be useful too, IMO.

If you're willing to introduce a second flag, then I'd consider using
that flag to define buffer sharing groups independently of groups for
parallel execution.

I think it's usually preferable to build the semantics you need by
combining flags that provide independent building blocks with a
straightforward meaning than having a single complex building block and
then flags that modify the way the complex concept works.

> > > and it can support generic device zero copy: any device internal
> > > buffer can be linked with io_uring operations in this way, which can't
> > > be done by traditional splice/pipe.
> > 
> > Is this actually implemented or is it just a potential direction for the
> > future?
> 
> It is potential direction since sqe group & provide buffer provides one
> generic framework to export device internal buffer for further consuming
> in zero copy & non-mmap way.

I see. This contributes a bit to the impression that much of the design
is driven by ublk alone, because it's the only thing that seems to make
use of group buffers so far.

Anyway, I'm just throwing in a few thoughts and ideas from outside. In
the end, Jens and you need to agree on something.

Kevin

Re: [PATCH 5/9] io_uring: support SQE group

[Ming Lei]

On Fri, Apr 26, 2024 at 07:05:17PM +0200, Kevin Wolf wrote:
> Am 26.04.2024 um 09:53 hat Ming Lei geschrieben:
> > On Thu, Apr 25, 2024 at 11:27:32AM +0200, Kevin Wolf wrote:
> > > Am 24.04.2024 um 03:39 hat Ming Lei geschrieben:
> > > > On Tue, Apr 23, 2024 at 03:08:55PM +0200, Kevin Wolf wrote:
> > > > > When I first read this patch, I wondered if it wouldn't make sense to
> > > > > allow linking a group with subsequent requests, e.g. first having a few
> > > > > requests that run in parallel and once all of them have completed
> > > > > continue with the next linked one sequentially.
> > > > > 
> > > > > For SQE bundles, you reused the LINK flag, which doesn't easily allow
> > > > > this. Ming's patch uses a new flag for groups, so the interface would be
> > > > > more obvious, you simply set the LINK flag on the last member of the
> > > > > group (or on the leader, doesn't really matter). Of course, this doesn't
> > > > > mean it has to be implemented now, but there is a clear way forward if
> > > > > it's wanted.
> > > > 
> > > > Reusing LINK for bundle breaks existed link chains (BUNDLE linked to
> > > > existed link chain), so I think it may not work.
> > > 
> > > You can always extend things *somehow*, but it wouldn't fit very
> > > naturally. That's why I feel your approach on this detail is a little
> > > better.
> > 
> > Linking group in traditionally way is real use case, please see
> > ublk-nbd's zero copy implementation.
> > 
> > https://github.com/ublk-org/ublksrv/blob/group-provide-buf/nbd/tgt_nbd.cpp
> 
> I'm not sure what you're trying to argue, I agreed with you twice?
> 
> I don't think Jens's bundles break existing users of links because the
> special meaning is only triggered with IORING_OP_BUNDLE. But obviously
> they don't allow linking a bundle with something else after it, which
> feels more limiting than necessary.

OK.

> 
> > > > The link rule is explicit for sqe group:
> > > > 
> > > > - only group leader can set link flag, which is applied on the whole
> > > > group: the next sqe in the link chain won't be started until the
> > > > previous linked sqe group is completed
> > > > 
> > > > - link flag can't be set for group members
> > > > 
> > > > Also sqe group doesn't limit async for both group leader and member.
> > > > 
> > > > sqe group vs link & async is covered in the last liburing test code.
> > > 
> > > Oh right, I didn't actually notice that you already implement what I
> > > proposed!
> > > 
> > > I was expecting the flag on the last SQE and I saw in the code that this
> > > isn't allowed, but I completely missed your comment that explicitly
> > > states that it's the group leader that gets the link flag. Of course,
> > > this is just as good.
> > > 
> > > > > The part that looks a bit arbitrary in Ming's patch is that the group
> > > > > leader is always completed before the rest starts. It makes perfect
> > > > > sense in the context that this series is really after (enabling zero
> > > > > copy for ublk), but it doesn't really allow the case you mention in the
> > > > > SQE bundle commit message, running everything in parallel and getting a
> > > > > single CQE for the whole group.
> > > > 
> > > > I think it should be easy to cover bundle in this way, such as add one
> > > > new op IORING_OP_BUNDLE as Jens did, and implement the single CQE for
> > > > whole group/bundle.
> > > 
> > > This requires an extra SQE compared to just creating the group with
> > > flags, but I suppose this is not a big problem. An alternative might be
> > > sending the CQE for the group leader only after the whole group has
> > > completed if we're okay with userspace never knowing when the leader
> > > itself completed.
> > > 
> > > However, assuming an IORING_OP_BUNDLE command, if this command only
> > > completes after the whole group, doesn't that conflict with the
> > 
> > Here the completion means committing CQE to userspace ring.
> > 
> > > principle that all other commands are only started after the first one
> > > has completed?
> > 
> > I meant IORING_OP_BUNDLE is the group leader, and the first one is the
> > the leader.
> > 
> > The member requests won't be started until the leader is completed, and
> > here the completion means that the request is completed from subsystem
> > (FS, netowork, ...), so there isn't conflict, but yes, we need to
> > describe the whole ideas/terms more carefully.
> 
> Is there precedence for requests that are completed, but don't result in
> a CQE immediately? But yes, it's the same as I had in mind above when I
> was talking about completing the leader only after the whole group has
> completed.
> 
> > > Maybe we shouldn't wait for the whole group leader request to complete,
> > > but just give the group leader a chance to prepare the group before all
> > > requests in the group (including the leader itself) are run in parallel.
> > > Maybe io_issue_sqe() could just start the rest of the group somewhere
> > > after calling def->issue() for the leader. Then you can't prepare the
> > > group buffer asynchronously, but I don't think this is needed, right?
> > 
> > That isn't true, if leader request is one network RX, we need to wait
> > until the recv is done, then the following member requests can be
> > started for consuming the received data.
> > 
> > Same example with the multiple copy one in last patch.
> 
> I don't see a group kernel buffer in the last patch at all? It seems to
> use userspace buffers. In which case the read doesn't have to be part of
> the group at all: You can have a read and link that with a group of
> writes. Then you have the clear semantics of link = sequential,
> group = parallel again.
> 
> But let's assume that the read actually did provide a group buffer.
> 
> What this example showed me is that grouping requests for parallel
> submission is logically independent from grouping requests for sharing a
> buffer. For full flexibility, they would probably have to be separate
> concepts. You could then have the same setup as before (read linked to a
> group of writes), but still share a group kernel buffer for the whole
> sequence.

kernel buffer lifetime is aligned with the whole group lifetime, that is why
the leader can't be moved out of group. Otherwise the two groups are
actually same, but still lots of things are in common, such as, how to handle
link for whole group, and the implementation should share most of
code, same with the group concept.

> 
> However, it's not clear if that the full flexibility is needed, and it
> would probably complicate the implementation a bit.
> 
> > > Your example with one read followed by multiple writes would then have
> > > to be written slightly differently: First the read outside of the group,
> > > linked to a group of writes. I honestly think this makes more sense as
> > > an interface, too, because then links are for sequential things and
> > > groups are (only) for parallel things. This feels clearer than having
> > > both a sequential and a parallel element in groups.
> > 
> > Group also implements 1:N dependency, in which N members depends on
> > single group leader, meantime there isn't any dependency among each
> > members. That is something the current io_uring is missing.
> 
> Dependencies are currently expressed with links, which is why I felt
> that it would be good to use them in this case, too. Groups that only
> include parallel requests and can be part of a link chain even provide
> N:M dependencies, so are even more powerful than the fixed 1:N of your
> groups.
> 
> The only thing that doesn't work as nicely then is sharing the buffer as
> long as it's not treated as a separate concept.

Right, kernel buffer lifetime is really one hard problem, aligning it
with group lifetime simplifies a lot for zero copy problem.

> 
> > > > > I suppose you could hack around the sequential nature of the first
> > > > > request by using an extra NOP as the group leader - which isn't any
> > > > > worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
> > > > > the group completion would still be missing. (Of course, removing the
> > > > > sequential first operation would mean that ublk wouldn't have the buffer
> > > > > ready any more when the other requests try to use it, so that would
> > > > > defeat the purpose of the series...)
> > > > > 
> > > > > I wonder if we can still combine both approaches and create some
> > > > > generally useful infrastructure and not something where it's visible
> > > > > that it was designed mostly for ublk's special case and other use cases
> > > > > just happened to be enabled as a side effect.
> > > > 
> > > > sqe group is actually one generic interface, please see the multiple
> > > > copy( copy one file to multiple destinations in single syscall for one
> > > > range) example in the last patch
> > > 
> > > Yes, that's an example that happens to work well with the model that you
> > > derived from ublk.
> > 
> > Not only for ublk and device zero copy, it also have the multiple copy example.
> 
> This is what I replied to. Yes, it's an example where the model works
> fine. This is not evidence that the model is as generic as it could be,
> just that it's an example that fits it.
> 
> > > If you have the opposite case, reading a buffer that is spread across
> > > multiple files and then writing it to one target (i.e. first step
> > > parallel, second step sequential), you can't represent this well
> > > currently. You could work around it by having a NOP leader, but that's
> > > not very elegant.
> > 
> > Yeah, linking the group(nop & reads) with the following write does
> > work for the above copy case, :-)
> > 
> > > 
> > > This asymmetry suggests that it's not the perfect interface yet.
> > 
> > 1:N dependency requires the asymmetry, an nothing in this world is perfect, :-)
> > But we can try to make it better.
> 
> The asymmetry doesn't contribute anything to the 1:N dependency. As
> discussed above, normal links combined with fully parallel (and
> therefore symmetrical) groups provide this functionality, too.
> 
> The only real reason I see for justifying it is the group kernel buffer.

Yes, but that is also exactly what the N depends on.

> 
> > > If the whole group runs in parallel instead, including the leader, then
> > > both examples become symmetrical. You have a group for the parallel I/O
> > > and a linked single request for the other operation.
> > > 
> > > Or if both steps are parallel, you can just have two linked groups.
> > 
> > I think sqe group can be extended to this way easily by one new flag if
> > there is such real use case. We still can use leader's link flag for
> > same purpose, an only advance the linking chain until the whole group
> > is done. 
> > 
> > Then sqe group just degrades to single link group without 1:N dependency
> > covered and leader is just for providing group link flag, looks it can be
> > well defined and documented, and could be useful too, IMO.
> 
> If you're willing to introduce a second flag, then I'd consider using
> that flag to define buffer sharing groups independently of groups for
> parallel execution.
> 
> I think it's usually preferable to build the semantics you need by
> combining flags that provide independent building blocks with a
> straightforward meaning than having a single complex building block and
> then flags that modify the way the complex concept works.

This way is fine.

> 
> > > > and it can support generic device zero copy: any device internal
> > > > buffer can be linked with io_uring operations in this way, which can't
> > > > be done by traditional splice/pipe.
> > > 
> > > Is this actually implemented or is it just a potential direction for the
> > > future?
> > 
> > It is potential direction since sqe group & provide buffer provides one
> > generic framework to export device internal buffer for further consuming
> > in zero copy & non-mmap way.
> 
> I see. This contributes a bit to the impression that much of the design
> is driven by ublk alone, because it's the only thing that seems to make
> use of group buffers so far.
> 
> Anyway, I'm just throwing in a few thoughts and ideas from outside. In
> the end, Jens and you need to agree on something.

Your thoughts and ideas are really helpful, thanks!


Thanks,
Ming

Re: [PATCH 5/9] io_uring: support SQE group

[Pavel Begunkov]

On 4/24/24 02:39, Ming Lei wrote:
> On Tue, Apr 23, 2024 at 03:08:55PM +0200, Kevin Wolf wrote:
>> Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
>>> On 4/7/24 7:03 PM, Ming Lei wrote:
>>>> SQE group is defined as one chain of SQEs starting with the first sqe that
>>>> has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
>>>> doesn't have it set, and it is similar with chain of linked sqes.
>>>>
>>>> The 1st SQE is group leader, and the other SQEs are group member. The group
>>>> leader is always freed after all members are completed. Group members
>>>> aren't submitted until the group leader is completed, and there isn't any
>>>> dependency among group members, and IOSQE_IO_LINK can't be set for group
>>>> members, same with IOSQE_IO_DRAIN.
>>>>
>>>> Typically the group leader provides or makes resource, and the other members
>>>> consume the resource, such as scenario of multiple backup, the 1st SQE is to
>>>> read data from source file into fixed buffer, the other SQEs write data from
>>>> the same buffer into other destination files. SQE group provides very
>>>> efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
>>>> submitted in single syscall, no need to submit fs read SQE first, and wait
>>>> until read SQE is completed, 2) no need to link all write SQEs together, then
>>>> write SQEs can be submitted to files concurrently. Meantime application is
>>>> simplified a lot in this way.
>>>>
>>>> Another use case is to for supporting generic device zero copy:
>>>>
>>>> - the lead SQE is for providing device buffer, which is owned by device or
>>>>    kernel, can't be cross userspace, otherwise easy to cause leak for devil
>>>>    application or panic
>>>>
>>>> - member SQEs reads or writes concurrently against the buffer provided by lead
>>>>    SQE
>>>
>>> In concept, this looks very similar to "sqe bundles" that I played with
>>> in the past:
>>>
>>> https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
>>>
>>> Didn't look too closely yet at the implementation, but in spirit it's
>>> about the same in that the first entry is processed first, and there's
>>> no ordering implied between the test of the members of the bundle /
>>> group.
>>
>> When I first read this patch, I wondered if it wouldn't make sense to
>> allow linking a group with subsequent requests, e.g. first having a few
>> requests that run in parallel and once all of them have completed
>> continue with the next linked one sequentially.
>>
>> For SQE bundles, you reused the LINK flag, which doesn't easily allow
>> this. Ming's patch uses a new flag for groups, so the interface would be
>> more obvious, you simply set the LINK flag on the last member of the
>> group (or on the leader, doesn't really matter). Of course, this doesn't
>> mean it has to be implemented now, but there is a clear way forward if
>> it's wanted.
> 
> Reusing LINK for bundle breaks existed link chains(BUNDLE linked to existed
> link chain), so I think it may not work.
> 
> The link rule is explicit for sqe group:
> 
> - only group leader can set link flag, which is applied on the whole
> group: the next sqe in the link chain won't be started until the
> previous linked sqe group is completed
> 
> - link flag can't be set for group members
> 
> Also sqe group doesn't limit async for both group leader and member.
> 
> sqe group vs link & async is covered in the last liburing test code.
> 
>>
>> The part that looks a bit arbitrary in Ming's patch is that the group
>> leader is always completed before the rest starts. It makes perfect
>> sense in the context that this series is really after (enabling zero
>> copy for ublk), but it doesn't really allow the case you mention in the
>> SQE bundle commit message, running everything in parallel and getting a
>> single CQE for the whole group.
> 
> I think it should be easy to cover bundle in this way, such as add one new
> op IORING_OP_BUNDLE as Jens did, and implement the single CQE for whole group/bundle.
> 
>>
>> I suppose you could hack around the sequential nature of the first
>> request by using an extra NOP as the group leader - which isn't any
>> worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
>> the group completion would still be missing. (Of course, removing the
>> sequential first operation would mean that ublk wouldn't have the buffer
>> ready any more when the other requests try to use it, so that would
>> defeat the purpose of the series...)
>>
>> I wonder if we can still combine both approaches and create some
>> generally useful infrastructure and not something where it's visible
>> that it was designed mostly for ublk's special case and other use cases
>> just happened to be enabled as a side effect.
> 
> sqe group is actually one generic interface, please see the multiple copy(
> copy one file to multiple destinations in single syscall for one range) example
> in the last patch, and it can support generic device zero copy: any device internal
> buffer can be linked with io_uring operations in this way, which can't
> be done by traditional splice/pipe.
> 
> I guess it can be used in network Rx zero copy too, but may depend on actual
> network Rx use case.

I doubt. With storage same data can be read twice. Socket recv consumes
data. Locking a buffer over the duration of another IO doesn't really sound
plausible, same we returning a buffer back. It'd be different if you can
read the buffer into the userspace if something goes wrong, but perhaps
you remember the fused discussion.

-- 
Pavel Begunkov

Re: [PATCH 5/9] io_uring: support SQE group

[Ming Lei]

On Mon, Apr 29, 2024 at 04:48:37PM +0100, Pavel Begunkov wrote:
> On 4/24/24 02:39, Ming Lei wrote:
> > On Tue, Apr 23, 2024 at 03:08:55PM +0200, Kevin Wolf wrote:
> > > Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
> > > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > > SQE group is defined as one chain of SQEs starting with the first sqe that
> > > > > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > > > > doesn't have it set, and it is similar with chain of linked sqes.
> > > > > 
> > > > > The 1st SQE is group leader, and the other SQEs are group member. The group
> > > > > leader is always freed after all members are completed. Group members
> > > > > aren't submitted until the group leader is completed, and there isn't any
> > > > > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > > > > members, same with IOSQE_IO_DRAIN.
> > > > > 
> > > > > Typically the group leader provides or makes resource, and the other members
> > > > > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > > > > read data from source file into fixed buffer, the other SQEs write data from
> > > > > the same buffer into other destination files. SQE group provides very
> > > > > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > > > > submitted in single syscall, no need to submit fs read SQE first, and wait
> > > > > until read SQE is completed, 2) no need to link all write SQEs together, then
> > > > > write SQEs can be submitted to files concurrently. Meantime application is
> > > > > simplified a lot in this way.
> > > > > 
> > > > > Another use case is to for supporting generic device zero copy:
> > > > > 
> > > > > - the lead SQE is for providing device buffer, which is owned by device or
> > > > >    kernel, can't be cross userspace, otherwise easy to cause leak for devil
> > > > >    application or panic
> > > > > 
> > > > > - member SQEs reads or writes concurrently against the buffer provided by lead
> > > > >    SQE
> > > > 
> > > > In concept, this looks very similar to "sqe bundles" that I played with
> > > > in the past:
> > > > 
> > > > https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
> > > > 
> > > > Didn't look too closely yet at the implementation, but in spirit it's
> > > > about the same in that the first entry is processed first, and there's
> > > > no ordering implied between the test of the members of the bundle /
> > > > group.
> > > 
> > > When I first read this patch, I wondered if it wouldn't make sense to
> > > allow linking a group with subsequent requests, e.g. first having a few
> > > requests that run in parallel and once all of them have completed
> > > continue with the next linked one sequentially.
> > > 
> > > For SQE bundles, you reused the LINK flag, which doesn't easily allow
> > > this. Ming's patch uses a new flag for groups, so the interface would be
> > > more obvious, you simply set the LINK flag on the last member of the
> > > group (or on the leader, doesn't really matter). Of course, this doesn't
> > > mean it has to be implemented now, but there is a clear way forward if
> > > it's wanted.
> > 
> > Reusing LINK for bundle breaks existed link chains(BUNDLE linked to existed
> > link chain), so I think it may not work.
> > 
> > The link rule is explicit for sqe group:
> > 
> > - only group leader can set link flag, which is applied on the whole
> > group: the next sqe in the link chain won't be started until the
> > previous linked sqe group is completed
> > 
> > - link flag can't be set for group members
> > 
> > Also sqe group doesn't limit async for both group leader and member.
> > 
> > sqe group vs link & async is covered in the last liburing test code.
> > 
> > > 
> > > The part that looks a bit arbitrary in Ming's patch is that the group
> > > leader is always completed before the rest starts. It makes perfect
> > > sense in the context that this series is really after (enabling zero
> > > copy for ublk), but it doesn't really allow the case you mention in the
> > > SQE bundle commit message, running everything in parallel and getting a
> > > single CQE for the whole group.
> > 
> > I think it should be easy to cover bundle in this way, such as add one new
> > op IORING_OP_BUNDLE as Jens did, and implement the single CQE for whole group/bundle.
> > 
> > > 
> > > I suppose you could hack around the sequential nature of the first
> > > request by using an extra NOP as the group leader - which isn't any
> > > worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
> > > the group completion would still be missing. (Of course, removing the
> > > sequential first operation would mean that ublk wouldn't have the buffer
> > > ready any more when the other requests try to use it, so that would
> > > defeat the purpose of the series...)
> > > 
> > > I wonder if we can still combine both approaches and create some
> > > generally useful infrastructure and not something where it's visible
> > > that it was designed mostly for ublk's special case and other use cases
> > > just happened to be enabled as a side effect.
> > 
> > sqe group is actually one generic interface, please see the multiple copy(
> > copy one file to multiple destinations in single syscall for one range) example
> > in the last patch, and it can support generic device zero copy: any device internal
> > buffer can be linked with io_uring operations in this way, which can't
> > be done by traditional splice/pipe.
> > 
> > I guess it can be used in network Rx zero copy too, but may depend on actual
> > network Rx use case.
> 
> I doubt. With storage same data can be read twice. Socket recv consumes

No, we don't depend on read twice of storage.

> data. Locking a buffer over the duration of another IO doesn't really sound
> plausible, same we returning a buffer back. It'd be different if you can
> read the buffer into the userspace if something goes wrong, but perhaps
> you remember the fused discussion.

As I mentioned, it depends on actual use case. If the received data can
be consumed by following io_uring OPs, such as be sent to network or
FS, this way works just fine, but if data needs to live longer or
further processing, the data can still be saved to userpsace by io_uring
OPs linked to the sqe group.


Thanks,
Ming

Re: [PATCH 5/9] io_uring: support SQE group

[Pavel Begunkov]

On 4/23/24 14:08, Kevin Wolf wrote:
> Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
>> On 4/7/24 7:03 PM, Ming Lei wrote:
>>> SQE group is defined as one chain of SQEs starting with the first sqe that
>>> has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
>>> doesn't have it set, and it is similar with chain of linked sqes.
>>>
>>> The 1st SQE is group leader, and the other SQEs are group member. The group
>>> leader is always freed after all members are completed. Group members
>>> aren't submitted until the group leader is completed, and there isn't any
>>> dependency among group members, and IOSQE_IO_LINK can't be set for group
>>> members, same with IOSQE_IO_DRAIN.
>>>
>>> Typically the group leader provides or makes resource, and the other members
>>> consume the resource, such as scenario of multiple backup, the 1st SQE is to
>>> read data from source file into fixed buffer, the other SQEs write data from
>>> the same buffer into other destination files. SQE group provides very
>>> efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
>>> submitted in single syscall, no need to submit fs read SQE first, and wait
>>> until read SQE is completed, 2) no need to link all write SQEs together, then
>>> write SQEs can be submitted to files concurrently. Meantime application is
>>> simplified a lot in this way.
>>>
>>> Another use case is to for supporting generic device zero copy:
>>>
>>> - the lead SQE is for providing device buffer, which is owned by device or
>>>    kernel, can't be cross userspace, otherwise easy to cause leak for devil
>>>    application or panic
>>>
>>> - member SQEs reads or writes concurrently against the buffer provided by lead
>>>    SQE
>>
>> In concept, this looks very similar to "sqe bundles" that I played with
>> in the past:
>>
>> https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
>>
>> Didn't look too closely yet at the implementation, but in spirit it's
>> about the same in that the first entry is processed first, and there's
>> no ordering implied between the test of the members of the bundle /
>> group.
> 
> When I first read this patch, I wondered if it wouldn't make sense to
> allow linking a group with subsequent requests, e.g. first having a few
> requests that run in parallel and once all of them have completed
> continue with the next linked one sequentially.
> 
> For SQE bundles, you reused the LINK flag, which doesn't easily allow
> this. Ming's patch uses a new flag for groups, so the interface would be
> more obvious, you simply set the LINK flag on the last member of the
> group (or on the leader, doesn't really matter). Of course, this doesn't
> mean it has to be implemented now, but there is a clear way forward if
> it's wanted.

Putting zc aside, links, graphs, groups, it all sounds interesting in
concept but let's not fool anyone, all the different ordering
relationships between requests proved to be a bad idea.

I can complaint for long, error handling is miserable, user handling
resubmitting a part of a link is horrible, the concept of errors is
hard coded (time to appreciate "beautifulness" of IOSQE_IO_HARDLINK
and the MSG_WAITALL workaround). The handling and workarounds are
leaking into generic paths, e.g. we can't init files when it's the most
convenient. For cancellation we're walking links, which need more care
than just looking at a request (is cancellation by user_data of a
"linked" to a group request even supported?). The list goes on

And what does it achieve? The infra has matured since early days,
it saves user-kernel transitions at best but not context switching
overhead, and not even that if you do wait(1) and happen to catch
middle CQEs. And it disables LAZY_WAKE, so CQ side batching with
timers and what not is effectively useless with links.

So, please, please! instead of trying to invent a new uber scheme
of request linking, which surely wouldn't step on same problems
over and over again, and would definitely be destined to overshadow
all previous attempts and finally conquer the world, let's rather
focus on minimasing the damage from this patchset's zero copy if
it's going to be taken.

Piggy backing bits on top of links should be just fine. May help
to save space in io_kiocb by unionising with links. And half
of the overhead (including completely destroying all inlining
in the submission patch) can be mitigated by folding it into
REQ_F_LINK handling and generally borowing the code structure
for it in the submission path.


> The part that looks a bit arbitrary in Ming's patch is that the group
> leader is always completed before the rest starts. It makes perfect
> sense in the context that this series is really after (enabling zero
> copy for ublk), but it doesn't really allow the case you mention in the
> SQE bundle commit message, running everything in parallel and getting a
> single CQE for the whole group.
> 
> I suppose you could hack around the sequential nature of the first
> request by using an extra NOP as the group leader - which isn't any
> worse than having an IORING_OP_BUNDLE really, just looks a bit odd - but
> the group completion would still be missing. (Of course, removing the
> sequential first operation would mean that ublk wouldn't have the buffer
> ready any more when the other requests try to use it, so that would
> defeat the purpose of the series...)
> 
> I wonder if we can still combine both approaches and create some
> generally useful infrastructure and not something where it's visible
> that it was designed mostly for ublk's special case and other use cases
> just happened to be enabled as a side effect.


-- 
Pavel Begunkov

Re: [PATCH 5/9] io_uring: support SQE group

[Ming Lei]

On Mon, Apr 29, 2024 at 04:32:35PM +0100, Pavel Begunkov wrote:
> On 4/23/24 14:08, Kevin Wolf wrote:
> > Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
> > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > SQE group is defined as one chain of SQEs starting with the first sqe that
> > > > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > > > doesn't have it set, and it is similar with chain of linked sqes.
> > > > 
> > > > The 1st SQE is group leader, and the other SQEs are group member. The group
> > > > leader is always freed after all members are completed. Group members
> > > > aren't submitted until the group leader is completed, and there isn't any
> > > > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > > > members, same with IOSQE_IO_DRAIN.
> > > > 
> > > > Typically the group leader provides or makes resource, and the other members
> > > > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > > > read data from source file into fixed buffer, the other SQEs write data from
> > > > the same buffer into other destination files. SQE group provides very
> > > > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > > > submitted in single syscall, no need to submit fs read SQE first, and wait
> > > > until read SQE is completed, 2) no need to link all write SQEs together, then
> > > > write SQEs can be submitted to files concurrently. Meantime application is
> > > > simplified a lot in this way.
> > > > 
> > > > Another use case is to for supporting generic device zero copy:
> > > > 
> > > > - the lead SQE is for providing device buffer, which is owned by device or
> > > >    kernel, can't be cross userspace, otherwise easy to cause leak for devil
> > > >    application or panic
> > > > 
> > > > - member SQEs reads or writes concurrently against the buffer provided by lead
> > > >    SQE
> > > 
> > > In concept, this looks very similar to "sqe bundles" that I played with
> > > in the past:
> > > 
> > > https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
> > > 
> > > Didn't look too closely yet at the implementation, but in spirit it's
> > > about the same in that the first entry is processed first, and there's
> > > no ordering implied between the test of the members of the bundle /
> > > group.
> > 
> > When I first read this patch, I wondered if it wouldn't make sense to
> > allow linking a group with subsequent requests, e.g. first having a few
> > requests that run in parallel and once all of them have completed
> > continue with the next linked one sequentially.
> > 
> > For SQE bundles, you reused the LINK flag, which doesn't easily allow
> > this. Ming's patch uses a new flag for groups, so the interface would be
> > more obvious, you simply set the LINK flag on the last member of the
> > group (or on the leader, doesn't really matter). Of course, this doesn't
> > mean it has to be implemented now, but there is a clear way forward if
> > it's wanted.
> 
> Putting zc aside, links, graphs, groups, it all sounds interesting in
> concept but let's not fool anyone, all the different ordering
> relationships between requests proved to be a bad idea.

As Jens mentioned, sqe group is very similar with bundle:

https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=io_uring-bundle

which is really something io_uring is missing.

> 
> I can complaint for long, error handling is miserable, user handling
> resubmitting a part of a link is horrible, the concept of errors is
> hard coded (time to appreciate "beautifulness" of IOSQE_IO_HARDLINK
> and the MSG_WAITALL workaround). The handling and workarounds are
> leaking into generic paths, e.g. we can't init files when it's the most
> convenient. For cancellation we're walking links, which need more care
> than just looking at a request (is cancellation by user_data of a
> "linked" to a group request even supported?). The list goes on

Only the group leader is linked, if the group leader is canceled, all
requests in the whole group will be canceled.

But yes, cancelling by user_data for group members can't be supported,
and it can be documented clearly, since user still can cancel the whole
group with group leader's user_data.

> 
> And what does it achieve? The infra has matured since early days,
> it saves user-kernel transitions at best but not context switching
> overhead, and not even that if you do wait(1) and happen to catch
> middle CQEs. And it disables LAZY_WAKE, so CQ side batching with
> timers and what not is effectively useless with links.

Not only the context switch, it supports 1:N or N:M dependency which
is missing in io_uring, but also makes async application easier to write by
saving extra context switches, which just adds extra intermediate states for
application.

> 
> So, please, please! instead of trying to invent a new uber scheme
> of request linking, which surely wouldn't step on same problems
> over and over again, and would definitely be destined to overshadow
> all previous attempts and finally conquer the world, let's rather
> focus on minimasing the damage from this patchset's zero copy if
> it's going to be taken.

One key problem for zero copy is lifetime of the kernel buffer, which
can't cross OPs, that is why sqe group is introduced, for aligning
kernel buffer lifetime with the group.


Thanks, 
Ming

Re: [PATCH 5/9] io_uring: support SQE group

[Pavel Begunkov]

On 4/30/24 04:03, Ming Lei wrote:
> On Mon, Apr 29, 2024 at 04:32:35PM +0100, Pavel Begunkov wrote:
>> On 4/23/24 14:08, Kevin Wolf wrote:
>>> Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
>>>> On 4/7/24 7:03 PM, Ming Lei wrote:
>>>>> SQE group is defined as one chain of SQEs starting with the first sqe that
>>>>> has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
>>>>> doesn't have it set, and it is similar with chain of linked sqes.
>>>>>
>>>>> The 1st SQE is group leader, and the other SQEs are group member. The group
>>>>> leader is always freed after all members are completed. Group members
>>>>> aren't submitted until the group leader is completed, and there isn't any
>>>>> dependency among group members, and IOSQE_IO_LINK can't be set for group
>>>>> members, same with IOSQE_IO_DRAIN.
>>>>>
>>>>> Typically the group leader provides or makes resource, and the other members
>>>>> consume the resource, such as scenario of multiple backup, the 1st SQE is to
>>>>> read data from source file into fixed buffer, the other SQEs write data from
>>>>> the same buffer into other destination files. SQE group provides very
>>>>> efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
>>>>> submitted in single syscall, no need to submit fs read SQE first, and wait
>>>>> until read SQE is completed, 2) no need to link all write SQEs together, then
>>>>> write SQEs can be submitted to files concurrently. Meantime application is
>>>>> simplified a lot in this way.
>>>>>
>>>>> Another use case is to for supporting generic device zero copy:
>>>>>
>>>>> - the lead SQE is for providing device buffer, which is owned by device or
>>>>>     kernel, can't be cross userspace, otherwise easy to cause leak for devil
>>>>>     application or panic
>>>>>
>>>>> - member SQEs reads or writes concurrently against the buffer provided by lead
>>>>>     SQE
>>>>
>>>> In concept, this looks very similar to "sqe bundles" that I played with
>>>> in the past:
>>>>
>>>> https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
>>>>
>>>> Didn't look too closely yet at the implementation, but in spirit it's
>>>> about the same in that the first entry is processed first, and there's
>>>> no ordering implied between the test of the members of the bundle /
>>>> group.
>>>
>>> When I first read this patch, I wondered if it wouldn't make sense to
>>> allow linking a group with subsequent requests, e.g. first having a few
>>> requests that run in parallel and once all of them have completed
>>> continue with the next linked one sequentially.
>>>
>>> For SQE bundles, you reused the LINK flag, which doesn't easily allow
>>> this. Ming's patch uses a new flag for groups, so the interface would be
>>> more obvious, you simply set the LINK flag on the last member of the
>>> group (or on the leader, doesn't really matter). Of course, this doesn't
>>> mean it has to be implemented now, but there is a clear way forward if
>>> it's wanted.
>>
>> Putting zc aside, links, graphs, groups, it all sounds interesting in
>> concept but let's not fool anyone, all the different ordering
>> relationships between requests proved to be a bad idea.
> 
> As Jens mentioned, sqe group is very similar with bundle:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=io_uring-bundle
> 
> which is really something io_uring is missing.

One could've said same about links, retrospectively I argue that it
was a mistake, so I pretty much doubt arguments like "io_uring is
missing it". Another thing is that zero copy, which is not possible
to implement by returning to the userspace.

>> I can complaint for long, error handling is miserable, user handling
>> resubmitting a part of a link is horrible, the concept of errors is
>> hard coded (time to appreciate "beautifulness" of IOSQE_IO_HARDLINK
>> and the MSG_WAITALL workaround). The handling and workarounds are
>> leaking into generic paths, e.g. we can't init files when it's the most
>> convenient. For cancellation we're walking links, which need more care
>> than just looking at a request (is cancellation by user_data of a
>> "linked" to a group request even supported?). The list goes on
> 
> Only the group leader is linked, if the group leader is canceled, all
> requests in the whole group will be canceled.
> 
> But yes, cancelling by user_data for group members can't be supported,
> and it can be documented clearly, since user still can cancel the whole
> group with group leader's user_data.

Which means it'd break the case REQ_F_INFLIGHT covers, and you need
to disallow linking REQ_F_INFLIGHT marked requests.

>> And what does it achieve? The infra has matured since early days,
>> it saves user-kernel transitions at best but not context switching
>> overhead, and not even that if you do wait(1) and happen to catch
>> middle CQEs. And it disables LAZY_WAKE, so CQ side batching with
>> timers and what not is effectively useless with links.
> 
> Not only the context switch, it supports 1:N or N:M dependency which

I completely missed, how N:M is supported? That starting to sound
terrifying.

> is missing in io_uring, but also makes async application easier to write by
> saving extra context switches, which just adds extra intermediate states for
> application.

You're still executing requests (i.e. ->issue) primarily from the
submitter task context, they would still fly back to the task and
wake it up. You may save something by completing all of them
together via that refcounting, but you might just as well try to
batch CQ, which is a more generic issue. It's not clear what
context switches you save then.

As for simplicity, using the link example and considering error
handling, it only complicates it. In case of an error you need to
figure out a middle req failed, collect all failed CQEs linked to
it and automatically cancelled (unless SKIP_COMPLETE is used), and
then resubmit the failed. That's great your reads are idempotent
and presumably you don't have to resubmit half a link, but in the
grand picture of things it's rather one of use cases where a generic
feature can be used.

>> So, please, please! instead of trying to invent a new uber scheme
>> of request linking, which surely wouldn't step on same problems
>> over and over again, and would definitely be destined to overshadow
>> all previous attempts and finally conquer the world, let's rather
>> focus on minimasing the damage from this patchset's zero copy if
>> it's going to be taken.
> 
> One key problem for zero copy is lifetime of the kernel buffer, which
> can't cross OPs, that is why sqe group is introduced, for aligning
> kernel buffer lifetime with the group.

Right, which is why I'm saying if we're leaving groups with zero
copy, let's rather try to make them simple and not intrusive as
much as possible, instead of creating an unsupportable overarching
beast out of it, which would fail as a generic feature.

-- 
Pavel Begunkov

Re: [PATCH 5/9] io_uring: support SQE group

[Ming Lei]

On Tue, Apr 30, 2024 at 01:27:10PM +0100, Pavel Begunkov wrote:
> On 4/30/24 04:03, Ming Lei wrote:
> > On Mon, Apr 29, 2024 at 04:32:35PM +0100, Pavel Begunkov wrote:
> > > On 4/23/24 14:08, Kevin Wolf wrote:
> > > > Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
> > > > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > > > SQE group is defined as one chain of SQEs starting with the first sqe that
> > > > > > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > > > > > doesn't have it set, and it is similar with chain of linked sqes.
> > > > > > 
> > > > > > The 1st SQE is group leader, and the other SQEs are group member. The group
> > > > > > leader is always freed after all members are completed. Group members
> > > > > > aren't submitted until the group leader is completed, and there isn't any
> > > > > > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > > > > > members, same with IOSQE_IO_DRAIN.
> > > > > > 
> > > > > > Typically the group leader provides or makes resource, and the other members
> > > > > > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > > > > > read data from source file into fixed buffer, the other SQEs write data from
> > > > > > the same buffer into other destination files. SQE group provides very
> > > > > > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > > > > > submitted in single syscall, no need to submit fs read SQE first, and wait
> > > > > > until read SQE is completed, 2) no need to link all write SQEs together, then
> > > > > > write SQEs can be submitted to files concurrently. Meantime application is
> > > > > > simplified a lot in this way.
> > > > > > 
> > > > > > Another use case is to for supporting generic device zero copy:
> > > > > > 
> > > > > > - the lead SQE is for providing device buffer, which is owned by device or
> > > > > >     kernel, can't be cross userspace, otherwise easy to cause leak for devil
> > > > > >     application or panic
> > > > > > 
> > > > > > - member SQEs reads or writes concurrently against the buffer provided by lead
> > > > > >     SQE
> > > > > 
> > > > > In concept, this looks very similar to "sqe bundles" that I played with
> > > > > in the past:
> > > > > 
> > > > > https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
> > > > > 
> > > > > Didn't look too closely yet at the implementation, but in spirit it's
> > > > > about the same in that the first entry is processed first, and there's
> > > > > no ordering implied between the test of the members of the bundle /
> > > > > group.
> > > > 
> > > > When I first read this patch, I wondered if it wouldn't make sense to
> > > > allow linking a group with subsequent requests, e.g. first having a few
> > > > requests that run in parallel and once all of them have completed
> > > > continue with the next linked one sequentially.
> > > > 
> > > > For SQE bundles, you reused the LINK flag, which doesn't easily allow
> > > > this. Ming's patch uses a new flag for groups, so the interface would be
> > > > more obvious, you simply set the LINK flag on the last member of the
> > > > group (or on the leader, doesn't really matter). Of course, this doesn't
> > > > mean it has to be implemented now, but there is a clear way forward if
> > > > it's wanted.
> > > 
> > > Putting zc aside, links, graphs, groups, it all sounds interesting in
> > > concept but let's not fool anyone, all the different ordering
> > > relationships between requests proved to be a bad idea.
> > 
> > As Jens mentioned, sqe group is very similar with bundle:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=io_uring-bundle
> > 
> > which is really something io_uring is missing.
> 
> One could've said same about links, retrospectively I argue that it
> was a mistake, so I pretty much doubt arguments like "io_uring is
> missing it". Another thing is that zero copy, which is not possible
> to implement by returning to the userspace.
> 
> > > I can complaint for long, error handling is miserable, user handling
> > > resubmitting a part of a link is horrible, the concept of errors is
> > > hard coded (time to appreciate "beautifulness" of IOSQE_IO_HARDLINK
> > > and the MSG_WAITALL workaround). The handling and workarounds are
> > > leaking into generic paths, e.g. we can't init files when it's the most
> > > convenient. For cancellation we're walking links, which need more care
> > > than just looking at a request (is cancellation by user_data of a
> > > "linked" to a group request even supported?). The list goes on
> > 
> > Only the group leader is linked, if the group leader is canceled, all
> > requests in the whole group will be canceled.
> > 
> > But yes, cancelling by user_data for group members can't be supported,
> > and it can be documented clearly, since user still can cancel the whole
> > group with group leader's user_data.
> 
> Which means it'd break the case REQ_F_INFLIGHT covers, and you need
> to disallow linking REQ_F_INFLIGHT marked requests.

Both io_match_linked() and io_match_task() only iterates over req's
link chain, and only the group leader can appear in this link chain,
which is exactly the usual handling.

So care to explain it a bit what the real link issue is about sqe group?

> 
> > > And what does it achieve? The infra has matured since early days,
> > > it saves user-kernel transitions at best but not context switching
> > > overhead, and not even that if you do wait(1) and happen to catch
> > > middle CQEs. And it disables LAZY_WAKE, so CQ side batching with
> > > timers and what not is effectively useless with links.
> > 
> > Not only the context switch, it supports 1:N or N:M dependency which
> 
> I completely missed, how N:M is supported? That starting to sound
> terrifying.

N:M is actually from Kevin's idea.

sqe group can be made to be more flexible by:

    Inside the group, all SQEs are submitted in parallel, so there isn't any
    dependency among SQEs in one group.
    
    The 1st SQE is group leader, and the other SQEs are group member. The whole
    group share single IOSQE_IO_LINK and IOSQE_IO_DRAIN from group leader, and
    the two flags can't be set for group members.
    
    When the group is in one link chain, this group isn't submitted until
    the previous SQE or group is completed. And the following SQE or group
    can't be started if this group isn't completed.
    
    When IOSQE_IO_DRAIN is set for group leader, all requests in this group
    and previous requests submitted are drained. Given IOSQE_IO_DRAIN can
    be set for group leader only, we respect IO_DRAIN for SQE group by
    always completing group leader as the last on in the group.
    
    SQE group provides flexible way to support N:M dependency, such as:
    
    - group A is chained with group B together by IOSQE_IO_LINK
    - group A has N SQEs
    - group B has M SQEs
    
    then M SQEs in group B depend on N SQEs in group A.


> 
> > is missing in io_uring, but also makes async application easier to write by
> > saving extra context switches, which just adds extra intermediate states for
> > application.
> 
> You're still executing requests (i.e. ->issue) primarily from the
> submitter task context, they would still fly back to the task and
> wake it up. You may save something by completing all of them
> together via that refcounting, but you might just as well try to
> batch CQ, which is a more generic issue. It's not clear what
> context switches you save then.

Wrt. the above N:M example, one io_uring_enter() is enough, and
it can't be done in single context switch without sqe group, please
see the liburing test code:

https://lore.kernel.org/io-uring/ZiHA+pN28hRdprhX@fedora/T/#ma755c500eab0b7dc8c1473448dd98f093097e066

> 
> As for simplicity, using the link example and considering error
> handling, it only complicates it. In case of an error you need to
> figure out a middle req failed, collect all failed CQEs linked to
> it and automatically cancelled (unless SKIP_COMPLETE is used), and
> then resubmit the failed. That's great your reads are idempotent
> and presumably you don't have to resubmit half a link, but in the
> grand picture of things it's rather one of use cases where a generic
> feature can be used.

SQE group doesn't change the current link implementation, and N:M
dependency is built over IOSQE_IO_LINK actually.

> 
> > > So, please, please! instead of trying to invent a new uber scheme
> > > of request linking, which surely wouldn't step on same problems
> > > over and over again, and would definitely be destined to overshadow
> > > all previous attempts and finally conquer the world, let's rather
> > > focus on minimasing the damage from this patchset's zero copy if
> > > it's going to be taken.
> > 
> > One key problem for zero copy is lifetime of the kernel buffer, which
> > can't cross OPs, that is why sqe group is introduced, for aligning
> > kernel buffer lifetime with the group.
> 
> Right, which is why I'm saying if we're leaving groups with zero
> copy, let's rather try to make them simple and not intrusive as
> much as possible, instead of creating an unsupportable overarching
> beast out of it, which would fail as a generic feature.

Then it degraded to the original fused command, :-)

Thanks,
Ming

Re: [PATCH 5/9] io_uring: support SQE group

[Pavel Begunkov]

On 4/30/24 16:00, Ming Lei wrote:
> On Tue, Apr 30, 2024 at 01:27:10PM +0100, Pavel Begunkov wrote:
>> On 4/30/24 04:03, Ming Lei wrote:
>>> On Mon, Apr 29, 2024 at 04:32:35PM +0100, Pavel Begunkov wrote:
>>>> On 4/23/24 14:08, Kevin Wolf wrote:
>>>>> Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
>>>>>> On 4/7/24 7:03 PM, Ming Lei wrote:
>>>>>>> SQE group is defined as one chain of SQEs starting with the first sqe that
>>>>>>> has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
>>>>>>> doesn't have it set, and it is similar with chain of linked sqes.
>>>>>>>
>>>>>>> The 1st SQE is group leader, and the other SQEs are group member. The group
>>>>>>> leader is always freed after all members are completed. Group members
>>>>>>> aren't submitted until the group leader is completed, and there isn't any
>>>>>>> dependency among group members, and IOSQE_IO_LINK can't be set for group
>>>>>>> members, same with IOSQE_IO_DRAIN.
>>>>>>>
>>>>>>> Typically the group leader provides or makes resource, and the other members
>>>>>>> consume the resource, such as scenario of multiple backup, the 1st SQE is to
>>>>>>> read data from source file into fixed buffer, the other SQEs write data from
>>>>>>> the same buffer into other destination files. SQE group provides very
>>>>>>> efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
>>>>>>> submitted in single syscall, no need to submit fs read SQE first, and wait
>>>>>>> until read SQE is completed, 2) no need to link all write SQEs together, then
>>>>>>> write SQEs can be submitted to files concurrently. Meantime application is
>>>>>>> simplified a lot in this way.
>>>>>>>
>>>>>>> Another use case is to for supporting generic device zero copy:
>>>>>>>
>>>>>>> - the lead SQE is for providing device buffer, which is owned by device or
>>>>>>>      kernel, can't be cross userspace, otherwise easy to cause leak for devil
>>>>>>>      application or panic
>>>>>>>
>>>>>>> - member SQEs reads or writes concurrently against the buffer provided by lead
>>>>>>>      SQE
>>>>>>
>>>>>> In concept, this looks very similar to "sqe bundles" that I played with
>>>>>> in the past:
>>>>>>
>>>>>> https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
>>>>>>
>>>>>> Didn't look too closely yet at the implementation, but in spirit it's
>>>>>> about the same in that the first entry is processed first, and there's
>>>>>> no ordering implied between the test of the members of the bundle /
>>>>>> group.
>>>>>
>>>>> When I first read this patch, I wondered if it wouldn't make sense to
>>>>> allow linking a group with subsequent requests, e.g. first having a few
>>>>> requests that run in parallel and once all of them have completed
>>>>> continue with the next linked one sequentially.
>>>>>
>>>>> For SQE bundles, you reused the LINK flag, which doesn't easily allow
>>>>> this. Ming's patch uses a new flag for groups, so the interface would be
>>>>> more obvious, you simply set the LINK flag on the last member of the
>>>>> group (or on the leader, doesn't really matter). Of course, this doesn't
>>>>> mean it has to be implemented now, but there is a clear way forward if
>>>>> it's wanted.
>>>>
>>>> Putting zc aside, links, graphs, groups, it all sounds interesting in
>>>> concept but let's not fool anyone, all the different ordering
>>>> relationships between requests proved to be a bad idea.
>>>
>>> As Jens mentioned, sqe group is very similar with bundle:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=io_uring-bundle
>>>
>>> which is really something io_uring is missing.
>>
>> One could've said same about links, retrospectively I argue that it
>> was a mistake, so I pretty much doubt arguments like "io_uring is
>> missing it". Another thing is that zero copy, which is not possible
>> to implement by returning to the userspace.
>>
>>>> I can complaint for long, error handling is miserable, user handling
>>>> resubmitting a part of a link is horrible, the concept of errors is
>>>> hard coded (time to appreciate "beautifulness" of IOSQE_IO_HARDLINK
>>>> and the MSG_WAITALL workaround). The handling and workarounds are
>>>> leaking into generic paths, e.g. we can't init files when it's the most
>>>> convenient. For cancellation we're walking links, which need more care
>>>> than just looking at a request (is cancellation by user_data of a
>>>> "linked" to a group request even supported?). The list goes on
>>>
>>> Only the group leader is linked, if the group leader is canceled, all
>>> requests in the whole group will be canceled.
>>>
>>> But yes, cancelling by user_data for group members can't be supported,
>>> and it can be documented clearly, since user still can cancel the whole
>>> group with group leader's user_data.
>>
>> Which means it'd break the case REQ_F_INFLIGHT covers, and you need
>> to disallow linking REQ_F_INFLIGHT marked requests.
> 
> Both io_match_linked() and io_match_task() only iterates over req's
> link chain, and only the group leader can appear in this link chain,
> which is exactly the usual handling.
> 
> So care to explain it a bit what the real link issue is about sqe group?

Because of ref deps when a task exits it has to cancel REQ_F_INFLIGHT
requests, and therefore they should be discoverable. The flag is only
for POLL_ADD requests polling io_uring fds, should be good enough if
you disallow such requests from grouping.

>>>> And what does it achieve? The infra has matured since early days,
>>>> it saves user-kernel transitions at best but not context switching
>>>> overhead, and not even that if you do wait(1) and happen to catch
>>>> middle CQEs. And it disables LAZY_WAKE, so CQ side batching with
>>>> timers and what not is effectively useless with links.
>>>
>>> Not only the context switch, it supports 1:N or N:M dependency which
>>
>> I completely missed, how N:M is supported? That starting to sound
>> terrifying.
> 
> N:M is actually from Kevin's idea.
> 
> sqe group can be made to be more flexible by:
> 
>      Inside the group, all SQEs are submitted in parallel, so there isn't any
>      dependency among SQEs in one group.
>      
>      The 1st SQE is group leader, and the other SQEs are group member. The whole
>      group share single IOSQE_IO_LINK and IOSQE_IO_DRAIN from group leader, and
>      the two flags can't be set for group members.
>      
>      When the group is in one link chain, this group isn't submitted until
>      the previous SQE or group is completed. And the following SQE or group
>      can't be started if this group isn't completed.
>      
>      When IOSQE_IO_DRAIN is set for group leader, all requests in this group
>      and previous requests submitted are drained. Given IOSQE_IO_DRAIN can
>      be set for group leader only, we respect IO_DRAIN for SQE group by
>      always completing group leader as the last on in the group.
>      
>      SQE group provides flexible way to support N:M dependency, such as:
>      
>      - group A is chained with group B together by IOSQE_IO_LINK
>      - group A has N SQEs
>      - group B has M SQEs
>      
>      then M SQEs in group B depend on N SQEs in group A.

In other words, linking groups together with basically no extra rules.
Fwiw, sounds generic, but if there are complications with IOSQE_IO_DRAIN
that I don't immediately see, it'd be more reasonable to just disable it.

>>> is missing in io_uring, but also makes async application easier to write by
>>> saving extra context switches, which just adds extra intermediate states for
>>> application.
>>
>> You're still executing requests (i.e. ->issue) primarily from the
>> submitter task context, they would still fly back to the task and
>> wake it up. You may save something by completing all of them
>> together via that refcounting, but you might just as well try to
>> batch CQ, which is a more generic issue. It's not clear what
>> context switches you save then.
> 
> Wrt. the above N:M example, one io_uring_enter() is enough, and
> it can't be done in single context switch without sqe group, please
> see the liburing test code:
> 
> https://lore.kernel.org/io-uring/ZiHA+pN28hRdprhX@fedora/T/#ma755c500eab0b7dc8c1473448dd98f093097e066
> 
>>
>> As for simplicity, using the link example and considering error
>> handling, it only complicates it. In case of an error you need to
>> figure out a middle req failed, collect all failed CQEs linked to
>> it and automatically cancelled (unless SKIP_COMPLETE is used), and
>> then resubmit the failed. That's great your reads are idempotent
>> and presumably you don't have to resubmit half a link, but in the
>> grand picture of things it's rather one of use cases where a generic
>> feature can be used.
> 
> SQE group doesn't change the current link implementation, and N:M
> dependency is built over IOSQE_IO_LINK actually.
> 
>>
>>>> So, please, please! instead of trying to invent a new uber scheme
>>>> of request linking, which surely wouldn't step on same problems
>>>> over and over again, and would definitely be destined to overshadow
>>>> all previous attempts and finally conquer the world, let's rather
>>>> focus on minimasing the damage from this patchset's zero copy if
>>>> it's going to be taken.
>>>
>>> One key problem for zero copy is lifetime of the kernel buffer, which
>>> can't cross OPs, that is why sqe group is introduced, for aligning
>>> kernel buffer lifetime with the group.
>>
>> Right, which is why I'm saying if we're leaving groups with zero
>> copy, let's rather try to make them simple and not intrusive as
>> much as possible, instead of creating an unsupportable overarching
>> beast out of it, which would fail as a generic feature.
> 
> Then it degraded to the original fused command, :-)

I'm not arguing about restricting it to 1 request in a group apart
from the master/leader/etc., if that's what you mean. The argument
is rather to limit the overhead and abstraction leakage into hot
paths.

For example that N:M, all that sounds great on paper until it sees
the harsh reality. And instead of linking groups together, you
can perfectly fine be sending one group at a time and queuing the
next group from the userspace when the previous completes. And
that would save space in io_kiocb, maybe(?) a flag, maybe something
else.

Regardless of interoperability with links, I'd also prefer it to
be folded into the link code structure and state machine, so
it's not all over in the submission/completion paths adding
overhead for one narrow feature, including un-inlining the
entire submission path.

E.g. there should be no separate hand coded duplicated SQE
/ request assembling like in io_init_req_group(). Instead
it should be able to consume SQEs and requests it's given
from submit_sqes(), and e.g. process grouping in
io_submit_sqe() around the place requests are linked.

-- 
Pavel Begunkov

Re: [PATCH 5/9] io_uring: support SQE group

[Ming Lei]

On Thu, May 02, 2024 at 03:09:13PM +0100, Pavel Begunkov wrote:
> On 4/30/24 16:00, Ming Lei wrote:
> > On Tue, Apr 30, 2024 at 01:27:10PM +0100, Pavel Begunkov wrote:
> > > On 4/30/24 04:03, Ming Lei wrote:
> > > > On Mon, Apr 29, 2024 at 04:32:35PM +0100, Pavel Begunkov wrote:
> > > > > On 4/23/24 14:08, Kevin Wolf wrote:
> > > > > > Am 22.04.2024 um 20:27 hat Jens Axboe geschrieben:
> > > > > > > On 4/7/24 7:03 PM, Ming Lei wrote:
> > > > > > > > SQE group is defined as one chain of SQEs starting with the first sqe that
> > > > > > > > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > > > > > > > doesn't have it set, and it is similar with chain of linked sqes.
> > > > > > > > 
> > > > > > > > The 1st SQE is group leader, and the other SQEs are group member. The group
> > > > > > > > leader is always freed after all members are completed. Group members
> > > > > > > > aren't submitted until the group leader is completed, and there isn't any
> > > > > > > > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > > > > > > > members, same with IOSQE_IO_DRAIN.
> > > > > > > > 
> > > > > > > > Typically the group leader provides or makes resource, and the other members
> > > > > > > > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > > > > > > > read data from source file into fixed buffer, the other SQEs write data from
> > > > > > > > the same buffer into other destination files. SQE group provides very
> > > > > > > > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > > > > > > > submitted in single syscall, no need to submit fs read SQE first, and wait
> > > > > > > > until read SQE is completed, 2) no need to link all write SQEs together, then
> > > > > > > > write SQEs can be submitted to files concurrently. Meantime application is
> > > > > > > > simplified a lot in this way.
> > > > > > > > 
> > > > > > > > Another use case is to for supporting generic device zero copy:
> > > > > > > > 
> > > > > > > > - the lead SQE is for providing device buffer, which is owned by device or
> > > > > > > >      kernel, can't be cross userspace, otherwise easy to cause leak for devil
> > > > > > > >      application or panic
> > > > > > > > 
> > > > > > > > - member SQEs reads or writes concurrently against the buffer provided by lead
> > > > > > > >      SQE
> > > > > > > 
> > > > > > > In concept, this looks very similar to "sqe bundles" that I played with
> > > > > > > in the past:
> > > > > > > 
> > > > > > > https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle
> > > > > > > 
> > > > > > > Didn't look too closely yet at the implementation, but in spirit it's
> > > > > > > about the same in that the first entry is processed first, and there's
> > > > > > > no ordering implied between the test of the members of the bundle /
> > > > > > > group.
> > > > > > 
> > > > > > When I first read this patch, I wondered if it wouldn't make sense to
> > > > > > allow linking a group with subsequent requests, e.g. first having a few
> > > > > > requests that run in parallel and once all of them have completed
> > > > > > continue with the next linked one sequentially.
> > > > > > 
> > > > > > For SQE bundles, you reused the LINK flag, which doesn't easily allow
> > > > > > this. Ming's patch uses a new flag for groups, so the interface would be
> > > > > > more obvious, you simply set the LINK flag on the last member of the
> > > > > > group (or on the leader, doesn't really matter). Of course, this doesn't
> > > > > > mean it has to be implemented now, but there is a clear way forward if
> > > > > > it's wanted.
> > > > > 
> > > > > Putting zc aside, links, graphs, groups, it all sounds interesting in
> > > > > concept but let's not fool anyone, all the different ordering
> > > > > relationships between requests proved to be a bad idea.
> > > > 
> > > > As Jens mentioned, sqe group is very similar with bundle:
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=io_uring-bundle
> > > > 
> > > > which is really something io_uring is missing.
> > > 
> > > One could've said same about links, retrospectively I argue that it
> > > was a mistake, so I pretty much doubt arguments like "io_uring is
> > > missing it". Another thing is that zero copy, which is not possible
> > > to implement by returning to the userspace.
> > > 
> > > > > I can complaint for long, error handling is miserable, user handling
> > > > > resubmitting a part of a link is horrible, the concept of errors is
> > > > > hard coded (time to appreciate "beautifulness" of IOSQE_IO_HARDLINK
> > > > > and the MSG_WAITALL workaround). The handling and workarounds are
> > > > > leaking into generic paths, e.g. we can't init files when it's the most
> > > > > convenient. For cancellation we're walking links, which need more care
> > > > > than just looking at a request (is cancellation by user_data of a
> > > > > "linked" to a group request even supported?). The list goes on
> > > > 
> > > > Only the group leader is linked, if the group leader is canceled, all
> > > > requests in the whole group will be canceled.
> > > > 
> > > > But yes, cancelling by user_data for group members can't be supported,
> > > > and it can be documented clearly, since user still can cancel the whole
> > > > group with group leader's user_data.
> > > 
> > > Which means it'd break the case REQ_F_INFLIGHT covers, and you need
> > > to disallow linking REQ_F_INFLIGHT marked requests.
> > 
> > Both io_match_linked() and io_match_task() only iterates over req's
> > link chain, and only the group leader can appear in this link chain,
> > which is exactly the usual handling.
> > 
> > So care to explain it a bit what the real link issue is about sqe group?
> 
> Because of ref deps when a task exits it has to cancel REQ_F_INFLIGHT
> requests, and therefore they should be discoverable. The flag is only
> for POLL_ADD requests polling io_uring fds, should be good enough if
> you disallow such requests from grouping.

The group leader is always marked as REQ_F_INFLIGHT and discoverable,
and the cancel code path cancels group leader request which will
guarantees that all group members are canceled, so I think this change
isn't needed.

But io_get_sequence() needs to take it into account, such as:

@@ -1680,8 +1846,12 @@ static u32 io_get_sequence(struct io_kiocb *req)
        struct io_kiocb *cur;

        /* need original cached_sq_head, but it was increased for each req */
-       io_for_each_link(cur, req)
-               seq--;
+       io_for_each_link(cur, req) {
+               if (req_is_group_lead(cur))
+                       seq -= atomic_read(&cur->grp_refs);
+               else
+                       seq--;
+       }

> 
> > > > > And what does it achieve? The infra has matured since early days,
> > > > > it saves user-kernel transitions at best but not context switching
> > > > > overhead, and not even that if you do wait(1) and happen to catch
> > > > > middle CQEs. And it disables LAZY_WAKE, so CQ side batching with
> > > > > timers and what not is effectively useless with links.
> > > > 
> > > > Not only the context switch, it supports 1:N or N:M dependency which
> > > 
> > > I completely missed, how N:M is supported? That starting to sound
> > > terrifying.
> > 
> > N:M is actually from Kevin's idea.
> > 
> > sqe group can be made to be more flexible by:
> > 
> >      Inside the group, all SQEs are submitted in parallel, so there isn't any
> >      dependency among SQEs in one group.
> >      The 1st SQE is group leader, and the other SQEs are group member. The whole
> >      group share single IOSQE_IO_LINK and IOSQE_IO_DRAIN from group leader, and
> >      the two flags can't be set for group members.
> >      When the group is in one link chain, this group isn't submitted until
> >      the previous SQE or group is completed. And the following SQE or group
> >      can't be started if this group isn't completed.
> >      When IOSQE_IO_DRAIN is set for group leader, all requests in this group
> >      and previous requests submitted are drained. Given IOSQE_IO_DRAIN can
> >      be set for group leader only, we respect IO_DRAIN for SQE group by
> >      always completing group leader as the last on in the group.
> >      SQE group provides flexible way to support N:M dependency, such as:
> >      - group A is chained with group B together by IOSQE_IO_LINK
> >      - group A has N SQEs
> >      - group B has M SQEs
> >      then M SQEs in group B depend on N SQEs in group A.
> 
> In other words, linking groups together with basically no extra rules.
> Fwiw, sounds generic, but if there are complications with IOSQE_IO_DRAIN
> that I don't immediately see, it'd be more reasonable to just disable it.

The only change on IO_DRAIN is on io_get_sequence() for taking leader->grp_refs
into account, and leader->grp_refs covers all requests in this group.

And my local patchset passes all related sqe->flags combination(DRAIN, LINKING,
ASYNC) on both single group or linked groups, meantime with extra change
of sharing same FORCE_ASYNC for both leader and members.

> 
> > > > is missing in io_uring, but also makes async application easier to write by
> > > > saving extra context switches, which just adds extra intermediate states for
> > > > application.
> > > 
> > > You're still executing requests (i.e. ->issue) primarily from the
> > > submitter task context, they would still fly back to the task and
> > > wake it up. You may save something by completing all of them
> > > together via that refcounting, but you might just as well try to
> > > batch CQ, which is a more generic issue. It's not clear what
> > > context switches you save then.
> > 
> > Wrt. the above N:M example, one io_uring_enter() is enough, and
> > it can't be done in single context switch without sqe group, please
> > see the liburing test code:
> > 
> > https://lore.kernel.org/io-uring/ZiHA+pN28hRdprhX@fedora/T/#ma755c500eab0b7dc8c1473448dd98f093097e066
> > 
> > > 
> > > As for simplicity, using the link example and considering error
> > > handling, it only complicates it. In case of an error you need to
> > > figure out a middle req failed, collect all failed CQEs linked to
> > > it and automatically cancelled (unless SKIP_COMPLETE is used), and
> > > then resubmit the failed. That's great your reads are idempotent
> > > and presumably you don't have to resubmit half a link, but in the
> > > grand picture of things it's rather one of use cases where a generic
> > > feature can be used.
> > 
> > SQE group doesn't change the current link implementation, and N:M
> > dependency is built over IOSQE_IO_LINK actually.
> > 
> > > 
> > > > > So, please, please! instead of trying to invent a new uber scheme
> > > > > of request linking, which surely wouldn't step on same problems
> > > > > over and over again, and would definitely be destined to overshadow
> > > > > all previous attempts and finally conquer the world, let's rather
> > > > > focus on minimasing the damage from this patchset's zero copy if
> > > > > it's going to be taken.
> > > > 
> > > > One key problem for zero copy is lifetime of the kernel buffer, which
> > > > can't cross OPs, that is why sqe group is introduced, for aligning
> > > > kernel buffer lifetime with the group.
> > > 
> > > Right, which is why I'm saying if we're leaving groups with zero
> > > copy, let's rather try to make them simple and not intrusive as
> > > much as possible, instead of creating an unsupportable overarching
> > > beast out of it, which would fail as a generic feature.
> > 
> > Then it degraded to the original fused command, :-)
> 
> I'm not arguing about restricting it to 1 request in a group apart
> from the master/leader/etc., if that's what you mean. The argument
> is rather to limit the overhead and abstraction leakage into hot
> paths.
> 
> For example that N:M, all that sounds great on paper until it sees
> the harsh reality. And instead of linking groups together, you
> can perfectly fine be sending one group at a time and queuing the
> next group from the userspace when the previous completes. And
> that would save space in io_kiocb, maybe(?) a flag, maybe something
> else.

Yes, it can be done with userspace, with cost of one extra io_uring_enter()
for handling two depended groups in userspace.

io_uring excellent performance is attributed to great batch processing,
if group linking is done in per-IO level, this way could degrade
performance a lot. And we know io_uring may perform worse in QD=1.

And the introduced 12 bytes doesn't add extra l1 cacheline, and won't
be touched in code path without using group.

> 
> Regardless of interoperability with links, I'd also prefer it to
> be folded into the link code structure and state machine, so
> it's not all over in the submission/completion paths adding
> overhead for one narrow feature, including un-inlining the
> entire submission path.

I can work toward this direction if the idea of sqe group may be accepted
but the biggest blocker could be where to allocate the extra SQE_GROUP
flag.

> 
> E.g. there should be no separate hand coded duplicated SQE
> / request assembling like in io_init_req_group(). Instead
> it should be able to consume SQEs and requests it's given
> from submit_sqes(), and e.g. process grouping in
> io_submit_sqe() around the place requests are linked.

Yeah, IOSQE_IO_LINK sort of handling could be needed for processing
requests in group.


Thanks,
Ming

Re: [PATCH 5/9] io_uring: support SQE group

[Pavel Begunkov]

On 4/30/24 16:00, Ming Lei wrote:
> On Tue, Apr 30, 2024 at 01:27:10PM +0100, Pavel Begunkov wrote:
...
>>>> And what does it achieve? The infra has matured since early days,
>>>> it saves user-kernel transitions at best but not context switching
>>>> overhead, and not even that if you do wait(1) and happen to catch
>>>> middle CQEs. And it disables LAZY_WAKE, so CQ side batching with
>>>> timers and what not is effectively useless with links.
>>>
>>> Not only the context switch, it supports 1:N or N:M dependency which
>>
>> I completely missed, how N:M is supported? That starting to sound
>> terrifying.
> 
> N:M is actually from Kevin's idea.
> 
> sqe group can be made to be more flexible by:
> 
>      Inside the group, all SQEs are submitted in parallel, so there isn't any
>      dependency among SQEs in one group.
>      
>      The 1st SQE is group leader, and the other SQEs are group member. The whole
>      group share single IOSQE_IO_LINK and IOSQE_IO_DRAIN from group leader, and
>      the two flags can't be set for group members.
>      
>      When the group is in one link chain, this group isn't submitted until
>      the previous SQE or group is completed. And the following SQE or group
>      can't be started if this group isn't completed.
>      
>      When IOSQE_IO_DRAIN is set for group leader, all requests in this group
>      and previous requests submitted are drained. Given IOSQE_IO_DRAIN can
>      be set for group leader only, we respect IO_DRAIN for SQE group by
>      always completing group leader as the last on in the group.
>      
>      SQE group provides flexible way to support N:M dependency, such as:
>      
>      - group A is chained with group B together by IOSQE_IO_LINK
>      - group A has N SQEs
>      - group B has M SQEs
>      
>      then M SQEs in group B depend on N SQEs in group A.
> 
> 
>>
>>> is missing in io_uring, but also makes async application easier to write by
>>> saving extra context switches, which just adds extra intermediate states for
>>> application.
>>
>> You're still executing requests (i.e. ->issue) primarily from the
>> submitter task context, they would still fly back to the task and
>> wake it up. You may save something by completing all of them
>> together via that refcounting, but you might just as well try to
>> batch CQ, which is a more generic issue. It's not clear what
>> context switches you save then.
> 
> Wrt. the above N:M example, one io_uring_enter() is enough, and
> it can't be done in single context switch without sqe group, please
> see the liburing test code:

Do you mean doing all that in a single system call? The main
performance problem for io_uring is waiting, i.e. schedule()ing
the task out and in, that's what I meant by context switching.

-- 
Pavel Begunkov

Re: [PATCH 5/9] io_uring: support SQE group

[Ming Lei]

On Mon, Apr 22, 2024 at 12:27:28PM -0600, Jens Axboe wrote:
> On 4/7/24 7:03 PM, Ming Lei wrote:
> > SQE group is defined as one chain of SQEs starting with the first sqe that
> > has IOSQE_EXT_SQE_GROUP set, and ending with the first subsequent sqe that
> > doesn't have it set, and it is similar with chain of linked sqes.
> > 
> > The 1st SQE is group leader, and the other SQEs are group member. The group
> > leader is always freed after all members are completed. Group members
> > aren't submitted until the group leader is completed, and there isn't any
> > dependency among group members, and IOSQE_IO_LINK can't be set for group
> > members, same with IOSQE_IO_DRAIN.
> > 
> > Typically the group leader provides or makes resource, and the other members
> > consume the resource, such as scenario of multiple backup, the 1st SQE is to
> > read data from source file into fixed buffer, the other SQEs write data from
> > the same buffer into other destination files. SQE group provides very
> > efficient way to complete this task: 1) fs write SQEs and fs read SQE can be
> > submitted in single syscall, no need to submit fs read SQE first, and wait
> > until read SQE is completed, 2) no need to link all write SQEs together, then
> > write SQEs can be submitted to files concurrently. Meantime application is
> > simplified a lot in this way.
> > 
> > Another use case is to for supporting generic device zero copy:
> > 
> > - the lead SQE is for providing device buffer, which is owned by device or
> >   kernel, can't be cross userspace, otherwise easy to cause leak for devil
> >   application or panic
> > 
> > - member SQEs reads or writes concurrently against the buffer provided by lead
> >   SQE
> 
> In concept, this looks very similar to "sqe bundles" that I played with
> in the past:
> 
> https://git.kernel.dk/cgit/linux/log/?h=io_uring-bundle

Indeed, so looks it is something which io_uring needs.

> 
> Didn't look too closely yet at the implementation, but in spirit it's
> about the same in that the first entry is processed first, and there's
> no ordering implied between the test of the members of the bundle /
> group.

Yeah.

> 
> I do think that's a flexible thing to support, particularly if:
> 
> 1) We can do it more efficiently than links, which are pretty horrible.

Agree, link is hard to use in async/.await of modern language per my
experience.

Also sqe group won't break link, and the group is thought as a whole
wrt. linking.

> 2) It enables new worthwhile use cases
> 3) It's done cleanly 
> 4) It's easily understandable and easy to document, so that users will
>    actually understand what this is and what use cases it enable. Part
>    of that is actually naming, it should be readily apparent what a
>    group is, what the lead is, and what the members are. Using your
>    terminology here, definitely worth spending some time on that to get
>    it just right and self evident.

All are nice suggestions, and I will follow above and make them in V2.


Thanks,
Ming

[PATCH 6/9] io_uring: support providing sqe group buffer

[Ming Lei]

SQE group introduces one new mechanism to share resource among one group of
requests, and all member requests can consume the resource provided by
group lead efficiently and concurrently.

This patch uses the added sqe group feature to share kernel buffer in sqe
group:

- the group lead provides kernel buffer to member requests

- member requests use the provided buffer to do FS or network IO, or more
operations in future

- this kernel buffer is returned back after member requests use it up

This way looks a bit similar with kernel's pipe/splice, but there are some
important differences:

- splice is for transferring data between two FDs via pipe, and fd_out can
only read data from pipe; this feature can borrow buffer from group lead to
members, so member request can write data to this buffer if the provided
buffer is allowed to write to.

- splice implements data transfer by moving pages between subsystem and
pipe, that means page ownership is transferred, and this way is one of the
most complicated thing of splice; this patch supports scenarios in which
the buffer can't be transferred, and buffer is only borrowed to member
requests, and is returned back after member requests consume the provided
buffer, so buffer lifetime is simplified a lot. Especially the buffer is
guaranteed to be returned back.

- splice can't run in async way basically

It can help to implement generic zero copy between device and related
operations, such as ublk, fuse, vdpa, even network receive or whatever.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/io_uring_types.h | 30 ++++++++++++++++
 io_uring/io_uring.c            | 10 +++++-
 io_uring/kbuf.c                | 62 ++++++++++++++++++++++++++++++++++
 io_uring/kbuf.h                | 12 +++++++
 io_uring/net.c                 | 29 ++++++++++++++++
 io_uring/opdef.c               |  5 +++
 io_uring/opdef.h               |  2 ++
 io_uring/rw.c                  | 20 ++++++++++-
 8 files changed, 168 insertions(+), 2 deletions(-)

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index 7ce4a2d4a8b8..e632c3584800 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -6,6 +6,7 @@
 #include <linux/task_work.h>
 #include <linux/bitmap.h>
 #include <linux/llist.h>
+#include <linux/bvec.h>
 #include <uapi/linux/io_uring.h>
 
 enum {
@@ -39,6 +40,22 @@ enum io_uring_cmd_flags {
 	IO_URING_F_COMPAT		= (1 << 12),
 };
 
+/* buffer provided from kernel */
+struct io_uring_kernel_buf {
+	unsigned long	len;
+	unsigned short	nr_bvecs;
+	unsigned short	dir;	/* ITER_SOURCE or ITER_DEST */
+
+	/* offset in the 1st bvec */
+	unsigned int		offset;
+	const struct bio_vec	*bvec;
+
+	/* private field, user don't touch it */
+	struct bio_vec		__bvec[];
+};
+
+typedef void (io_uring_buf_giveback_t) (const struct io_uring_kernel_buf *);
+
 struct io_wq_work_node {
 	struct io_wq_work_node *next;
 };
@@ -476,6 +493,7 @@ enum {
 	REQ_F_BL_EMPTY_BIT,
 	REQ_F_BL_NO_RECYCLE_BIT,
 	REQ_F_SQE_GROUP_LEAD_BIT,
+	REQ_F_GROUP_KBUF_BIT,
 
 	/* not a real bit, just to check we're not overflowing the space */
 	__REQ_F_LAST_BIT,
@@ -558,6 +576,8 @@ enum {
 	REQ_F_BL_NO_RECYCLE	= IO_REQ_FLAG(REQ_F_BL_NO_RECYCLE_BIT),
 	/* sqe group lead */
 	REQ_F_SQE_GROUP_LEAD	= IO_REQ_FLAG(REQ_F_SQE_GROUP_LEAD_BIT),
+	/* group lead provides kbuf for members, set for both lead and member */
+	REQ_F_GROUP_KBUF	= IO_REQ_FLAG(REQ_F_GROUP_KBUF_BIT),
 };
 
 typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts);
@@ -641,6 +661,15 @@ struct io_kiocb {
 		 * REQ_F_BUFFER_RING is set.
 		 */
 		struct io_buffer_list	*buf_list;
+
+		/*
+		 * store kernel buffer provided by sqe group lead, valid
+		 * IFF REQ_F_GROUP_KBUF
+		 *
+		 * The buffer meta is immutable since it is shared by
+		 * all member requests
+		 */
+		const struct io_uring_kernel_buf *grp_kbuf;
 	};
 
 	union {
@@ -675,6 +704,7 @@ struct io_kiocb {
 	/* all SQE group members linked here for group lead */
 	struct io_kiocb			*grp_link;
 	atomic_t			grp_refs;
+	io_uring_buf_giveback_t		*grp_kbuf_ack;
 };
 
 struct io_overflow_cqe {
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 0f41b26723a8..596c4442c3c6 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -114,7 +114,7 @@
 
 #define IO_REQ_CLEAN_FLAGS (REQ_F_BUFFER_SELECTED | REQ_F_NEED_CLEANUP | \
 				REQ_F_POLLED | REQ_F_INFLIGHT | REQ_F_CREDS | \
-				REQ_F_ASYNC_DATA)
+				REQ_F_ASYNC_DATA | REQ_F_GROUP_KBUF)
 
 #define IO_REQ_CLEAN_SLOW_FLAGS (REQ_F_REFCOUNT | REQ_F_LINK | REQ_F_HARDLINK |\
 				 IO_REQ_CLEAN_FLAGS)
@@ -384,6 +384,9 @@ static bool req_need_defer(struct io_kiocb *req, u32 seq)
 
 static void io_clean_op(struct io_kiocb *req)
 {
+	if (req->flags & REQ_F_GROUP_KBUF)
+		io_group_kbuf_drop(req);
+
 	if (req->flags & REQ_F_BUFFER_SELECTED) {
 		spin_lock(&req->ctx->completion_lock);
 		io_kbuf_drop(req);
@@ -989,8 +992,13 @@ void io_complete_group_lead(struct io_kiocb *req, unsigned issue_flags)
 
 	while (member) {
 		struct io_kiocb *next = member->grp_link;
+		const struct io_issue_def *def = &io_issue_defs[member->opcode];
 
 		member->grp_link = req;
+		if ((req->flags & REQ_F_GROUP_KBUF) && def->accept_group_kbuf) {
+			member->flags |= REQ_F_GROUP_KBUF;
+			member->grp_kbuf_ack = NULL;
+		}
 		if (unlikely(req->flags & REQ_F_FAIL)) {
 			/*
 			 * Now group lead is failed, so simply fail members
diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index 3846a055df44..ab175fa6d57c 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -672,3 +672,65 @@ int io_pbuf_mmap(struct file *file, struct vm_area_struct *vma)
 	io_put_bl(ctx, bl);
 	return ret;
 }
+
+int io_provide_group_kbuf(struct io_kiocb *req,
+		const struct io_uring_kernel_buf *grp_kbuf,
+		io_uring_buf_giveback_t grp_kbuf_ack)
+{
+	if (unlikely(!req_is_group_lead(req)))
+		return -EINVAL;
+
+	/*
+	 * Borrow this buffer from one kernel subsystem, and return them
+	 * by calling `grp_kbuf_ack` when the group lead is freed.
+	 *
+	 * Not like pipe/splice, this kernel buffer is always owned by the
+	 * provider, and has to be returned back.
+	 */
+	req->grp_kbuf = grp_kbuf;
+	req->grp_kbuf_ack = grp_kbuf_ack;
+	req->flags |= REQ_F_GROUP_KBUF;
+
+	return 0;
+}
+
+int io_import_group_kbuf(struct io_kiocb *req, unsigned long buf_off,
+		unsigned int len, int dir, struct iov_iter *iter)
+{
+	struct io_kiocb *lead = req->grp_link;
+	const struct io_uring_kernel_buf *kbuf;
+	unsigned long offset;
+
+	WARN_ON_ONCE(!(req->flags & REQ_F_GROUP_KBUF));
+
+	if (!req_is_group_member(req))
+		return -EINVAL;
+
+	if (!lead || !req_is_group_lead(lead) || !lead->grp_kbuf)
+		return -EINVAL;
+
+	/* req->fused_cmd_kbuf is immutable */
+	kbuf = lead->grp_kbuf;
+	offset = kbuf->offset;
+
+	if (!kbuf->bvec)
+		return -EINVAL;
+
+	if (dir != kbuf->dir)
+		return -EINVAL;
+
+	if (unlikely(buf_off > kbuf->len))
+		return -EFAULT;
+
+	if (unlikely(len > kbuf->len - buf_off))
+		return -EFAULT;
+
+	/* don't use io_import_fixed which doesn't support multipage bvec */
+	offset += buf_off;
+	iov_iter_bvec(iter, dir, kbuf->bvec, kbuf->nr_bvecs, offset + len);
+
+	if (offset)
+		iov_iter_advance(iter, offset);
+
+	return 0;
+}
diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h
index 5a9635ee0217..5d731ed52a28 100644
--- a/io_uring/kbuf.h
+++ b/io_uring/kbuf.h
@@ -64,6 +64,12 @@ struct io_buffer_list *io_pbuf_get_bl(struct io_ring_ctx *ctx,
 				      unsigned long bgid);
 int io_pbuf_mmap(struct file *file, struct vm_area_struct *vma);
 
+int io_provide_group_kbuf(struct io_kiocb *req,
+		const struct io_uring_kernel_buf *grp_kbuf,
+		io_uring_buf_giveback_t grp_kbuf_ack);
+int io_import_group_kbuf(struct io_kiocb *req, unsigned long buf_off,
+		unsigned int len, int dir, struct iov_iter *iter);
+
 static inline bool io_kbuf_recycle_ring(struct io_kiocb *req)
 {
 	/*
@@ -145,4 +151,10 @@ static inline unsigned int io_put_kbuf(struct io_kiocb *req,
 		__io_put_kbuf(req, issue_flags);
 	return ret;
 }
+
+static inline void io_group_kbuf_drop(struct io_kiocb *req)
+{
+	if (req->grp_kbuf_ack)
+		req->grp_kbuf_ack(req->grp_kbuf);
+}
 #endif
diff --git a/io_uring/net.c b/io_uring/net.c
index 9c0567892945..5371f0f2c0d8 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -78,6 +78,13 @@ struct io_sr_msg {
  */
 #define MULTISHOT_MAX_RETRY	32
 
+#define user_ptr_to_u64(x) (		\
+{					\
+	typecheck(void __user *, (x));	\
+	(u64)(unsigned long)(x);	\
+}					\
+)
+
 int io_shutdown_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	struct io_shutdown *shutdown = io_kiocb_to_cmd(req, struct io_shutdown);
@@ -479,6 +486,14 @@ int io_send(struct io_kiocb *req, unsigned int issue_flags)
 	    (sr->flags & IORING_RECVSEND_POLL_FIRST))
 		return -EAGAIN;
 
+	if (req->flags & REQ_F_GROUP_KBUF) {
+		ret = io_import_group_kbuf(req,
+					user_ptr_to_u64(sr->buf),
+					sr->len, ITER_SOURCE,
+					&kmsg->msg.msg_iter);
+		if (unlikely(ret))
+			return ret;
+	}
 	flags = sr->msg_flags;
 	if (issue_flags & IO_URING_F_NONBLOCK)
 		flags |= MSG_DONTWAIT;
@@ -927,6 +942,11 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
 				  &kmsg->msg.msg_iter);
 		if (unlikely(ret))
 			goto out_free;
+	} else if (req->flags & REQ_F_GROUP_KBUF) {
+		ret = io_import_group_kbuf(req, user_ptr_to_u64(sr->buf),
+				sr->len, ITER_DEST, &kmsg->msg.msg_iter);
+		if (unlikely(ret))
+			goto out_free;
 	}
 
 	kmsg->msg.msg_inq = -1;
@@ -1125,8 +1145,17 @@ static int io_send_zc_import(struct io_kiocb *req, struct io_async_msghdr *kmsg)
 		if (unlikely(ret))
 			return ret;
 		kmsg->msg.sg_from_iter = io_sg_from_iter;
+	} else if (req->flags & REQ_F_GROUP_KBUF) {
+		struct io_sr_msg *sr = io_kiocb_to_cmd(req, struct io_sr_msg);
+
+		ret = io_import_group_kbuf(req, user_ptr_to_u64(sr->buf),
+				sr->len, ITER_SOURCE, &kmsg->msg.msg_iter);
+		if (unlikely(ret))
+			return ret;
+		kmsg->msg.sg_from_iter = io_sg_from_iter;
 	} else {
 		io_notif_set_extended(sr->notif);
+
 		ret = import_ubuf(ITER_SOURCE, sr->buf, sr->len, &kmsg->msg.msg_iter);
 		if (unlikely(ret))
 			return ret;
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index a16f73938ebb..705f0333f9e0 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -246,6 +246,7 @@ const struct io_issue_def io_issue_defs[] = {
 		.ioprio			= 1,
 		.iopoll			= 1,
 		.iopoll_queue		= 1,
+		.accept_group_kbuf	= 1,
 		.async_size		= sizeof(struct io_async_rw),
 		.prep			= io_prep_read,
 		.issue			= io_read,
@@ -260,6 +261,7 @@ const struct io_issue_def io_issue_defs[] = {
 		.ioprio			= 1,
 		.iopoll			= 1,
 		.iopoll_queue		= 1,
+		.accept_group_kbuf	= 1,
 		.async_size		= sizeof(struct io_async_rw),
 		.prep			= io_prep_write,
 		.issue			= io_write,
@@ -281,6 +283,7 @@ const struct io_issue_def io_issue_defs[] = {
 		.pollout		= 1,
 		.audit_skip		= 1,
 		.ioprio			= 1,
+		.accept_group_kbuf	= 1,
 #if defined(CONFIG_NET)
 		.async_size		= sizeof(struct io_async_msghdr),
 		.prep			= io_sendmsg_prep,
@@ -296,6 +299,7 @@ const struct io_issue_def io_issue_defs[] = {
 		.buffer_select		= 1,
 		.audit_skip		= 1,
 		.ioprio			= 1,
+		.accept_group_kbuf	= 1,
 #if defined(CONFIG_NET)
 		.async_size		= sizeof(struct io_async_msghdr),
 		.prep			= io_recvmsg_prep,
@@ -423,6 +427,7 @@ const struct io_issue_def io_issue_defs[] = {
 		.pollout		= 1,
 		.audit_skip		= 1,
 		.ioprio			= 1,
+		.accept_group_kbuf	= 1,
 #if defined(CONFIG_NET)
 		.async_size		= sizeof(struct io_async_msghdr),
 		.prep			= io_send_zc_prep,
diff --git a/io_uring/opdef.h b/io_uring/opdef.h
index 7ee6f5aa90aa..a53970655c82 100644
--- a/io_uring/opdef.h
+++ b/io_uring/opdef.h
@@ -29,6 +29,8 @@ struct io_issue_def {
 	unsigned		iopoll_queue : 1;
 	/* vectored opcode, set if 1) vectored, and 2) handler needs to know */
 	unsigned		vectored : 1;
+	/* opcodes which accept provided group kbuf */
+	unsigned		accept_group_kbuf : 1;
 
 	/* size of async data needed, if any */
 	unsigned short		async_size;
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 3134a6ece1be..f1052af40563 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -235,7 +235,8 @@ static int io_prep_rw_setup(struct io_kiocb *req, int ddir, bool do_import)
 	if (io_rw_alloc_async(req))
 		return -ENOMEM;
 
-	if (!do_import || io_do_buffer_select(req))
+	if (!do_import || io_do_buffer_select(req) ||
+	    (req->flags & REQ_F_GROUP_KBUF))
 		return 0;
 
 	rw = req->async_data;
@@ -603,11 +604,16 @@ static inline loff_t *io_kiocb_ppos(struct kiocb *kiocb)
  */
 static ssize_t loop_rw_iter(int ddir, struct io_rw *rw, struct iov_iter *iter)
 {
+	struct io_kiocb *req = cmd_to_io_kiocb(rw);
 	struct kiocb *kiocb = &rw->kiocb;
 	struct file *file = kiocb->ki_filp;
 	ssize_t ret = 0;
 	loff_t *ppos;
 
+	/* group buffer is kernel buffer and doesn't have userspace addr */
+	if (req->flags & REQ_F_GROUP_KBUF)
+		return -EOPNOTSUPP;
+
 	/*
 	 * Don't support polled IO through this interface, and we can't
 	 * support non-blocking either. For the latter, this just causes
@@ -813,6 +819,11 @@ static int __io_read(struct io_kiocb *req, unsigned int issue_flags)
 		ret = io_import_iovec(ITER_DEST, req, io, issue_flags);
 		if (unlikely(ret < 0))
 			return ret;
+	} else if (req->flags & REQ_F_GROUP_KBUF) {
+		ret = io_import_group_kbuf(req, rw->addr, rw->len, ITER_DEST,
+				&io->iter);
+		if (unlikely(ret))
+			return ret;
 	}
 
 	ret = io_rw_init_file(req, FMODE_READ);
@@ -995,6 +1006,13 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)
 	ssize_t ret, ret2;
 	loff_t *ppos;
 
+	if (req->flags & REQ_F_GROUP_KBUF) {
+		ret = io_import_group_kbuf(req, rw->addr, rw->len, ITER_SOURCE,
+				&io->iter);
+		if (unlikely(ret))
+			return ret;
+	}
+
 	ret = io_rw_init_file(req, FMODE_WRITE);
 	if (unlikely(ret))
 		return ret;
-- 
2.42.0

[PATCH 7/9] io_uring/uring_cmd: support provide group kernel buffer

[Ming Lei]

Allow uring command to be group leader for providing kernel buffer,
and this way can support generic device zero copy over device buffer.

The following patch will use the way to support zero copy for ublk.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/io_uring/cmd.h  |  9 +++++++++
 include/uapi/linux/io_uring.h |  7 ++++++-
 io_uring/uring_cmd.c          | 22 ++++++++++++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index 447fbfd32215..707660df1083 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -48,6 +48,9 @@ void __io_uring_cmd_do_in_task(struct io_uring_cmd *ioucmd,
 void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,
 		unsigned int issue_flags);
 
+int io_uring_cmd_provide_kbuf(struct io_uring_cmd *ioucmd,
+		const struct io_uring_kernel_buf *grp_kbuf,
+		io_uring_buf_giveback_t grp_kbuf_ack);
 #else
 static inline int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
 			      struct iov_iter *iter, void *ioucmd)
@@ -67,6 +70,12 @@ static inline void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,
 		unsigned int issue_flags)
 {
 }
+static inline int io_uring_cmd_provide_kbuf(struct io_uring_cmd *ioucmd,
+		const struct io_uring_kernel_buf *grp_kbuf,
+		io_uring_buf_giveback_t grp_kbuf_ack)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 /*
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index c0d34f2a2c17..0060971721d0 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -281,9 +281,14 @@ enum io_uring_op {
  * bit23 ~ bit16		sqe ext flags
  * IORING_URING_CMD_FIXED	use registered buffer; pass this flag
  *				along with setting sqe->buf_index.
+ * IORING_PROVIDE_GROUP_KBUF	this command provides group kernel buffer
+ *				for member requests which can retrieve
+ *				any sub-buffer with offset(sqe->addr) and
+ *				len(sqe->len)
  */
 #define IORING_URING_CMD_FIXED	(1U << 0)
-#define IORING_URING_CMD_MASK	IORING_URING_CMD_FIXED
+#define IORING_PROVIDE_GROUP_KBUF	(1U << 1)
+#define IORING_URING_CMD_MASK	(IORING_URING_CMD_FIXED | IORING_PROVIDE_GROUP_KBUF)
 #define IORING_URING_CMD_EXT_MASK	0x00ff0000
 
 
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 43b71f29e7b3..ec03c9fe965c 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -14,6 +14,7 @@
 #include "alloc_cache.h"
 #include "rsrc.h"
 #include "uring_cmd.h"
+#include "kbuf.h"
 
 static struct uring_cache *io_uring_async_get(struct io_kiocb *req)
 {
@@ -174,6 +175,27 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,
 }
 EXPORT_SYMBOL_GPL(io_uring_cmd_done);
 
+/*
+ * Provide kernel buffer for sqe group members to consume, and the caller
+ * has to guarantee that the provided buffer and the callback are valid
+ * until the callback is called.
+ */
+int io_uring_cmd_provide_kbuf(struct io_uring_cmd *ioucmd,
+		const struct io_uring_kernel_buf *grp_kbuf,
+		io_uring_buf_giveback_t grp_kbuf_ack)
+{
+	struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
+
+	if (unlikely(!(ioucmd->flags & IORING_PROVIDE_GROUP_KBUF)))
+		return -EINVAL;
+
+	if (unlikely(!(req->flags & REQ_F_SQE_GROUP)))
+		return -EINVAL;
+
+	return io_provide_group_kbuf(req, grp_kbuf, grp_kbuf_ack);
+}
+EXPORT_SYMBOL_GPL(io_uring_cmd_provide_kbuf);
+
 static int io_uring_cmd_prep_setup(struct io_kiocb *req,
 				   const struct io_uring_sqe *sqe)
 {
-- 
2.42.0

[PATCH 8/9] ublk: support provide io buffer

[Ming Lei]

Implement uring command's IORING_PROVIDE_GROUP_KBUF, and provide
io buffer for userpace to run io_uring operations(FS, network IO),
then ublk zero copy can be supported.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c      | 156 ++++++++++++++++++++++++++++++++--
 include/uapi/linux/ublk_cmd.h |   7 +-
 2 files changed, 152 insertions(+), 11 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index bea3d5cf8a83..76e3e9e421f3 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -71,6 +71,8 @@ struct ublk_rq_data {
 	__u64 sector;
 	__u32 operation;
 	__u32 nr_zones;
+	bool allocated_bvec;
+	struct io_uring_kernel_buf buf[0];
 };
 
 struct ublk_uring_cmd_pdu {
@@ -189,6 +191,8 @@ struct ublk_params_header {
 	__u32	types;
 };
 
+static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
+		struct ublk_queue *ubq, int tag, size_t offset);
 static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq);
 
 static inline unsigned int ublk_req_build_flags(struct request *req);
@@ -566,6 +570,11 @@ static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
 	return ublk_support_user_copy(ubq);
 }
 
+static inline bool ublk_support_zc(const struct ublk_queue *ubq)
+{
+	return ubq->flags & UBLK_F_SUPPORT_ZERO_COPY;
+}
+
 static inline void ublk_init_req_ref(const struct ublk_queue *ubq,
 		struct request *req)
 {
@@ -829,6 +838,70 @@ static size_t ublk_copy_user_pages(const struct request *req,
 	return done;
 }
 
+/*
+ * The built command buffer is immutable, so it is fine to feed it to
+ * concurrent io_uring provide buf commands
+ */
+static int ublk_init_zero_copy_buffer(struct request *req)
+{
+	struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
+	struct io_uring_kernel_buf *imu = data->buf;
+	struct req_iterator rq_iter;
+	unsigned int nr_bvecs = 0;
+	struct bio_vec *bvec;
+	unsigned int offset;
+	struct bio_vec bv;
+
+	if (!ublk_rq_has_data(req))
+		goto exit;
+
+	rq_for_each_bvec(bv, req, rq_iter)
+		nr_bvecs++;
+
+	if (!nr_bvecs)
+		goto exit;
+
+	if (req->bio != req->biotail) {
+		int idx = 0;
+
+		bvec = kvmalloc_array(nr_bvecs, sizeof(struct bio_vec),
+				GFP_NOIO);
+		if (!bvec)
+			return -ENOMEM;
+
+		offset = 0;
+		rq_for_each_bvec(bv, req, rq_iter)
+			bvec[idx++] = bv;
+		data->allocated_bvec = true;
+	} else {
+		struct bio *bio = req->bio;
+
+		offset = bio->bi_iter.bi_bvec_done;
+		bvec = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
+	}
+	imu->bvec = bvec;
+	imu->nr_bvecs = nr_bvecs;
+	imu->offset = offset;
+	imu->len = blk_rq_bytes(req);
+	imu->dir = req_op(req) == REQ_OP_READ ? ITER_DEST : ITER_SOURCE;
+
+	return 0;
+exit:
+	imu->bvec = NULL;
+	return 0;
+}
+
+static void ublk_deinit_zero_copy_buffer(struct request *req)
+{
+	struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
+	struct io_uring_kernel_buf *imu = data->buf;
+
+	if (data->allocated_bvec) {
+		kvfree(imu->bvec);
+		data->allocated_bvec = false;
+	}
+}
+
 static inline bool ublk_need_map_req(const struct request *req)
 {
 	return ublk_rq_has_data(req) && req_op(req) == REQ_OP_WRITE;
@@ -840,13 +913,25 @@ static inline bool ublk_need_unmap_req(const struct request *req)
 	       (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN);
 }
 
-static int ublk_map_io(const struct ublk_queue *ubq, const struct request *req,
+static int ublk_map_io(const struct ublk_queue *ubq, struct request *req,
 		struct ublk_io *io)
 {
 	const unsigned int rq_bytes = blk_rq_bytes(req);
 
-	if (ublk_support_user_copy(ubq))
+	if (ublk_support_user_copy(ubq)) {
+		if (ublk_support_zc(ubq)) {
+			int ret = ublk_init_zero_copy_buffer(req);
+
+			/*
+			 * The only failure is -ENOMEM for allocating providing
+			 * buffer command, return zero so that we can requeue
+			 * this req.
+			 */
+			if (unlikely(ret))
+				return 0;
+		}
 		return rq_bytes;
+	}
 
 	/*
 	 * no zero copy, we delay copy WRITE request data into ublksrv
@@ -864,13 +949,16 @@ static int ublk_map_io(const struct ublk_queue *ubq, const struct request *req,
 }
 
 static int ublk_unmap_io(const struct ublk_queue *ubq,
-		const struct request *req,
+		struct request *req,
 		struct ublk_io *io)
 {
 	const unsigned int rq_bytes = blk_rq_bytes(req);
 
-	if (ublk_support_user_copy(ubq))
+	if (ublk_support_user_copy(ubq)) {
+		if (ublk_support_zc(ubq))
+			ublk_deinit_zero_copy_buffer(req);
 		return rq_bytes;
+	}
 
 	if (ublk_need_unmap_req(req)) {
 		struct iov_iter iter;
@@ -1016,6 +1104,7 @@ static inline void __ublk_complete_rq(struct request *req)
 
 	return;
 exit:
+	ublk_deinit_zero_copy_buffer(req);
 	blk_mq_end_request(req, res);
 }
 
@@ -1658,6 +1747,46 @@ static inline void ublk_prep_cancel(struct io_uring_cmd *cmd,
 	io_uring_cmd_mark_cancelable(cmd, issue_flags);
 }
 
+static void ublk_io_buf_giveback_cb(const struct io_uring_kernel_buf *buf)
+{
+	struct ublk_rq_data *data = container_of(buf, struct ublk_rq_data, buf[0]);
+	struct request *req = blk_mq_rq_from_pdu(data);
+	struct ublk_queue *ubq = req->mq_hctx->driver_data;
+
+	ublk_put_req_ref(ubq, req);
+}
+
+static int ublk_provide_io_buf(struct io_uring_cmd *cmd,
+		struct ublk_queue *ubq, int tag)
+{
+	struct ublk_device *ub = cmd->file->private_data;
+	struct ublk_rq_data *data;
+	struct request *req;
+
+	if (!ub)
+		return -EPERM;
+
+	req = __ublk_check_and_get_req(ub, ubq, tag, 0);
+	if (!req)
+		return -EINVAL;
+
+	pr_devel("%s: qid %d tag %u request bytes %u\n",
+			__func__, tag, ubq->q_id, blk_rq_bytes(req));
+
+	data = blk_mq_rq_to_pdu(req);
+
+	/*
+	 * io_uring guarantees that the callback will be called after
+	 * the provided buffer is consumed, and it is automatic removal
+	 * before this uring command is freed.
+	 *
+	 * This request won't be completed unless the callback is called,
+	 * so ublk module won't be unloaded too.
+	 */
+	return io_uring_cmd_provide_kbuf(cmd, data->buf,
+			ublk_io_buf_giveback_cb);
+}
+
 static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 			       unsigned int issue_flags,
 			       const struct ublksrv_io_cmd *ub_cmd)
@@ -1674,6 +1803,10 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 			__func__, cmd->cmd_op, ub_cmd->q_id, tag,
 			ub_cmd->result);
 
+	if ((cmd->flags & IORING_PROVIDE_GROUP_KBUF) &&
+			cmd_op != UBLK_U_IO_PROVIDE_IO_BUF)
+		return -EOPNOTSUPP;
+
 	if (ub_cmd->q_id >= ub->dev_info.nr_hw_queues)
 		goto out;
 
@@ -1709,6 +1842,8 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 
 	ret = -EINVAL;
 	switch (_IOC_NR(cmd_op)) {
+	case _IOC_NR(UBLK_U_IO_PROVIDE_IO_BUF):
+		return ublk_provide_io_buf(cmd, ubq, tag);
 	case UBLK_IO_FETCH_REQ:
 		/* UBLK_IO_FETCH_REQ is only allowed before queue is setup */
 		if (ublk_queue_ready(ubq)) {
@@ -2128,11 +2263,14 @@ static void ublk_align_max_io_size(struct ublk_device *ub)
 
 static int ublk_add_tag_set(struct ublk_device *ub)
 {
+	int zc = !!(ub->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY);
+	struct ublk_rq_data *data;
+
 	ub->tag_set.ops = &ublk_mq_ops;
 	ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues;
 	ub->tag_set.queue_depth = ub->dev_info.queue_depth;
 	ub->tag_set.numa_node = NUMA_NO_NODE;
-	ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
+	ub->tag_set.cmd_size = struct_size(data, buf, zc);
 	ub->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
 	ub->tag_set.driver_data = ub;
 	return blk_mq_alloc_tag_set(&ub->tag_set);
@@ -2417,8 +2555,12 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 		goto out_free_dev_number;
 	}
 
-	/* We are not ready to support zero copy */
-	ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY;
+	/* zero copy depends on user copy */
+	if ((ub->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY) &&
+			!ublk_dev_is_user_copy(ub)) {
+		ret = -EINVAL;
+		goto out_free_dev_number;
+	}
 
 	ub->dev_info.nr_hw_queues = min_t(unsigned int,
 			ub->dev_info.nr_hw_queues, nr_cpu_ids);
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index c8dc5f8ea699..897ace0794c2 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -94,6 +94,8 @@
 	_IOWR('u', UBLK_IO_COMMIT_AND_FETCH_REQ, struct ublksrv_io_cmd)
 #define	UBLK_U_IO_NEED_GET_DATA		\
 	_IOWR('u', UBLK_IO_NEED_GET_DATA, struct ublksrv_io_cmd)
+#define	UBLK_U_IO_PROVIDE_IO_BUF	\
+	_IOWR('u', 0x23, struct ublksrv_io_cmd)
 
 /* only ABORT means that no re-fetch */
 #define UBLK_IO_RES_OK			0
@@ -126,10 +128,7 @@
 #define UBLKSRV_IO_BUF_TOTAL_BITS	(UBLK_QID_OFF + UBLK_QID_BITS)
 #define UBLKSRV_IO_BUF_TOTAL_SIZE	(1ULL << UBLKSRV_IO_BUF_TOTAL_BITS)
 
-/*
- * zero copy requires 4k block size, and can remap ublk driver's io
- * request into ublksrv's vm space
- */
+/* io_uring provide kbuf command based zero copy */
 #define UBLK_F_SUPPORT_ZERO_COPY	(1ULL << 0)
 
 /*
-- 
2.42.0

[RFC PATCH 9/9] liburing: support sqe ext_flags & sqe group

[Ming Lei]

From: Ming Lei <tom.leiming@gmail.com>

Provide helper to set sqe ext_flags.

Add one test case to cover sqe group feature by applying sqe group for
copying one part of source file into multiple destinations via single
syscall.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 src/include/liburing.h          |  15 ++
 src/include/liburing/io_uring.h |  20 +++
 test/Makefile                   |   1 +
 test/group_cp.c                 | 260 ++++++++++++++++++++++++++++++++
 4 files changed, 296 insertions(+)
 create mode 100644 test/group_cp.c

diff --git a/src/include/liburing.h b/src/include/liburing.h
index 3e47298..5379d53 100644
--- a/src/include/liburing.h
+++ b/src/include/liburing.h
@@ -368,6 +368,21 @@ IOURINGINLINE void io_uring_sqe_set_flags(struct io_uring_sqe *sqe,
 	sqe->flags = (__u8) flags;
 }
 
+IOURINGINLINE void io_uring_sqe_set_ext_flags(struct io_uring_sqe *sqe,
+					  unsigned flags)
+{
+	sqe->ext_flags = (__u8) flags;
+	sqe->flags |= IOSQE_HAS_EXT_FLAGS;
+}
+
+IOURINGINLINE void io_uring_cmd_set_ext_flags(struct io_uring_sqe *sqe,
+					  unsigned flags)
+{
+	sqe->uring_cmd_flags &= ~IORING_URING_CMD_EXT_MASK;
+	sqe->uring_cmd_flags |= ((__u8) flags) << 16;
+	sqe->flags |= IOSQE_HAS_EXT_FLAGS;
+}
+
 IOURINGINLINE void __io_uring_set_target_fixed_file(struct io_uring_sqe *sqe,
 						    unsigned int file_index)
 {
diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h
index bde1199..b0a0318 100644
--- a/src/include/liburing/io_uring.h
+++ b/src/include/liburing/io_uring.h
@@ -98,6 +98,10 @@ struct io_uring_sqe {
 			__u64	__pad2[1];
 		};
 		__u64	optval;
+		struct {
+			__u8	__pad4[15];
+			__u8	ext_flags;
+		};
 		/*
 		 * If the ring is initialized with IORING_SETUP_SQE128, then
 		 * this field is used for 80 bytes of arbitrary command data
@@ -123,6 +127,9 @@ enum {
 	IOSQE_ASYNC_BIT,
 	IOSQE_BUFFER_SELECT_BIT,
 	IOSQE_CQE_SKIP_SUCCESS_BIT,
+	IOSQE_HAS_EXT_FLAGS_BIT,
+
+	IOSQE_EXT_SQE_GROUP_BIT = 0,
 };
 
 /*
@@ -142,6 +149,13 @@ enum {
 #define IOSQE_BUFFER_SELECT	(1U << IOSQE_BUFFER_SELECT_BIT)
 /* don't post CQE if request succeeded */
 #define IOSQE_CQE_SKIP_SUCCESS	(1U << IOSQE_CQE_SKIP_SUCCESS_BIT)
+/*
+ * sqe ext flags carried in the last byte, or bit23~bit16 of
+ * sqe->uring_cmd_flags for IORING_URING_CMD.
+ */
+#define IOSQE_HAS_EXT_FLAGS	(1U << IOSQE_HAS_EXT_FLAGS_BIT)
+/* defines sqe group */
+#define IOSQE_EXT_SQE_GROUP	(1U << IOSQE_EXT_SQE_GROUP_BIT)
 
 /*
  * io_uring_setup() flags
@@ -265,8 +279,14 @@ enum io_uring_op {
  * sqe->uring_cmd_flags
  * IORING_URING_CMD_FIXED	use registered buffer; pass this flag
  *				along with setting sqe->buf_index.
+ * IORING_PROVIDE_GROUP_KBUF	this command provides group kernel buffer
+ *				for member requests which can retrieve
+ *				any sub-buffer with offset(sqe->addr) and
+ *				len(sqe->len)
  */
 #define IORING_URING_CMD_FIXED	(1U << 0)
+#define IORING_PROVIDE_GROUP_KBUF	(1U << 1)
+#define IORING_URING_CMD_EXT_MASK	0x00ff0000
 
 
 /*
diff --git a/test/Makefile b/test/Makefile
index 32848ec..dd3b394 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -207,6 +207,7 @@ test_srcs := \
 	wakeup-hang.c \
 	wq-aff.c \
 	xattr.c \
+	group_cp.c \
 	# EOL
 
 all_targets :=
diff --git a/test/group_cp.c b/test/group_cp.c
new file mode 100644
index 0000000..4ac5cdb
--- /dev/null
+++ b/test/group_cp.c
@@ -0,0 +1,260 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Test SQE group feature
+ *
+ */
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <fcntl.h>
+#include <sys/types.h>
+#include <assert.h>
+#include <sys/stat.h>
+#include <linux/fs.h>
+
+#include "liburing.h"
+#include "helpers.h"
+
+#define BUF_SIZE    8192
+#define BUFFERS     6
+
+
+struct test_data {
+	const char *src_file;
+	struct iovec iov[BUFFERS];
+	int fd_in, fd_out1, fd_out2;
+};
+
+static void set_sqe_group(struct io_uring_sqe *sqe)
+{
+	io_uring_sqe_set_ext_flags(sqe, IOSQE_EXT_SQE_GROUP);
+}
+
+static int check_cqe(struct io_uring *ring, unsigned int nr)
+{
+	int i, ret;
+
+        for (i = 0; i < nr; ++i) {
+		struct io_uring_cqe *cqe;
+                int res;
+
+                ret = io_uring_peek_cqe(ring, &cqe);
+                res = cqe->res;
+                if (ret) {
+                        fprintf(stderr, "peek failed: %d\n", ret);
+                        return ret;
+                }
+                io_uring_cqe_seen(ring, cqe);
+                if (res != BUF_SIZE)
+                        fprintf(stderr, "bad result %d, user_data %llx\n",
+					res, cqe->user_data);
+		//printf("cqe %lld res %d\n", cqe->user_data, cqe->res);
+        }
+
+	return 0;
+}
+
+static int prep_test(struct io_uring *ring, struct test_data *data)
+{
+	int ret, i;
+
+	data->fd_in = open(data->src_file, O_RDONLY | O_DIRECT, 0644);
+	if (data->fd_in < 0) {
+		perror("open in");
+		return 1;
+	}
+
+	data->fd_out1 = open(".test_group_cp1", O_RDWR | O_CREAT | O_DIRECT, 0644);
+	unlink(".test_group_cp1");
+
+	data->fd_out2 = open(".test_group_cp2", O_RDWR | O_CREAT| O_DIRECT, 0644);
+	unlink(".test_group_cp2");
+
+	if (data->fd_out1 < 0 || data->fd_out2 < 0) {
+		perror("open out");
+		return 1;
+	}
+
+	for (i = 0; i < BUFFERS; i++) {
+		void *buf;
+
+		assert(!posix_memalign(&buf, 4096, BUF_SIZE));
+		data->iov[i].iov_base = buf;
+		data->iov[i].iov_len = BUF_SIZE;
+		memset(data->iov[i].iov_base, 0, BUF_SIZE);
+	}
+
+	ret = io_uring_register_buffers(ring, data->iov, BUFFERS);
+	if (ret) {
+		fprintf(stderr, "Error registering buffers: %s",
+				strerror(-ret));
+		return 1;
+	}
+
+	return 0;
+}
+
+static void unprep_test(struct io_uring *ring, struct test_data *d)
+{
+	io_uring_unregister_buffers(ring);
+	close(d->fd_in);
+	close(d->fd_out1);
+	close(d->fd_out2);
+}
+
+static unsigned build_group_sqes(struct io_uring *ring, struct test_data *d,
+	off_t off, int buf_idx, __u8 lead_flags, __u8 mem_flags)
+{
+	struct io_uring_sqe *sqe, *sqe2, *sqe1;
+
+	sqe = io_uring_get_sqe(ring);
+	sqe1 = io_uring_get_sqe(ring);
+	sqe2 = io_uring_get_sqe(ring);
+	assert(sqe && sqe1 && sqe2);
+
+	io_uring_prep_read_fixed(sqe, d->fd_in, d->iov[buf_idx].iov_base,
+			BUF_SIZE, 0, buf_idx);
+	set_sqe_group(sqe);
+	sqe->user_data = buf_idx + 1;
+	sqe->flags |= lead_flags;
+
+	io_uring_prep_write_fixed(sqe1, d->fd_out1, d->iov[buf_idx].iov_base,
+			BUF_SIZE, off, buf_idx);
+	set_sqe_group(sqe1);
+	sqe1->user_data = buf_idx + 2;
+	sqe1->flags |= mem_flags;
+
+	io_uring_prep_write_fixed(sqe2, d->fd_out2, d->iov[buf_idx].iov_base,
+			BUF_SIZE, off, buf_idx);
+	sqe2->user_data = buf_idx + 3;
+	sqe2->flags |= mem_flags;
+
+	return 3;
+}
+
+static int verify_cp(struct io_uring *ring, struct test_data *d, off_t off,
+		unsigned int buf_idx)
+{
+	struct io_uring_sqe *sqe2, *sqe1;
+	int ret;
+
+	sqe1 = io_uring_get_sqe(ring);
+	sqe2 = io_uring_get_sqe(ring);
+	assert(sqe1 && sqe2);
+
+	io_uring_prep_read_fixed(sqe1, d->fd_out1, d->iov[buf_idx + 1].iov_base,
+			BUF_SIZE, off, buf_idx + 1);
+	sqe1->user_data = buf_idx + 7;
+	io_uring_prep_read_fixed(sqe2, d->fd_out2, d->iov[buf_idx + 2].iov_base,
+			BUF_SIZE, off, buf_idx + 2);
+	sqe1->user_data = buf_idx + 8;
+	ret = io_uring_submit_and_wait(ring, 2);
+	if (ret < 0) {
+                fprintf(stderr, "submit failed: %d\n", ret);
+                return 1;
+        }
+
+	ret = check_cqe(ring, 2);
+	if (ret)
+		return ret;
+
+	if (memcmp(d->iov[buf_idx].iov_base, d->iov[buf_idx + 1].iov_base, BUF_SIZE)) {
+		fprintf(stderr, "data not match for destination 1\n");
+		return 1;
+	}
+
+	if (memcmp(d->iov[buf_idx].iov_base, d->iov[buf_idx + 2].iov_base, BUF_SIZE)) {
+		fprintf(stderr, "data not match for destination 2\n");
+		return 1;
+	}
+	return 0;
+}
+
+static int test(struct io_uring *ring, struct test_data *d,
+		__u8 lead_flags, __u8 mem_flags)
+{
+	unsigned test_link = lead_flags & IOSQE_IO_LINK;
+	unsigned nr;
+	int ret;
+
+	if (!test_link) {
+		nr = build_group_sqes(ring, d, 0, 0, lead_flags, mem_flags);
+	} else {
+		/* two groups linked together */
+		nr = build_group_sqes(ring, d, 0, 0, lead_flags, mem_flags);
+		nr += build_group_sqes(ring, d, BUF_SIZE, 3, lead_flags,
+				mem_flags);
+	}
+
+	ret = io_uring_submit_and_wait(ring, nr);
+	if (ret < 0) {
+                fprintf(stderr, "submit failed: %d\n", ret);
+                return 1;
+        }
+
+	if (check_cqe(ring, nr))
+                return 1;
+
+	ret = verify_cp(ring, d, 0, 0);
+	if (ret)
+		return ret;
+	if (test_link)
+		return verify_cp(ring, d, BUF_SIZE, 3);
+	return 0;
+}
+
+static int run_test(struct io_uring *ring, struct test_data *d,
+		__u8 lead_flags, __u8 mem_flags)
+{
+	int ret = test(ring, d, lead_flags, mem_flags);
+	if (ret) {
+		fprintf(stderr, "Test failed lead flags %x mem flags %x\n",
+				lead_flags, mem_flags);
+		return T_EXIT_FAIL;
+	}
+
+	exit(0);
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	struct io_uring ring;
+	struct test_data data = {
+		.src_file = argv[0],
+	};
+	int ret;
+	int g_link, g_async, m_async;
+
+	if (argc > 1)
+		return T_EXIT_SKIP;
+
+	ret = t_create_ring(16, &ring, 0);
+	if (ret == T_SETUP_SKIP)
+		return T_EXIT_SKIP;
+	else if (ret < 0)
+		return T_EXIT_FAIL;
+
+	ret = prep_test(&ring, &data);
+	if (ret) {
+		fprintf(stderr, "Prepare Test failed\n");
+		return T_EXIT_FAIL;
+	}
+
+	for (g_async = 0; g_async < 2; g_async += 1)
+		for (g_link = 0; g_link < 2; g_link += 1)
+			for (m_async = 0; m_async < 2; m_async += 1) {
+				__u8 g_flags = (g_async ? IOSQE_ASYNC : 0) |
+					(g_link ? IOSQE_IO_LINK : 0);
+				__u8 m_flags = (g_async ? IOSQE_ASYNC : 0);
+
+				if (run_test(&ring, &data, g_flags, m_flags))
+					return T_EXIT_FAIL;
+			}
+	unprep_test(&ring, &data);
+
+	io_uring_queue_exit(&ring);
+	return T_EXIT_PASS;
+}
-- 
2.41.0

Re: [RFC PATCH 0/9] io_uring: support sqe group and provide group kbuf

[Ming Lei]

On Mon, Apr 08, 2024 at 09:03:13AM +0800, Ming Lei wrote:
> Hello,
> 
> This patch adds sqe user ext flags, generic sqe group usage, and
> provide group kbuf based on sqe group. sqe group provides one efficient
> way to share resource among one group of sqes, such as, it can be for
> implementing multiple copying(copy data from single source to multiple
> destinations) via single syscall.
> 
> Finally implements provide group kbuf for uring command, and ublk use this
> for supporting zero copy, and actually this feature can be used to support
> generic device zero copy.
> 
> The last liburing patch adds helpers for using sqe group, also adds
> tests for sqe group. 
> 
> ublksrv userspace implements zero copy by sqe group & provide group
> kbuf:
> 
> 	https://github.com/ublk-org/ublksrv/commits/group-provide-buf/
> 	git clone https://github.com/ublk-org/ublksrv.git -b group-provide-buf
> 
> 	make test T=loop/009:nbd/061:nbd/062	#ublk zc tests
> 
> Any comments are welcome!

Hello Jens and Guys,

Any comments on this patchset?



thanks,
Ming