* [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf
@ 2024-08-08 16:24 Ming Lei
2024-08-08 16:24 ` [PATCH V5 1/8] io_uring: add io_link_req() helper Ming Lei
` (8 more replies)
0 siblings, 9 replies; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: Ming Lei
Hello,
The 1st 3 patches are cleanup, and prepare for adding sqe group.
The 4th patch supports generic sqe group which is like link chain, but
allows each sqe in group to be issued in parallel and the group shares
same IO_LINK & IO_DRAIN boundary, so N:M dependency can be supported with
sqe group & io link together. sqe group changes nothing on
IOSQE_IO_LINK.
The 5th patch supports one variant of sqe group: allow members to depend
on group leader, so that kernel resource lifetime can be aligned with
group leader or group, then any kernel resource can be shared in this
sqe group, and can be used in generic device zero copy.
The 6th & 7th patches supports providing sqe group buffer via the sqe
group variant.
The 8th patch supports ublk zero copy based on io_uring providing sqe
group buffer.
Tests:
1) pass liburing test
- make runtests
2) write/pass two sqe group test cases:
https://github.com/axboe/liburing/compare/master...ming1:liburing:sqe_group_v2
- covers related sqe flags combination and linking groups, both nop and
one multi-destination file copy.
- cover failure handling test: fail leader IO or member IO in both single
group and linked groups, which is done in each sqe flags combination
test
3) ublksrv zero copy:
ublksrv userspace implements zero copy by sqe group & provide group
kbuf:
git clone https://github.com/ublk-org/ublksrv.git -b group-provide-buf_v2
make test T=loop/009:nbd/061 #ublk zc tests
When running 64KB/512KB block size test on ublk-loop('ublk add -t loop --buffered_io -f $backing'),
it is observed that perf is doubled.
Any comments are welcome!
V5:
- follow Pavel's suggestion to minimize change on io_uring fast code
path: sqe group code is called in by single 'if (unlikely())' from
both issue & completion code path
- simplify & re-write group request completion
avoid to touch io-wq code by completing group leader via tw
directly, just like ->task_complete
re-write group member & leader completion handling, one
simplification is always to free leader via the last member
simplify queueing group members, not support issuing leader
and members in parallel
- fail the whole group if IO_*LINK & IO_DRAIN is set on group
members, and test code to cover this change
- misc cleanup
V4:
- address most comments from Pavel
- fix request double free
- don't use io_req_commit_cqe() in io_req_complete_defer()
- make members' REQ_F_INFLIGHT discoverable
- use common assembling check in submission code path
- drop patch 3 and don't move REQ_F_CQE_SKIP out of io_free_req()
- don't set .accept_group_kbuf for net send zc, in which members
need to be queued after buffer notification is got, and can be
enabled in future
- add .grp_leader field via union, and share storage with .grp_link
- move .grp_refs into one hole of io_kiocb, so that one extra
cacheline isn't needed for io_kiocb
- cleanup & document improvement
V3:
- add IORING_FEAT_SQE_GROUP
- simplify group completion, and minimize change on io_req_complete_defer()
- simplify & cleanup io_queue_group_members()
- fix many failure handling issues
- cover failure handling code in added liburing tests
- remove RFC
V2:
- add generic sqe group, suggested by Kevin Wolf
- add REQ_F_SQE_GROUP_DEP which is based on IOSQE_SQE_GROUP, for sharing
kernel resource in group wide, suggested by Kevin Wolf
- remove sqe ext flag, and use the last bit for IOSQE_SQE_GROUP(Pavel),
in future we still can extend sqe flags with one uring context flag
- initialize group requests via submit state pattern, suggested by Pavel
- all kinds of cleanup & bug fixes
Ming Lei (8):
io_uring: add io_link_req() helper
io_uring: add io_submit_fail_link() helper
io_uring: add helper of io_req_commit_cqe()
io_uring: support SQE group
io_uring: support sqe group with members depending on leader
io_uring: support providing sqe group buffer
io_uring/uring_cmd: support provide group kernel buffer
ublk: support provide io buffer
drivers/block/ublk_drv.c | 160 ++++++++++++++-
include/linux/io_uring/cmd.h | 7 +
include/linux/io_uring_types.h | 54 +++++
include/uapi/linux/io_uring.h | 11 +-
include/uapi/linux/ublk_cmd.h | 7 +-
io_uring/io_uring.c | 359 ++++++++++++++++++++++++++++++---
io_uring/io_uring.h | 16 ++
io_uring/kbuf.c | 60 ++++++
io_uring/kbuf.h | 13 ++
io_uring/net.c | 23 ++-
io_uring/opdef.c | 4 +
io_uring/opdef.h | 2 +
io_uring/rw.c | 20 +-
io_uring/timeout.c | 2 +
io_uring/uring_cmd.c | 28 +++
15 files changed, 720 insertions(+), 46 deletions(-)
--
2.42.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V5 1/8] io_uring: add io_link_req() helper
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
@ 2024-08-08 16:24 ` Ming Lei
2024-08-08 16:24 ` [PATCH V5 2/8] io_uring: add io_submit_fail_link() helper Ming Lei
` (7 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: Ming Lei
Add io_link_req() helper, so that io_submit_sqe() becomes more readable.
Signed-off-by: Ming Lei <[email protected]>
---
io_uring/io_uring.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 3942db160f18..4848cd84af3d 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2132,19 +2132,11 @@ static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
return 0;
}
-static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
- const struct io_uring_sqe *sqe)
- __must_hold(&ctx->uring_lock)
+/*
+ * Return NULL if nothing to be queued, otherwise return request for queueing */
+static struct io_kiocb *io_link_sqe(struct io_submit_link *link,
+ struct io_kiocb *req)
{
- struct io_submit_link *link = &ctx->submit_state.link;
- int ret;
-
- ret = io_init_req(ctx, req, sqe);
- if (unlikely(ret))
- return io_submit_fail_init(sqe, req, ret);
-
- trace_io_uring_submit_req(req);
-
/*
* If we already have a head request, queue this one for async
* submittal once the head completes. If we don't have a head but
@@ -2158,7 +2150,7 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
link->last = req;
if (req->flags & IO_REQ_LINK_FLAGS)
- return 0;
+ return NULL;
/* last request of the link, flush it */
req = link->head;
link->head = NULL;
@@ -2174,9 +2166,30 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
fallback:
io_queue_sqe_fallback(req);
}
- return 0;
+ return NULL;
}
+ return req;
+}
+
+static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
+ const struct io_uring_sqe *sqe)
+ __must_hold(&ctx->uring_lock)
+{
+ struct io_submit_link *link = &ctx->submit_state.link;
+ int ret;
+ ret = io_init_req(ctx, req, sqe);
+ if (unlikely(ret))
+ return io_submit_fail_init(sqe, req, ret);
+
+ trace_io_uring_submit_req(req);
+
+ if (unlikely(link->head || (req->flags & (IO_REQ_LINK_FLAGS |
+ REQ_F_FORCE_ASYNC | REQ_F_FAIL)))) {
+ req = io_link_sqe(link, req);
+ if (!req)
+ return 0;
+ }
io_queue_sqe(req);
return 0;
}
--
2.42.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V5 2/8] io_uring: add io_submit_fail_link() helper
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
2024-08-08 16:24 ` [PATCH V5 1/8] io_uring: add io_link_req() helper Ming Lei
@ 2024-08-08 16:24 ` Ming Lei
2024-08-08 16:24 ` [PATCH V5 3/8] io_uring: add helper of io_req_commit_cqe() Ming Lei
` (6 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: Ming Lei
Add io_submit_fail_link() helper and put linking fail logic into this
helper.
This way simplifies io_submit_fail_init(), and becomes easier to add
sqe group failing logic.
Signed-off-by: Ming Lei <[email protected]>
---
io_uring/io_uring.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 4848cd84af3d..34020cadee03 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2095,22 +2095,17 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
return def->prep(req, sqe);
}
-static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
+static __cold int io_submit_fail_link(struct io_submit_link *link,
struct io_kiocb *req, int ret)
{
- struct io_ring_ctx *ctx = req->ctx;
- struct io_submit_link *link = &ctx->submit_state.link;
struct io_kiocb *head = link->head;
- trace_io_uring_req_failed(sqe, req, ret);
-
/*
* Avoid breaking links in the middle as it renders links with SQPOLL
* unusable. Instead of failing eagerly, continue assembling the link if
* applicable and mark the head with REQ_F_FAIL. The link flushing code
* should find the flag and handle the rest.
*/
- req_fail_link_node(req, ret);
if (head && !(head->flags & REQ_F_FAIL))
req_fail_link_node(head, -ECANCELED);
@@ -2129,9 +2124,24 @@ static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
else
link->head = req;
link->last = req;
+
return 0;
}
+static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
+ struct io_kiocb *req, int ret)
+{
+ struct io_ring_ctx *ctx = req->ctx;
+ struct io_submit_link *link = &ctx->submit_state.link;
+
+ trace_io_uring_req_failed(sqe, req, ret);
+
+ req_fail_link_node(req, ret);
+
+ /* cover both linked and non-linked request */
+ return io_submit_fail_link(link, req, ret);
+}
+
/*
* Return NULL if nothing to be queued, otherwise return request for queueing */
static struct io_kiocb *io_link_sqe(struct io_submit_link *link,
--
2.42.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V5 3/8] io_uring: add helper of io_req_commit_cqe()
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
2024-08-08 16:24 ` [PATCH V5 1/8] io_uring: add io_link_req() helper Ming Lei
2024-08-08 16:24 ` [PATCH V5 2/8] io_uring: add io_submit_fail_link() helper Ming Lei
@ 2024-08-08 16:24 ` Ming Lei
2024-08-08 16:24 ` [PATCH V5 4/8] io_uring: support SQE group Ming Lei
` (5 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: Ming Lei
Add helper of io_req_commit_cqe() for simplifying
__io_submit_flush_completions() a bit.
No functional change, and the added helper will be reused in sqe group
code with same lock rule.
Signed-off-by: Ming Lei <[email protected]>
---
io_uring/io_uring.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 34020cadee03..f112e9fa90d8 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -861,6 +861,20 @@ bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags)
return posted;
}
+static __always_inline void io_req_commit_cqe(struct io_ring_ctx *ctx,
+ struct io_kiocb *req)
+{
+ if (unlikely(!io_fill_cqe_req(ctx, req))) {
+ if (ctx->lockless_cq) {
+ spin_lock(&ctx->completion_lock);
+ io_req_cqe_overflow(req);
+ spin_unlock(&ctx->completion_lock);
+ } else {
+ io_req_cqe_overflow(req);
+ }
+ }
+}
+
static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
{
struct io_ring_ctx *ctx = req->ctx;
@@ -1413,16 +1427,8 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx)
struct io_kiocb *req = container_of(node, struct io_kiocb,
comp_list);
- if (!(req->flags & REQ_F_CQE_SKIP) &&
- unlikely(!io_fill_cqe_req(ctx, req))) {
- if (ctx->lockless_cq) {
- spin_lock(&ctx->completion_lock);
- io_req_cqe_overflow(req);
- spin_unlock(&ctx->completion_lock);
- } else {
- io_req_cqe_overflow(req);
- }
- }
+ if (!(req->flags & REQ_F_CQE_SKIP))
+ io_req_commit_cqe(ctx, req);
}
__io_cq_unlock_post(ctx);
--
2.42.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V5 4/8] io_uring: support SQE group
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
` (2 preceding siblings ...)
2024-08-08 16:24 ` [PATCH V5 3/8] io_uring: add helper of io_req_commit_cqe() Ming Lei
@ 2024-08-08 16:24 ` Ming Lei
2024-08-27 15:18 ` Pavel Begunkov
2024-08-08 16:24 ` [PATCH V5 5/8] io_uring: support sqe group with members depending on leader Ming Lei
` (4 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: Ming Lei, Kevin Wolf
SQE group is defined as one chain of SQEs starting with the first SQE that
has IOSQE_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.
Not like linked SQEs, each sqe is issued after the previous one is
completed. All SQEs in one group can be submitted in parallel. To simplify
the implementation from beginning, all members are queued after the leader
is completed, however, this way may be changed and leader and members may
be issued concurrently in future.
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. Failure from any group member will
fail the group leader, then the link chain can be terminated.
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 by always completing group leader as
the last one in the group. Meantime it is natural to post leader's CQE
as the last one from application viewpoint.
Working together with IOSQE_IO_LINK, SQE group provides flexible way to
support N:M dependency, such as:
- group A is chained with group B together
- group A has N SQEs
- group B has M SQEs
then M SQEs in group B depend on N SQEs in group A.
N:M dependency can support some interesting use cases in efficient way:
1) read from multiple files, then write the read data into single file
2) read from single file, and write the read data into multiple files
3) write same data into multiple files, and read data from multiple files and
compare if correct data is written
Also IOSQE_SQE_GROUP takes the last bit in sqe->flags, but we still can
extend sqe->flags with one uring context flag, such as use __pad3 for
non-uring_cmd OPs and part of uring_cmd_flags for uring_cmd OP.
Suggested-by: Kevin Wolf <[email protected]>
Signed-off-by: Ming Lei <[email protected]>
---
include/linux/io_uring_types.h | 18 +++
include/uapi/linux/io_uring.h | 4 +
io_uring/io_uring.c | 282 +++++++++++++++++++++++++++++++--
io_uring/io_uring.h | 6 +
io_uring/timeout.c | 2 +
5 files changed, 297 insertions(+), 15 deletions(-)
diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index 3315005df117..c5250e585289 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -202,6 +202,8 @@ struct io_submit_state {
/* batch completion logic */
struct io_wq_work_list compl_reqs;
struct io_submit_link link;
+ /* points to current group */
+ struct io_submit_link group;
bool plug_started;
bool need_plug;
@@ -435,6 +437,7 @@ 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_GROUP_BIT = IOSQE_SQE_GROUP_BIT,
/* first byte is taken by user flags, shift it to not overlap */
REQ_F_FAIL_BIT = 8,
@@ -465,6 +468,7 @@ enum {
REQ_F_BL_EMPTY_BIT,
REQ_F_BL_NO_RECYCLE_BIT,
REQ_F_BUFFERS_COMMIT_BIT,
+ REQ_F_SQE_GROUP_LEADER_BIT,
/* not a real bit, just to check we're not overflowing the space */
__REQ_F_LAST_BIT,
@@ -488,6 +492,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_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),
@@ -543,6 +549,8 @@ enum {
REQ_F_BL_NO_RECYCLE = IO_REQ_FLAG(REQ_F_BL_NO_RECYCLE_BIT),
/* buffer ring head needs incrementing on put */
REQ_F_BUFFERS_COMMIT = IO_REQ_FLAG(REQ_F_BUFFERS_COMMIT_BIT),
+ /* sqe group lead */
+ REQ_F_SQE_GROUP_LEADER = IO_REQ_FLAG(REQ_F_SQE_GROUP_LEADER_BIT),
};
typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts);
@@ -648,6 +656,8 @@ struct io_kiocb {
void *async_data;
/* linked requests, IFF REQ_F_HARDLINK or REQ_F_LINK are set */
atomic_t poll_refs;
+ /* reference for group leader requests */
+ int grp_refs;
struct io_kiocb *link;
/* custom credentials, valid IFF REQ_F_CREDS is set */
const struct cred *creds;
@@ -657,6 +667,14 @@ struct io_kiocb {
u64 extra1;
u64 extra2;
} big_cqe;
+
+ union {
+ /* links all group members for leader */
+ struct io_kiocb *grp_link;
+
+ /* points to group leader for member */
+ struct io_kiocb *grp_leader;
+ };
};
struct io_overflow_cqe {
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 2aaf7ee256ac..e6d321b3add7 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -124,6 +124,7 @@ enum io_uring_sqe_flags_bit {
IOSQE_ASYNC_BIT,
IOSQE_BUFFER_SELECT_BIT,
IOSQE_CQE_SKIP_SUCCESS_BIT,
+ IOSQE_SQE_GROUP_BIT,
};
/*
@@ -143,6 +144,8 @@ 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)
+/* defines sqe group */
+#define IOSQE_SQE_GROUP (1U << IOSQE_SQE_GROUP_BIT)
/*
* io_uring_setup() flags
@@ -542,6 +545,7 @@ struct io_uring_params {
#define IORING_FEAT_LINKED_FILE (1U << 12)
#define IORING_FEAT_REG_REG_RING (1U << 13)
#define IORING_FEAT_RECVSEND_BUNDLE (1U << 14)
+#define IORING_FEAT_SQE_GROUP (1U << 15)
/*
* io_uring_register(2) opcodes and arguments
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index f112e9fa90d8..45a292445b18 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -111,13 +111,15 @@
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_SQE_GROUP)
#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)
#define IO_REQ_CLEAN_SLOW_FLAGS (REQ_F_REFCOUNT | REQ_F_LINK | REQ_F_HARDLINK |\
+ REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER | \
IO_REQ_CLEAN_FLAGS)
#define IO_TCTX_REFS_CACHE_NR (1U << 10)
@@ -875,6 +877,116 @@ static __always_inline void io_req_commit_cqe(struct io_ring_ctx *ctx,
}
}
+/* Can only be called after this request is issued */
+static inline struct io_kiocb *get_group_leader(struct io_kiocb *req)
+{
+ if (req->flags & REQ_F_SQE_GROUP) {
+ if (req_is_group_leader(req))
+ return req;
+ return req->grp_leader;
+ }
+ return NULL;
+}
+
+void io_cancel_group_members(struct io_kiocb *req, bool ignore_cqes)
+{
+ struct io_kiocb *member = req->grp_link;
+
+ while (member) {
+ struct io_kiocb *next = member->grp_link;
+
+ if (ignore_cqes)
+ member->flags |= REQ_F_CQE_SKIP;
+ if (!(member->flags & REQ_F_FAIL)) {
+ req_set_fail(member);
+ io_req_set_res(member, -ECANCELED, 0);
+ }
+ member = next;
+ }
+}
+
+static void io_queue_group_members(struct io_kiocb *req)
+{
+ struct io_kiocb *member = req->grp_link;
+
+ if (!member)
+ return;
+
+ req->grp_link = NULL;
+ while (member) {
+ struct io_kiocb *next = member->grp_link;
+
+ member->grp_leader = req;
+ if (unlikely(member->flags & REQ_F_FAIL)) {
+ io_req_task_queue_fail(member, member->cqe.res);
+ } else if (unlikely(req->flags & REQ_F_FAIL)) {
+ io_req_task_queue_fail(member, -ECANCELED);
+ } else {
+ io_req_task_queue(member);
+ }
+ member = next;
+ }
+}
+
+static inline bool __io_complete_group_member(struct io_kiocb *req,
+ struct io_kiocb *lead)
+{
+ if (WARN_ON_ONCE(lead->grp_refs <= 0))
+ return false;
+
+ req->flags &= ~REQ_F_SQE_GROUP;
+ /*
+ * Set linked leader as failed if any member is failed, so
+ * the remained link chain can be terminated
+ */
+ if (unlikely((req->flags & REQ_F_FAIL) &&
+ ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)))
+ req_set_fail(lead);
+ return !--lead->grp_refs;
+}
+
+static inline bool leader_is_the_last(struct io_kiocb *lead)
+{
+ return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP);
+}
+
+static void io_complete_group_member(struct io_kiocb *req)
+{
+ struct io_kiocb *lead = get_group_leader(req);
+
+ if (WARN_ON_ONCE(!(req->flags & REQ_F_SQE_GROUP)))
+ return;
+
+ /* member CQE needs to be posted first */
+ if (!(req->flags & REQ_F_CQE_SKIP))
+ io_req_commit_cqe(req->ctx, req);
+
+ if (__io_complete_group_member(req, lead)) {
+ /*
+ * SQE_GROUP flag is kept for the last member, so the leader
+ * can be retrieved & freed from this last member
+ */
+ req->flags |= REQ_F_SQE_GROUP;
+ if (!(lead->flags & REQ_F_CQE_SKIP))
+ io_req_commit_cqe(lead->ctx, lead);
+ } else if (leader_is_the_last(lead)) {
+ /* leader will degenerate to plain req if it is the last */
+ lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER);
+ }
+}
+
+static void io_complete_group_leader(struct io_kiocb *req)
+{
+ WARN_ON_ONCE(req->grp_refs <= 0);
+ req->flags &= ~REQ_F_SQE_GROUP;
+ req->grp_refs -= 1;
+ WARN_ON_ONCE(req->grp_refs == 0);
+
+ /* TODO: queue members with leader in parallel */
+ if (req->grp_link)
+ io_queue_group_members(req);
+}
+
static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
{
struct io_ring_ctx *ctx = req->ctx;
@@ -890,7 +1002,8 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
* Handle special CQ sync cases via task_work. DEFER_TASKRUN requires
* the submitter task context, IOPOLL protects with uring_lock.
*/
- if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL)) {
+ if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL) ||
+ req_is_group_leader(req)) {
req->io_task_work.func = io_req_task_complete;
io_req_task_work_add(req);
return;
@@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx,
comp_list);
if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) {
+ if (req->flags & (REQ_F_SQE_GROUP |
+ REQ_F_SQE_GROUP_LEADER)) {
+ struct io_kiocb *leader;
+
+ /* Leader is freed via the last member */
+ if (req_is_group_leader(req)) {
+ node = req->comp_list.next;
+ continue;
+ }
+
+ /*
+ * Only the last member keeps GROUP flag,
+ * free leader and this member together
+ */
+ leader = get_group_leader(req);
+ leader->flags &= ~REQ_F_SQE_GROUP_LEADER;
+ req->flags &= ~REQ_F_SQE_GROUP;
+ wq_stack_add_head(&leader->comp_list,
+ &req->comp_list);
+ }
+
if (req->flags & REQ_F_REFCOUNT) {
node = req->comp_list.next;
if (!req_ref_put_and_test(req))
continue;
}
+
if ((req->flags & REQ_F_POLLED) && req->apoll) {
struct async_poll *apoll = req->apoll;
@@ -1427,8 +1562,19 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx)
struct io_kiocb *req = container_of(node, struct io_kiocb,
comp_list);
- if (!(req->flags & REQ_F_CQE_SKIP))
- io_req_commit_cqe(ctx, req);
+ if (unlikely(req->flags & (REQ_F_CQE_SKIP | REQ_F_SQE_GROUP))) {
+ if (req->flags & REQ_F_SQE_GROUP) {
+ if (req_is_group_leader(req))
+ io_complete_group_leader(req);
+ else
+ io_complete_group_member(req);
+ continue;
+ }
+
+ if (req->flags & REQ_F_CQE_SKIP)
+ continue;
+ }
+ io_req_commit_cqe(ctx, req);
}
__io_cq_unlock_post(ctx);
@@ -1638,8 +1784,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_leader(cur))
+ seq -= cur->grp_refs;
+ else
+ seq--;
+ }
return seq;
}
@@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
return def->prep(req, sqe);
}
+static struct io_kiocb *io_group_sqe(struct io_submit_link *group,
+ struct io_kiocb *req)
+{
+ /*
+ * Group chain is similar with link chain: starts with 1st sqe with
+ * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP
+ */
+ if (group->head) {
+ struct io_kiocb *lead = group->head;
+
+ /* members can't be in link chain, can't be drained */
+ if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN))
+ req_fail_link_node(lead, -EINVAL);
+
+ lead->grp_refs += 1;
+ group->last->grp_link = req;
+ group->last = req;
+
+ if (req->flags & REQ_F_SQE_GROUP)
+ return NULL;
+
+ req->grp_link = NULL;
+ req->flags |= REQ_F_SQE_GROUP;
+ group->head = NULL;
+ if (lead->flags & REQ_F_FAIL) {
+ io_queue_sqe_fallback(lead);
+ return NULL;
+ }
+ return lead;
+ } else if (req->flags & REQ_F_SQE_GROUP) {
+ group->head = req;
+ group->last = req;
+ req->grp_refs = 1;
+ req->flags |= REQ_F_SQE_GROUP_LEADER;
+ return NULL;
+ } else {
+ return req;
+ }
+}
+
+static __cold struct io_kiocb *io_submit_fail_group(
+ struct io_submit_link *link, struct io_kiocb *req)
+{
+ struct io_kiocb *lead = link->head;
+
+ /*
+ * Instead of failing eagerly, continue assembling the group link
+ * if applicable and mark the leader with REQ_F_FAIL. The group
+ * flushing code should find the flag and handle the rest
+ */
+ if (lead && (lead->flags & IO_REQ_LINK_FLAGS) && !(lead->flags & REQ_F_FAIL))
+ req_fail_link_node(lead, -ECANCELED);
+
+ return io_group_sqe(link, req);
+}
+
static __cold int io_submit_fail_link(struct io_submit_link *link,
struct io_kiocb *req, int ret)
{
@@ -2139,11 +2345,18 @@ static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
{
struct io_ring_ctx *ctx = req->ctx;
struct io_submit_link *link = &ctx->submit_state.link;
+ struct io_submit_link *group = &ctx->submit_state.group;
trace_io_uring_req_failed(sqe, req, ret);
req_fail_link_node(req, ret);
+ if (group->head || (req->flags & REQ_F_SQE_GROUP)) {
+ req = io_submit_fail_group(group, req);
+ if (!req)
+ return 0;
+ }
+
/* cover both linked and non-linked request */
return io_submit_fail_link(link, req, ret);
}
@@ -2187,11 +2400,28 @@ static struct io_kiocb *io_link_sqe(struct io_submit_link *link,
return req;
}
+static inline bool io_group_assembling(const struct io_submit_state *state,
+ const struct io_kiocb *req)
+{
+ if (state->group.head || req->flags & REQ_F_SQE_GROUP)
+ return true;
+ return false;
+}
+
+static inline bool io_link_assembling(const struct io_submit_state *state,
+ const struct io_kiocb *req)
+{
+ if (state->link.head || (req->flags & (IO_REQ_LINK_FLAGS |
+ REQ_F_FORCE_ASYNC | REQ_F_FAIL)))
+ return true;
+ return false;
+}
+
static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
const struct io_uring_sqe *sqe)
__must_hold(&ctx->uring_lock)
{
- struct io_submit_link *link = &ctx->submit_state.link;
+ struct io_submit_state *state = &ctx->submit_state;
int ret;
ret = io_init_req(ctx, req, sqe);
@@ -2200,11 +2430,18 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
trace_io_uring_submit_req(req);
- if (unlikely(link->head || (req->flags & (IO_REQ_LINK_FLAGS |
- REQ_F_FORCE_ASYNC | REQ_F_FAIL)))) {
- req = io_link_sqe(link, req);
- if (!req)
- return 0;
+ if (unlikely(io_link_assembling(state, req) ||
+ io_group_assembling(state, req))) {
+ if (io_group_assembling(state, req)) {
+ req = io_group_sqe(&state->group, req);
+ if (!req)
+ return 0;
+ }
+ if (io_link_assembling(state, req)) {
+ req = io_link_sqe(&state->link, req);
+ if (!req)
+ return 0;
+ }
}
io_queue_sqe(req);
return 0;
@@ -2217,8 +2454,22 @@ static void io_submit_state_end(struct io_ring_ctx *ctx)
{
struct io_submit_state *state = &ctx->submit_state;
- if (unlikely(state->link.head))
- io_queue_sqe_fallback(state->link.head);
+ if (unlikely(state->group.head || state->link.head)) {
+ /* the last member must set REQ_F_SQE_GROUP */
+ if (state->group.head) {
+ struct io_kiocb *lead = state->group.head;
+
+ state->group.last->grp_link = NULL;
+ if (lead->flags & IO_REQ_LINK_FLAGS)
+ io_link_sqe(&state->link, lead);
+ else
+ io_queue_sqe_fallback(lead);
+ }
+
+ if (unlikely(state->link.head))
+ io_queue_sqe_fallback(state->link.head);
+ }
+
/* flush only after queuing links as they can generate completions */
io_submit_flush_completions(ctx);
if (state->plug_started)
@@ -2236,6 +2487,7 @@ static void io_submit_state_start(struct io_submit_state *state,
state->submit_nr = max_ios;
/* set only head, no need to init link_last in advance */
state->link.head = NULL;
+ state->group.head = NULL;
}
static void io_commit_sqring(struct io_ring_ctx *ctx)
@@ -3564,7 +3816,7 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
IORING_FEAT_EXT_ARG | IORING_FEAT_NATIVE_WORKERS |
IORING_FEAT_RSRC_TAGS | IORING_FEAT_CQE_SKIP |
IORING_FEAT_LINKED_FILE | IORING_FEAT_REG_REG_RING |
- IORING_FEAT_RECVSEND_BUNDLE;
+ IORING_FEAT_RECVSEND_BUNDLE | IORING_FEAT_SQE_GROUP;
if (copy_to_user(params, p, sizeof(*p))) {
ret = -EFAULT;
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index c2acf6180845..0125daa8dfe7 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -68,6 +68,7 @@ bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags
void io_add_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags);
bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags);
void __io_commit_cqring_flush(struct io_ring_ctx *ctx);
+void io_cancel_group_members(struct io_kiocb *req, bool ignore_cqes);
struct file *io_file_get_normal(struct io_kiocb *req, int fd);
struct file *io_file_get_fixed(struct io_kiocb *req, int fd,
@@ -339,6 +340,11 @@ static inline void io_tw_lock(struct io_ring_ctx *ctx, struct io_tw_state *ts)
lockdep_assert_held(&ctx->uring_lock);
}
+static inline bool req_is_group_leader(struct io_kiocb *req)
+{
+ return req->flags & REQ_F_SQE_GROUP_LEADER;
+}
+
/*
* Don't complete immediately but use deferred completion infrastructure.
* Protected by ->uring_lock and can only be used either with
diff --git a/io_uring/timeout.c b/io_uring/timeout.c
index 9973876d91b0..671d6093bf36 100644
--- a/io_uring/timeout.c
+++ b/io_uring/timeout.c
@@ -168,6 +168,8 @@ static void io_fail_links(struct io_kiocb *req)
link->flags |= REQ_F_CQE_SKIP;
else
link->flags &= ~REQ_F_CQE_SKIP;
+ if (req_is_group_leader(link))
+ io_cancel_group_members(link, ignore_cqes);
trace_io_uring_fail_link(req, link);
link = link->link;
}
--
2.42.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V5 5/8] io_uring: support sqe group with members depending on leader
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
` (3 preceding siblings ...)
2024-08-08 16:24 ` [PATCH V5 4/8] io_uring: support SQE group Ming Lei
@ 2024-08-08 16:24 ` Ming Lei
2024-08-08 16:24 ` [PATCH V5 6/8] io_uring: support providing sqe group buffer Ming Lei
` (3 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: Ming Lei
IOSQE_SQE_GROUP just starts to queue members after the leader is completed,
which way is just for simplifying implementation, and this behavior is never
part of UAPI, and it may be relaxed and members can be queued concurrently
with leader in future.
However, some resource can't cross OPs, such as kernel buffer, otherwise
the buffer may be leaked easily in case that any OP failure or application
panic.
Add flag REQ_F_SQE_GROUP_DEP for allowing members to depend on group leader
explicitly, so that group members won't be queued until the leader request is
completed, and we still commit leader's CQE after all members CQE are posted.
With this way, the kernel resource lifetime can be aligned with group leader
or group, one typical use case is to support zero copy for device internal
buffer.
Signed-off-by: Ming Lei <[email protected]>
---
include/linux/io_uring_types.h | 3 +++
io_uring/io_uring.c | 8 +++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index c5250e585289..d0972e2a098f 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -469,6 +469,7 @@ enum {
REQ_F_BL_NO_RECYCLE_BIT,
REQ_F_BUFFERS_COMMIT_BIT,
REQ_F_SQE_GROUP_LEADER_BIT,
+ REQ_F_SQE_GROUP_DEP_BIT,
/* not a real bit, just to check we're not overflowing the space */
__REQ_F_LAST_BIT,
@@ -551,6 +552,8 @@ enum {
REQ_F_BUFFERS_COMMIT = IO_REQ_FLAG(REQ_F_BUFFERS_COMMIT_BIT),
/* sqe group lead */
REQ_F_SQE_GROUP_LEADER = IO_REQ_FLAG(REQ_F_SQE_GROUP_LEADER_BIT),
+ /* sqe group with members depending on leader */
+ REQ_F_SQE_GROUP_DEP = IO_REQ_FLAG(REQ_F_SQE_GROUP_DEP_BIT),
};
typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts);
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 45a292445b18..b4f5dac85fa4 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -982,7 +982,13 @@ static void io_complete_group_leader(struct io_kiocb *req)
req->grp_refs -= 1;
WARN_ON_ONCE(req->grp_refs == 0);
- /* TODO: queue members with leader in parallel */
+ /*
+ * TODO: queue members with leader in parallel
+ *
+ * So far, REQ_F_SQE_GROUP_DEP depends that members are queued
+ * after leader is completed, which may be changed in future,
+ * then REQ_F_SQE_GROUP_DEP has to be respected in another way.
+ */
if (req->grp_link)
io_queue_group_members(req);
}
--
2.42.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V5 6/8] io_uring: support providing sqe group buffer
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
` (4 preceding siblings ...)
2024-08-08 16:24 ` [PATCH V5 5/8] io_uring: support sqe group with members depending on leader Ming Lei
@ 2024-08-08 16:24 ` Ming Lei
2024-08-08 16:24 ` [PATCH V5 7/8] io_uring/uring_cmd: support provide group kernel buffer Ming Lei
` (2 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: Ming Lei
SQE group with REQ_F_SQE_GROUP_DEP introduces one new mechanism to share
resource among one group of requests, and all member requests can consume
the resource provided by group leader efficiently in parallel.
This patch uses the added sqe group feature REQ_F_SQE_GROUP_DEP to share
kernel buffer in sqe group:
- the group leader 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 consume it
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, but data can't be written to; 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 aligned with group leader lifetime, and
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 <[email protected]>
---
include/linux/io_uring_types.h | 33 +++++++++++++++++++
io_uring/io_uring.c | 10 +++++-
io_uring/io_uring.h | 10 ++++++
io_uring/kbuf.c | 60 ++++++++++++++++++++++++++++++++++
io_uring/kbuf.h | 13 ++++++++
io_uring/net.c | 23 ++++++++++++-
io_uring/opdef.c | 4 +++
io_uring/opdef.h | 2 ++
io_uring/rw.c | 20 +++++++++++-
9 files changed, 172 insertions(+), 3 deletions(-)
diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index d0972e2a098f..78196f0d5437 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,26 @@ enum io_uring_cmd_flags {
IO_URING_F_COMPAT = (1 << 12),
};
+struct io_uring_kernel_buf;
+typedef void (io_uring_buf_giveback_t) (const struct io_uring_kernel_buf *);
+
+/* buffer provided from kernel */
+struct io_uring_kernel_buf {
+ unsigned long len;
+ unsigned short nr_bvecs;
+ unsigned char dir; /* ITER_SOURCE or ITER_DEST */
+
+ /* offset in the 1st bvec */
+ unsigned int offset;
+ const struct bio_vec *bvec;
+
+ /* called when we are done with this buffer */
+ io_uring_buf_giveback_t *grp_kbuf_ack;
+
+ /* private field, user don't touch it */
+ struct bio_vec __bvec[];
+};
+
struct io_wq_work_node {
struct io_wq_work_node *next;
};
@@ -470,6 +491,7 @@ enum {
REQ_F_BUFFERS_COMMIT_BIT,
REQ_F_SQE_GROUP_LEADER_BIT,
REQ_F_SQE_GROUP_DEP_BIT,
+ REQ_F_GROUP_KBUF_BIT,
/* not a real bit, just to check we're not overflowing the space */
__REQ_F_LAST_BIT,
@@ -554,6 +576,8 @@ enum {
REQ_F_SQE_GROUP_LEADER = IO_REQ_FLAG(REQ_F_SQE_GROUP_LEADER_BIT),
/* sqe group with members depending on leader */
REQ_F_SQE_GROUP_DEP = IO_REQ_FLAG(REQ_F_SQE_GROUP_DEP_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);
@@ -637,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 {
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index b4f5dac85fa4..4b0ca320cde4 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -116,7 +116,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 |\
REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER | \
@@ -387,6 +387,11 @@ static bool req_need_defer(struct io_kiocb *req, u32 seq)
static void io_clean_op(struct io_kiocb *req)
{
+ /* GROUP_KBUF is only available for REQ_F_SQE_GROUP_DEP */
+ if ((req->flags & (REQ_F_GROUP_KBUF | REQ_F_SQE_GROUP_DEP)) ==
+ (REQ_F_GROUP_KBUF | REQ_F_SQE_GROUP_DEP))
+ io_group_kbuf_drop(req);
+
if (req->flags & REQ_F_BUFFER_SELECTED) {
spin_lock(&req->ctx->completion_lock);
io_kbuf_drop(req);
@@ -914,9 +919,12 @@ static void io_queue_group_members(struct io_kiocb *req)
req->grp_link = NULL;
while (member) {
+ const struct io_issue_def *def = &io_issue_defs[member->opcode];
struct io_kiocb *next = member->grp_link;
member->grp_leader = req;
+ if ((req->flags & REQ_F_GROUP_KBUF) && def->accept_group_kbuf)
+ member->flags |= REQ_F_GROUP_KBUF;
if (unlikely(member->flags & REQ_F_FAIL)) {
io_req_task_queue_fail(member, member->cqe.res);
} else if (unlikely(req->flags & REQ_F_FAIL)) {
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 0125daa8dfe7..f399e7b0dfce 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -345,6 +345,16 @@ static inline bool req_is_group_leader(struct io_kiocb *req)
return req->flags & REQ_F_SQE_GROUP_LEADER;
}
+static inline bool req_is_group_member(struct io_kiocb *req)
+{
+ return !req_is_group_leader(req) && (req->flags & REQ_F_SQE_GROUP);
+}
+
+static inline bool req_support_group_dep(struct io_kiocb *req)
+{
+ return req_is_group_leader(req) && (req->flags & REQ_F_SQE_GROUP_DEP);
+}
+
/*
* Don't complete immediately but use deferred completion infrastructure.
* Protected by ->uring_lock and can only be used either with
diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index c95dc1736dd9..24f09adf92d2 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -825,3 +825,63 @@ 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)
+{
+ if (unlikely(!req_support_group_dep(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->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_support_group_dep(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 b90aca3a57fa..2e1b7f91efb6 100644
--- a/io_uring/kbuf.h
+++ b/io_uring/kbuf.h
@@ -82,6 +82,11 @@ 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);
+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)
{
/*
@@ -180,4 +185,12 @@ static inline unsigned int io_put_kbufs(struct io_kiocb *req, int nbufs,
{
return __io_put_kbufs(req, nbufs, issue_flags);
}
+
+static inline void io_group_kbuf_drop(struct io_kiocb *req)
+{
+ const struct io_uring_kernel_buf *gbuf = req->grp_kbuf;
+
+ if (gbuf && gbuf->grp_kbuf_ack)
+ gbuf->grp_kbuf_ack(gbuf);
+}
#endif
diff --git a/io_uring/net.c b/io_uring/net.c
index 594490a1389b..313da5b0bb5d 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -89,6 +89,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);
@@ -375,7 +382,7 @@ static int io_send_setup(struct io_kiocb *req)
kmsg->msg.msg_name = &kmsg->addr;
kmsg->msg.msg_namelen = sr->addr_len;
}
- if (!io_do_buffer_select(req)) {
+ if (!io_do_buffer_select(req) && !(req->flags & REQ_F_GROUP_KBUF)) {
ret = import_ubuf(ITER_SOURCE, sr->buf, sr->len,
&kmsg->msg.msg_iter);
if (unlikely(ret < 0))
@@ -595,6 +602,15 @@ int io_send(struct io_kiocb *req, unsigned int issue_flags)
if (issue_flags & IO_URING_F_NONBLOCK)
flags |= MSG_DONTWAIT;
+ 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;
+ }
+
retry_bundle:
if (io_do_buffer_select(req)) {
struct buf_sel_arg arg = {
@@ -1144,6 +1160,11 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
goto out_free;
}
sr->buf = NULL;
+ } 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_flags = 0;
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index a2be3bbca5ff..c12f57619a33 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,
@@ -282,6 +284,7 @@ const struct io_issue_def io_issue_defs[] = {
.audit_skip = 1,
.ioprio = 1,
.buffer_select = 1,
+ .accept_group_kbuf = 1,
#if defined(CONFIG_NET)
.async_size = sizeof(struct io_async_msghdr),
.prep = io_sendmsg_prep,
@@ -297,6 +300,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,
diff --git a/io_uring/opdef.h b/io_uring/opdef.h
index 14456436ff74..328c8a3c4fa7 100644
--- a/io_uring/opdef.h
+++ b/io_uring/opdef.h
@@ -27,6 +27,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 c004d21e2f12..81ca22094e82 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;
@@ -620,11 +621,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
@@ -831,6 +837,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, READ);
if (unlikely(ret))
@@ -1012,6 +1023,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, WRITE);
if (unlikely(ret))
return ret;
--
2.42.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V5 7/8] io_uring/uring_cmd: support provide group kernel buffer
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
` (5 preceding siblings ...)
2024-08-08 16:24 ` [PATCH V5 6/8] io_uring: support providing sqe group buffer Ming Lei
@ 2024-08-08 16:24 ` Ming Lei
2024-08-08 16:24 ` [PATCH V5 8/8] ublk: support provide io buffer Ming Lei
2024-08-17 4:16 ` [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
8 siblings, 0 replies; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: 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 <[email protected]>
---
include/linux/io_uring/cmd.h | 7 +++++++
include/uapi/linux/io_uring.h | 7 ++++++-
io_uring/uring_cmd.c | 28 ++++++++++++++++++++++++++++
3 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index 447fbfd32215..fde3a2ec7d9a 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -48,6 +48,8 @@ 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);
#else
static inline int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
struct iov_iter *iter, void *ioucmd)
@@ -67,6 +69,11 @@ 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)
+{
+ return -EOPNOTSUPP;
+}
#endif
/*
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index e6d321b3add7..2e1f33aeea2e 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -271,9 +271,14 @@ enum io_uring_op {
* sqe->uring_cmd_flags top 8bits aren't available for userspace
* 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)
/*
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 8391c7c7c1ec..ac92ba70de9d 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -15,6 +15,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)
{
@@ -175,6 +176,26 @@ 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)
+{
+ struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
+
+ if (unlikely(!(ioucmd->flags & IORING_PROVIDE_GROUP_KBUF)))
+ return -EINVAL;
+
+ if (unlikely(!req_support_group_dep(req)))
+ return -EINVAL;
+
+ return io_provide_group_kbuf(req, grp_kbuf);
+}
+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)
{
@@ -207,6 +228,13 @@ int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
if (ioucmd->flags & ~IORING_URING_CMD_MASK)
return -EINVAL;
+ if (ioucmd->flags & IORING_PROVIDE_GROUP_KBUF) {
+ /* LEADER flag isn't set yet, so check GROUP only */
+ if (!(req->flags & REQ_F_SQE_GROUP))
+ return -EINVAL;
+ req->flags |= REQ_F_SQE_GROUP_DEP;
+ }
+
if (ioucmd->flags & IORING_URING_CMD_FIXED) {
struct io_ring_ctx *ctx = req->ctx;
u16 index;
--
2.42.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V5 8/8] ublk: support provide io buffer
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
` (6 preceding siblings ...)
2024-08-08 16:24 ` [PATCH V5 7/8] io_uring/uring_cmd: support provide group kernel buffer Ming Lei
@ 2024-08-08 16:24 ` Ming Lei
2024-08-17 4:16 ` [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
8 siblings, 0 replies; 19+ messages in thread
From: Ming Lei @ 2024-08-08 16:24 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov; +Cc: 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.
userspace code:
https://github.com/ublk-org/ublksrv/tree/group-provide-buf.v2
git clone https://github.com/ublk-org/ublksrv.git -b group-provide-buf.v2
And both loop and nbd zero copy(io_uring send and send zc) are covered.
Performance improvement is quite obvious in big block size test, such as
'loop --buffered_io' perf is doubled in 64KB block test("loop/007 vs
loop/009").
Signed-off-by: Ming Lei <[email protected]>
---
drivers/block/ublk_drv.c | 160 ++++++++++++++++++++++++++++++++--
include/uapi/linux/ublk_cmd.h | 7 +-
2 files changed, 156 insertions(+), 11 deletions(-)
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 890c08792ba8..d5813e20c177 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -51,6 +51,8 @@
/* private ioctl command mirror */
#define UBLK_CMD_DEL_DEV_ASYNC _IOC_NR(UBLK_U_CMD_DEL_DEV_ASYNC)
+#define UBLK_IO_PROVIDE_IO_BUF _IOC_NR(UBLK_U_IO_PROVIDE_IO_BUF)
+
/* All UBLK_F_* have to be included into UBLK_F_ALL */
#define UBLK_F_ALL (UBLK_F_SUPPORT_ZERO_COPY \
| UBLK_F_URING_CMD_COMP_IN_TASK \
@@ -74,6 +76,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 {
@@ -192,11 +196,15 @@ 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);
static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
int tag);
+static void ublk_io_buf_giveback_cb(const struct io_uring_kernel_buf *buf);
+
static inline bool ublk_dev_is_user_copy(const struct ublk_device *ub)
{
return ub->dev_info.flags & UBLK_F_USER_COPY;
@@ -558,6 +566,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)
{
@@ -821,6 +834,71 @@ 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;
+ imu->grp_kbuf_ack = ublk_io_buf_giveback_cb;
+
+ 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;
@@ -832,13 +910,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
@@ -856,13 +946,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;
@@ -1008,6 +1101,7 @@ static inline void __ublk_complete_rq(struct request *req)
return;
exit:
+ ublk_deinit_zero_copy_buffer(req);
blk_mq_end_request(req, res);
}
@@ -1650,6 +1744,45 @@ 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);
+}
+
static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
unsigned int issue_flags,
const struct ublksrv_io_cmd *ub_cmd)
@@ -1666,6 +1799,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;
@@ -1701,6 +1838,8 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
ret = -EINVAL;
switch (_IOC_NR(cmd_op)) {
+ case UBLK_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)) {
@@ -2120,11 +2259,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);
@@ -2420,8 +2562,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
` (7 preceding siblings ...)
2024-08-08 16:24 ` [PATCH V5 8/8] ublk: support provide io buffer Ming Lei
@ 2024-08-17 4:16 ` Ming Lei
2024-08-17 19:48 ` Pavel Begunkov
8 siblings, 1 reply; 19+ messages in thread
From: Ming Lei @ 2024-08-17 4:16 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-block, Pavel Begunkov
On Fri, Aug 9, 2024 at 12:25 AM Ming Lei <[email protected]> wrote:
>
> Hello,
>
> The 1st 3 patches are cleanup, and prepare for adding sqe group.
>
> The 4th patch supports generic sqe group which is like link chain, but
> allows each sqe in group to be issued in parallel and the group shares
> same IO_LINK & IO_DRAIN boundary, so N:M dependency can be supported with
> sqe group & io link together. sqe group changes nothing on
> IOSQE_IO_LINK.
>
> The 5th patch supports one variant of sqe group: allow members to depend
> on group leader, so that kernel resource lifetime can be aligned with
> group leader or group, then any kernel resource can be shared in this
> sqe group, and can be used in generic device zero copy.
>
> The 6th & 7th patches supports providing sqe group buffer via the sqe
> group variant.
>
> The 8th patch supports ublk zero copy based on io_uring providing sqe
> group buffer.
>
> Tests:
>
> 1) pass liburing test
> - make runtests
>
> 2) write/pass two sqe group test cases:
>
> https://github.com/axboe/liburing/compare/master...ming1:liburing:sqe_group_v2
>
> - covers related sqe flags combination and linking groups, both nop and
> one multi-destination file copy.
>
> - cover failure handling test: fail leader IO or member IO in both single
> group and linked groups, which is done in each sqe flags combination
> test
>
> 3) ublksrv zero copy:
>
> ublksrv userspace implements zero copy by sqe group & provide group
> kbuf:
>
> git clone https://github.com/ublk-org/ublksrv.git -b group-provide-buf_v2
> make test T=loop/009:nbd/061 #ublk zc tests
>
> When running 64KB/512KB block size test on ublk-loop('ublk add -t loop --buffered_io -f $backing'),
> it is observed that perf is doubled.
>
> Any comments are welcome!
>
> V5:
> - follow Pavel's suggestion to minimize change on io_uring fast code
> path: sqe group code is called in by single 'if (unlikely())' from
> both issue & completion code path
>
> - simplify & re-write group request completion
> avoid to touch io-wq code by completing group leader via tw
> directly, just like ->task_complete
>
> re-write group member & leader completion handling, one
> simplification is always to free leader via the last member
>
> simplify queueing group members, not support issuing leader
> and members in parallel
>
> - fail the whole group if IO_*LINK & IO_DRAIN is set on group
> members, and test code to cover this change
>
> - misc cleanup
Hi Pavel,
V5 should address all your comments on V4, so care to take a look?
Thanks,
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf
2024-08-17 4:16 ` [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
@ 2024-08-17 19:48 ` Pavel Begunkov
0 siblings, 0 replies; 19+ messages in thread
From: Pavel Begunkov @ 2024-08-17 19:48 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, io-uring, linux-block
On 8/17/24 05:16, Ming Lei wrote:
> On Fri, Aug 9, 2024 at 12:25 AM Ming Lei <[email protected]> wrote:
>>
>> Hello,
>>
>> The 1st 3 patches are cleanup, and prepare for adding sqe group.
>>
>> The 4th patch supports generic sqe group which is like link chain, but
>> allows each sqe in group to be issued in parallel and the group shares
>> same IO_LINK & IO_DRAIN boundary, so N:M dependency can be supported with
>> sqe group & io link together. sqe group changes nothing on
>> IOSQE_IO_LINK.
>>
>> The 5th patch supports one variant of sqe group: allow members to depend
>> on group leader, so that kernel resource lifetime can be aligned with
>> group leader or group, then any kernel resource can be shared in this
>> sqe group, and can be used in generic device zero copy.
>>
>> The 6th & 7th patches supports providing sqe group buffer via the sqe
>> group variant.
>>
>> The 8th patch supports ublk zero copy based on io_uring providing sqe
>> group buffer.
>>
>> Tests:
>>
>> 1) pass liburing test
>> - make runtests
>>
>> 2) write/pass two sqe group test cases:
>>
>> https://github.com/axboe/liburing/compare/master...ming1:liburing:sqe_group_v2
>>
>> - covers related sqe flags combination and linking groups, both nop and
>> one multi-destination file copy.
>>
>> - cover failure handling test: fail leader IO or member IO in both single
>> group and linked groups, which is done in each sqe flags combination
>> test
>>
>> 3) ublksrv zero copy:
>>
>> ublksrv userspace implements zero copy by sqe group & provide group
>> kbuf:
>>
>> git clone https://github.com/ublk-org/ublksrv.git -b group-provide-buf_v2
>> make test T=loop/009:nbd/061 #ublk zc tests
>>
>> When running 64KB/512KB block size test on ublk-loop('ublk add -t loop --buffered_io -f $backing'),
>> it is observed that perf is doubled.
>>
>> Any comments are welcome!
>>
>> V5:
>> - follow Pavel's suggestion to minimize change on io_uring fast code
>> path: sqe group code is called in by single 'if (unlikely())' from
>> both issue & completion code path
>>
>> - simplify & re-write group request completion
>> avoid to touch io-wq code by completing group leader via tw
>> directly, just like ->task_complete
>>
>> re-write group member & leader completion handling, one
>> simplification is always to free leader via the last member
>>
>> simplify queueing group members, not support issuing leader
>> and members in parallel
>>
>> - fail the whole group if IO_*LINK & IO_DRAIN is set on group
>> members, and test code to cover this change
>>
>> - misc cleanup
>
> Hi Pavel,
>
> V5 should address all your comments on V4, so care to take a look?
I will, didn't forget about it, but thanks for the reminder.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 4/8] io_uring: support SQE group
2024-08-08 16:24 ` [PATCH V5 4/8] io_uring: support SQE group Ming Lei
@ 2024-08-27 15:18 ` Pavel Begunkov
2024-08-29 4:29 ` Ming Lei
0 siblings, 1 reply; 19+ messages in thread
From: Pavel Begunkov @ 2024-08-27 15:18 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, io-uring, linux-block; +Cc: Kevin Wolf
On 8/8/24 17:24, Ming Lei wrote:
> SQE group is defined as one chain of SQEs starting with the first SQE that
> has IOSQE_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.
>
> Not like linked SQEs, each sqe is issued after the previous one is
> completed. All SQEs in one group can be submitted in parallel. To simplify
> the implementation from beginning, all members are queued after the leader
> is completed, however, this way may be changed and leader and members may
> be issued concurrently in future.
>
> 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. Failure from any group member will
> fail the group leader, then the link chain can be terminated.
>
> 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 by always completing group leader as
> the last one in the group. Meantime it is natural to post leader's CQE
> as the last one from application viewpoint.
>
> Working together with IOSQE_IO_LINK, SQE group provides flexible way to
> support N:M dependency, such as:
>
> - group A is chained with group B together
> - group A has N SQEs
> - group B has M SQEs
>
> then M SQEs in group B depend on N SQEs in group A.
>
> N:M dependency can support some interesting use cases in efficient way:
>
> 1) read from multiple files, then write the read data into single file
>
> 2) read from single file, and write the read data into multiple files
>
> 3) write same data into multiple files, and read data from multiple files and
> compare if correct data is written
>
> Also IOSQE_SQE_GROUP takes the last bit in sqe->flags, but we still can
> extend sqe->flags with one uring context flag, such as use __pad3 for
> non-uring_cmd OPs and part of uring_cmd_flags for uring_cmd OP.
>
> Suggested-by: Kevin Wolf <[email protected]>
> Signed-off-by: Ming Lei <[email protected]>
> ---
> include/linux/io_uring_types.h | 18 +++
...
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index f112e9fa90d8..45a292445b18 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
...
> @@ -875,6 +877,116 @@ static __always_inline void io_req_commit_cqe(struct io_ring_ctx *ctx,
> }
> }
...
> +static void io_queue_group_members(struct io_kiocb *req)
> +{
> + struct io_kiocb *member = req->grp_link;
> +
> + if (!member)
> + return;
> +
> + req->grp_link = NULL;
> + while (member) {
> + struct io_kiocb *next = member->grp_link;
> +
> + member->grp_leader = req;
> + if (unlikely(member->flags & REQ_F_FAIL)) {
> + io_req_task_queue_fail(member, member->cqe.res);
> + } else if (unlikely(req->flags & REQ_F_FAIL)) {
> + io_req_task_queue_fail(member, -ECANCELED);
> + } else {
> + io_req_task_queue(member);
> + }
> + member = next;
> + }
> +}
> +
> +static inline bool __io_complete_group_member(struct io_kiocb *req,
> + struct io_kiocb *lead)
> +{
I think it'd be better if you inline this function, it only
obfuscates things.
> + if (WARN_ON_ONCE(lead->grp_refs <= 0))
> + return false;
> +
> + req->flags &= ~REQ_F_SQE_GROUP;
I'm getting completely lost when and why it clears and sets
back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any
rule?
> + /*
> + * Set linked leader as failed if any member is failed, so
> + * the remained link chain can be terminated
> + */
> + if (unlikely((req->flags & REQ_F_FAIL) &&
> + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)))
> + req_set_fail(lead);
if (req->flags & REQ_F_FAIL)
req_set_fail(lead);
REQ_F_FAIL is not specific to links, if a request fails we need
to mark it as such.
> + return !--lead->grp_refs;
> +}
> +
> +static inline bool leader_is_the_last(struct io_kiocb *lead)
> +{
> + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP);
> +}
> +
> +static void io_complete_group_member(struct io_kiocb *req)
> +{
> + struct io_kiocb *lead = get_group_leader(req);
> +
> + if (WARN_ON_ONCE(!(req->flags & REQ_F_SQE_GROUP)))
> + return;
> +
> + /* member CQE needs to be posted first */
> + if (!(req->flags & REQ_F_CQE_SKIP))
> + io_req_commit_cqe(req->ctx, req);
> +
> + if (__io_complete_group_member(req, lead)) {
> + /*
> + * SQE_GROUP flag is kept for the last member, so the leader
> + * can be retrieved & freed from this last member
> + */
> + req->flags |= REQ_F_SQE_GROUP;
> + if (!(lead->flags & REQ_F_CQE_SKIP))
> + io_req_commit_cqe(lead->ctx, lead);
> + } else if (leader_is_the_last(lead)) {
> + /* leader will degenerate to plain req if it is the last */
> + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER);
What's this chunk is about?
> + }
> +}
> +
> +static void io_complete_group_leader(struct io_kiocb *req)
> +{
> + WARN_ON_ONCE(req->grp_refs <= 0);
> + req->flags &= ~REQ_F_SQE_GROUP;
> + req->grp_refs -= 1;
> + WARN_ON_ONCE(req->grp_refs == 0);
Why not combine these WARN_ON_ONCE into one?
> +
> + /* TODO: queue members with leader in parallel */
no todos, please
> + if (req->grp_link)
> + io_queue_group_members(req);
> +}
It's spinlock'ed, we really don't want to do too much here
like potentially queueing a ton of task works.
io_queue_group_members() can move into io_free_batch_list().
> +
> static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
> {
> struct io_ring_ctx *ctx = req->ctx;
> @@ -890,7 +1002,8 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
> * Handle special CQ sync cases via task_work. DEFER_TASKRUN requires
> * the submitter task context, IOPOLL protects with uring_lock.
> */
> - if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL)) {
> + if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL) ||
> + req_is_group_leader(req)) {
> req->io_task_work.func = io_req_task_complete;
> io_req_task_work_add(req);
> return;
> @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx,
> comp_list);
>
> if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) {
> + if (req->flags & (REQ_F_SQE_GROUP |
> + REQ_F_SQE_GROUP_LEADER)) {
> + struct io_kiocb *leader;
> +
> + /* Leader is freed via the last member */
> + if (req_is_group_leader(req)) {
> + node = req->comp_list.next;
> + continue;
> + }
> +
> + /*
> + * Only the last member keeps GROUP flag,
> + * free leader and this member together
> + */
> + leader = get_group_leader(req);
> + leader->flags &= ~REQ_F_SQE_GROUP_LEADER;
> + req->flags &= ~REQ_F_SQE_GROUP;
> + wq_stack_add_head(&leader->comp_list,
> + &req->comp_list);
That's quite hacky, but at least we can replace it with
task work if it gets in the way later on.
> + }
> +
> if (req->flags & REQ_F_REFCOUNT) {
> node = req->comp_list.next;
> if (!req_ref_put_and_test(req))
> continue;
> }
> +
> if ((req->flags & REQ_F_POLLED) && req->apoll) {
> struct async_poll *apoll = req->apoll;
>
> @@ -1427,8 +1562,19 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx)
> struct io_kiocb *req = container_of(node, struct io_kiocb,
> comp_list);
>
> - if (!(req->flags & REQ_F_CQE_SKIP))
> - io_req_commit_cqe(ctx, req);
> + if (unlikely(req->flags & (REQ_F_CQE_SKIP | REQ_F_SQE_GROUP))) {
> + if (req->flags & REQ_F_SQE_GROUP) {
> + if (req_is_group_leader(req))
> + io_complete_group_leader(req);
> + else
> + io_complete_group_member(req);
> + continue;
> + }
> +
> + if (req->flags & REQ_F_CQE_SKIP)
> + continue;
> + }
> + io_req_commit_cqe(ctx, req);
> }
> __io_cq_unlock_post(ctx);
>
...
> @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
> return def->prep(req, sqe);
> }
>
> +static struct io_kiocb *io_group_sqe(struct io_submit_link *group,
> + struct io_kiocb *req)
> +{
> + /*
> + * Group chain is similar with link chain: starts with 1st sqe with
> + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP
> + */
> + if (group->head) {
> + struct io_kiocb *lead = group->head;
> +
> + /* members can't be in link chain, can't be drained */
> + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN))
> + req_fail_link_node(lead, -EINVAL);
That should fail the entire link (if any) as well.
I have even more doubts we even want to mix links and groups. Apart
from nuances as such, which would be quite hard to track, the semantics
of IOSQE_CQE_SKIP_SUCCESS is unclear. And also it doen't work with
IORING_OP_LINK_TIMEOUT.
> +
> + lead->grp_refs += 1;
> + group->last->grp_link = req;
> + group->last = req;
> +
> + if (req->flags & REQ_F_SQE_GROUP)
> + return NULL;
> +
> + req->grp_link = NULL;
> + req->flags |= REQ_F_SQE_GROUP;
> + group->head = NULL;
> + if (lead->flags & REQ_F_FAIL) {
> + io_queue_sqe_fallback(lead);
Let's say the group was in the middle of a link, it'll
complete that group and continue with assembling / executing
the link when it should've failed it and honoured the
request order.
> + return NULL;
> + }
> + return lead;
> + } else if (req->flags & REQ_F_SQE_GROUP) {
> + group->head = req;
> + group->last = req;
> + req->grp_refs = 1;
> + req->flags |= REQ_F_SQE_GROUP_LEADER;
> + return NULL;
> + } else {
We shouldn't be able to get here.
if (WARN_ONCE(!(req->flags & GROUP)))
...
group->head = req;
...
> + return req;
> + }
> +}
> +
> +static __cold struct io_kiocb *io_submit_fail_group(
> + struct io_submit_link *link, struct io_kiocb *req)
> +{
> + struct io_kiocb *lead = link->head;
> +
> + /*
> + * Instead of failing eagerly, continue assembling the group link
> + * if applicable and mark the leader with REQ_F_FAIL. The group
> + * flushing code should find the flag and handle the rest
> + */
> + if (lead && (lead->flags & IO_REQ_LINK_FLAGS) && !(lead->flags & REQ_F_FAIL))
> + req_fail_link_node(lead, -ECANCELED);
Same as above, you don't need to check for IO_REQ_LINK_FLAGS.
io_queue_sqe_fallback()
> +
> + return io_group_sqe(link, req);
> +}
> +
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 4/8] io_uring: support SQE group
2024-08-27 15:18 ` Pavel Begunkov
@ 2024-08-29 4:29 ` Ming Lei
2024-09-06 17:15 ` Pavel Begunkov
0 siblings, 1 reply; 19+ messages in thread
From: Ming Lei @ 2024-08-29 4:29 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: Jens Axboe, io-uring, linux-block, Kevin Wolf, ming.lei
Hi Pavel,
Thanks for review!
On Tue, Aug 27, 2024 at 04:18:26PM +0100, Pavel Begunkov wrote:
> On 8/8/24 17:24, Ming Lei wrote:
> > SQE group is defined as one chain of SQEs starting with the first SQE that
> > has IOSQE_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.
> >
> > Not like linked SQEs, each sqe is issued after the previous one is
> > completed. All SQEs in one group can be submitted in parallel. To simplify
> > the implementation from beginning, all members are queued after the leader
> > is completed, however, this way may be changed and leader and members may
> > be issued concurrently in future.
> >
> > 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. Failure from any group member will
> > fail the group leader, then the link chain can be terminated.
> >
> > 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 by always completing group leader as
> > the last one in the group. Meantime it is natural to post leader's CQE
> > as the last one from application viewpoint.
> >
> > Working together with IOSQE_IO_LINK, SQE group provides flexible way to
> > support N:M dependency, such as:
> >
> > - group A is chained with group B together
> > - group A has N SQEs
> > - group B has M SQEs
> >
> > then M SQEs in group B depend on N SQEs in group A.
> >
> > N:M dependency can support some interesting use cases in efficient way:
> >
> > 1) read from multiple files, then write the read data into single file
> >
> > 2) read from single file, and write the read data into multiple files
> >
> > 3) write same data into multiple files, and read data from multiple files and
> > compare if correct data is written
> >
> > Also IOSQE_SQE_GROUP takes the last bit in sqe->flags, but we still can
> > extend sqe->flags with one uring context flag, such as use __pad3 for
> > non-uring_cmd OPs and part of uring_cmd_flags for uring_cmd OP.
> >
> > Suggested-by: Kevin Wolf <[email protected]>
> > Signed-off-by: Ming Lei <[email protected]>
> > ---
> > include/linux/io_uring_types.h | 18 +++
> ...
> > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> > index f112e9fa90d8..45a292445b18 100644
> > --- a/io_uring/io_uring.c
> > +++ b/io_uring/io_uring.c
> ...
> > @@ -875,6 +877,116 @@ static __always_inline void io_req_commit_cqe(struct io_ring_ctx *ctx,
> > }
> > }
> ...
> > +static void io_queue_group_members(struct io_kiocb *req)
> > +{
> > + struct io_kiocb *member = req->grp_link;
> > +
> > + if (!member)
> > + return;
> > +
> > + req->grp_link = NULL;
> > + while (member) {
> > + struct io_kiocb *next = member->grp_link;
> > +
> > + member->grp_leader = req;
> > + if (unlikely(member->flags & REQ_F_FAIL)) {
> > + io_req_task_queue_fail(member, member->cqe.res);
> > + } else if (unlikely(req->flags & REQ_F_FAIL)) {
> > + io_req_task_queue_fail(member, -ECANCELED);
> > + } else {
> > + io_req_task_queue(member);
> > + }
> > + member = next;
> > + }
> > +}
> > +
> > +static inline bool __io_complete_group_member(struct io_kiocb *req,
> > + struct io_kiocb *lead)
> > +{
>
> I think it'd be better if you inline this function, it only
> obfuscates things.
OK.
>
> > + if (WARN_ON_ONCE(lead->grp_refs <= 0))
> > + return false;
> > +
> > + req->flags &= ~REQ_F_SQE_GROUP;
>
> I'm getting completely lost when and why it clears and sets
> back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any
> rule?
My fault, it should have been documented somewhere.
REQ_F_SQE_GROUP is cleared when the request is completed, but it is
reused as flag for marking the last request in this group, so we can
free the group leader when observing the 'last' member request.
The only other difference about the two flags is that both are cleared
when the group leader becomes the last one in the group, then
this leader degenerates as normal request, which way can simplify
group leader freeing.
>
> > + /*
> > + * Set linked leader as failed if any member is failed, so
> > + * the remained link chain can be terminated
> > + */
> > + if (unlikely((req->flags & REQ_F_FAIL) &&
> > + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)))
> > + req_set_fail(lead);
>
> if (req->flags & REQ_F_FAIL)
> req_set_fail(lead);
>
> REQ_F_FAIL is not specific to links, if a request fails we need
> to mark it as such.
It is for handling group failure.
The following condition
((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))
means that this group is in one link-chain.
If any member in this group is failed, we need to fail this group(lead),
then the remained requests in this chain can be failed.
Otherwise, it isn't necessary to fail group leader in case of any member
io failure.
>
>
> > + return !--lead->grp_refs;
> > +}
> > +
> > +static inline bool leader_is_the_last(struct io_kiocb *lead)
> > +{
> > + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP);
> > +}
> > +
> > +static void io_complete_group_member(struct io_kiocb *req)
> > +{
> > + struct io_kiocb *lead = get_group_leader(req);
> > +
> > + if (WARN_ON_ONCE(!(req->flags & REQ_F_SQE_GROUP)))
> > + return;
> > +
> > + /* member CQE needs to be posted first */
> > + if (!(req->flags & REQ_F_CQE_SKIP))
> > + io_req_commit_cqe(req->ctx, req);
> > +
> > + if (__io_complete_group_member(req, lead)) {
> > + /*
> > + * SQE_GROUP flag is kept for the last member, so the leader
> > + * can be retrieved & freed from this last member
> > + */
> > + req->flags |= REQ_F_SQE_GROUP;
'req' is the last completed request, so mark it as the last one
by reusing REQ_F_SQE_GROUP, so we can free group leader in
io_free_batch_list() when observing the last flag.
But it should have been documented.
> > + if (!(lead->flags & REQ_F_CQE_SKIP))
> > + io_req_commit_cqe(lead->ctx, lead);
> > + } else if (leader_is_the_last(lead)) {
> > + /* leader will degenerate to plain req if it is the last */
> > + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER);
>
> What's this chunk is about?
The leader becomes the only request not completed in group, so it is
degenerated as normal one by clearing the two flags. This way simplifies
logic for completing group leader.
>
> > + }
> > +}
> > +
> > +static void io_complete_group_leader(struct io_kiocb *req)
> > +{
> > + WARN_ON_ONCE(req->grp_refs <= 0);
> > + req->flags &= ~REQ_F_SQE_GROUP;
> > + req->grp_refs -= 1;
> > + WARN_ON_ONCE(req->grp_refs == 0);
>
> Why not combine these WARN_ON_ONCE into one?
OK.
>
> > +
> > + /* TODO: queue members with leader in parallel */
>
> no todos, please
Fine, will remove the todos.
>
> > + if (req->grp_link)
> > + io_queue_group_members(req);
> > +}
>
> It's spinlock'ed, we really don't want to do too much here
> like potentially queueing a ton of task works.
> io_queue_group_members() can move into io_free_batch_list().
Fair enough, will do it in V6.
>
> > +
> > static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
> > {
> > struct io_ring_ctx *ctx = req->ctx;
> > @@ -890,7 +1002,8 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
> > * Handle special CQ sync cases via task_work. DEFER_TASKRUN requires
> > * the submitter task context, IOPOLL protects with uring_lock.
> > */
> > - if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL)) {
> > + if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL) ||
> > + req_is_group_leader(req)) {
> > req->io_task_work.func = io_req_task_complete;
> > io_req_task_work_add(req);
> > return;
> > @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx,
> > comp_list);
> > if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) {
> > + if (req->flags & (REQ_F_SQE_GROUP |
> > + REQ_F_SQE_GROUP_LEADER)) {
> > + struct io_kiocb *leader;
> > +
> > + /* Leader is freed via the last member */
> > + if (req_is_group_leader(req)) {
> > + node = req->comp_list.next;
> > + continue;
> > + }
> > +
> > + /*
> > + * Only the last member keeps GROUP flag,
> > + * free leader and this member together
> > + */
> > + leader = get_group_leader(req);
> > + leader->flags &= ~REQ_F_SQE_GROUP_LEADER;
> > + req->flags &= ~REQ_F_SQE_GROUP;
> > + wq_stack_add_head(&leader->comp_list,
> > + &req->comp_list);
>
> That's quite hacky, but at least we can replace it with
> task work if it gets in the way later on.
io_free_batch_list() is already called in task context, and it isn't
necessary to schedule one extra tw, which hurts perf more or less.
Another way is to store these leaders into one temp list, and
call io_free_batch_list() for this temp list one more time.
>
> > + }
> > +
> > if (req->flags & REQ_F_REFCOUNT) {
> > node = req->comp_list.next;
> > if (!req_ref_put_and_test(req))
> > continue;
> > }
> > +
> > if ((req->flags & REQ_F_POLLED) && req->apoll) {
> > struct async_poll *apoll = req->apoll;
> > @@ -1427,8 +1562,19 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx)
> > struct io_kiocb *req = container_of(node, struct io_kiocb,
> > comp_list);
> > - if (!(req->flags & REQ_F_CQE_SKIP))
> > - io_req_commit_cqe(ctx, req);
> > + if (unlikely(req->flags & (REQ_F_CQE_SKIP | REQ_F_SQE_GROUP))) {
> > + if (req->flags & REQ_F_SQE_GROUP) {
> > + if (req_is_group_leader(req))
> > + io_complete_group_leader(req);
> > + else
> > + io_complete_group_member(req);
> > + continue;
> > + }
> > +
> > + if (req->flags & REQ_F_CQE_SKIP)
> > + continue;
> > + }
> > + io_req_commit_cqe(ctx, req);
> > }
> > __io_cq_unlock_post(ctx);
> ...
> > @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
> > return def->prep(req, sqe);
> > }
> > +static struct io_kiocb *io_group_sqe(struct io_submit_link *group,
> > + struct io_kiocb *req)
> > +{
> > + /*
> > + * Group chain is similar with link chain: starts with 1st sqe with
> > + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP
> > + */
> > + if (group->head) {
> > + struct io_kiocb *lead = group->head;
> > +
> > + /* members can't be in link chain, can't be drained */
> > + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN))
> > + req_fail_link_node(lead, -EINVAL);
>
> That should fail the entire link (if any) as well.
Good catch, here we should fail link head by following the logic
in io_submit_fail_init().
>
> I have even more doubts we even want to mix links and groups. Apart
Wrt. ublk, group provides zero copy, and the ublk io(group) is generic
IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd,
send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK
for generic io_uring IO.
> from nuances as such, which would be quite hard to track, the semantics
> of IOSQE_CQE_SKIP_SUCCESS is unclear.
IO group just follows every normal request.
1) fail in linked chain
- follows IO_LINK's behavior since io_fail_links() covers io group
2) otherwise
- just respect IOSQE_CQE_SKIP_SUCCESS
> And also it doen't work with IORING_OP_LINK_TIMEOUT.
REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I
will document it in V6.
>
> > +
> > + lead->grp_refs += 1;
> > + group->last->grp_link = req;
> > + group->last = req;
> > +
> > + if (req->flags & REQ_F_SQE_GROUP)
> > + return NULL;
> > +
> > + req->grp_link = NULL;
> > + req->flags |= REQ_F_SQE_GROUP;
> > + group->head = NULL;
> > + if (lead->flags & REQ_F_FAIL) {
> > + io_queue_sqe_fallback(lead);
>
> Let's say the group was in the middle of a link, it'll
> complete that group and continue with assembling / executing
> the link when it should've failed it and honoured the
> request order.
OK, here we can simply remove the above two lines, and link submit
state can handle this failure in link chain.
>
>
> > + return NULL;
> > + }
> > + return lead;
> > + } else if (req->flags & REQ_F_SQE_GROUP) {
> > + group->head = req;
> > + group->last = req;
> > + req->grp_refs = 1;
> > + req->flags |= REQ_F_SQE_GROUP_LEADER;
> > + return NULL;
> > + } else {
>
> We shouldn't be able to get here.
>
> if (WARN_ONCE(!(req->flags & GROUP)))
> ...
> group->head = req;
> ...
OK.
>
> > + return req;
> > + }
> > +}
> > +
> > +static __cold struct io_kiocb *io_submit_fail_group(
> > + struct io_submit_link *link, struct io_kiocb *req)
> > +{
> > + struct io_kiocb *lead = link->head;
> > +
> > + /*
> > + * Instead of failing eagerly, continue assembling the group link
> > + * if applicable and mark the leader with REQ_F_FAIL. The group
> > + * flushing code should find the flag and handle the rest
> > + */
> > + if (lead && (lead->flags & IO_REQ_LINK_FLAGS) && !(lead->flags & REQ_F_FAIL))
> > + req_fail_link_node(lead, -ECANCELED);
>
> Same as above, you don't need to check for IO_REQ_LINK_FLAGS.
> io_queue_sqe_fallback()
OK.
Thanks,
Ming
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 4/8] io_uring: support SQE group
2024-08-29 4:29 ` Ming Lei
@ 2024-09-06 17:15 ` Pavel Begunkov
2024-09-07 9:36 ` Ming Lei
0 siblings, 1 reply; 19+ messages in thread
From: Pavel Begunkov @ 2024-09-06 17:15 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, io-uring, linux-block, Kevin Wolf
On 8/29/24 05:29, Ming Lei wrote:
...
>>> + if (WARN_ON_ONCE(lead->grp_refs <= 0))
>>> + return false;
>>> +
>>> + req->flags &= ~REQ_F_SQE_GROUP;
>>
>> I'm getting completely lost when and why it clears and sets
>> back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any
>> rule?
>
> My fault, it should have been documented somewhere.
>
> REQ_F_SQE_GROUP is cleared when the request is completed, but it is
> reused as flag for marking the last request in this group, so we can
> free the group leader when observing the 'last' member request.
Maybe it'd be cleaner to use a second flag?
> The only other difference about the two flags is that both are cleared
> when the group leader becomes the last one in the group, then
> this leader degenerates as normal request, which way can simplify
> group leader freeing.
>
>>
>>> + /*
>>> + * Set linked leader as failed if any member is failed, so
>>> + * the remained link chain can be terminated
>>> + */
>>> + if (unlikely((req->flags & REQ_F_FAIL) &&
>>> + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)))
>>> + req_set_fail(lead);
>>
>> if (req->flags & REQ_F_FAIL)
>> req_set_fail(lead);
>>
>> REQ_F_FAIL is not specific to links, if a request fails we need
>> to mark it as such.
>
> It is for handling group failure.
>
> The following condition
>
> ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))
>
> means that this group is in one link-chain.
>
> If any member in this group is failed, we need to fail this group(lead),
> then the remained requests in this chain can be failed.
>
> Otherwise, it isn't necessary to fail group leader in case of any member
> io failure.
What bad would happen if you do it like this?
if (req->flags & REQ_F_FAIL)
req_set_fail(lead);
I'm asking because if you rely on some particular combination
of F_FAIL and F_LINK somewhere, it's likely wrong, but otherwise
we F_FAIL a larger set of requests, which should never be an
issue.
>>> + return !--lead->grp_refs;
>>> +}
>>> +
>>> +static inline bool leader_is_the_last(struct io_kiocb *lead)
>>> +{
>>> + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP);
>>> +}
>>> +
>>> +static void io_complete_group_member(struct io_kiocb *req)
>>> +{
>>> + struct io_kiocb *lead = get_group_leader(req);
>>> +
>>> + if (WARN_ON_ONCE(!(req->flags & REQ_F_SQE_GROUP)))
>>> + return;
>>> +
>>> + /* member CQE needs to be posted first */
>>> + if (!(req->flags & REQ_F_CQE_SKIP))
>>> + io_req_commit_cqe(req->ctx, req);
>>> +
>>> + if (__io_complete_group_member(req, lead)) {
>>> + /*
>>> + * SQE_GROUP flag is kept for the last member, so the leader
>>> + * can be retrieved & freed from this last member
>>> + */
>>> + req->flags |= REQ_F_SQE_GROUP;
>
> 'req' is the last completed request, so mark it as the last one
> by reusing REQ_F_SQE_GROUP, so we can free group leader in
> io_free_batch_list() when observing the last flag.
>
> But it should have been documented.
>
>>> + if (!(lead->flags & REQ_F_CQE_SKIP))
>>> + io_req_commit_cqe(lead->ctx, lead);
>>> + } else if (leader_is_the_last(lead)) {
>>> + /* leader will degenerate to plain req if it is the last */
>>> + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER);
>>
>> What's this chunk is about?
>
> The leader becomes the only request not completed in group, so it is
> degenerated as normal one by clearing the two flags. This way simplifies
> logic for completing group leader.
>
...
>>> @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx,
>>> comp_list);
>>> if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) {
>>> + if (req->flags & (REQ_F_SQE_GROUP |
>>> + REQ_F_SQE_GROUP_LEADER)) {
>>> + struct io_kiocb *leader;
>>> +
>>> + /* Leader is freed via the last member */
>>> + if (req_is_group_leader(req)) {
>>> + node = req->comp_list.next;
>>> + continue;
>>> + }
>>> +
>>> + /*
>>> + * Only the last member keeps GROUP flag,
>>> + * free leader and this member together
>>> + */
>>> + leader = get_group_leader(req);
>>> + leader->flags &= ~REQ_F_SQE_GROUP_LEADER;
>>> + req->flags &= ~REQ_F_SQE_GROUP;
>>> + wq_stack_add_head(&leader->comp_list,
>>> + &req->comp_list);
>>
>> That's quite hacky, but at least we can replace it with
>> task work if it gets in the way later on.
>
> io_free_batch_list() is already called in task context, and it isn't
> necessary to schedule one extra tw, which hurts perf more or less.
>
> Another way is to store these leaders into one temp list, and
> call io_free_batch_list() for this temp list one more time.
What I'm saying, it's fine to leave it as is for now. In the
future if it becomes a problem for ome reason or another, we can
do it the task_work like way.
...
>>> @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>> return def->prep(req, sqe);
>>> }
>>> +static struct io_kiocb *io_group_sqe(struct io_submit_link *group,
>>> + struct io_kiocb *req)
>>> +{
>>> + /*
>>> + * Group chain is similar with link chain: starts with 1st sqe with
>>> + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP
>>> + */
>>> + if (group->head) {
>>> + struct io_kiocb *lead = group->head;
>>> +
>>> + /* members can't be in link chain, can't be drained */
>>> + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN))
>>> + req_fail_link_node(lead, -EINVAL);
>>
>> That should fail the entire link (if any) as well.
>
> Good catch, here we should fail link head by following the logic
> in io_submit_fail_init().
>
>>
>> I have even more doubts we even want to mix links and groups. Apart
>
> Wrt. ublk, group provides zero copy, and the ublk io(group) is generic
> IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd,
> send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK
> for generic io_uring IO.
>
>> from nuances as such, which would be quite hard to track, the semantics
>> of IOSQE_CQE_SKIP_SUCCESS is unclear.
>
> IO group just follows every normal request.
It tries to mimic but groups don't and essentially can't do it the
same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS
usually means that all following will be silenced. What if a
member is CQE_SKIP, should it stop the leader from posting a CQE?
And whatever the answer is, it'll be different from the link's
behaviour.
Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts
for groups, that can be discussed afterwards.
> 1) fail in linked chain
> - follows IO_LINK's behavior since io_fail_links() covers io group
>
> 2) otherwise
> - just respect IOSQE_CQE_SKIP_SUCCESS
>
>> And also it doen't work with IORING_OP_LINK_TIMEOUT.
>
> REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I
> will document it in V6.
It would still be troublesome. When a linked timeout fires it searches
for the request it's attached to and cancels it, however, group leaders
that queued up their members are discoverable. But let's say you can find
them in some way, then the only sensbile thing to do is cancel members,
which should be doable by checking req->grp_leader, but might be easier
to leave it to follow up patches.
>>> +
>>> + lead->grp_refs += 1;
>>> + group->last->grp_link = req;
>>> + group->last = req;
>>> +
>>> + if (req->flags & REQ_F_SQE_GROUP)
>>> + return NULL;
>>> +
>>> + req->grp_link = NULL;
>>> + req->flags |= REQ_F_SQE_GROUP;
>>> + group->head = NULL;
>>> + if (lead->flags & REQ_F_FAIL) {
>>> + io_queue_sqe_fallback(lead);
>>
>> Let's say the group was in the middle of a link, it'll
>> complete that group and continue with assembling / executing
>> the link when it should've failed it and honoured the
>> request order.
>
> OK, here we can simply remove the above two lines, and link submit
> state can handle this failure in link chain.
If you just delete then nobody would check for REQ_F_FAIL and
fail the request. Assuming you'd also set the fail flag to the
link head when appropriate, how about deleting these two line
and do like below? (can be further prettified)
bool io_group_assembling()
{
return state->group.head || (req->flags & REQ_F_SQE_GROUP);
}
bool io_link_assembling()
{
return state->link.head || (req->flags & IO_REQ_LINK_FLAGS);
}
static inline int io_submit_sqe()
{
...
if (unlikely(io_link_assembling(state, req) ||
io_group_assembling(state, req) ||
req->flags & REQ_F_FAIL)) {
if (io_group_assembling(state, req)) {
req = io_group_sqe(&state->group, req);
if (!req)
return 0;
}
if (io_link_assembling(state, req)) {
req = io_link_sqe(&state->link, req);
if (!req)
return 0;
}
if (req->flags & REQ_F_FAIL) {
io_queue_sqe_fallback(req);
return 0;
}
}
io_queue_sqe(req);
return 0;
}
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 4/8] io_uring: support SQE group
2024-09-06 17:15 ` Pavel Begunkov
@ 2024-09-07 9:36 ` Ming Lei
2024-09-10 13:12 ` Pavel Begunkov
0 siblings, 1 reply; 19+ messages in thread
From: Ming Lei @ 2024-09-07 9:36 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: Jens Axboe, io-uring, linux-block, Kevin Wolf, ming.lei
On Fri, Sep 06, 2024 at 06:15:32PM +0100, Pavel Begunkov wrote:
> On 8/29/24 05:29, Ming Lei wrote:
> ...
> > > > + if (WARN_ON_ONCE(lead->grp_refs <= 0))
> > > > + return false;
> > > > +
> > > > + req->flags &= ~REQ_F_SQE_GROUP;
> > >
> > > I'm getting completely lost when and why it clears and sets
> > > back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any
> > > rule?
> >
> > My fault, it should have been documented somewhere.
> >
> > REQ_F_SQE_GROUP is cleared when the request is completed, but it is
> > reused as flag for marking the last request in this group, so we can
> > free the group leader when observing the 'last' member request.
>
> Maybe it'd be cleaner to use a second flag?
I will add one new flag with same value, since the two's lifetime
is non-overlapping.
>
> > The only other difference about the two flags is that both are cleared
> > when the group leader becomes the last one in the group, then
> > this leader degenerates as normal request, which way can simplify
> > group leader freeing.
> >
> > >
> > > > + /*
> > > > + * Set linked leader as failed if any member is failed, so
> > > > + * the remained link chain can be terminated
> > > > + */
> > > > + if (unlikely((req->flags & REQ_F_FAIL) &&
> > > > + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)))
> > > > + req_set_fail(lead);
> > >
> > > if (req->flags & REQ_F_FAIL)
> > > req_set_fail(lead);
> > >
> > > REQ_F_FAIL is not specific to links, if a request fails we need
> > > to mark it as such.
> >
> > It is for handling group failure.
> >
> > The following condition
> >
> > ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))
> >
> > means that this group is in one link-chain.
> >
> > If any member in this group is failed, we need to fail this group(lead),
> > then the remained requests in this chain can be failed.
> >
> > Otherwise, it isn't necessary to fail group leader in case of any member
> > io failure.
>
> What bad would happen if you do it like this?
>
> if (req->flags & REQ_F_FAIL)
> req_set_fail(lead);
>
> I'm asking because if you rely on some particular combination
> of F_FAIL and F_LINK somewhere, it's likely wrong, but otherwise
> we F_FAIL a larger set of requests, which should never be an
> issue.
From dependency relation viewpoint it is not necessary to fail all, but it
makes us easier to start with this more determinate behavior, will
change to this way in V6.
>
> > > > + return !--lead->grp_refs;
> > > > +}
> > > > +
> > > > +static inline bool leader_is_the_last(struct io_kiocb *lead)
> > > > +{
> > > > + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP);
> > > > +}
> > > > +
> > > > +static void io_complete_group_member(struct io_kiocb *req)
> > > > +{
> > > > + struct io_kiocb *lead = get_group_leader(req);
> > > > +
> > > > + if (WARN_ON_ONCE(!(req->flags & REQ_F_SQE_GROUP)))
> > > > + return;
> > > > +
> > > > + /* member CQE needs to be posted first */
> > > > + if (!(req->flags & REQ_F_CQE_SKIP))
> > > > + io_req_commit_cqe(req->ctx, req);
> > > > +
> > > > + if (__io_complete_group_member(req, lead)) {
> > > > + /*
> > > > + * SQE_GROUP flag is kept for the last member, so the leader
> > > > + * can be retrieved & freed from this last member
> > > > + */
> > > > + req->flags |= REQ_F_SQE_GROUP;
> >
> > 'req' is the last completed request, so mark it as the last one
> > by reusing REQ_F_SQE_GROUP, so we can free group leader in
> > io_free_batch_list() when observing the last flag.
> >
> > But it should have been documented.
> >
> > > > + if (!(lead->flags & REQ_F_CQE_SKIP))
> > > > + io_req_commit_cqe(lead->ctx, lead);
> > > > + } else if (leader_is_the_last(lead)) {
> > > > + /* leader will degenerate to plain req if it is the last */
> > > > + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER);
> > >
> > > What's this chunk is about?
> >
> > The leader becomes the only request not completed in group, so it is
> > degenerated as normal one by clearing the two flags. This way simplifies
> > logic for completing group leader.
> >
> ...
> > > > @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx,
> > > > comp_list);
> > > > if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) {
> > > > + if (req->flags & (REQ_F_SQE_GROUP |
> > > > + REQ_F_SQE_GROUP_LEADER)) {
> > > > + struct io_kiocb *leader;
> > > > +
> > > > + /* Leader is freed via the last member */
> > > > + if (req_is_group_leader(req)) {
> > > > + node = req->comp_list.next;
> > > > + continue;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Only the last member keeps GROUP flag,
> > > > + * free leader and this member together
> > > > + */
> > > > + leader = get_group_leader(req);
> > > > + leader->flags &= ~REQ_F_SQE_GROUP_LEADER;
> > > > + req->flags &= ~REQ_F_SQE_GROUP;
> > > > + wq_stack_add_head(&leader->comp_list,
> > > > + &req->comp_list);
> > >
> > > That's quite hacky, but at least we can replace it with
> > > task work if it gets in the way later on.
> >
> > io_free_batch_list() is already called in task context, and it isn't
> > necessary to schedule one extra tw, which hurts perf more or less.
> >
> > Another way is to store these leaders into one temp list, and
> > call io_free_batch_list() for this temp list one more time.
>
> What I'm saying, it's fine to leave it as is for now. In the
> future if it becomes a problem for ome reason or another, we can
> do it the task_work like way.
OK, got it, I will leave it as is, and document the potential risk
with future changes.
>
> ...
> > > > @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
> > > > return def->prep(req, sqe);
> > > > }
> > > > +static struct io_kiocb *io_group_sqe(struct io_submit_link *group,
> > > > + struct io_kiocb *req)
> > > > +{
> > > > + /*
> > > > + * Group chain is similar with link chain: starts with 1st sqe with
> > > > + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP
> > > > + */
> > > > + if (group->head) {
> > > > + struct io_kiocb *lead = group->head;
> > > > +
> > > > + /* members can't be in link chain, can't be drained */
> > > > + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN))
> > > > + req_fail_link_node(lead, -EINVAL);
> > >
> > > That should fail the entire link (if any) as well.
> >
> > Good catch, here we should fail link head by following the logic
> > in io_submit_fail_init().
> >
> > >
> > > I have even more doubts we even want to mix links and groups. Apart
> >
> > Wrt. ublk, group provides zero copy, and the ublk io(group) is generic
> > IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd,
> > send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK
> > for generic io_uring IO.
> >
> > > from nuances as such, which would be quite hard to track, the semantics
> > > of IOSQE_CQE_SKIP_SUCCESS is unclear.
> >
> > IO group just follows every normal request.
>
> It tries to mimic but groups don't and essentially can't do it the
> same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS
> usually means that all following will be silenced. What if a
> member is CQE_SKIP, should it stop the leader from posting a CQE?
> And whatever the answer is, it'll be different from the link's
> behaviour.
Here it looks easier than link's:
- only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule
- all members just respects the flag for its own, and not related with
leader's
>
> Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts
> for groups, that can be discussed afterwards.
It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do
it in V6.
I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which
covers all linked sqes, and group leader could be just one of them.
Can you share any idea about the implementation to forbid LINK_TIMEOUT
for sqe group?
>
> > 1) fail in linked chain
> > - follows IO_LINK's behavior since io_fail_links() covers io group
> >
> > 2) otherwise
> > - just respect IOSQE_CQE_SKIP_SUCCESS
> >
> > > And also it doen't work with IORING_OP_LINK_TIMEOUT.
> >
> > REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I
> > will document it in V6.
>
> It would still be troublesome. When a linked timeout fires it searches
> for the request it's attached to and cancels it, however, group leaders
> that queued up their members are discoverable. But let's say you can find
> them in some way, then the only sensbile thing to do is cancel members,
> which should be doable by checking req->grp_leader, but might be easier
> to leave it to follow up patches.
We have changed sqe group to start queuing members after leader is
completed. link timeout will cancel leader with all its members via
leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT
completely.
Please see io_fail_links() and io_cancel_group_members().
>
>
> > > > +
> > > > + lead->grp_refs += 1;
> > > > + group->last->grp_link = req;
> > > > + group->last = req;
> > > > +
> > > > + if (req->flags & REQ_F_SQE_GROUP)
> > > > + return NULL;
> > > > +
> > > > + req->grp_link = NULL;
> > > > + req->flags |= REQ_F_SQE_GROUP;
> > > > + group->head = NULL;
> > > > + if (lead->flags & REQ_F_FAIL) {
> > > > + io_queue_sqe_fallback(lead);
> > >
> > > Let's say the group was in the middle of a link, it'll
> > > complete that group and continue with assembling / executing
> > > the link when it should've failed it and honoured the
> > > request order.
> >
> > OK, here we can simply remove the above two lines, and link submit
> > state can handle this failure in link chain.
>
> If you just delete then nobody would check for REQ_F_FAIL and
> fail the request.
io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call
io_queue_sqe_fallback() either if it is in link chain or
not.
> Assuming you'd also set the fail flag to the
> link head when appropriate, how about deleting these two line
> and do like below? (can be further prettified)
>
>
> bool io_group_assembling()
> {
> return state->group.head || (req->flags & REQ_F_SQE_GROUP);
> }
> bool io_link_assembling()
> {
> return state->link.head || (req->flags & IO_REQ_LINK_FLAGS);
> }
>
> static inline int io_submit_sqe()
> {
> ...
> if (unlikely(io_link_assembling(state, req) ||
> io_group_assembling(state, req) ||
> req->flags & REQ_F_FAIL)) {
> if (io_group_assembling(state, req)) {
> req = io_group_sqe(&state->group, req);
> if (!req)
> return 0;
> }
> if (io_link_assembling(state, req)) {
> req = io_link_sqe(&state->link, req);
> if (!req)
> return 0;
> }
> if (req->flags & REQ_F_FAIL) {
> io_queue_sqe_fallback(req);
> return 0;
As I mentioned above, io_link_assembling() & io_link_sqe() covers
the failure handling.
thanks,
Ming
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 4/8] io_uring: support SQE group
2024-09-07 9:36 ` Ming Lei
@ 2024-09-10 13:12 ` Pavel Begunkov
2024-09-10 15:04 ` Ming Lei
0 siblings, 1 reply; 19+ messages in thread
From: Pavel Begunkov @ 2024-09-10 13:12 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, io-uring, linux-block, Kevin Wolf
On 9/7/24 10:36, Ming Lei wrote:
...
>>> Wrt. ublk, group provides zero copy, and the ublk io(group) is generic
>>> IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd,
>>> send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK
>>> for generic io_uring IO.
>>>
>>>> from nuances as such, which would be quite hard to track, the semantics
>>>> of IOSQE_CQE_SKIP_SUCCESS is unclear.
>>>
>>> IO group just follows every normal request.
>>
>> It tries to mimic but groups don't and essentially can't do it the
>> same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS
>> usually means that all following will be silenced. What if a
>> member is CQE_SKIP, should it stop the leader from posting a CQE?
>> And whatever the answer is, it'll be different from the link's
>> behaviour.
>
> Here it looks easier than link's:
>
> - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule
> - all members just respects the flag for its own, and not related with
> leader's
>
>>
>> Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts
>> for groups, that can be discussed afterwards.
>
> It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do
> it in V6.
>
> I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which
> covers all linked sqes, and group leader could be just one of them.
> Can you share any idea about the implementation to forbid LINK_TIMEOUT
> for sqe group?
diff --git a/io_uring/timeout.c b/io_uring/timeout.c
index 671d6093bf36..83b5fd64b4e9 100644
--- a/io_uring/timeout.c
+++ b/io_uring/timeout.c
@@ -542,6 +542,9 @@ static int __io_timeout_prep(struct io_kiocb *req,
data->mode = io_translate_timeout_mode(flags);
hrtimer_init(&data->timer, io_timeout_get_clock(data), data->mode);
+ if (is_timeout_link && req->ctx->submit_state.group.head)
+ return -EINVAL;
+
if (is_timeout_link) {
struct io_submit_link *link = &req->ctx->submit_state.link;
This should do, they already look into the ctx's link list. Just move
it into the "if (is_timeout_link)" block.
>>> 1) fail in linked chain
>>> - follows IO_LINK's behavior since io_fail_links() covers io group
>>>
>>> 2) otherwise
>>> - just respect IOSQE_CQE_SKIP_SUCCESS
>>>
>>>> And also it doen't work with IORING_OP_LINK_TIMEOUT.
>>>
>>> REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I
>>> will document it in V6.
>>
>> It would still be troublesome. When a linked timeout fires it searches
>> for the request it's attached to and cancels it, however, group leaders
>> that queued up their members are discoverable. But let's say you can find
>> them in some way, then the only sensbile thing to do is cancel members,
>> which should be doable by checking req->grp_leader, but might be easier
>> to leave it to follow up patches.
>
> We have changed sqe group to start queuing members after leader is
> completed. link timeout will cancel leader with all its members via
> leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT
> completely.
>
> Please see io_fail_links() and io_cancel_group_members().
>
>>
>>
>>>>> +
>>>>> + lead->grp_refs += 1;
>>>>> + group->last->grp_link = req;
>>>>> + group->last = req;
>>>>> +
>>>>> + if (req->flags & REQ_F_SQE_GROUP)
>>>>> + return NULL;
>>>>> +
>>>>> + req->grp_link = NULL;
>>>>> + req->flags |= REQ_F_SQE_GROUP;
>>>>> + group->head = NULL;
>>>>> + if (lead->flags & REQ_F_FAIL) {
>>>>> + io_queue_sqe_fallback(lead);
>>>>
>>>> Let's say the group was in the middle of a link, it'll
>>>> complete that group and continue with assembling / executing
>>>> the link when it should've failed it and honoured the
>>>> request order.
>>>
>>> OK, here we can simply remove the above two lines, and link submit
>>> state can handle this failure in link chain.
>>
>> If you just delete then nobody would check for REQ_F_FAIL and
>> fail the request.
>
> io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call
> io_queue_sqe_fallback() either if it is in link chain or
> not.
The case we're talking about is failing a group, which is
also in the middle of a link.
LINK_HEAD -> {GROUP_LEAD, GROUP_MEMBER}
Let's say GROUP_MEMBER fails and sets REQ_F_FAIL to the lead,
then in v5 does:
if (lead->flags & REQ_F_FAIL) {
io_queue_sqe_fallback(lead);
return NULL;
}
In which case it posts cqes for GROUP_LEAD and GROUP_MEMBER,
and then try to execute LINK_HEAD (without failing it), which
is wrong. So first we need:
if (state.linked_link.head)
req_fail_link_node(state.linked_link.head);
And then we can't just remove io_queue_sqe_fallback(), because
when a group is not linked there would be no io_link_sqe()
to fail it. You can do:
io_group_sqe()
{
if ((lead->flags & REQ_F_FAIL) && !ctx->state.link.head) {
io_queue_sqe_fallback(lead);
return NULL;
}
...
}
but it's much cleaner to move REQ_F_FAIL out of group assembling.
We'd also want to move same REQ_F_FAIL / io_queue_sqe_fallback()
out of io_link_sqe(), but I didn't mentioned because it's not
strictly required for your set AFAIR.
>> Assuming you'd also set the fail flag to the
>> link head when appropriate, how about deleting these two line
>> and do like below? (can be further prettified)
>>
>>
>> bool io_group_assembling()
>> {
>> return state->group.head || (req->flags & REQ_F_SQE_GROUP);
>> }
>> bool io_link_assembling()
>> {
>> return state->link.head || (req->flags & IO_REQ_LINK_FLAGS);
>> }
>>
>> static inline int io_submit_sqe()
>> {
>> ...
>> if (unlikely(io_link_assembling(state, req) ||
>> io_group_assembling(state, req) ||
>> req->flags & REQ_F_FAIL)) {
>> if (io_group_assembling(state, req)) {
>> req = io_group_sqe(&state->group, req);
>> if (!req)
>> return 0;
>> }
>> if (io_link_assembling(state, req)) {
>> req = io_link_sqe(&state->link, req);
>> if (!req)
>> return 0;
>> }
>> if (req->flags & REQ_F_FAIL) {
>> io_queue_sqe_fallback(req);
>> return 0;
>
> As I mentioned above, io_link_assembling() & io_link_sqe() covers
> the failure handling.
--
Pavel Begunkov
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V5 4/8] io_uring: support SQE group
2024-09-10 13:12 ` Pavel Begunkov
@ 2024-09-10 15:04 ` Ming Lei
2024-09-10 20:31 ` Pavel Begunkov
0 siblings, 1 reply; 19+ messages in thread
From: Ming Lei @ 2024-09-10 15:04 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: Jens Axboe, io-uring, linux-block, Kevin Wolf, ming.lei
On Tue, Sep 10, 2024 at 02:12:53PM +0100, Pavel Begunkov wrote:
> On 9/7/24 10:36, Ming Lei wrote:
> ...
> > > > Wrt. ublk, group provides zero copy, and the ublk io(group) is generic
> > > > IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd,
> > > > send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK
> > > > for generic io_uring IO.
> > > >
> > > > > from nuances as such, which would be quite hard to track, the semantics
> > > > > of IOSQE_CQE_SKIP_SUCCESS is unclear.
> > > >
> > > > IO group just follows every normal request.
> > >
> > > It tries to mimic but groups don't and essentially can't do it the
> > > same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS
> > > usually means that all following will be silenced. What if a
> > > member is CQE_SKIP, should it stop the leader from posting a CQE?
> > > And whatever the answer is, it'll be different from the link's
> > > behaviour.
> >
> > Here it looks easier than link's:
> >
> > - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule
> > - all members just respects the flag for its own, and not related with
> > leader's
> >
> > >
> > > Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts
> > > for groups, that can be discussed afterwards.
> >
> > It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do
> > it in V6.
> >
> > I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which
> > covers all linked sqes, and group leader could be just one of them.
> > Can you share any idea about the implementation to forbid LINK_TIMEOUT
> > for sqe group?
>
> diff --git a/io_uring/timeout.c b/io_uring/timeout.c
> index 671d6093bf36..83b5fd64b4e9 100644
> --- a/io_uring/timeout.c
> +++ b/io_uring/timeout.c
> @@ -542,6 +542,9 @@ static int __io_timeout_prep(struct io_kiocb *req,
> data->mode = io_translate_timeout_mode(flags);
> hrtimer_init(&data->timer, io_timeout_get_clock(data), data->mode);
> + if (is_timeout_link && req->ctx->submit_state.group.head)
> + return -EINVAL;
> +
> if (is_timeout_link) {
> struct io_submit_link *link = &req->ctx->submit_state.link;
>
> This should do, they already look into the ctx's link list. Just move
> it into the "if (is_timeout_link)" block.
OK.
>
>
> > > > 1) fail in linked chain
> > > > - follows IO_LINK's behavior since io_fail_links() covers io group
> > > >
> > > > 2) otherwise
> > > > - just respect IOSQE_CQE_SKIP_SUCCESS
> > > >
> > > > > And also it doen't work with IORING_OP_LINK_TIMEOUT.
> > > >
> > > > REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I
> > > > will document it in V6.
> > >
> > > It would still be troublesome. When a linked timeout fires it searches
> > > for the request it's attached to and cancels it, however, group leaders
> > > that queued up their members are discoverable. But let's say you can find
> > > them in some way, then the only sensbile thing to do is cancel members,
> > > which should be doable by checking req->grp_leader, but might be easier
> > > to leave it to follow up patches.
> >
> > We have changed sqe group to start queuing members after leader is
> > completed. link timeout will cancel leader with all its members via
> > leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT
> > completely.
> >
> > Please see io_fail_links() and io_cancel_group_members().
> >
> > >
> > >
> > > > > > +
> > > > > > + lead->grp_refs += 1;
> > > > > > + group->last->grp_link = req;
> > > > > > + group->last = req;
> > > > > > +
> > > > > > + if (req->flags & REQ_F_SQE_GROUP)
> > > > > > + return NULL;
> > > > > > +
> > > > > > + req->grp_link = NULL;
> > > > > > + req->flags |= REQ_F_SQE_GROUP;
> > > > > > + group->head = NULL;
> > > > > > + if (lead->flags & REQ_F_FAIL) {
> > > > > > + io_queue_sqe_fallback(lead);
> > > > >
> > > > > Let's say the group was in the middle of a link, it'll
> > > > > complete that group and continue with assembling / executing
> > > > > the link when it should've failed it and honoured the
> > > > > request order.
> > > >
> > > > OK, here we can simply remove the above two lines, and link submit
> > > > state can handle this failure in link chain.
> > >
> > > If you just delete then nobody would check for REQ_F_FAIL and
> > > fail the request.
> >
> > io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call
> > io_queue_sqe_fallback() either if it is in link chain or
> > not.
>
> The case we're talking about is failing a group, which is
> also in the middle of a link.
>
> LINK_HEAD -> {GROUP_LEAD, GROUP_MEMBER}
>
> Let's say GROUP_MEMBER fails and sets REQ_F_FAIL to the lead,
> then in v5 does:
>
> if (lead->flags & REQ_F_FAIL) {
> io_queue_sqe_fallback(lead);
> return NULL;
> }
>
> In which case it posts cqes for GROUP_LEAD and GROUP_MEMBER,
> and then try to execute LINK_HEAD (without failing it), which
> is wrong. So first we need:
>
> if (state.linked_link.head)
> req_fail_link_node(state.linked_link.head);
For group leader, link advancing is always done via io_queue_next(), in
which io_disarm_next() is called for failing the whole remained link
if the current request is marked as FAIL.
>
> And then we can't just remove io_queue_sqe_fallback(), because
> when a group is not linked there would be no io_link_sqe()
> to fail it. You can do:
If one request in group is marked as FAIL, io_link_assembling()
will return true, and io_link_sqe() will fail it.
Thanks,
Ming
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 4/8] io_uring: support SQE group
2024-09-10 15:04 ` Ming Lei
@ 2024-09-10 20:31 ` Pavel Begunkov
2024-09-11 1:28 ` Ming Lei
0 siblings, 1 reply; 19+ messages in thread
From: Pavel Begunkov @ 2024-09-10 20:31 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, io-uring, linux-block, Kevin Wolf
On 9/10/24 16:04, Ming Lei wrote:
> On Tue, Sep 10, 2024 at 02:12:53PM +0100, Pavel Begunkov wrote:
>> On 9/7/24 10:36, Ming Lei wrote:
>> ...
>>>>> Wrt. ublk, group provides zero copy, and the ublk io(group) is generic
>>>>> IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd,
>>>>> send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK
>>>>> for generic io_uring IO.
>>>>>
>>>>>> from nuances as such, which would be quite hard to track, the semantics
>>>>>> of IOSQE_CQE_SKIP_SUCCESS is unclear.
>>>>>
>>>>> IO group just follows every normal request.
>>>>
>>>> It tries to mimic but groups don't and essentially can't do it the
>>>> same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS
>>>> usually means that all following will be silenced. What if a
>>>> member is CQE_SKIP, should it stop the leader from posting a CQE?
>>>> And whatever the answer is, it'll be different from the link's
>>>> behaviour.
>>>
>>> Here it looks easier than link's:
>>>
>>> - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule
>>> - all members just respects the flag for its own, and not related with
>>> leader's
>>>
>>>>
>>>> Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts
>>>> for groups, that can be discussed afterwards.
>>>
>>> It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do
>>> it in V6.
>>>
>>> I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which
>>> covers all linked sqes, and group leader could be just one of them.
>>> Can you share any idea about the implementation to forbid LINK_TIMEOUT
>>> for sqe group?
>>
>> diff --git a/io_uring/timeout.c b/io_uring/timeout.c
>> index 671d6093bf36..83b5fd64b4e9 100644
>> --- a/io_uring/timeout.c
>> +++ b/io_uring/timeout.c
>> @@ -542,6 +542,9 @@ static int __io_timeout_prep(struct io_kiocb *req,
>> data->mode = io_translate_timeout_mode(flags);
>> hrtimer_init(&data->timer, io_timeout_get_clock(data), data->mode);
>> + if (is_timeout_link && req->ctx->submit_state.group.head)
>> + return -EINVAL;
>> +
>> if (is_timeout_link) {
>> struct io_submit_link *link = &req->ctx->submit_state.link;
>>
>> This should do, they already look into the ctx's link list. Just move
>> it into the "if (is_timeout_link)" block.
>
> OK.
>
>>
>>
>>>>> 1) fail in linked chain
>>>>> - follows IO_LINK's behavior since io_fail_links() covers io group
>>>>>
>>>>> 2) otherwise
>>>>> - just respect IOSQE_CQE_SKIP_SUCCESS
>>>>>
>>>>>> And also it doen't work with IORING_OP_LINK_TIMEOUT.
>>>>>
>>>>> REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I
>>>>> will document it in V6.
>>>>
>>>> It would still be troublesome. When a linked timeout fires it searches
>>>> for the request it's attached to and cancels it, however, group leaders
>>>> that queued up their members are discoverable. But let's say you can find
>>>> them in some way, then the only sensbile thing to do is cancel members,
>>>> which should be doable by checking req->grp_leader, but might be easier
>>>> to leave it to follow up patches.
>>>
>>> We have changed sqe group to start queuing members after leader is
>>> completed. link timeout will cancel leader with all its members via
>>> leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT
>>> completely.
>>>
>>> Please see io_fail_links() and io_cancel_group_members().
>>>
>>>>
>>>>
>>>>>>> +
>>>>>>> + lead->grp_refs += 1;
>>>>>>> + group->last->grp_link = req;
>>>>>>> + group->last = req;
>>>>>>> +
>>>>>>> + if (req->flags & REQ_F_SQE_GROUP)
>>>>>>> + return NULL;
>>>>>>> +
>>>>>>> + req->grp_link = NULL;
>>>>>>> + req->flags |= REQ_F_SQE_GROUP;
>>>>>>> + group->head = NULL;
>>>>>>> + if (lead->flags & REQ_F_FAIL) {
>>>>>>> + io_queue_sqe_fallback(lead);
>>>>>>
>>>>>> Let's say the group was in the middle of a link, it'll
>>>>>> complete that group and continue with assembling / executing
>>>>>> the link when it should've failed it and honoured the
>>>>>> request order.
>>>>>
>>>>> OK, here we can simply remove the above two lines, and link submit
>>>>> state can handle this failure in link chain.
>>>>
>>>> If you just delete then nobody would check for REQ_F_FAIL and
>>>> fail the request.
>>>
>>> io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call
>>> io_queue_sqe_fallback() either if it is in link chain or
>>> not.
>>
>> The case we're talking about is failing a group, which is
>> also in the middle of a link.
>>
>> LINK_HEAD -> {GROUP_LEAD, GROUP_MEMBER}
>>
>> Let's say GROUP_MEMBER fails and sets REQ_F_FAIL to the lead,
>> then in v5 does:
>>
>> if (lead->flags & REQ_F_FAIL) {
>> io_queue_sqe_fallback(lead);
>> return NULL;
>> }
>>
>> In which case it posts cqes for GROUP_LEAD and GROUP_MEMBER,
>> and then try to execute LINK_HEAD (without failing it), which
>> is wrong. So first we need:
>>
>> if (state.linked_link.head)
>> req_fail_link_node(state.linked_link.head);
>
> For group leader, link advancing is always done via io_queue_next(), in
> which io_disarm_next() is called for failing the whole remained link
> if the current request is marked as FAIL.
>
>>
>> And then we can't just remove io_queue_sqe_fallback(), because
>> when a group is not linked there would be no io_link_sqe()
>> to fail it. You can do:
>
> If one request in group is marked as FAIL, io_link_assembling()
> will return true, and io_link_sqe() will fail it.
Hmm, you're right, even though it's not a great way of doing it,
i.e. pushing a req into io_link_sqe() even when linking has never
been requested, but that's fine. I can drop a quick patch on
top if it bothers me.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 4/8] io_uring: support SQE group
2024-09-10 20:31 ` Pavel Begunkov
@ 2024-09-11 1:28 ` Ming Lei
0 siblings, 0 replies; 19+ messages in thread
From: Ming Lei @ 2024-09-11 1:28 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: Jens Axboe, io-uring, linux-block, Kevin Wolf
On Tue, Sep 10, 2024 at 09:31:45PM +0100, Pavel Begunkov wrote:
> On 9/10/24 16:04, Ming Lei wrote:
> > On Tue, Sep 10, 2024 at 02:12:53PM +0100, Pavel Begunkov wrote:
> > > On 9/7/24 10:36, Ming Lei wrote:
> > > ...
> > > > > > Wrt. ublk, group provides zero copy, and the ublk io(group) is generic
> > > > > > IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd,
> > > > > > send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK
> > > > > > for generic io_uring IO.
> > > > > >
> > > > > > > from nuances as such, which would be quite hard to track, the semantics
> > > > > > > of IOSQE_CQE_SKIP_SUCCESS is unclear.
> > > > > >
> > > > > > IO group just follows every normal request.
> > > > >
> > > > > It tries to mimic but groups don't and essentially can't do it the
> > > > > same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS
> > > > > usually means that all following will be silenced. What if a
> > > > > member is CQE_SKIP, should it stop the leader from posting a CQE?
> > > > > And whatever the answer is, it'll be different from the link's
> > > > > behaviour.
> > > >
> > > > Here it looks easier than link's:
> > > >
> > > > - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule
> > > > - all members just respects the flag for its own, and not related with
> > > > leader's
> > > >
> > > > >
> > > > > Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts
> > > > > for groups, that can be discussed afterwards.
> > > >
> > > > It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do
> > > > it in V6.
> > > >
> > > > I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which
> > > > covers all linked sqes, and group leader could be just one of them.
> > > > Can you share any idea about the implementation to forbid LINK_TIMEOUT
> > > > for sqe group?
> > >
> > > diff --git a/io_uring/timeout.c b/io_uring/timeout.c
> > > index 671d6093bf36..83b5fd64b4e9 100644
> > > --- a/io_uring/timeout.c
> > > +++ b/io_uring/timeout.c
> > > @@ -542,6 +542,9 @@ static int __io_timeout_prep(struct io_kiocb *req,
> > > data->mode = io_translate_timeout_mode(flags);
> > > hrtimer_init(&data->timer, io_timeout_get_clock(data), data->mode);
> > > + if (is_timeout_link && req->ctx->submit_state.group.head)
> > > + return -EINVAL;
> > > +
> > > if (is_timeout_link) {
> > > struct io_submit_link *link = &req->ctx->submit_state.link;
> > >
> > > This should do, they already look into the ctx's link list. Just move
> > > it into the "if (is_timeout_link)" block.
> >
> > OK.
> >
> > >
> > >
> > > > > > 1) fail in linked chain
> > > > > > - follows IO_LINK's behavior since io_fail_links() covers io group
> > > > > >
> > > > > > 2) otherwise
> > > > > > - just respect IOSQE_CQE_SKIP_SUCCESS
> > > > > >
> > > > > > > And also it doen't work with IORING_OP_LINK_TIMEOUT.
> > > > > >
> > > > > > REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I
> > > > > > will document it in V6.
> > > > >
> > > > > It would still be troublesome. When a linked timeout fires it searches
> > > > > for the request it's attached to and cancels it, however, group leaders
> > > > > that queued up their members are discoverable. But let's say you can find
> > > > > them in some way, then the only sensbile thing to do is cancel members,
> > > > > which should be doable by checking req->grp_leader, but might be easier
> > > > > to leave it to follow up patches.
> > > >
> > > > We have changed sqe group to start queuing members after leader is
> > > > completed. link timeout will cancel leader with all its members via
> > > > leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT
> > > > completely.
> > > >
> > > > Please see io_fail_links() and io_cancel_group_members().
> > > >
> > > > >
> > > > >
> > > > > > > > +
> > > > > > > > + lead->grp_refs += 1;
> > > > > > > > + group->last->grp_link = req;
> > > > > > > > + group->last = req;
> > > > > > > > +
> > > > > > > > + if (req->flags & REQ_F_SQE_GROUP)
> > > > > > > > + return NULL;
> > > > > > > > +
> > > > > > > > + req->grp_link = NULL;
> > > > > > > > + req->flags |= REQ_F_SQE_GROUP;
> > > > > > > > + group->head = NULL;
> > > > > > > > + if (lead->flags & REQ_F_FAIL) {
> > > > > > > > + io_queue_sqe_fallback(lead);
> > > > > > >
> > > > > > > Let's say the group was in the middle of a link, it'll
> > > > > > > complete that group and continue with assembling / executing
> > > > > > > the link when it should've failed it and honoured the
> > > > > > > request order.
> > > > > >
> > > > > > OK, here we can simply remove the above two lines, and link submit
> > > > > > state can handle this failure in link chain.
> > > > >
> > > > > If you just delete then nobody would check for REQ_F_FAIL and
> > > > > fail the request.
> > > >
> > > > io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call
> > > > io_queue_sqe_fallback() either if it is in link chain or
> > > > not.
> > >
> > > The case we're talking about is failing a group, which is
> > > also in the middle of a link.
> > >
> > > LINK_HEAD -> {GROUP_LEAD, GROUP_MEMBER}
> > >
> > > Let's say GROUP_MEMBER fails and sets REQ_F_FAIL to the lead,
> > > then in v5 does:
> > >
> > > if (lead->flags & REQ_F_FAIL) {
> > > io_queue_sqe_fallback(lead);
> > > return NULL;
> > > }
> > >
> > > In which case it posts cqes for GROUP_LEAD and GROUP_MEMBER,
> > > and then try to execute LINK_HEAD (without failing it), which
> > > is wrong. So first we need:
> > >
> > > if (state.linked_link.head)
> > > req_fail_link_node(state.linked_link.head);
> >
> > For group leader, link advancing is always done via io_queue_next(), in
> > which io_disarm_next() is called for failing the whole remained link
> > if the current request is marked as FAIL.
> >
> > >
> > > And then we can't just remove io_queue_sqe_fallback(), because
> > > when a group is not linked there would be no io_link_sqe()
> > > to fail it. You can do:
> >
> > If one request in group is marked as FAIL, io_link_assembling()
> > will return true, and io_link_sqe() will fail it.
>
> Hmm, you're right, even though it's not a great way of doing it,
> i.e. pushing a req into io_link_sqe() even when linking has never
> been requested, but that's fine. I can drop a quick patch on
> top if it bothers me.
Yeah, it isn't very readable, but following the original logic.
Anyway, I will comment that non-linked request is covered by
the code block.
Thanks,
Ming
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-09-11 1:28 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-08 16:24 [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
2024-08-08 16:24 ` [PATCH V5 1/8] io_uring: add io_link_req() helper Ming Lei
2024-08-08 16:24 ` [PATCH V5 2/8] io_uring: add io_submit_fail_link() helper Ming Lei
2024-08-08 16:24 ` [PATCH V5 3/8] io_uring: add helper of io_req_commit_cqe() Ming Lei
2024-08-08 16:24 ` [PATCH V5 4/8] io_uring: support SQE group Ming Lei
2024-08-27 15:18 ` Pavel Begunkov
2024-08-29 4:29 ` Ming Lei
2024-09-06 17:15 ` Pavel Begunkov
2024-09-07 9:36 ` Ming Lei
2024-09-10 13:12 ` Pavel Begunkov
2024-09-10 15:04 ` Ming Lei
2024-09-10 20:31 ` Pavel Begunkov
2024-09-11 1:28 ` Ming Lei
2024-08-08 16:24 ` [PATCH V5 5/8] io_uring: support sqe group with members depending on leader Ming Lei
2024-08-08 16:24 ` [PATCH V5 6/8] io_uring: support providing sqe group buffer Ming Lei
2024-08-08 16:24 ` [PATCH V5 7/8] io_uring/uring_cmd: support provide group kernel buffer Ming Lei
2024-08-08 16:24 ` [PATCH V5 8/8] ublk: support provide io buffer Ming Lei
2024-08-17 4:16 ` [PATCH V5 0/8] io_uring: support sqe group and provide group kbuf Ming Lei
2024-08-17 19:48 ` Pavel Begunkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox