* [PATCH v6 1/8] ublk: have a per-io daemon instead of a per-queue daemon
2025-05-07 21:49 [PATCH v6 0/8] ublk: decouple server threads from hctxs Uday Shankar
@ 2025-05-07 21:49 ` Uday Shankar
2025-05-09 3:29 ` Ming Lei
2025-05-11 0:17 ` Caleb Sander Mateos
2025-05-07 21:49 ` [PATCH v6 2/8] sbitmap: fix off-by-one when wrapping hint Uday Shankar
` (6 subsequent siblings)
7 siblings, 2 replies; 22+ messages in thread
From: Uday Shankar @ 2025-05-07 21:49 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Caleb Sander Mateos, Andrew Morton,
Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc,
Uday Shankar
Currently, ublk_drv associates to each hardware queue (hctx) a unique
task (called the queue's ubq_daemon) which is allowed to issue
COMMIT_AND_FETCH commands against the hctx. If any other task attempts
to do so, the command fails immediately with EINVAL. When considered
together with the block layer architecture, the result is that for each
CPU C on the system, there is a unique ublk server thread which is
allowed to handle I/O submitted on CPU C. This can lead to suboptimal
performance under imbalanced load generation. For an extreme example,
suppose all the load is generated on CPUs mapping to a single ublk
server thread. Then that thread may be fully utilized and become the
bottleneck in the system, while other ublk server threads are totally
idle.
This issue can also be addressed directly in the ublk server without
kernel support by having threads dequeue I/Os and pass them around to
ensure even load. But this solution requires inter-thread communication
at least twice for each I/O (submission and completion), which is
generally a bad pattern for performance. The problem gets even worse
with zero copy, as more inter-thread communication would be required to
have the buffer register/unregister calls to come from the correct
thread.
Therefore, address this issue in ublk_drv by allowing each I/O to have
its own daemon task. Two I/Os in the same queue are now allowed to be
serviced by different daemon tasks - this was not possible before.
Imbalanced load can then be balanced across all ublk server threads as
follows:
- specifying the new UBLK_F_RR_TAGS flag when creating the ublk device,
to ensure round-robin tag allocation
- having the ublk server threads issue FETCH_REQs in a round-robin
manner. As a small toy example, consider a system with a single ublk
device having 2 queues, each of depth 4. A ublk server having 4
threads could issue its FETCH_REQs against this device as follows
(where each entry is the qid,tag pair that the FETCH_REQ targets):
ublk server thread: T0 T1 T2 T3
0,0 0,1 0,2 0,3
1,3 1,0 1,1 1,2
The combination of these two changes allow a ublk server to balance
load. For example, suppose a program quickly issues 4 I/Os concurrently.
Before this change, all 4 I/Os would have had to be serviced by the same
thread, and that thread may become a bottleneck. With this change, those
4 I/Os would get tags 0-3 for the same queue, and would thus spread out
evenly over all the ublk server threads. Peak bandwidth in CPU-limited
workloads under imbalanced load thus will increase.
The UBLK_F_RR_TAGS flag is also added as a feature, so that ublk servers
can essentially test for the presence of this patch and tailor their
behavior accordingly.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
---
drivers/block/ublk_drv.c | 82 ++++++++++++++++++++++---------------------
include/uapi/linux/ublk_cmd.h | 8 +++++
2 files changed, 50 insertions(+), 40 deletions(-)
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index cb612151e9a1de7630c49a2b78d278e005d52856..ff5beab32220ea96a1e15ab26c02f4883a6c3d6e 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -66,7 +66,8 @@
| UBLK_F_USER_COPY \
| UBLK_F_ZONED \
| UBLK_F_USER_RECOVERY_FAIL_IO \
- | UBLK_F_UPDATE_SIZE)
+ | UBLK_F_UPDATE_SIZE \
+ | UBLK_F_RR_TAGS)
#define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
| UBLK_F_USER_RECOVERY_REISSUE \
@@ -148,6 +149,8 @@ struct ublk_io {
/* valid if UBLK_IO_FLAG_OWNED_BY_SRV is set */
struct request *req;
};
+
+ struct task_struct *task;
};
struct ublk_queue {
@@ -155,11 +158,9 @@ struct ublk_queue {
int q_depth;
unsigned long flags;
- struct task_struct *ubq_daemon;
struct ublksrv_io_desc *io_cmd_buf;
bool force_abort;
- bool timeout;
bool canceling;
bool fail_io; /* copy of dev->state == UBLK_S_DEV_FAIL_IO */
unsigned short nr_io_ready; /* how many ios setup */
@@ -1069,11 +1070,6 @@ static inline struct ublk_uring_cmd_pdu *ublk_get_uring_cmd_pdu(
return io_uring_cmd_to_pdu(ioucmd, struct ublk_uring_cmd_pdu);
}
-static inline bool ubq_daemon_is_dying(struct ublk_queue *ubq)
-{
- return !ubq->ubq_daemon || ubq->ubq_daemon->flags & PF_EXITING;
-}
-
/* todo: handle partial completion */
static inline void __ublk_complete_rq(struct request *req)
{
@@ -1207,13 +1203,13 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
/*
* Task is exiting if either:
*
- * (1) current != ubq_daemon.
+ * (1) current != io->task.
* io_uring_cmd_complete_in_task() tries to run task_work
- * in a workqueue if ubq_daemon(cmd's task) is PF_EXITING.
+ * in a workqueue if cmd's task is PF_EXITING.
*
* (2) current->flags & PF_EXITING.
*/
- if (unlikely(current != ubq->ubq_daemon || current->flags & PF_EXITING)) {
+ if (unlikely(current != io->task || current->flags & PF_EXITING)) {
__ublk_abort_rq(ubq, req);
return;
}
@@ -1286,13 +1282,10 @@ static void ublk_queue_cmd_list(struct ublk_queue *ubq, struct rq_list *l)
static enum blk_eh_timer_return ublk_timeout(struct request *rq)
{
struct ublk_queue *ubq = rq->mq_hctx->driver_data;
+ struct ublk_io *io = &ubq->ios[rq->tag];
if (ubq->flags & UBLK_F_UNPRIVILEGED_DEV) {
- if (!ubq->timeout) {
- send_sig(SIGKILL, ubq->ubq_daemon, 0);
- ubq->timeout = true;
- }
-
+ send_sig(SIGKILL, io->task, 0);
return BLK_EH_DONE;
}
@@ -1405,17 +1398,6 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
/* All old ioucmds have to be completed */
ubq->nr_io_ready = 0;
- /*
- * old daemon is PF_EXITING, put it now
- *
- * It could be NULL in case of closing one quisced device.
- */
- if (ubq->ubq_daemon)
- put_task_struct(ubq->ubq_daemon);
- /* We have to reset it to NULL, otherwise ub won't accept new FETCH_REQ */
- ubq->ubq_daemon = NULL;
- ubq->timeout = false;
-
for (i = 0; i < ubq->q_depth; i++) {
struct ublk_io *io = &ubq->ios[i];
@@ -1426,6 +1408,17 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
io->flags &= UBLK_IO_FLAG_CANCELED;
io->cmd = NULL;
io->addr = 0;
+
+ /*
+ * old task is PF_EXITING, put it now
+ *
+ * It could be NULL in case of closing one quiesced
+ * device.
+ */
+ if (io->task) {
+ put_task_struct(io->task);
+ io->task = NULL;
+ }
}
}
@@ -1447,7 +1440,7 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
ublk_queue_reinit(ub, ublk_get_queue(ub, i));
- /* set to NULL, otherwise new ubq_daemon cannot mmap the io_cmd_buf */
+ /* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
ub->mm = NULL;
ub->nr_queues_ready = 0;
ub->nr_privileged_daemon = 0;
@@ -1722,7 +1715,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
return;
task = io_uring_cmd_get_task(cmd);
- if (WARN_ON_ONCE(task && task != ubq->ubq_daemon))
+ if (WARN_ON_ONCE(task && task != ubq->ios[pdu->tag].task))
return;
if (!ubq->canceling)
@@ -1861,8 +1854,6 @@ static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
{
ubq->nr_io_ready++;
if (ublk_queue_ready(ubq)) {
- ubq->ubq_daemon = current;
- get_task_struct(ubq->ubq_daemon);
ub->nr_queues_ready++;
if (capable(CAP_SYS_ADMIN))
@@ -1995,6 +1986,7 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
}
ublk_fill_io_cmd(io, cmd, buf_addr);
+ WRITE_ONCE(io->task, get_task_struct(current));
ublk_mark_io_ready(ub, ubq);
out:
mutex_unlock(&ub->mutex);
@@ -2062,6 +2054,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
const struct ublksrv_io_cmd *ub_cmd)
{
struct ublk_device *ub = cmd->file->private_data;
+ struct task_struct *task;
struct ublk_queue *ubq;
struct ublk_io *io;
u32 cmd_op = cmd->cmd_op;
@@ -2076,13 +2069,14 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
goto out;
ubq = ublk_get_queue(ub, ub_cmd->q_id);
- if (ubq->ubq_daemon && ubq->ubq_daemon != current)
- goto out;
if (tag >= ubq->q_depth)
goto out;
io = &ubq->ios[tag];
+ task = READ_ONCE(io->task);
+ if (task && task != current)
+ goto out;
/* there is pending io cmd, something must be wrong */
if (io->flags & UBLK_IO_FLAG_ACTIVE) {
@@ -2332,9 +2326,15 @@ static void ublk_deinit_queue(struct ublk_device *ub, int q_id)
{
int size = ublk_queue_cmd_buf_size(ub, q_id);
struct ublk_queue *ubq = ublk_get_queue(ub, q_id);
+ struct ublk_io *io;
+ int i;
+
+ for (i = 0; i < ubq->q_depth; i++) {
+ io = &ubq->ios[i];
+ if (io->task)
+ put_task_struct(io->task);
+ }
- if (ubq->ubq_daemon)
- put_task_struct(ubq->ubq_daemon);
if (ubq->io_cmd_buf)
free_pages((unsigned long)ubq->io_cmd_buf, get_order(size));
}
@@ -2487,6 +2487,8 @@ static int ublk_add_tag_set(struct ublk_device *ub)
ub->tag_set.numa_node = NUMA_NO_NODE;
ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
ub->tag_set.driver_data = ub;
+ if (ub->dev_info.flags & UBLK_F_RR_TAGS)
+ ub->tag_set.flags |= BLK_MQ_F_TAG_RR;
return blk_mq_alloc_tag_set(&ub->tag_set);
}
@@ -3062,14 +3064,14 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
int ublksrv_pid = (int)header->data[0];
int ret = -EINVAL;
- pr_devel("%s: Waiting for new ubq_daemons(nr: %d) are ready, dev id %d...\n",
- __func__, ub->dev_info.nr_hw_queues, header->dev_id);
- /* wait until new ubq_daemon sending all FETCH_REQ */
+ pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
+ header->dev_id);
+
if (wait_for_completion_interruptible(&ub->completion))
return -EINTR;
- pr_devel("%s: All new ubq_daemons(nr: %d) are ready, dev id %d\n",
- __func__, ub->dev_info.nr_hw_queues, header->dev_id);
+ pr_devel("%s: All FETCH_REQs received, dev id %d\n", __func__,
+ header->dev_id);
mutex_lock(&ub->mutex);
if (ublk_nosrv_should_stop_dev(ub))
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index be5c6c6b16e098838a2bf790e588b79656defdda..3ba8f26d60bc70ade95b4f4d5990157af2bd5b2e 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -219,6 +219,14 @@
*/
#define UBLK_F_UPDATE_SIZE (1ULL << 10)
+/*
+ * Force tags to be allocated round-robin on each queue. If ublk server
+ * threads also issue FETCH_REQs against the queues in a round-robin
+ * manner, load can be balanced across all threads even if it is issued
+ * to the queues in an imbalanced way.
+ */
+#define UBLK_F_RR_TAGS (1ULL << 11)
+
/* device state */
#define UBLK_S_DEV_DEAD 0
#define UBLK_S_DEV_LIVE 1
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v6 1/8] ublk: have a per-io daemon instead of a per-queue daemon
2025-05-07 21:49 ` [PATCH v6 1/8] ublk: have a per-io daemon instead of a per-queue daemon Uday Shankar
@ 2025-05-09 3:29 ` Ming Lei
2025-05-10 23:54 ` Caleb Sander Mateos
2025-05-11 0:17 ` Caleb Sander Mateos
1 sibling, 1 reply; 22+ messages in thread
From: Ming Lei @ 2025-05-09 3:29 UTC (permalink / raw)
To: Uday Shankar
Cc: Jens Axboe, Caleb Sander Mateos, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Wed, May 07, 2025 at 03:49:35PM -0600, Uday Shankar wrote:
> Currently, ublk_drv associates to each hardware queue (hctx) a unique
> task (called the queue's ubq_daemon) which is allowed to issue
> COMMIT_AND_FETCH commands against the hctx. If any other task attempts
> to do so, the command fails immediately with EINVAL. When considered
> together with the block layer architecture, the result is that for each
> CPU C on the system, there is a unique ublk server thread which is
> allowed to handle I/O submitted on CPU C. This can lead to suboptimal
> performance under imbalanced load generation. For an extreme example,
> suppose all the load is generated on CPUs mapping to a single ublk
> server thread. Then that thread may be fully utilized and become the
> bottleneck in the system, while other ublk server threads are totally
> idle.
>
> This issue can also be addressed directly in the ublk server without
> kernel support by having threads dequeue I/Os and pass them around to
> ensure even load. But this solution requires inter-thread communication
> at least twice for each I/O (submission and completion), which is
> generally a bad pattern for performance. The problem gets even worse
> with zero copy, as more inter-thread communication would be required to
> have the buffer register/unregister calls to come from the correct
> thread.
>
> Therefore, address this issue in ublk_drv by allowing each I/O to have
> its own daemon task. Two I/Os in the same queue are now allowed to be
> serviced by different daemon tasks - this was not possible before.
> Imbalanced load can then be balanced across all ublk server threads as
> follows:
>
> - specifying the new UBLK_F_RR_TAGS flag when creating the ublk device,
> to ensure round-robin tag allocation
> - having the ublk server threads issue FETCH_REQs in a round-robin
> manner. As a small toy example, consider a system with a single ublk
> device having 2 queues, each of depth 4. A ublk server having 4
> threads could issue its FETCH_REQs against this device as follows
> (where each entry is the qid,tag pair that the FETCH_REQ targets):
>
> ublk server thread: T0 T1 T2 T3
> 0,0 0,1 0,2 0,3
> 1,3 1,0 1,1 1,2
>
> The combination of these two changes allow a ublk server to balance
> load. For example, suppose a program quickly issues 4 I/Os concurrently.
> Before this change, all 4 I/Os would have had to be serviced by the same
> thread, and that thread may become a bottleneck. With this change, those
> 4 I/Os would get tags 0-3 for the same queue, and would thus spread out
> evenly over all the ublk server threads. Peak bandwidth in CPU-limited
> workloads under imbalanced load thus will increase.
>
> The UBLK_F_RR_TAGS flag is also added as a feature, so that ublk servers
> can essentially test for the presence of this patch and tailor their
> behavior accordingly.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
> ---
> drivers/block/ublk_drv.c | 82 ++++++++++++++++++++++---------------------
> include/uapi/linux/ublk_cmd.h | 8 +++++
> 2 files changed, 50 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index cb612151e9a1de7630c49a2b78d278e005d52856..ff5beab32220ea96a1e15ab26c02f4883a6c3d6e 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -66,7 +66,8 @@
> | UBLK_F_USER_COPY \
> | UBLK_F_ZONED \
> | UBLK_F_USER_RECOVERY_FAIL_IO \
> - | UBLK_F_UPDATE_SIZE)
> + | UBLK_F_UPDATE_SIZE \
> + | UBLK_F_RR_TAGS)
>
> #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> | UBLK_F_USER_RECOVERY_REISSUE \
> @@ -148,6 +149,8 @@ struct ublk_io {
> /* valid if UBLK_IO_FLAG_OWNED_BY_SRV is set */
> struct request *req;
> };
> +
> + struct task_struct *task;
> };
>
> struct ublk_queue {
> @@ -155,11 +158,9 @@ struct ublk_queue {
> int q_depth;
>
> unsigned long flags;
> - struct task_struct *ubq_daemon;
> struct ublksrv_io_desc *io_cmd_buf;
>
> bool force_abort;
> - bool timeout;
> bool canceling;
> bool fail_io; /* copy of dev->state == UBLK_S_DEV_FAIL_IO */
> unsigned short nr_io_ready; /* how many ios setup */
> @@ -1069,11 +1070,6 @@ static inline struct ublk_uring_cmd_pdu *ublk_get_uring_cmd_pdu(
> return io_uring_cmd_to_pdu(ioucmd, struct ublk_uring_cmd_pdu);
> }
>
> -static inline bool ubq_daemon_is_dying(struct ublk_queue *ubq)
> -{
> - return !ubq->ubq_daemon || ubq->ubq_daemon->flags & PF_EXITING;
> -}
> -
> /* todo: handle partial completion */
> static inline void __ublk_complete_rq(struct request *req)
> {
> @@ -1207,13 +1203,13 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> /*
> * Task is exiting if either:
> *
> - * (1) current != ubq_daemon.
> + * (1) current != io->task.
> * io_uring_cmd_complete_in_task() tries to run task_work
> - * in a workqueue if ubq_daemon(cmd's task) is PF_EXITING.
> + * in a workqueue if cmd's task is PF_EXITING.
> *
> * (2) current->flags & PF_EXITING.
> */
> - if (unlikely(current != ubq->ubq_daemon || current->flags & PF_EXITING)) {
> + if (unlikely(current != io->task || current->flags & PF_EXITING)) {
> __ublk_abort_rq(ubq, req);
> return;
> }
> @@ -1286,13 +1282,10 @@ static void ublk_queue_cmd_list(struct ublk_queue *ubq, struct rq_list *l)
> static enum blk_eh_timer_return ublk_timeout(struct request *rq)
> {
> struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> + struct ublk_io *io = &ubq->ios[rq->tag];
>
> if (ubq->flags & UBLK_F_UNPRIVILEGED_DEV) {
> - if (!ubq->timeout) {
> - send_sig(SIGKILL, ubq->ubq_daemon, 0);
> - ubq->timeout = true;
> - }
> -
> + send_sig(SIGKILL, io->task, 0);
> return BLK_EH_DONE;
> }
>
> @@ -1405,17 +1398,6 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
> /* All old ioucmds have to be completed */
> ubq->nr_io_ready = 0;
>
> - /*
> - * old daemon is PF_EXITING, put it now
> - *
> - * It could be NULL in case of closing one quisced device.
> - */
> - if (ubq->ubq_daemon)
> - put_task_struct(ubq->ubq_daemon);
> - /* We have to reset it to NULL, otherwise ub won't accept new FETCH_REQ */
> - ubq->ubq_daemon = NULL;
> - ubq->timeout = false;
> -
> for (i = 0; i < ubq->q_depth; i++) {
> struct ublk_io *io = &ubq->ios[i];
>
> @@ -1426,6 +1408,17 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
> io->flags &= UBLK_IO_FLAG_CANCELED;
> io->cmd = NULL;
> io->addr = 0;
> +
> + /*
> + * old task is PF_EXITING, put it now
> + *
> + * It could be NULL in case of closing one quiesced
> + * device.
> + */
> + if (io->task) {
> + put_task_struct(io->task);
> + io->task = NULL;
> + }
> }
> }
>
> @@ -1447,7 +1440,7 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
> for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
> ublk_queue_reinit(ub, ublk_get_queue(ub, i));
>
> - /* set to NULL, otherwise new ubq_daemon cannot mmap the io_cmd_buf */
> + /* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
> ub->mm = NULL;
> ub->nr_queues_ready = 0;
> ub->nr_privileged_daemon = 0;
> @@ -1722,7 +1715,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> return;
>
> task = io_uring_cmd_get_task(cmd);
> - if (WARN_ON_ONCE(task && task != ubq->ubq_daemon))
> + if (WARN_ON_ONCE(task && task != ubq->ios[pdu->tag].task))
> return;
>
> if (!ubq->canceling)
> @@ -1861,8 +1854,6 @@ static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
> {
> ubq->nr_io_ready++;
> if (ublk_queue_ready(ubq)) {
> - ubq->ubq_daemon = current;
> - get_task_struct(ubq->ubq_daemon);
> ub->nr_queues_ready++;
>
> if (capable(CAP_SYS_ADMIN))
> @@ -1995,6 +1986,7 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
> }
>
> ublk_fill_io_cmd(io, cmd, buf_addr);
> + WRITE_ONCE(io->task, get_task_struct(current));
> ublk_mark_io_ready(ub, ubq);
> out:
> mutex_unlock(&ub->mutex);
> @@ -2062,6 +2054,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> const struct ublksrv_io_cmd *ub_cmd)
> {
> struct ublk_device *ub = cmd->file->private_data;
> + struct task_struct *task;
> struct ublk_queue *ubq;
> struct ublk_io *io;
> u32 cmd_op = cmd->cmd_op;
> @@ -2076,13 +2069,14 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> goto out;
>
> ubq = ublk_get_queue(ub, ub_cmd->q_id);
> - if (ubq->ubq_daemon && ubq->ubq_daemon != current)
> - goto out;
>
> if (tag >= ubq->q_depth)
> goto out;
>
> io = &ubq->ios[tag];
> + task = READ_ONCE(io->task);
> + if (task && task != current)
> + goto out;
>
> /* there is pending io cmd, something must be wrong */
> if (io->flags & UBLK_IO_FLAG_ACTIVE) {
> @@ -2332,9 +2326,15 @@ static void ublk_deinit_queue(struct ublk_device *ub, int q_id)
> {
> int size = ublk_queue_cmd_buf_size(ub, q_id);
> struct ublk_queue *ubq = ublk_get_queue(ub, q_id);
> + struct ublk_io *io;
> + int i;
> +
> + for (i = 0; i < ubq->q_depth; i++) {
> + io = &ubq->ios[i];
> + if (io->task)
> + put_task_struct(io->task);
> + }
>
> - if (ubq->ubq_daemon)
> - put_task_struct(ubq->ubq_daemon);
> if (ubq->io_cmd_buf)
> free_pages((unsigned long)ubq->io_cmd_buf, get_order(size));
> }
> @@ -2487,6 +2487,8 @@ static int ublk_add_tag_set(struct ublk_device *ub)
> ub->tag_set.numa_node = NUMA_NO_NODE;
> ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
> ub->tag_set.driver_data = ub;
> + if (ub->dev_info.flags & UBLK_F_RR_TAGS)
> + ub->tag_set.flags |= BLK_MQ_F_TAG_RR;
> return blk_mq_alloc_tag_set(&ub->tag_set);
> }
>
> @@ -3062,14 +3064,14 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
> int ublksrv_pid = (int)header->data[0];
> int ret = -EINVAL;
>
> - pr_devel("%s: Waiting for new ubq_daemons(nr: %d) are ready, dev id %d...\n",
> - __func__, ub->dev_info.nr_hw_queues, header->dev_id);
> - /* wait until new ubq_daemon sending all FETCH_REQ */
> + pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
> + header->dev_id);
> +
> if (wait_for_completion_interruptible(&ub->completion))
> return -EINTR;
>
> - pr_devel("%s: All new ubq_daemons(nr: %d) are ready, dev id %d\n",
> - __func__, ub->dev_info.nr_hw_queues, header->dev_id);
> + pr_devel("%s: All FETCH_REQs received, dev id %d\n", __func__,
> + header->dev_id);
>
> mutex_lock(&ub->mutex);
> if (ublk_nosrv_should_stop_dev(ub))
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index be5c6c6b16e098838a2bf790e588b79656defdda..3ba8f26d60bc70ade95b4f4d5990157af2bd5b2e 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -219,6 +219,14 @@
> */
> #define UBLK_F_UPDATE_SIZE (1ULL << 10)
>
> +/*
> + * Force tags to be allocated round-robin on each queue. If ublk server
> + * threads also issue FETCH_REQs against the queues in a round-robin
> + * manner, load can be balanced across all threads even if it is issued
> + * to the queues in an imbalanced way.
> + */
> +#define UBLK_F_RR_TAGS (1ULL << 11)
Please make UBLK_F_RR_TAGS as one standalone patch, which is also helpful
for evaluating round-robin performance, and doesn't have to be used by
per-io task.
Thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v6 1/8] ublk: have a per-io daemon instead of a per-queue daemon
2025-05-09 3:29 ` Ming Lei
@ 2025-05-10 23:54 ` Caleb Sander Mateos
0 siblings, 0 replies; 22+ messages in thread
From: Caleb Sander Mateos @ 2025-05-10 23:54 UTC (permalink / raw)
To: Ming Lei
Cc: Uday Shankar, Jens Axboe, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Thu, May 8, 2025 at 8:29 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Wed, May 07, 2025 at 03:49:35PM -0600, Uday Shankar wrote:
> > Currently, ublk_drv associates to each hardware queue (hctx) a unique
> > task (called the queue's ubq_daemon) which is allowed to issue
> > COMMIT_AND_FETCH commands against the hctx. If any other task attempts
> > to do so, the command fails immediately with EINVAL. When considered
> > together with the block layer architecture, the result is that for each
> > CPU C on the system, there is a unique ublk server thread which is
> > allowed to handle I/O submitted on CPU C. This can lead to suboptimal
> > performance under imbalanced load generation. For an extreme example,
> > suppose all the load is generated on CPUs mapping to a single ublk
> > server thread. Then that thread may be fully utilized and become the
> > bottleneck in the system, while other ublk server threads are totally
> > idle.
> >
> > This issue can also be addressed directly in the ublk server without
> > kernel support by having threads dequeue I/Os and pass them around to
> > ensure even load. But this solution requires inter-thread communication
> > at least twice for each I/O (submission and completion), which is
> > generally a bad pattern for performance. The problem gets even worse
> > with zero copy, as more inter-thread communication would be required to
> > have the buffer register/unregister calls to come from the correct
> > thread.
> >
> > Therefore, address this issue in ublk_drv by allowing each I/O to have
> > its own daemon task. Two I/Os in the same queue are now allowed to be
> > serviced by different daemon tasks - this was not possible before.
> > Imbalanced load can then be balanced across all ublk server threads as
> > follows:
> >
> > - specifying the new UBLK_F_RR_TAGS flag when creating the ublk device,
> > to ensure round-robin tag allocation
> > - having the ublk server threads issue FETCH_REQs in a round-robin
> > manner. As a small toy example, consider a system with a single ublk
> > device having 2 queues, each of depth 4. A ublk server having 4
> > threads could issue its FETCH_REQs against this device as follows
> > (where each entry is the qid,tag pair that the FETCH_REQ targets):
> >
> > ublk server thread: T0 T1 T2 T3
> > 0,0 0,1 0,2 0,3
> > 1,3 1,0 1,1 1,2
> >
> > The combination of these two changes allow a ublk server to balance
> > load. For example, suppose a program quickly issues 4 I/Os concurrently.
> > Before this change, all 4 I/Os would have had to be serviced by the same
> > thread, and that thread may become a bottleneck. With this change, those
> > 4 I/Os would get tags 0-3 for the same queue, and would thus spread out
> > evenly over all the ublk server threads. Peak bandwidth in CPU-limited
> > workloads under imbalanced load thus will increase.
> >
> > The UBLK_F_RR_TAGS flag is also added as a feature, so that ublk servers
> > can essentially test for the presence of this patch and tailor their
> > behavior accordingly.
> >
> > Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> > Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
> > ---
> > drivers/block/ublk_drv.c | 82 ++++++++++++++++++++++---------------------
> > include/uapi/linux/ublk_cmd.h | 8 +++++
> > 2 files changed, 50 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index cb612151e9a1de7630c49a2b78d278e005d52856..ff5beab32220ea96a1e15ab26c02f4883a6c3d6e 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -66,7 +66,8 @@
> > | UBLK_F_USER_COPY \
> > | UBLK_F_ZONED \
> > | UBLK_F_USER_RECOVERY_FAIL_IO \
> > - | UBLK_F_UPDATE_SIZE)
> > + | UBLK_F_UPDATE_SIZE \
> > + | UBLK_F_RR_TAGS)
> >
> > #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> > | UBLK_F_USER_RECOVERY_REISSUE \
> > @@ -148,6 +149,8 @@ struct ublk_io {
> > /* valid if UBLK_IO_FLAG_OWNED_BY_SRV is set */
> > struct request *req;
> > };
> > +
> > + struct task_struct *task;
> > };
> >
> > struct ublk_queue {
> > @@ -155,11 +158,9 @@ struct ublk_queue {
> > int q_depth;
> >
> > unsigned long flags;
> > - struct task_struct *ubq_daemon;
> > struct ublksrv_io_desc *io_cmd_buf;
> >
> > bool force_abort;
> > - bool timeout;
> > bool canceling;
> > bool fail_io; /* copy of dev->state == UBLK_S_DEV_FAIL_IO */
> > unsigned short nr_io_ready; /* how many ios setup */
> > @@ -1069,11 +1070,6 @@ static inline struct ublk_uring_cmd_pdu *ublk_get_uring_cmd_pdu(
> > return io_uring_cmd_to_pdu(ioucmd, struct ublk_uring_cmd_pdu);
> > }
> >
> > -static inline bool ubq_daemon_is_dying(struct ublk_queue *ubq)
> > -{
> > - return !ubq->ubq_daemon || ubq->ubq_daemon->flags & PF_EXITING;
> > -}
> > -
> > /* todo: handle partial completion */
> > static inline void __ublk_complete_rq(struct request *req)
> > {
> > @@ -1207,13 +1203,13 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> > /*
> > * Task is exiting if either:
> > *
> > - * (1) current != ubq_daemon.
> > + * (1) current != io->task.
> > * io_uring_cmd_complete_in_task() tries to run task_work
> > - * in a workqueue if ubq_daemon(cmd's task) is PF_EXITING.
> > + * in a workqueue if cmd's task is PF_EXITING.
> > *
> > * (2) current->flags & PF_EXITING.
> > */
> > - if (unlikely(current != ubq->ubq_daemon || current->flags & PF_EXITING)) {
> > + if (unlikely(current != io->task || current->flags & PF_EXITING)) {
> > __ublk_abort_rq(ubq, req);
> > return;
> > }
> > @@ -1286,13 +1282,10 @@ static void ublk_queue_cmd_list(struct ublk_queue *ubq, struct rq_list *l)
> > static enum blk_eh_timer_return ublk_timeout(struct request *rq)
> > {
> > struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> > + struct ublk_io *io = &ubq->ios[rq->tag];
> >
> > if (ubq->flags & UBLK_F_UNPRIVILEGED_DEV) {
> > - if (!ubq->timeout) {
> > - send_sig(SIGKILL, ubq->ubq_daemon, 0);
> > - ubq->timeout = true;
> > - }
> > -
> > + send_sig(SIGKILL, io->task, 0);
> > return BLK_EH_DONE;
> > }
> >
> > @@ -1405,17 +1398,6 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
> > /* All old ioucmds have to be completed */
> > ubq->nr_io_ready = 0;
> >
> > - /*
> > - * old daemon is PF_EXITING, put it now
> > - *
> > - * It could be NULL in case of closing one quisced device.
> > - */
> > - if (ubq->ubq_daemon)
> > - put_task_struct(ubq->ubq_daemon);
> > - /* We have to reset it to NULL, otherwise ub won't accept new FETCH_REQ */
> > - ubq->ubq_daemon = NULL;
> > - ubq->timeout = false;
> > -
> > for (i = 0; i < ubq->q_depth; i++) {
> > struct ublk_io *io = &ubq->ios[i];
> >
> > @@ -1426,6 +1408,17 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
> > io->flags &= UBLK_IO_FLAG_CANCELED;
> > io->cmd = NULL;
> > io->addr = 0;
> > +
> > + /*
> > + * old task is PF_EXITING, put it now
> > + *
> > + * It could be NULL in case of closing one quiesced
> > + * device.
> > + */
> > + if (io->task) {
> > + put_task_struct(io->task);
> > + io->task = NULL;
> > + }
> > }
> > }
> >
> > @@ -1447,7 +1440,7 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
> > for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
> > ublk_queue_reinit(ub, ublk_get_queue(ub, i));
> >
> > - /* set to NULL, otherwise new ubq_daemon cannot mmap the io_cmd_buf */
> > + /* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
> > ub->mm = NULL;
> > ub->nr_queues_ready = 0;
> > ub->nr_privileged_daemon = 0;
> > @@ -1722,7 +1715,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> > return;
> >
> > task = io_uring_cmd_get_task(cmd);
> > - if (WARN_ON_ONCE(task && task != ubq->ubq_daemon))
> > + if (WARN_ON_ONCE(task && task != ubq->ios[pdu->tag].task))
> > return;
> >
> > if (!ubq->canceling)
> > @@ -1861,8 +1854,6 @@ static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
> > {
> > ubq->nr_io_ready++;
> > if (ublk_queue_ready(ubq)) {
> > - ubq->ubq_daemon = current;
> > - get_task_struct(ubq->ubq_daemon);
> > ub->nr_queues_ready++;
> >
> > if (capable(CAP_SYS_ADMIN))
> > @@ -1995,6 +1986,7 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
> > }
> >
> > ublk_fill_io_cmd(io, cmd, buf_addr);
> > + WRITE_ONCE(io->task, get_task_struct(current));
> > ublk_mark_io_ready(ub, ubq);
> > out:
> > mutex_unlock(&ub->mutex);
> > @@ -2062,6 +2054,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> > const struct ublksrv_io_cmd *ub_cmd)
> > {
> > struct ublk_device *ub = cmd->file->private_data;
> > + struct task_struct *task;
> > struct ublk_queue *ubq;
> > struct ublk_io *io;
> > u32 cmd_op = cmd->cmd_op;
> > @@ -2076,13 +2069,14 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> > goto out;
> >
> > ubq = ublk_get_queue(ub, ub_cmd->q_id);
> > - if (ubq->ubq_daemon && ubq->ubq_daemon != current)
> > - goto out;
> >
> > if (tag >= ubq->q_depth)
> > goto out;
> >
> > io = &ubq->ios[tag];
> > + task = READ_ONCE(io->task);
> > + if (task && task != current)
> > + goto out;
> >
> > /* there is pending io cmd, something must be wrong */
> > if (io->flags & UBLK_IO_FLAG_ACTIVE) {
> > @@ -2332,9 +2326,15 @@ static void ublk_deinit_queue(struct ublk_device *ub, int q_id)
> > {
> > int size = ublk_queue_cmd_buf_size(ub, q_id);
> > struct ublk_queue *ubq = ublk_get_queue(ub, q_id);
> > + struct ublk_io *io;
> > + int i;
> > +
> > + for (i = 0; i < ubq->q_depth; i++) {
> > + io = &ubq->ios[i];
> > + if (io->task)
> > + put_task_struct(io->task);
> > + }
> >
> > - if (ubq->ubq_daemon)
> > - put_task_struct(ubq->ubq_daemon);
> > if (ubq->io_cmd_buf)
> > free_pages((unsigned long)ubq->io_cmd_buf, get_order(size));
> > }
> > @@ -2487,6 +2487,8 @@ static int ublk_add_tag_set(struct ublk_device *ub)
> > ub->tag_set.numa_node = NUMA_NO_NODE;
> > ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
> > ub->tag_set.driver_data = ub;
> > + if (ub->dev_info.flags & UBLK_F_RR_TAGS)
> > + ub->tag_set.flags |= BLK_MQ_F_TAG_RR;
> > return blk_mq_alloc_tag_set(&ub->tag_set);
> > }
> >
> > @@ -3062,14 +3064,14 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
> > int ublksrv_pid = (int)header->data[0];
> > int ret = -EINVAL;
> >
> > - pr_devel("%s: Waiting for new ubq_daemons(nr: %d) are ready, dev id %d...\n",
> > - __func__, ub->dev_info.nr_hw_queues, header->dev_id);
> > - /* wait until new ubq_daemon sending all FETCH_REQ */
> > + pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
> > + header->dev_id);
> > +
> > if (wait_for_completion_interruptible(&ub->completion))
> > return -EINTR;
> >
> > - pr_devel("%s: All new ubq_daemons(nr: %d) are ready, dev id %d\n",
> > - __func__, ub->dev_info.nr_hw_queues, header->dev_id);
> > + pr_devel("%s: All FETCH_REQs received, dev id %d\n", __func__,
> > + header->dev_id);
> >
> > mutex_lock(&ub->mutex);
> > if (ublk_nosrv_should_stop_dev(ub))
> > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> > index be5c6c6b16e098838a2bf790e588b79656defdda..3ba8f26d60bc70ade95b4f4d5990157af2bd5b2e 100644
> > --- a/include/uapi/linux/ublk_cmd.h
> > +++ b/include/uapi/linux/ublk_cmd.h
> > @@ -219,6 +219,14 @@
> > */
> > #define UBLK_F_UPDATE_SIZE (1ULL << 10)
> >
> > +/*
> > + * Force tags to be allocated round-robin on each queue. If ublk server
> > + * threads also issue FETCH_REQs against the queues in a round-robin
> > + * manner, load can be balanced across all threads even if it is issued
> > + * to the queues in an imbalanced way.
> > + */
> > +#define UBLK_F_RR_TAGS (1ULL << 11)
>
> Please make UBLK_F_RR_TAGS as one standalone patch, which is also helpful
> for evaluating round-robin performance, and doesn't have to be used by
> per-io task.
I don't think UBLK_F_RR_TAGS has to be set in order to use per-io
tasks. But I agree it's a bit odd to check for per-io task support via
the UBLK_F_RR_TAGS feature flag. Using a separate feature flag to
report per-io task support would seem clearer to me.
Best,
Caleb
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 1/8] ublk: have a per-io daemon instead of a per-queue daemon
2025-05-07 21:49 ` [PATCH v6 1/8] ublk: have a per-io daemon instead of a per-queue daemon Uday Shankar
2025-05-09 3:29 ` Ming Lei
@ 2025-05-11 0:17 ` Caleb Sander Mateos
1 sibling, 0 replies; 22+ messages in thread
From: Caleb Sander Mateos @ 2025-05-11 0:17 UTC (permalink / raw)
To: Uday Shankar
Cc: Ming Lei, Jens Axboe, Andrew Morton, Shuah Khan, Jonathan Corbet,
linux-block, linux-kernel, linux-kselftest, linux-doc
On Wed, May 7, 2025 at 2:50 PM Uday Shankar <ushankar@purestorage.com> wrote:
>
> Currently, ublk_drv associates to each hardware queue (hctx) a unique
> task (called the queue's ubq_daemon) which is allowed to issue
> COMMIT_AND_FETCH commands against the hctx. If any other task attempts
> to do so, the command fails immediately with EINVAL. When considered
> together with the block layer architecture, the result is that for each
> CPU C on the system, there is a unique ublk server thread which is
> allowed to handle I/O submitted on CPU C. This can lead to suboptimal
> performance under imbalanced load generation. For an extreme example,
> suppose all the load is generated on CPUs mapping to a single ublk
> server thread. Then that thread may be fully utilized and become the
> bottleneck in the system, while other ublk server threads are totally
> idle.
>
> This issue can also be addressed directly in the ublk server without
> kernel support by having threads dequeue I/Os and pass them around to
> ensure even load. But this solution requires inter-thread communication
> at least twice for each I/O (submission and completion), which is
> generally a bad pattern for performance. The problem gets even worse
> with zero copy, as more inter-thread communication would be required to
> have the buffer register/unregister calls to come from the correct
> thread.
>
> Therefore, address this issue in ublk_drv by allowing each I/O to have
> its own daemon task. Two I/Os in the same queue are now allowed to be
> serviced by different daemon tasks - this was not possible before.
> Imbalanced load can then be balanced across all ublk server threads as
> follows:
>
> - specifying the new UBLK_F_RR_TAGS flag when creating the ublk device,
> to ensure round-robin tag allocation
> - having the ublk server threads issue FETCH_REQs in a round-robin
> manner. As a small toy example, consider a system with a single ublk
> device having 2 queues, each of depth 4. A ublk server having 4
> threads could issue its FETCH_REQs against this device as follows
> (where each entry is the qid,tag pair that the FETCH_REQ targets):
>
> ublk server thread: T0 T1 T2 T3
> 0,0 0,1 0,2 0,3
> 1,3 1,0 1,1 1,2
>
> The combination of these two changes allow a ublk server to balance
> load. For example, suppose a program quickly issues 4 I/Os concurrently.
> Before this change, all 4 I/Os would have had to be serviced by the same
> thread, and that thread may become a bottleneck. With this change, those
> 4 I/Os would get tags 0-3 for the same queue, and would thus spread out
> evenly over all the ublk server threads. Peak bandwidth in CPU-limited
> workloads under imbalanced load thus will increase.
>
> The UBLK_F_RR_TAGS flag is also added as a feature, so that ublk servers
> can essentially test for the presence of this patch and tailor their
> behavior accordingly.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
> ---
> drivers/block/ublk_drv.c | 82 ++++++++++++++++++++++---------------------
> include/uapi/linux/ublk_cmd.h | 8 +++++
> 2 files changed, 50 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index cb612151e9a1de7630c49a2b78d278e005d52856..ff5beab32220ea96a1e15ab26c02f4883a6c3d6e 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -66,7 +66,8 @@
> | UBLK_F_USER_COPY \
> | UBLK_F_ZONED \
> | UBLK_F_USER_RECOVERY_FAIL_IO \
> - | UBLK_F_UPDATE_SIZE)
> + | UBLK_F_UPDATE_SIZE \
> + | UBLK_F_RR_TAGS)
>
> #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> | UBLK_F_USER_RECOVERY_REISSUE \
> @@ -148,6 +149,8 @@ struct ublk_io {
> /* valid if UBLK_IO_FLAG_OWNED_BY_SRV is set */
> struct request *req;
> };
> +
> + struct task_struct *task;
> };
>
> struct ublk_queue {
> @@ -155,11 +158,9 @@ struct ublk_queue {
> int q_depth;
>
> unsigned long flags;
> - struct task_struct *ubq_daemon;
> struct ublksrv_io_desc *io_cmd_buf;
>
> bool force_abort;
> - bool timeout;
> bool canceling;
> bool fail_io; /* copy of dev->state == UBLK_S_DEV_FAIL_IO */
> unsigned short nr_io_ready; /* how many ios setup */
> @@ -1069,11 +1070,6 @@ static inline struct ublk_uring_cmd_pdu *ublk_get_uring_cmd_pdu(
> return io_uring_cmd_to_pdu(ioucmd, struct ublk_uring_cmd_pdu);
> }
>
> -static inline bool ubq_daemon_is_dying(struct ublk_queue *ubq)
> -{
> - return !ubq->ubq_daemon || ubq->ubq_daemon->flags & PF_EXITING;
> -}
> -
> /* todo: handle partial completion */
> static inline void __ublk_complete_rq(struct request *req)
> {
> @@ -1207,13 +1203,13 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> /*
> * Task is exiting if either:
> *
> - * (1) current != ubq_daemon.
> + * (1) current != io->task.
> * io_uring_cmd_complete_in_task() tries to run task_work
> - * in a workqueue if ubq_daemon(cmd's task) is PF_EXITING.
> + * in a workqueue if cmd's task is PF_EXITING.
> *
> * (2) current->flags & PF_EXITING.
> */
> - if (unlikely(current != ubq->ubq_daemon || current->flags & PF_EXITING)) {
> + if (unlikely(current != io->task || current->flags & PF_EXITING)) {
> __ublk_abort_rq(ubq, req);
> return;
> }
> @@ -1286,13 +1282,10 @@ static void ublk_queue_cmd_list(struct ublk_queue *ubq, struct rq_list *l)
> static enum blk_eh_timer_return ublk_timeout(struct request *rq)
> {
> struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> + struct ublk_io *io = &ubq->ios[rq->tag];
>
> if (ubq->flags & UBLK_F_UNPRIVILEGED_DEV) {
> - if (!ubq->timeout) {
> - send_sig(SIGKILL, ubq->ubq_daemon, 0);
> - ubq->timeout = true;
> - }
> -
> + send_sig(SIGKILL, io->task, 0);
> return BLK_EH_DONE;
> }
>
> @@ -1405,17 +1398,6 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
> /* All old ioucmds have to be completed */
> ubq->nr_io_ready = 0;
>
> - /*
> - * old daemon is PF_EXITING, put it now
> - *
> - * It could be NULL in case of closing one quisced device.
> - */
> - if (ubq->ubq_daemon)
> - put_task_struct(ubq->ubq_daemon);
> - /* We have to reset it to NULL, otherwise ub won't accept new FETCH_REQ */
> - ubq->ubq_daemon = NULL;
> - ubq->timeout = false;
> -
> for (i = 0; i < ubq->q_depth; i++) {
> struct ublk_io *io = &ubq->ios[i];
>
> @@ -1426,6 +1408,17 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
> io->flags &= UBLK_IO_FLAG_CANCELED;
> io->cmd = NULL;
> io->addr = 0;
> +
> + /*
> + * old task is PF_EXITING, put it now
> + *
> + * It could be NULL in case of closing one quiesced
> + * device.
> + */
> + if (io->task) {
> + put_task_struct(io->task);
> + io->task = NULL;
> + }
> }
> }
>
> @@ -1447,7 +1440,7 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
> for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
> ublk_queue_reinit(ub, ublk_get_queue(ub, i));
>
> - /* set to NULL, otherwise new ubq_daemon cannot mmap the io_cmd_buf */
> + /* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
> ub->mm = NULL;
> ub->nr_queues_ready = 0;
> ub->nr_privileged_daemon = 0;
> @@ -1722,7 +1715,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> return;
>
> task = io_uring_cmd_get_task(cmd);
> - if (WARN_ON_ONCE(task && task != ubq->ubq_daemon))
> + if (WARN_ON_ONCE(task && task != ubq->ios[pdu->tag].task))
ubq->ios[pdu->tag] is computed again a few lines later. Perhaps the
struct ublk_io * could be computed just once and stored in a local
variable?
> return;
>
> if (!ubq->canceling)
> @@ -1861,8 +1854,6 @@ static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
> {
> ubq->nr_io_ready++;
> if (ublk_queue_ready(ubq)) {
> - ubq->ubq_daemon = current;
> - get_task_struct(ubq->ubq_daemon);
> ub->nr_queues_ready++;
>
> if (capable(CAP_SYS_ADMIN))
> @@ -1995,6 +1986,7 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
> }
>
> ublk_fill_io_cmd(io, cmd, buf_addr);
> + WRITE_ONCE(io->task, get_task_struct(current));
> ublk_mark_io_ready(ub, ubq);
> out:
> mutex_unlock(&ub->mutex);
> @@ -2062,6 +2054,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> const struct ublksrv_io_cmd *ub_cmd)
> {
> struct ublk_device *ub = cmd->file->private_data;
> + struct task_struct *task;
> struct ublk_queue *ubq;
> struct ublk_io *io;
> u32 cmd_op = cmd->cmd_op;
> @@ -2076,13 +2069,14 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> goto out;
>
> ubq = ublk_get_queue(ub, ub_cmd->q_id);
> - if (ubq->ubq_daemon && ubq->ubq_daemon != current)
> - goto out;
>
> if (tag >= ubq->q_depth)
> goto out;
>
> io = &ubq->ios[tag];
> + task = READ_ONCE(io->task);
> + if (task && task != current)
> + goto out;
>
> /* there is pending io cmd, something must be wrong */
> if (io->flags & UBLK_IO_FLAG_ACTIVE) {
> @@ -2332,9 +2326,15 @@ static void ublk_deinit_queue(struct ublk_device *ub, int q_id)
> {
> int size = ublk_queue_cmd_buf_size(ub, q_id);
> struct ublk_queue *ubq = ublk_get_queue(ub, q_id);
> + struct ublk_io *io;
This variable could be moved into the loop scope, where it is initialized.
> + int i;
> +
> + for (i = 0; i < ubq->q_depth; i++) {
> + io = &ubq->ios[i];
> + if (io->task)
> + put_task_struct(io->task);
> + }
>
> - if (ubq->ubq_daemon)
> - put_task_struct(ubq->ubq_daemon);
> if (ubq->io_cmd_buf)
> free_pages((unsigned long)ubq->io_cmd_buf, get_order(size));
> }
> @@ -2487,6 +2487,8 @@ static int ublk_add_tag_set(struct ublk_device *ub)
> ub->tag_set.numa_node = NUMA_NO_NODE;
> ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
> ub->tag_set.driver_data = ub;
> + if (ub->dev_info.flags & UBLK_F_RR_TAGS)
> + ub->tag_set.flags |= BLK_MQ_F_TAG_RR;
> return blk_mq_alloc_tag_set(&ub->tag_set);
> }
>
> @@ -3062,14 +3064,14 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
> int ublksrv_pid = (int)header->data[0];
> int ret = -EINVAL;
>
> - pr_devel("%s: Waiting for new ubq_daemons(nr: %d) are ready, dev id %d...\n",
> - __func__, ub->dev_info.nr_hw_queues, header->dev_id);
> - /* wait until new ubq_daemon sending all FETCH_REQ */
> + pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
> + header->dev_id);
> +
> if (wait_for_completion_interruptible(&ub->completion))
> return -EINTR;
>
> - pr_devel("%s: All new ubq_daemons(nr: %d) are ready, dev id %d\n",
> - __func__, ub->dev_info.nr_hw_queues, header->dev_id);
> + pr_devel("%s: All FETCH_REQs received, dev id %d\n", __func__,
> + header->dev_id);
>
> mutex_lock(&ub->mutex);
> if (ublk_nosrv_should_stop_dev(ub))
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index be5c6c6b16e098838a2bf790e588b79656defdda..3ba8f26d60bc70ade95b4f4d5990157af2bd5b2e 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -219,6 +219,14 @@
> */
> #define UBLK_F_UPDATE_SIZE (1ULL << 10)
>
> +/*
> + * Force tags to be allocated round-robin on each queue. If ublk server
> + * threads also issue FETCH_REQs against the queues in a round-robin
> + * manner, load can be balanced across all threads even if it is issued
> + * to the queues in an imbalanced way.
> + */
> +#define UBLK_F_RR_TAGS (1ULL << 11)
FYI Ming also has a patch series out that will use this feature flag
bit: https://lore.kernel.org/linux-block/20250509150611.3395206-5-ming.lei@redhat.com/
There are some other merge conflicts with this series too. Not sure
which one is likely to land first.
Best,
Caleb
> +
> /* device state */
> #define UBLK_S_DEV_DEAD 0
> #define UBLK_S_DEV_LIVE 1
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6 2/8] sbitmap: fix off-by-one when wrapping hint
2025-05-07 21:49 [PATCH v6 0/8] ublk: decouple server threads from hctxs Uday Shankar
2025-05-07 21:49 ` [PATCH v6 1/8] ublk: have a per-io daemon instead of a per-queue daemon Uday Shankar
@ 2025-05-07 21:49 ` Uday Shankar
2025-05-09 3:51 ` Ming Lei
2025-05-11 0:35 ` Caleb Sander Mateos
2025-05-07 21:49 ` [PATCH v6 3/8] selftests: ublk: kublk: plumb q_id in io_uring user_data Uday Shankar
` (5 subsequent siblings)
7 siblings, 2 replies; 22+ messages in thread
From: Uday Shankar @ 2025-05-07 21:49 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Caleb Sander Mateos, Andrew Morton,
Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc,
Uday Shankar
In update_alloc_hint_after_get, we wrap the new hint back to 0 one bit
too early. This breaks round robin tag allocation (BLK_MQ_F_TAG_RR) -
some tags get skipped, so we don't get round robin tags even in the
simple case of single-threaded load on a single hctx. Fix the off-by-one
in the wrapping condition so that round robin tag allocation works
properly.
The same pattern occurs in __sbitmap_get_word, so fix it there too.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
lib/sbitmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/sbitmap.c b/lib/sbitmap.c
index d3412984170c03dc6600bbe53f130404b765ac5a..aa1cec78b9649f1f3e8ef2d617dd7ee724391a8c 100644
--- a/lib/sbitmap.c
+++ b/lib/sbitmap.c
@@ -51,7 +51,7 @@ static inline void update_alloc_hint_after_get(struct sbitmap *sb,
} else if (nr == hint || unlikely(sb->round_robin)) {
/* Only update the hint if we used it. */
hint = nr + 1;
- if (hint >= depth - 1)
+ if (hint >= depth)
hint = 0;
this_cpu_write(*sb->alloc_hint, hint);
}
@@ -182,7 +182,7 @@ static int __sbitmap_get_word(unsigned long *word, unsigned long depth,
break;
hint = nr + 1;
- if (hint >= depth - 1)
+ if (hint >= depth)
hint = 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v6 2/8] sbitmap: fix off-by-one when wrapping hint
2025-05-07 21:49 ` [PATCH v6 2/8] sbitmap: fix off-by-one when wrapping hint Uday Shankar
@ 2025-05-09 3:51 ` Ming Lei
2025-05-11 0:35 ` Caleb Sander Mateos
1 sibling, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-05-09 3:51 UTC (permalink / raw)
To: Uday Shankar
Cc: Jens Axboe, Caleb Sander Mateos, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Wed, May 07, 2025 at 03:49:36PM -0600, Uday Shankar wrote:
> In update_alloc_hint_after_get, we wrap the new hint back to 0 one bit
> too early. This breaks round robin tag allocation (BLK_MQ_F_TAG_RR) -
> some tags get skipped, so we don't get round robin tags even in the
> simple case of single-threaded load on a single hctx. Fix the off-by-one
> in the wrapping condition so that round robin tag allocation works
> properly.
>
> The same pattern occurs in __sbitmap_get_word, so fix it there too.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
> lib/sbitmap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbitmap.c b/lib/sbitmap.c
> index d3412984170c03dc6600bbe53f130404b765ac5a..aa1cec78b9649f1f3e8ef2d617dd7ee724391a8c 100644
> --- a/lib/sbitmap.c
> +++ b/lib/sbitmap.c
> @@ -51,7 +51,7 @@ static inline void update_alloc_hint_after_get(struct sbitmap *sb,
> } else if (nr == hint || unlikely(sb->round_robin)) {
> /* Only update the hint if we used it. */
> hint = nr + 1;
> - if (hint >= depth - 1)
> + if (hint >= depth)
> hint = 0;
> this_cpu_write(*sb->alloc_hint, hint);
This may help for round robin.
> }
> @@ -182,7 +182,7 @@ static int __sbitmap_get_word(unsigned long *word, unsigned long depth,
> break;
>
> hint = nr + 1;
> - if (hint >= depth - 1)
> + if (hint >= depth)
> hint = 0;
I guess round robin may need to return -1 if 'hint >= depth'.
Also the existing behavior starts from adding sbitmap, maybe Jens
has idea why hint is checked against 'depth -1' instead of 'depth'.
Thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v6 2/8] sbitmap: fix off-by-one when wrapping hint
2025-05-07 21:49 ` [PATCH v6 2/8] sbitmap: fix off-by-one when wrapping hint Uday Shankar
2025-05-09 3:51 ` Ming Lei
@ 2025-05-11 0:35 ` Caleb Sander Mateos
1 sibling, 0 replies; 22+ messages in thread
From: Caleb Sander Mateos @ 2025-05-11 0:35 UTC (permalink / raw)
To: Uday Shankar
Cc: Ming Lei, Jens Axboe, Andrew Morton, Shuah Khan, Jonathan Corbet,
linux-block, linux-kernel, linux-kselftest, linux-doc
On Wed, May 7, 2025 at 2:49 PM Uday Shankar <ushankar@purestorage.com> wrote:
>
> In update_alloc_hint_after_get, we wrap the new hint back to 0 one bit
> too early. This breaks round robin tag allocation (BLK_MQ_F_TAG_RR) -
> some tags get skipped, so we don't get round robin tags even in the
> simple case of single-threaded load on a single hctx. Fix the off-by-one
> in the wrapping condition so that round robin tag allocation works
> properly.
>
> The same pattern occurs in __sbitmap_get_word, so fix it there too.
Should this have a Fixes tag? Looks like the off-by-one wrapping has
existed since 4bb659b15699 ("blk-mq: implement new and more efficient
tagging scheme"), but it's only a correctness issue with round-robin
tag allocation, which was added in 24391c0dc57c ("blk-mq: add tag
allocation policy").
I don't have much background on blk-mq's round-robin tag allocation, but FWIW,
Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
> lib/sbitmap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbitmap.c b/lib/sbitmap.c
> index d3412984170c03dc6600bbe53f130404b765ac5a..aa1cec78b9649f1f3e8ef2d617dd7ee724391a8c 100644
> --- a/lib/sbitmap.c
> +++ b/lib/sbitmap.c
> @@ -51,7 +51,7 @@ static inline void update_alloc_hint_after_get(struct sbitmap *sb,
> } else if (nr == hint || unlikely(sb->round_robin)) {
> /* Only update the hint if we used it. */
> hint = nr + 1;
> - if (hint >= depth - 1)
> + if (hint >= depth)
> hint = 0;
> this_cpu_write(*sb->alloc_hint, hint);
> }
> @@ -182,7 +182,7 @@ static int __sbitmap_get_word(unsigned long *word, unsigned long depth,
> break;
>
> hint = nr + 1;
> - if (hint >= depth - 1)
> + if (hint >= depth)
> hint = 0;
> }
>
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6 3/8] selftests: ublk: kublk: plumb q_id in io_uring user_data
2025-05-07 21:49 [PATCH v6 0/8] ublk: decouple server threads from hctxs Uday Shankar
2025-05-07 21:49 ` [PATCH v6 1/8] ublk: have a per-io daemon instead of a per-queue daemon Uday Shankar
2025-05-07 21:49 ` [PATCH v6 2/8] sbitmap: fix off-by-one when wrapping hint Uday Shankar
@ 2025-05-07 21:49 ` Uday Shankar
2025-05-09 7:28 ` Ming Lei
2025-05-07 21:49 ` [PATCH v6 4/8] selftests: ublk: kublk: tie sqe allocation to io instead of queue Uday Shankar
` (4 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Uday Shankar @ 2025-05-07 21:49 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Caleb Sander Mateos, Andrew Morton,
Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc,
Uday Shankar
Currently, when we process CQEs, we know which ublk_queue we are working
on because we know which ring we are working on, and ublk_queues and
rings are in 1:1 correspondence. However, as we decouple ublk_queues
from ublk server threads, ublk_queues and rings will no longer be in 1:1
correspondence - each ublk server thread will have a ring, and each
thread may issue commands against more than one ublk_queue. So in order
to know which ublk_queue a CQE refers to, plumb that information in the
associated SQE's user_data.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
tools/testing/selftests/ublk/fault_inject.c | 2 +-
tools/testing/selftests/ublk/file_backed.c | 10 +++++-----
tools/testing/selftests/ublk/kublk.c | 17 +++++++++--------
tools/testing/selftests/ublk/kublk.h | 23 +++++++++++++----------
tools/testing/selftests/ublk/null.c | 6 +++---
tools/testing/selftests/ublk/stripe.c | 9 +++++----
6 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/tools/testing/selftests/ublk/fault_inject.c b/tools/testing/selftests/ublk/fault_inject.c
index 94a8e729ba4c8f8bf8faa313655a738e480533c7..6bc8ee519b483ba6a365dccb03ad389425eefd3b 100644
--- a/tools/testing/selftests/ublk/fault_inject.c
+++ b/tools/testing/selftests/ublk/fault_inject.c
@@ -43,7 +43,7 @@ static int ublk_fault_inject_queue_io(struct ublk_queue *q, int tag)
ublk_queue_alloc_sqes(q, &sqe, 1);
io_uring_prep_timeout(sqe, &ts, 1, 0);
- sqe->user_data = build_user_data(tag, ublksrv_get_op(iod), 0, 1);
+ sqe->user_data = build_user_data(tag, ublksrv_get_op(iod), 0, q->q_id, 1);
ublk_queued_tgt_io(q, tag, 1);
diff --git a/tools/testing/selftests/ublk/file_backed.c b/tools/testing/selftests/ublk/file_backed.c
index 6f34eabfae9796cf8862f262358cf230d26ed55b..69991ac7a0a947acba7b23ac89348936a3fcef75 100644
--- a/tools/testing/selftests/ublk/file_backed.c
+++ b/tools/testing/selftests/ublk/file_backed.c
@@ -22,7 +22,7 @@ static int loop_queue_flush_io(struct ublk_queue *q, const struct ublksrv_io_des
io_uring_prep_fsync(sqe[0], 1 /*fds[1]*/, IORING_FSYNC_DATASYNC);
io_uring_sqe_set_flags(sqe[0], IOSQE_FIXED_FILE);
/* bit63 marks us as tgt io */
- sqe[0]->user_data = build_user_data(tag, ublk_op, 0, 1);
+ sqe[0]->user_data = build_user_data(tag, ublk_op, 0, q->q_id, 1);
return 1;
}
@@ -44,7 +44,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
iod->start_sector << 9);
io_uring_sqe_set_flags(sqe[0], IOSQE_FIXED_FILE);
/* bit63 marks us as tgt io */
- sqe[0]->user_data = build_user_data(tag, ublk_op, 0, 1);
+ sqe[0]->user_data = build_user_data(tag, ublk_op, 0, q->q_id, 1);
return 1;
}
@@ -53,17 +53,17 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
sqe[0]->user_data = build_user_data(tag,
- ublk_cmd_op_nr(sqe[0]->cmd_op), 0, 1);
+ ublk_cmd_op_nr(sqe[0]->cmd_op), 0, q->q_id, 1);
io_uring_prep_rw(op, sqe[1], 1 /*fds[1]*/, 0,
iod->nr_sectors << 9,
iod->start_sector << 9);
sqe[1]->buf_index = tag;
sqe[1]->flags |= IOSQE_FIXED_FILE | IOSQE_IO_HARDLINK;
- sqe[1]->user_data = build_user_data(tag, ublk_op, 0, 1);
+ sqe[1]->user_data = build_user_data(tag, ublk_op, 0, q->q_id, 1);
io_uring_prep_buf_unregister(sqe[2], 0, tag, q->q_id, tag);
- sqe[2]->user_data = build_user_data(tag, ublk_cmd_op_nr(sqe[2]->cmd_op), 0, 1);
+ sqe[2]->user_data = build_user_data(tag, ublk_cmd_op_nr(sqe[2]->cmd_op), 0, q->q_id, 1);
return 2;
}
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
index 842b40736a9b81507960bba21a246c8b5d3bddee..d0eaf06fadbbb00c0549bba0a08f1be23baa2359 100644
--- a/tools/testing/selftests/ublk/kublk.c
+++ b/tools/testing/selftests/ublk/kublk.c
@@ -579,7 +579,7 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
else
cmd->addr = 0;
- user_data = build_user_data(tag, _IOC_NR(cmd_op), 0, 0);
+ user_data = build_user_data(tag, _IOC_NR(cmd_op), 0, q->q_id, 0);
io_uring_sqe_set_data64(sqe[0], user_data);
io->flags = 0;
@@ -625,10 +625,11 @@ static inline void ublksrv_handle_tgt_cqe(struct ublk_queue *q,
q->tgt_ops->tgt_io_done(q, tag, cqe);
}
-static void ublk_handle_cqe(struct io_uring *r,
+static void ublk_handle_cqe(struct ublk_dev *dev,
struct io_uring_cqe *cqe, void *data)
{
- struct ublk_queue *q = container_of(r, struct ublk_queue, ring);
+ unsigned q_id = user_data_to_q_id(cqe->user_data);
+ struct ublk_queue *q = &dev->q[q_id];
unsigned tag = user_data_to_tag(cqe->user_data);
unsigned cmd_op = user_data_to_op(cqe->user_data);
int fetch = (cqe->res != UBLK_IO_RES_ABORT) &&
@@ -679,17 +680,17 @@ static void ublk_handle_cqe(struct io_uring *r,
}
}
-static int ublk_reap_events_uring(struct io_uring *r)
+static int ublk_reap_events_uring(struct ublk_queue *q)
{
struct io_uring_cqe *cqe;
unsigned head;
int count = 0;
- io_uring_for_each_cqe(r, head, cqe) {
- ublk_handle_cqe(r, cqe, NULL);
+ io_uring_for_each_cqe(&q->ring, head, cqe) {
+ ublk_handle_cqe(q->dev, cqe, NULL);
count += 1;
}
- io_uring_cq_advance(r, count);
+ io_uring_cq_advance(&q->ring, count);
return count;
}
@@ -708,7 +709,7 @@ static int ublk_process_io(struct ublk_queue *q)
return -ENODEV;
ret = io_uring_submit_and_wait(&q->ring, 1);
- reapped = ublk_reap_events_uring(&q->ring);
+ reapped = ublk_reap_events_uring(q);
ublk_dbg(UBLK_DBG_QUEUE, "submit result %d, reapped %d stop %d idle %d\n",
ret, reapped, (q->state & UBLKSRV_QUEUE_STOPPING),
diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
index 81fb5864ab722380d7aaca3450b5c642d0c95a16..34f92bb2c64d0ddc7690b2654613e0c77b2b8121 100644
--- a/tools/testing/selftests/ublk/kublk.h
+++ b/tools/testing/selftests/ublk/kublk.h
@@ -49,7 +49,8 @@
#define UBLKSRV_IO_IDLE_SECS 20
#define UBLK_IO_MAX_BYTES (1 << 20)
-#define UBLK_MAX_QUEUES 32
+#define UBLK_MAX_QUEUES_SHIFT 5
+#define UBLK_MAX_QUEUES (1 << UBLK_MAX_QUEUES_SHIFT)
#define UBLK_QUEUE_DEPTH 1024
#define UBLK_DBG_DEV (1U << 0)
@@ -190,12 +191,6 @@ struct ublk_dev {
#define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER)
#endif
-#ifndef container_of
-#define container_of(ptr, type, member) ({ \
- unsigned long __mptr = (unsigned long)(ptr); \
- ((type *)(__mptr - offsetof(type, member))); })
-#endif
-
#define round_up(val, rnd) \
(((val) + ((rnd) - 1)) & ~((rnd) - 1))
@@ -209,11 +204,14 @@ static inline int is_target_io(__u64 user_data)
}
static inline __u64 build_user_data(unsigned tag, unsigned op,
- unsigned tgt_data, unsigned is_target_io)
+ unsigned tgt_data, unsigned q_id, unsigned is_target_io)
{
- assert(!(tag >> 16) && !(op >> 8) && !(tgt_data >> 16));
+ /* we only have 7 bits to encode q_id */
+ _Static_assert(UBLK_MAX_QUEUES_SHIFT <= 7);
+ assert(!(tag >> 16) && !(op >> 8) && !(tgt_data >> 16) && !(q_id >> 7));
- return tag | (op << 16) | (tgt_data << 24) | (__u64)is_target_io << 63;
+ return tag | (op << 16) | (tgt_data << 24) |
+ (__u64)q_id << 56 | (__u64)is_target_io << 63;
}
static inline unsigned int user_data_to_tag(__u64 user_data)
@@ -231,6 +229,11 @@ static inline unsigned int user_data_to_tgt_data(__u64 user_data)
return (user_data >> 24) & 0xffff;
}
+static inline unsigned int user_data_to_q_id(__u64 user_data)
+{
+ return (user_data >> 56) & 0x7f;
+}
+
static inline unsigned short ublk_cmd_op_nr(unsigned int op)
{
return _IOC_NR(op);
diff --git a/tools/testing/selftests/ublk/null.c b/tools/testing/selftests/ublk/null.c
index 91fec3690d4ba0cc1e4b0231dff6fdad411b5ecc..8e8e3c27329bf3fa7aeaaef2150bfbe4b22fcd94 100644
--- a/tools/testing/selftests/ublk/null.c
+++ b/tools/testing/selftests/ublk/null.c
@@ -52,7 +52,7 @@ static int null_queue_zc_io(struct ublk_queue *q, int tag)
io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
sqe[0]->user_data = build_user_data(tag,
- ublk_cmd_op_nr(sqe[0]->cmd_op), 0, 1);
+ ublk_cmd_op_nr(sqe[0]->cmd_op), 0, q->q_id, 1);
sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
io_uring_prep_nop(sqe[1]);
@@ -60,10 +60,10 @@ static int null_queue_zc_io(struct ublk_queue *q, int tag)
sqe[1]->flags |= IOSQE_FIXED_FILE | IOSQE_IO_HARDLINK;
sqe[1]->rw_flags = IORING_NOP_FIXED_BUFFER | IORING_NOP_INJECT_RESULT;
sqe[1]->len = iod->nr_sectors << 9; /* injected result */
- sqe[1]->user_data = build_user_data(tag, ublk_op, 0, 1);
+ sqe[1]->user_data = build_user_data(tag, ublk_op, 0, q->q_id, 1);
io_uring_prep_buf_unregister(sqe[2], 0, tag, q->q_id, tag);
- sqe[2]->user_data = build_user_data(tag, ublk_cmd_op_nr(sqe[2]->cmd_op), 0, 1);
+ sqe[2]->user_data = build_user_data(tag, ublk_cmd_op_nr(sqe[2]->cmd_op), 0, q->q_id, 1);
// buf register is marked as IOSQE_CQE_SKIP_SUCCESS
return 2;
diff --git a/tools/testing/selftests/ublk/stripe.c b/tools/testing/selftests/ublk/stripe.c
index 5dbd6392d83de29faeac97b4f8e3e99afd791282..057d3132aa0d556c649f502a7d738be03207c1f3 100644
--- a/tools/testing/selftests/ublk/stripe.c
+++ b/tools/testing/selftests/ublk/stripe.c
@@ -142,7 +142,7 @@ static int stripe_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_
io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
sqe[0]->user_data = build_user_data(tag,
- ublk_cmd_op_nr(sqe[0]->cmd_op), 0, 1);
+ ublk_cmd_op_nr(sqe[0]->cmd_op), 0, q->q_id, 1);
}
for (i = zc; i < s->nr + extra - zc; i++) {
@@ -161,13 +161,14 @@ static int stripe_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_
io_uring_sqe_set_flags(sqe[i], IOSQE_FIXED_FILE);
}
/* bit63 marks us as tgt io */
- sqe[i]->user_data = build_user_data(tag, ublksrv_get_op(iod), i - zc, 1);
+ sqe[i]->user_data = build_user_data(tag, ublksrv_get_op(iod), i - zc, q->q_id, 1);
}
if (zc) {
struct io_uring_sqe *unreg = sqe[s->nr + 1];
io_uring_prep_buf_unregister(unreg, 0, tag, q->q_id, tag);
- unreg->user_data = build_user_data(tag, ublk_cmd_op_nr(unreg->cmd_op), 0, 1);
+ unreg->user_data = build_user_data(
+ tag, ublk_cmd_op_nr(unreg->cmd_op), 0, q->q_id, 1);
}
/* register buffer is skip_success */
@@ -184,7 +185,7 @@ static int handle_flush(struct ublk_queue *q, const struct ublksrv_io_desc *iod,
for (i = 0; i < conf->nr_files; i++) {
io_uring_prep_fsync(sqe[i], i + 1, IORING_FSYNC_DATASYNC);
io_uring_sqe_set_flags(sqe[i], IOSQE_FIXED_FILE);
- sqe[i]->user_data = build_user_data(tag, UBLK_IO_OP_FLUSH, 0, 1);
+ sqe[i]->user_data = build_user_data(tag, UBLK_IO_OP_FLUSH, 0, q->q_id, 1);
}
return conf->nr_files;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v6 3/8] selftests: ublk: kublk: plumb q_id in io_uring user_data
2025-05-07 21:49 ` [PATCH v6 3/8] selftests: ublk: kublk: plumb q_id in io_uring user_data Uday Shankar
@ 2025-05-09 7:28 ` Ming Lei
0 siblings, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-05-09 7:28 UTC (permalink / raw)
To: Uday Shankar
Cc: Jens Axboe, Caleb Sander Mateos, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Wed, May 07, 2025 at 03:49:37PM -0600, Uday Shankar wrote:
> Currently, when we process CQEs, we know which ublk_queue we are working
> on because we know which ring we are working on, and ublk_queues and
> rings are in 1:1 correspondence. However, as we decouple ublk_queues
> from ublk server threads, ublk_queues and rings will no longer be in 1:1
> correspondence - each ublk server thread will have a ring, and each
> thread may issue commands against more than one ublk_queue. So in order
> to know which ublk_queue a CQE refers to, plumb that information in the
> associated SQE's user_data.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
Looks fine,
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6 4/8] selftests: ublk: kublk: tie sqe allocation to io instead of queue
2025-05-07 21:49 [PATCH v6 0/8] ublk: decouple server threads from hctxs Uday Shankar
` (2 preceding siblings ...)
2025-05-07 21:49 ` [PATCH v6 3/8] selftests: ublk: kublk: plumb q_id in io_uring user_data Uday Shankar
@ 2025-05-07 21:49 ` Uday Shankar
2025-05-09 7:40 ` Ming Lei
2025-05-07 21:49 ` [PATCH v6 5/8] selftests: ublk: kublk: lift queue initialization out of thread Uday Shankar
` (3 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Uday Shankar @ 2025-05-07 21:49 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Caleb Sander Mateos, Andrew Morton,
Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc,
Uday Shankar
We currently have a helper ublk_queue_alloc_sqes which the ublk targets
use to allocate SQEs for their own operations. However, as we move
towards decoupled ublk_queues and ublk server threads, this helper does
not make sense anymore. SQEs are allocated from rings, and we will have
one ring per thread to avoid locking. Change the SQE allocation helper
to ublk_io_alloc_sqes. Currently this still allocates SQEs from the io's
queue's ring, but when we fully decouple threads and queues, it will
allocate from the io's thread's ring instead.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
tools/testing/selftests/ublk/fault_inject.c | 2 +-
tools/testing/selftests/ublk/file_backed.c | 6 +++---
tools/testing/selftests/ublk/kublk.c | 3 ++-
tools/testing/selftests/ublk/kublk.h | 11 +++++++----
tools/testing/selftests/ublk/null.c | 2 +-
tools/testing/selftests/ublk/stripe.c | 4 ++--
6 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/ublk/fault_inject.c b/tools/testing/selftests/ublk/fault_inject.c
index 6bc8ee519b483ba6a365dccb03ad389425eefd3b..101c6dad6cf1f6dd45bbc46baa793493b97646bf 100644
--- a/tools/testing/selftests/ublk/fault_inject.c
+++ b/tools/testing/selftests/ublk/fault_inject.c
@@ -41,7 +41,7 @@ static int ublk_fault_inject_queue_io(struct ublk_queue *q, int tag)
.tv_nsec = (long long)q->dev->private_data,
};
- ublk_queue_alloc_sqes(q, &sqe, 1);
+ ublk_io_alloc_sqes(ublk_get_io(q, tag), &sqe, 1);
io_uring_prep_timeout(sqe, &ts, 1, 0);
sqe->user_data = build_user_data(tag, ublksrv_get_op(iod), 0, q->q_id, 1);
diff --git a/tools/testing/selftests/ublk/file_backed.c b/tools/testing/selftests/ublk/file_backed.c
index 69991ac7a0a947acba7b23ac89348936a3fcef75..563f11a21604bbf5b9531f69f806d09cdd785960 100644
--- a/tools/testing/selftests/ublk/file_backed.c
+++ b/tools/testing/selftests/ublk/file_backed.c
@@ -18,7 +18,7 @@ static int loop_queue_flush_io(struct ublk_queue *q, const struct ublksrv_io_des
unsigned ublk_op = ublksrv_get_op(iod);
struct io_uring_sqe *sqe[1];
- ublk_queue_alloc_sqes(q, sqe, 1);
+ ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 1);
io_uring_prep_fsync(sqe[0], 1 /*fds[1]*/, IORING_FSYNC_DATASYNC);
io_uring_sqe_set_flags(sqe[0], IOSQE_FIXED_FILE);
/* bit63 marks us as tgt io */
@@ -34,7 +34,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
struct io_uring_sqe *sqe[3];
if (!zc) {
- ublk_queue_alloc_sqes(q, sqe, 1);
+ ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 1);
if (!sqe[0])
return -ENOMEM;
@@ -48,7 +48,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
return 1;
}
- ublk_queue_alloc_sqes(q, sqe, 3);
+ ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 3);
io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
index d0eaf06fadbbb00c0549bba0a08f1be23baa2359..7b3af98546803134dd7f959c40408cefda7cd45c 100644
--- a/tools/testing/selftests/ublk/kublk.c
+++ b/tools/testing/selftests/ublk/kublk.c
@@ -439,6 +439,7 @@ static int ublk_queue_init(struct ublk_queue *q)
for (i = 0; i < q->q_depth; i++) {
q->ios[i].buf_addr = NULL;
q->ios[i].flags = UBLKSRV_NEED_FETCH_RQ | UBLKSRV_IO_FREE;
+ q->ios[i].q = q;
if (q->state & UBLKSRV_NO_BUF)
continue;
@@ -554,7 +555,7 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
if (io_uring_sq_space_left(&q->ring) < 1)
io_uring_submit(&q->ring);
- ublk_queue_alloc_sqes(q, sqe, 1);
+ ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 1);
if (!sqe[0]) {
ublk_err("%s: run out of sqe %d, tag %d\n",
__func__, q->q_id, tag);
diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
index 34f92bb2c64d0ddc7690b2654613e0c77b2b8121..7c912116606429215af7dbc2a8ce6b40ef89bfbd 100644
--- a/tools/testing/selftests/ublk/kublk.h
+++ b/tools/testing/selftests/ublk/kublk.h
@@ -119,6 +119,8 @@ struct ublk_io {
unsigned short flags;
unsigned short refs; /* used by target code only */
+ struct ublk_queue *q;
+
int result;
unsigned short tgt_ios;
@@ -267,17 +269,18 @@ static inline void ublk_dbg(int level, const char *fmt, ...)
}
}
-static inline int ublk_queue_alloc_sqes(struct ublk_queue *q,
+static inline int ublk_io_alloc_sqes(struct ublk_io *io,
struct io_uring_sqe *sqes[], int nr_sqes)
{
- unsigned left = io_uring_sq_space_left(&q->ring);
+ struct io_uring *ring = &io->q->ring;
+ unsigned left = io_uring_sq_space_left(ring);
int i;
if (left < nr_sqes)
- io_uring_submit(&q->ring);
+ io_uring_submit(ring);
for (i = 0; i < nr_sqes; i++) {
- sqes[i] = io_uring_get_sqe(&q->ring);
+ sqes[i] = io_uring_get_sqe(ring);
if (!sqes[i])
return i;
}
diff --git a/tools/testing/selftests/ublk/null.c b/tools/testing/selftests/ublk/null.c
index 8e8e3c27329bf3fa7aeaaef2150bfbe4b22fcd94..7323fe61212f6041ef5a04758d30e62376ac9c6b 100644
--- a/tools/testing/selftests/ublk/null.c
+++ b/tools/testing/selftests/ublk/null.c
@@ -48,7 +48,7 @@ static int null_queue_zc_io(struct ublk_queue *q, int tag)
unsigned ublk_op = ublksrv_get_op(iod);
struct io_uring_sqe *sqe[3];
- ublk_queue_alloc_sqes(q, sqe, 3);
+ ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 3);
io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
sqe[0]->user_data = build_user_data(tag,
diff --git a/tools/testing/selftests/ublk/stripe.c b/tools/testing/selftests/ublk/stripe.c
index 057d3132aa0d556c649f502a7d738be03207c1f3..d569f62c9b7b5a6b9d82506c4aaab74b77a10305 100644
--- a/tools/testing/selftests/ublk/stripe.c
+++ b/tools/testing/selftests/ublk/stripe.c
@@ -136,7 +136,7 @@ static int stripe_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_
io->private_data = s;
calculate_stripe_array(conf, iod, s);
- ublk_queue_alloc_sqes(q, sqe, s->nr + extra);
+ ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, s->nr + extra);
if (zc) {
io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
@@ -181,7 +181,7 @@ static int handle_flush(struct ublk_queue *q, const struct ublksrv_io_desc *iod,
struct io_uring_sqe *sqe[NR_STRIPE];
int i;
- ublk_queue_alloc_sqes(q, sqe, conf->nr_files);
+ ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, conf->nr_files);
for (i = 0; i < conf->nr_files; i++) {
io_uring_prep_fsync(sqe[i], i + 1, IORING_FSYNC_DATASYNC);
io_uring_sqe_set_flags(sqe[i], IOSQE_FIXED_FILE);
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v6 4/8] selftests: ublk: kublk: tie sqe allocation to io instead of queue
2025-05-07 21:49 ` [PATCH v6 4/8] selftests: ublk: kublk: tie sqe allocation to io instead of queue Uday Shankar
@ 2025-05-09 7:40 ` Ming Lei
0 siblings, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-05-09 7:40 UTC (permalink / raw)
To: Uday Shankar
Cc: Jens Axboe, Caleb Sander Mateos, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Wed, May 07, 2025 at 03:49:38PM -0600, Uday Shankar wrote:
> We currently have a helper ublk_queue_alloc_sqes which the ublk targets
> use to allocate SQEs for their own operations. However, as we move
> towards decoupled ublk_queues and ublk server threads, this helper does
> not make sense anymore. SQEs are allocated from rings, and we will have
> one ring per thread to avoid locking. Change the SQE allocation helper
> to ublk_io_alloc_sqes. Currently this still allocates SQEs from the io's
> queue's ring, but when we fully decouple threads and queues, it will
> allocate from the io's thread's ring instead.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
> tools/testing/selftests/ublk/fault_inject.c | 2 +-
> tools/testing/selftests/ublk/file_backed.c | 6 +++---
> tools/testing/selftests/ublk/kublk.c | 3 ++-
> tools/testing/selftests/ublk/kublk.h | 11 +++++++----
> tools/testing/selftests/ublk/null.c | 2 +-
> tools/testing/selftests/ublk/stripe.c | 4 ++--
> 6 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/selftests/ublk/fault_inject.c b/tools/testing/selftests/ublk/fault_inject.c
> index 6bc8ee519b483ba6a365dccb03ad389425eefd3b..101c6dad6cf1f6dd45bbc46baa793493b97646bf 100644
> --- a/tools/testing/selftests/ublk/fault_inject.c
> +++ b/tools/testing/selftests/ublk/fault_inject.c
> @@ -41,7 +41,7 @@ static int ublk_fault_inject_queue_io(struct ublk_queue *q, int tag)
> .tv_nsec = (long long)q->dev->private_data,
> };
>
> - ublk_queue_alloc_sqes(q, &sqe, 1);
> + ublk_io_alloc_sqes(ublk_get_io(q, tag), &sqe, 1);
> io_uring_prep_timeout(sqe, &ts, 1, 0);
> sqe->user_data = build_user_data(tag, ublksrv_get_op(iod), 0, q->q_id, 1);
>
> diff --git a/tools/testing/selftests/ublk/file_backed.c b/tools/testing/selftests/ublk/file_backed.c
> index 69991ac7a0a947acba7b23ac89348936a3fcef75..563f11a21604bbf5b9531f69f806d09cdd785960 100644
> --- a/tools/testing/selftests/ublk/file_backed.c
> +++ b/tools/testing/selftests/ublk/file_backed.c
> @@ -18,7 +18,7 @@ static int loop_queue_flush_io(struct ublk_queue *q, const struct ublksrv_io_des
> unsigned ublk_op = ublksrv_get_op(iod);
> struct io_uring_sqe *sqe[1];
>
> - ublk_queue_alloc_sqes(q, sqe, 1);
> + ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 1);
> io_uring_prep_fsync(sqe[0], 1 /*fds[1]*/, IORING_FSYNC_DATASYNC);
> io_uring_sqe_set_flags(sqe[0], IOSQE_FIXED_FILE);
> /* bit63 marks us as tgt io */
> @@ -34,7 +34,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
> struct io_uring_sqe *sqe[3];
>
> if (!zc) {
> - ublk_queue_alloc_sqes(q, sqe, 1);
> + ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 1);
> if (!sqe[0])
> return -ENOMEM;
>
> @@ -48,7 +48,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
> return 1;
> }
>
> - ublk_queue_alloc_sqes(q, sqe, 3);
> + ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 3);
>
> io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
> sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
> diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
> index d0eaf06fadbbb00c0549bba0a08f1be23baa2359..7b3af98546803134dd7f959c40408cefda7cd45c 100644
> --- a/tools/testing/selftests/ublk/kublk.c
> +++ b/tools/testing/selftests/ublk/kublk.c
> @@ -439,6 +439,7 @@ static int ublk_queue_init(struct ublk_queue *q)
> for (i = 0; i < q->q_depth; i++) {
> q->ios[i].buf_addr = NULL;
> q->ios[i].flags = UBLKSRV_NEED_FETCH_RQ | UBLKSRV_IO_FREE;
> + q->ios[i].q = q;
>
> if (q->state & UBLKSRV_NO_BUF)
> continue;
> @@ -554,7 +555,7 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
> if (io_uring_sq_space_left(&q->ring) < 1)
> io_uring_submit(&q->ring);
>
> - ublk_queue_alloc_sqes(q, sqe, 1);
> + ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 1);
> if (!sqe[0]) {
> ublk_err("%s: run out of sqe %d, tag %d\n",
> __func__, q->q_id, tag);
> diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
> index 34f92bb2c64d0ddc7690b2654613e0c77b2b8121..7c912116606429215af7dbc2a8ce6b40ef89bfbd 100644
> --- a/tools/testing/selftests/ublk/kublk.h
> +++ b/tools/testing/selftests/ublk/kublk.h
> @@ -119,6 +119,8 @@ struct ublk_io {
> unsigned short flags;
> unsigned short refs; /* used by target code only */
>
> + struct ublk_queue *q;
> +
> int result;
In following patch, you added 'tag' to 'struct ublk_io', then 8bytes 'struct
ublk_queue *q' needn't to be added because it can be figured out by
container_of():
- queue->ios can be calculated by 'io' address & its tag.
- please add one helper ublk_io_to_queue(io) for it.
Also please try to avoid hole to 'ublk_io'.
Otherwise, this patch looks fine.
thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6 5/8] selftests: ublk: kublk: lift queue initialization out of thread
2025-05-07 21:49 [PATCH v6 0/8] ublk: decouple server threads from hctxs Uday Shankar
` (3 preceding siblings ...)
2025-05-07 21:49 ` [PATCH v6 4/8] selftests: ublk: kublk: tie sqe allocation to io instead of queue Uday Shankar
@ 2025-05-07 21:49 ` Uday Shankar
2025-05-09 7:44 ` Ming Lei
2025-05-07 21:49 ` [PATCH v6 6/8] selftests: ublk: kublk: move per-thread data out of ublk_queue Uday Shankar
` (2 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Uday Shankar @ 2025-05-07 21:49 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Caleb Sander Mateos, Andrew Morton,
Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc,
Uday Shankar
Currently, each ublk server I/O handler thread initializes its own
queue. However, as we move towards decoupled ublk_queues and ublk server
threads, this model does not make sense anymore, as there will no longer
be a concept of a thread having "its own" queue. So lift queue
initialization out of the per-thread ublk_io_handler_fn and into a loop
in ublk_start_daemon (which runs once for each device).
There is a part of ublk_queue_init (ring initialization) which does
actually need to happen on the thread that will use the ring; that is
separated into a separate ublk_thread_init which is still called by each
I/O handler thread.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
tools/testing/selftests/ublk/kublk.c | 58 ++++++++++++++++++++++++++----------
1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
index 7b3af98546803134dd7f959c40408cefda7cd45c..3ad9e162816c3a10e9928f9d530908cda7595530 100644
--- a/tools/testing/selftests/ublk/kublk.c
+++ b/tools/testing/selftests/ublk/kublk.c
@@ -388,6 +388,17 @@ static void ublk_queue_deinit(struct ublk_queue *q)
int i;
int nr_ios = q->q_depth;
+ if (q->io_cmd_buf)
+ munmap(q->io_cmd_buf, ublk_queue_cmd_buf_sz(q));
+
+ for (i = 0; i < nr_ios; i++)
+ free(q->ios[i].buf_addr);
+}
+
+static void ublk_thread_deinit(struct ublk_queue *q)
+{
+ q->tid = 0;
+
io_uring_unregister_buffers(&q->ring);
io_uring_unregister_ring_fd(&q->ring);
@@ -397,28 +408,20 @@ static void ublk_queue_deinit(struct ublk_queue *q)
close(q->ring.ring_fd);
q->ring.ring_fd = -1;
}
-
- if (q->io_cmd_buf)
- munmap(q->io_cmd_buf, ublk_queue_cmd_buf_sz(q));
-
- for (i = 0; i < nr_ios; i++)
- free(q->ios[i].buf_addr);
}
static int ublk_queue_init(struct ublk_queue *q)
{
struct ublk_dev *dev = q->dev;
int depth = dev->dev_info.queue_depth;
- int i, ret = -1;
+ int i;
int cmd_buf_size, io_buf_size;
unsigned long off;
- int ring_depth = dev->tgt.sq_depth, cq_depth = dev->tgt.cq_depth;
q->tgt_ops = dev->tgt.ops;
q->state = 0;
q->q_depth = depth;
q->cmd_inflight = 0;
- q->tid = gettid();
if (dev->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY) {
q->state |= UBLKSRV_NO_BUF;
@@ -452,6 +455,22 @@ static int ublk_queue_init(struct ublk_queue *q)
}
}
+ return 0;
+ fail:
+ ublk_queue_deinit(q);
+ ublk_err("ublk dev %d queue %d failed\n",
+ dev->dev_info.dev_id, q->q_id);
+ return -ENOMEM;
+}
+
+static int ublk_thread_init(struct ublk_queue *q)
+{
+ struct ublk_dev *dev = q->dev;
+ int ring_depth = dev->tgt.sq_depth, cq_depth = dev->tgt.cq_depth;
+ int ret;
+
+ q->tid = gettid();
+
ret = ublk_setup_ring(&q->ring, ring_depth, cq_depth,
IORING_SETUP_COOP_TASKRUN |
IORING_SETUP_SINGLE_ISSUER |
@@ -481,9 +500,9 @@ static int ublk_queue_init(struct ublk_queue *q)
}
return 0;
- fail:
- ublk_queue_deinit(q);
- ublk_err("ublk dev %d queue %d failed\n",
+fail:
+ ublk_thread_deinit(q);
+ ublk_err("ublk dev %d queue %d thread init failed\n",
dev->dev_info.dev_id, q->q_id);
return -ENOMEM;
}
@@ -740,9 +759,9 @@ static void *ublk_io_handler_fn(void *data)
int dev_id = q->dev->dev_info.dev_id;
int ret;
- ret = ublk_queue_init(q);
+ ret = ublk_thread_init(q);
if (ret) {
- ublk_err("ublk dev %d queue %d init queue failed\n",
+ ublk_err("ublk dev %d queue %d thread init failed\n",
dev_id, q->q_id);
return NULL;
}
@@ -761,7 +780,7 @@ static void *ublk_io_handler_fn(void *data)
} while (1);
ublk_dbg(UBLK_DBG_QUEUE, "ublk dev %d queue %d exited\n", dev_id, q->q_id);
- ublk_queue_deinit(q);
+ ublk_thread_deinit(q);
return NULL;
}
@@ -830,6 +849,13 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
dev->q[i].dev = dev;
dev->q[i].q_id = i;
+ ret = ublk_queue_init(&dev->q[i]);
+ if (ret) {
+ ublk_err("ublk dev %d queue %d init queue failed\n",
+ dinfo->dev_id, i);
+ goto fail;
+ }
+
qinfo[i].q = &dev->q[i];
qinfo[i].queue_sem = &queue_sem;
qinfo[i].affinity = &affinity_buf[i];
@@ -865,6 +891,8 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
for (i = 0; i < dinfo->nr_hw_queues; i++)
pthread_join(dev->q[i].thread, &thread_ret);
fail:
+ for (i = 0; i < dinfo->nr_hw_queues; i++)
+ ublk_queue_deinit(&dev->q[i]);
ublk_dev_unprep(dev);
ublk_dbg(UBLK_DBG_DEV, "%s exit\n", __func__);
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v6 5/8] selftests: ublk: kublk: lift queue initialization out of thread
2025-05-07 21:49 ` [PATCH v6 5/8] selftests: ublk: kublk: lift queue initialization out of thread Uday Shankar
@ 2025-05-09 7:44 ` Ming Lei
0 siblings, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-05-09 7:44 UTC (permalink / raw)
To: Uday Shankar
Cc: Jens Axboe, Caleb Sander Mateos, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Wed, May 07, 2025 at 03:49:39PM -0600, Uday Shankar wrote:
> Currently, each ublk server I/O handler thread initializes its own
> queue. However, as we move towards decoupled ublk_queues and ublk server
> threads, this model does not make sense anymore, as there will no longer
> be a concept of a thread having "its own" queue. So lift queue
> initialization out of the per-thread ublk_io_handler_fn and into a loop
> in ublk_start_daemon (which runs once for each device).
>
> There is a part of ublk_queue_init (ring initialization) which does
> actually need to happen on the thread that will use the ring; that is
> separated into a separate ublk_thread_init which is still called by each
> I/O handler thread.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6 6/8] selftests: ublk: kublk: move per-thread data out of ublk_queue
2025-05-07 21:49 [PATCH v6 0/8] ublk: decouple server threads from hctxs Uday Shankar
` (4 preceding siblings ...)
2025-05-07 21:49 ` [PATCH v6 5/8] selftests: ublk: kublk: lift queue initialization out of thread Uday Shankar
@ 2025-05-07 21:49 ` Uday Shankar
2025-05-09 8:14 ` Ming Lei
2025-05-07 21:49 ` [PATCH v6 7/8] selftests: ublk: kublk: decouple ublk_queues from ublk server threads Uday Shankar
2025-05-07 21:49 ` [PATCH v6 8/8] Documentation: ublk: document UBLK_F_RR_TAGS Uday Shankar
7 siblings, 1 reply; 22+ messages in thread
From: Uday Shankar @ 2025-05-07 21:49 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Caleb Sander Mateos, Andrew Morton,
Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc,
Uday Shankar
Towards the goal of decoupling ublk_queues from ublk server threads,
move resources/data that should be per-thread rather than per-queue out
of ublk_queue and into a new struct ublk_thread.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
tools/testing/selftests/ublk/kublk.c | 225 ++++++++++++++++++-----------------
tools/testing/selftests/ublk/kublk.h | 38 ++++--
2 files changed, 145 insertions(+), 118 deletions(-)
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
index 3ad9e162816c3a10e9928f9d530908cda7595530..313689f94cd6361a9a0f4b9257085b2a62bc8b8c 100644
--- a/tools/testing/selftests/ublk/kublk.c
+++ b/tools/testing/selftests/ublk/kublk.c
@@ -324,8 +324,8 @@ static void ublk_ctrl_dump(struct ublk_dev *dev)
for (i = 0; i < info->nr_hw_queues; i++) {
ublk_print_cpu_set(&affinity[i], buf, sizeof(buf));
- printf("\tqueue %u: tid %d affinity(%s)\n",
- i, dev->q[i].tid, buf);
+ printf("\tqueue %u: affinity(%s)\n",
+ i, buf);
}
free(affinity);
}
@@ -395,18 +395,16 @@ static void ublk_queue_deinit(struct ublk_queue *q)
free(q->ios[i].buf_addr);
}
-static void ublk_thread_deinit(struct ublk_queue *q)
+static void ublk_thread_deinit(struct ublk_thread *t)
{
- q->tid = 0;
+ io_uring_unregister_buffers(&t->ring);
- io_uring_unregister_buffers(&q->ring);
+ io_uring_unregister_ring_fd(&t->ring);
- io_uring_unregister_ring_fd(&q->ring);
-
- if (q->ring.ring_fd > 0) {
- io_uring_unregister_files(&q->ring);
- close(q->ring.ring_fd);
- q->ring.ring_fd = -1;
+ if (t->ring.ring_fd > 0) {
+ io_uring_unregister_files(&t->ring);
+ close(t->ring.ring_fd);
+ t->ring.ring_fd = -1;
}
}
@@ -421,7 +419,6 @@ static int ublk_queue_init(struct ublk_queue *q)
q->tgt_ops = dev->tgt.ops;
q->state = 0;
q->q_depth = depth;
- q->cmd_inflight = 0;
if (dev->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY) {
q->state |= UBLKSRV_NO_BUF;
@@ -443,6 +440,7 @@ static int ublk_queue_init(struct ublk_queue *q)
q->ios[i].buf_addr = NULL;
q->ios[i].flags = UBLKSRV_NEED_FETCH_RQ | UBLKSRV_IO_FREE;
q->ios[i].q = q;
+ q->ios[i].tag = i;
if (q->state & UBLKSRV_NO_BUF)
continue;
@@ -463,47 +461,46 @@ static int ublk_queue_init(struct ublk_queue *q)
return -ENOMEM;
}
-static int ublk_thread_init(struct ublk_queue *q)
+static int ublk_thread_init(struct ublk_thread *t)
{
- struct ublk_dev *dev = q->dev;
+ struct ublk_dev *dev = t->dev;
int ring_depth = dev->tgt.sq_depth, cq_depth = dev->tgt.cq_depth;
int ret;
- q->tid = gettid();
-
- ret = ublk_setup_ring(&q->ring, ring_depth, cq_depth,
+ ret = ublk_setup_ring(&t->ring, ring_depth, cq_depth,
IORING_SETUP_COOP_TASKRUN |
IORING_SETUP_SINGLE_ISSUER |
IORING_SETUP_DEFER_TASKRUN);
if (ret < 0) {
- ublk_err("ublk dev %d queue %d setup io_uring failed %d\n",
- q->dev->dev_info.dev_id, q->q_id, ret);
+ ublk_err("ublk dev %d thread %d setup io_uring failed %d\n",
+ dev->dev_info.dev_id, t->idx, ret);
goto fail;
}
if (dev->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY) {
- ret = io_uring_register_buffers_sparse(&q->ring, q->q_depth);
+ ret = io_uring_register_buffers_sparse(
+ &t->ring, dev->dev_info.queue_depth);
if (ret) {
- ublk_err("ublk dev %d queue %d register spare buffers failed %d",
- dev->dev_info.dev_id, q->q_id, ret);
+ ublk_err("ublk dev %d thread %d register spare buffers failed %d",
+ dev->dev_info.dev_id, t->idx, ret);
goto fail;
}
}
- io_uring_register_ring_fd(&q->ring);
+ io_uring_register_ring_fd(&t->ring);
- ret = io_uring_register_files(&q->ring, dev->fds, dev->nr_fds);
+ ret = io_uring_register_files(&t->ring, dev->fds, dev->nr_fds);
if (ret) {
- ublk_err("ublk dev %d queue %d register files failed %d\n",
- q->dev->dev_info.dev_id, q->q_id, ret);
+ ublk_err("ublk dev %d thread %d register files failed %d\n",
+ t->dev->dev_info.dev_id, t->idx, ret);
goto fail;
}
return 0;
fail:
- ublk_thread_deinit(q);
- ublk_err("ublk dev %d queue %d thread init failed\n",
- dev->dev_info.dev_id, q->q_id);
+ ublk_thread_deinit(t);
+ ublk_err("ublk dev %d thread %d init failed\n",
+ dev->dev_info.dev_id, t->idx);
return -ENOMEM;
}
@@ -545,8 +542,9 @@ static void ublk_dev_unprep(struct ublk_dev *dev)
close(dev->fds[0]);
}
-int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
+int ublk_queue_io_cmd(struct ublk_io *io)
{
+ struct ublk_thread *t = io->t;
struct ublksrv_io_cmd *cmd;
struct io_uring_sqe *sqe[1];
unsigned int cmd_op = 0;
@@ -571,13 +569,13 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
else if (io->flags & UBLKSRV_NEED_FETCH_RQ)
cmd_op = UBLK_U_IO_FETCH_REQ;
- if (io_uring_sq_space_left(&q->ring) < 1)
- io_uring_submit(&q->ring);
+ if (io_uring_sq_space_left(&t->ring) < 1)
+ io_uring_submit(&t->ring);
- ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 1);
+ ublk_io_alloc_sqes(io, sqe, 1);
if (!sqe[0]) {
- ublk_err("%s: run out of sqe %d, tag %d\n",
- __func__, q->q_id, tag);
+ ublk_err("%s: run out of sqe. thread %u, tag %d\n",
+ __func__, t->idx, io->tag);
return -1;
}
@@ -592,42 +590,51 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
sqe[0]->opcode = IORING_OP_URING_CMD;
sqe[0]->flags = IOSQE_FIXED_FILE;
sqe[0]->rw_flags = 0;
- cmd->tag = tag;
- cmd->q_id = q->q_id;
- if (!(q->state & UBLKSRV_NO_BUF))
+ cmd->tag = io->tag;
+ cmd->q_id = io->q->q_id;
+ if (!(io->q->state & UBLKSRV_NO_BUF))
cmd->addr = (__u64) (uintptr_t) io->buf_addr;
else
cmd->addr = 0;
- user_data = build_user_data(tag, _IOC_NR(cmd_op), 0, q->q_id, 0);
+ user_data = build_user_data(io->tag, _IOC_NR(cmd_op), 0, io->q->q_id, 0);
io_uring_sqe_set_data64(sqe[0], user_data);
io->flags = 0;
- q->cmd_inflight += 1;
+ t->cmd_inflight += 1;
- ublk_dbg(UBLK_DBG_IO_CMD, "%s: (qid %d tag %u cmd_op %u) iof %x stopping %d\n",
- __func__, q->q_id, tag, cmd_op,
- io->flags, !!(q->state & UBLKSRV_QUEUE_STOPPING));
+ ublk_dbg(UBLK_DBG_IO_CMD, "%s: (thread %u qid %d tag %u cmd_op %u) iof %x stopping %d\n",
+ __func__, t->idx, io->q->q_id, io->tag, cmd_op,
+ io->flags, !!(t->state & UBLKSRV_THREAD_STOPPING));
return 1;
}
-static void ublk_submit_fetch_commands(struct ublk_queue *q)
+static void ublk_submit_fetch_commands(struct ublk_thread *t)
{
+ /*
+ * Service exclusively the queue whose q_id matches our thread
+ * index. This may change in the future.
+ */
+ struct ublk_queue *q = &t->dev->q[t->idx];
+ struct ublk_io *io;
int i = 0;
- for (i = 0; i < q->q_depth; i++)
- ublk_queue_io_cmd(q, &q->ios[i], i);
+ for (i = 0; i < q->q_depth; i++) {
+ io = &q->ios[i];
+ io->t = t;
+ ublk_queue_io_cmd(io);
+ }
}
-static int ublk_queue_is_idle(struct ublk_queue *q)
+static int ublk_thread_is_idle(struct ublk_thread *t)
{
- return !io_uring_sq_ready(&q->ring) && !q->io_inflight;
+ return !io_uring_sq_ready(&t->ring) && !t->io_inflight;
}
-static int ublk_queue_is_done(struct ublk_queue *q)
+static int ublk_thread_is_done(struct ublk_thread *t)
{
- return (q->state & UBLKSRV_QUEUE_STOPPING) && ublk_queue_is_idle(q);
+ return (t->state & UBLKSRV_THREAD_STOPPING) && ublk_thread_is_idle(t);
}
static inline void ublksrv_handle_tgt_cqe(struct ublk_queue *q,
@@ -645,15 +652,16 @@ static inline void ublksrv_handle_tgt_cqe(struct ublk_queue *q,
q->tgt_ops->tgt_io_done(q, tag, cqe);
}
-static void ublk_handle_cqe(struct ublk_dev *dev,
+static void ublk_handle_cqe(struct ublk_thread *t,
struct io_uring_cqe *cqe, void *data)
{
+ struct ublk_dev *dev = t->dev;
unsigned q_id = user_data_to_q_id(cqe->user_data);
struct ublk_queue *q = &dev->q[q_id];
unsigned tag = user_data_to_tag(cqe->user_data);
unsigned cmd_op = user_data_to_op(cqe->user_data);
int fetch = (cqe->res != UBLK_IO_RES_ABORT) &&
- !(q->state & UBLKSRV_QUEUE_STOPPING);
+ !(t->state & UBLKSRV_THREAD_STOPPING);
struct ublk_io *io;
if (cqe->res < 0 && cqe->res != -ENODEV)
@@ -664,7 +672,7 @@ static void ublk_handle_cqe(struct ublk_dev *dev,
__func__, cqe->res, q->q_id, tag, cmd_op,
is_target_io(cqe->user_data),
user_data_to_tgt_data(cqe->user_data),
- (q->state & UBLKSRV_QUEUE_STOPPING));
+ (t->state & UBLKSRV_THREAD_STOPPING));
/* Don't retrieve io in case of target io */
if (is_target_io(cqe->user_data)) {
@@ -673,10 +681,10 @@ static void ublk_handle_cqe(struct ublk_dev *dev,
}
io = &q->ios[tag];
- q->cmd_inflight--;
+ t->cmd_inflight--;
if (!fetch) {
- q->state |= UBLKSRV_QUEUE_STOPPING;
+ t->state |= UBLKSRV_THREAD_STOPPING;
io->flags &= ~UBLKSRV_NEED_FETCH_RQ;
}
@@ -686,7 +694,7 @@ static void ublk_handle_cqe(struct ublk_dev *dev,
q->tgt_ops->queue_io(q, tag);
} else if (cqe->res == UBLK_IO_RES_NEED_GET_DATA) {
io->flags |= UBLKSRV_NEED_GET_DATA | UBLKSRV_IO_FREE;
- ublk_queue_io_cmd(q, io, tag);
+ ublk_queue_io_cmd(io);
} else {
/*
* COMMIT_REQ will be completed immediately since no fetching
@@ -700,87 +708,92 @@ static void ublk_handle_cqe(struct ublk_dev *dev,
}
}
-static int ublk_reap_events_uring(struct ublk_queue *q)
+static int ublk_reap_events_uring(struct ublk_thread *t)
{
struct io_uring_cqe *cqe;
unsigned head;
int count = 0;
- io_uring_for_each_cqe(&q->ring, head, cqe) {
- ublk_handle_cqe(q->dev, cqe, NULL);
+ io_uring_for_each_cqe(&t->ring, head, cqe) {
+ ublk_handle_cqe(t, cqe, NULL);
count += 1;
}
- io_uring_cq_advance(&q->ring, count);
+ io_uring_cq_advance(&t->ring, count);
return count;
}
-static int ublk_process_io(struct ublk_queue *q)
+static int ublk_process_io(struct ublk_thread *t)
{
int ret, reapped;
- ublk_dbg(UBLK_DBG_QUEUE, "dev%d-q%d: to_submit %d inflight cmd %u stopping %d\n",
- q->dev->dev_info.dev_id,
- q->q_id, io_uring_sq_ready(&q->ring),
- q->cmd_inflight,
- (q->state & UBLKSRV_QUEUE_STOPPING));
+ ublk_dbg(UBLK_DBG_THREAD, "dev%d-t%u: to_submit %d inflight cmd %u stopping %d\n",
+ t->dev->dev_info.dev_id,
+ t->idx, io_uring_sq_ready(&t->ring),
+ t->cmd_inflight,
+ (t->state & UBLKSRV_THREAD_STOPPING));
- if (ublk_queue_is_done(q))
+ if (ublk_thread_is_done(t))
return -ENODEV;
- ret = io_uring_submit_and_wait(&q->ring, 1);
- reapped = ublk_reap_events_uring(q);
+ ret = io_uring_submit_and_wait(&t->ring, 1);
+ reapped = ublk_reap_events_uring(t);
- ublk_dbg(UBLK_DBG_QUEUE, "submit result %d, reapped %d stop %d idle %d\n",
- ret, reapped, (q->state & UBLKSRV_QUEUE_STOPPING),
- (q->state & UBLKSRV_QUEUE_IDLE));
+ ublk_dbg(UBLK_DBG_THREAD, "submit result %d, reapped %d stop %d idle %d\n",
+ ret, reapped, (t->state & UBLKSRV_THREAD_STOPPING),
+ (t->state & UBLKSRV_THREAD_IDLE));
return reapped;
}
-static void ublk_queue_set_sched_affinity(const struct ublk_queue *q,
+static void ublk_thread_set_sched_affinity(const struct ublk_thread *t,
cpu_set_t *cpuset)
{
if (sched_setaffinity(0, sizeof(*cpuset), cpuset) < 0)
- ublk_err("ublk dev %u queue %u set affinity failed",
- q->dev->dev_info.dev_id, q->q_id);
+ ublk_err("ublk dev %u thread %u set affinity failed",
+ t->dev->dev_info.dev_id, t->idx);
}
-struct ublk_queue_info {
- struct ublk_queue *q;
- sem_t *queue_sem;
+struct ublk_thread_info {
+ struct ublk_dev *dev;
+ unsigned idx;
+ sem_t *ready;
cpu_set_t *affinity;
};
static void *ublk_io_handler_fn(void *data)
{
- struct ublk_queue_info *info = data;
- struct ublk_queue *q = info->q;
- int dev_id = q->dev->dev_info.dev_id;
+ struct ublk_thread_info *info = data;
+ struct ublk_thread *t = &info->dev->threads[info->idx];
+ int dev_id = info->dev->dev_info.dev_id;
int ret;
- ret = ublk_thread_init(q);
+ t->dev = info->dev;
+ t->idx = info->idx;
+
+ ret = ublk_thread_init(t);
if (ret) {
- ublk_err("ublk dev %d queue %d thread init failed\n",
- dev_id, q->q_id);
+ ublk_err("ublk dev %d thread %u init failed\n",
+ dev_id, t->idx);
return NULL;
}
/* IO perf is sensitive with queue pthread affinity on NUMA machine*/
- ublk_queue_set_sched_affinity(q, info->affinity);
- sem_post(info->queue_sem);
+ ublk_thread_set_sched_affinity(t, info->affinity);
+ sem_post(info->ready);
- ublk_dbg(UBLK_DBG_QUEUE, "tid %d: ublk dev %d queue %d started\n",
- q->tid, dev_id, q->q_id);
+ ublk_dbg(UBLK_DBG_THREAD, "tid %d: ublk dev %d thread %u started\n",
+ gettid(), dev_id, t->idx);
/* submit all io commands to ublk driver */
- ublk_submit_fetch_commands(q);
+ ublk_submit_fetch_commands(t);
do {
- if (ublk_process_io(q) < 0)
+ if (ublk_process_io(t) < 0)
break;
} while (1);
- ublk_dbg(UBLK_DBG_QUEUE, "ublk dev %d queue %d exited\n", dev_id, q->q_id);
- ublk_thread_deinit(q);
+ ublk_dbg(UBLK_DBG_THREAD, "tid %d: ublk dev %d thread %d exiting\n",
+ gettid(), dev_id, t->idx);
+ ublk_thread_deinit(t);
return NULL;
}
@@ -823,20 +836,19 @@ static int ublk_send_dev_event(const struct dev_ctx *ctx, struct ublk_dev *dev,
static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
{
const struct ublksrv_ctrl_dev_info *dinfo = &dev->dev_info;
- struct ublk_queue_info *qinfo;
+ struct ublk_thread_info *tinfo;
cpu_set_t *affinity_buf;
void *thread_ret;
- sem_t queue_sem;
+ sem_t ready;
int ret, i;
ublk_dbg(UBLK_DBG_DEV, "%s enter\n", __func__);
- qinfo = (struct ublk_queue_info *)calloc(sizeof(struct ublk_queue_info),
- dinfo->nr_hw_queues);
- if (!qinfo)
+ tinfo = calloc(sizeof(struct ublk_thread_info), dinfo->nr_hw_queues);
+ if (!tinfo)
return -ENOMEM;
- sem_init(&queue_sem, 0, 0);
+ sem_init(&ready, 0, 0);
ret = ublk_dev_prep(ctx, dev);
if (ret)
return ret;
@@ -856,17 +868,18 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
goto fail;
}
- qinfo[i].q = &dev->q[i];
- qinfo[i].queue_sem = &queue_sem;
- qinfo[i].affinity = &affinity_buf[i];
- pthread_create(&dev->q[i].thread, NULL,
+ tinfo[i].dev = dev;
+ tinfo[i].idx = i;
+ tinfo[i].ready = &ready;
+ tinfo[i].affinity = &affinity_buf[i];
+ pthread_create(&dev->threads[i].thread, NULL,
ublk_io_handler_fn,
- &qinfo[i]);
+ &tinfo[i]);
}
for (i = 0; i < dinfo->nr_hw_queues; i++)
- sem_wait(&queue_sem);
- free(qinfo);
+ sem_wait(&ready);
+ free(tinfo);
free(affinity_buf);
/* everything is fine now, start us */
@@ -889,7 +902,7 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
/* wait until we are terminated */
for (i = 0; i < dinfo->nr_hw_queues; i++)
- pthread_join(dev->q[i].thread, &thread_ret);
+ pthread_join(dev->threads[i].thread, &thread_ret);
fail:
for (i = 0; i < dinfo->nr_hw_queues; i++)
ublk_queue_deinit(&dev->q[i]);
diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
index 7c912116606429215af7dbc2a8ce6b40ef89bfbd..9eb2207fcebe96d34488d057c881db262b9767b3 100644
--- a/tools/testing/selftests/ublk/kublk.h
+++ b/tools/testing/selftests/ublk/kublk.h
@@ -51,10 +51,12 @@
#define UBLK_IO_MAX_BYTES (1 << 20)
#define UBLK_MAX_QUEUES_SHIFT 5
#define UBLK_MAX_QUEUES (1 << UBLK_MAX_QUEUES_SHIFT)
+#define UBLK_MAX_THREADS_SHIFT 5
+#define UBLK_MAX_THREADS (1 << UBLK_MAX_THREADS_SHIFT)
#define UBLK_QUEUE_DEPTH 1024
#define UBLK_DBG_DEV (1U << 0)
-#define UBLK_DBG_QUEUE (1U << 1)
+#define UBLK_DBG_THREAD (1U << 1)
#define UBLK_DBG_IO_CMD (1U << 2)
#define UBLK_DBG_IO (1U << 3)
#define UBLK_DBG_CTRL_CMD (1U << 4)
@@ -62,6 +64,7 @@
struct ublk_dev;
struct ublk_queue;
+struct ublk_thread;
struct stripe_ctx {
/* stripe */
@@ -120,6 +123,8 @@ struct ublk_io {
unsigned short refs; /* used by target code only */
struct ublk_queue *q;
+ struct ublk_thread *t;
+ int tag;
int result;
@@ -160,26 +165,35 @@ struct ublk_tgt {
struct ublk_queue {
int q_id;
int q_depth;
- unsigned int cmd_inflight;
- unsigned int io_inflight;
struct ublk_dev *dev;
const struct ublk_tgt_ops *tgt_ops;
struct ublksrv_io_desc *io_cmd_buf;
- struct io_uring ring;
+
struct ublk_io ios[UBLK_QUEUE_DEPTH];
-#define UBLKSRV_QUEUE_STOPPING (1U << 0)
-#define UBLKSRV_QUEUE_IDLE (1U << 1)
#define UBLKSRV_NO_BUF (1U << 2)
#define UBLKSRV_ZC (1U << 3)
unsigned state;
- pid_t tid;
+};
+
+struct ublk_thread {
+ struct ublk_dev *dev;
+ struct io_uring ring;
+ unsigned int cmd_inflight;
+ unsigned int io_inflight;
+
pthread_t thread;
+ unsigned idx;
+
+#define UBLKSRV_THREAD_STOPPING (1U << 0)
+#define UBLKSRV_THREAD_IDLE (1U << 1)
+ unsigned state;
};
struct ublk_dev {
struct ublk_tgt tgt;
struct ublksrv_ctrl_dev_info dev_info;
struct ublk_queue q[UBLK_MAX_QUEUES];
+ struct ublk_thread threads[UBLK_MAX_THREADS];
int fds[MAX_BACK_FILES + 1]; /* fds[0] points to /dev/ublkcN */
int nr_fds;
@@ -198,7 +212,7 @@ struct ublk_dev {
extern unsigned int ublk_dbg_mask;
-extern int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag);
+extern int ublk_queue_io_cmd(struct ublk_io *io);
static inline int is_target_io(__u64 user_data)
{
@@ -272,7 +286,7 @@ static inline void ublk_dbg(int level, const char *fmt, ...)
static inline int ublk_io_alloc_sqes(struct ublk_io *io,
struct io_uring_sqe *sqes[], int nr_sqes)
{
- struct io_uring *ring = &io->q->ring;
+ struct io_uring *ring = &io->t->ring;
unsigned left = io_uring_sq_space_left(ring);
int i;
@@ -363,7 +377,7 @@ static inline int ublk_complete_io(struct ublk_queue *q, unsigned tag, int res)
ublk_mark_io_done(io, res);
- return ublk_queue_io_cmd(q, io, tag);
+ return ublk_queue_io_cmd(io);
}
static inline void ublk_queued_tgt_io(struct ublk_queue *q, unsigned tag, int queued)
@@ -373,7 +387,7 @@ static inline void ublk_queued_tgt_io(struct ublk_queue *q, unsigned tag, int qu
else {
struct ublk_io *io = ublk_get_io(q, tag);
- q->io_inflight += queued;
+ io->t->io_inflight += queued;
io->tgt_ios = queued;
io->result = 0;
}
@@ -383,7 +397,7 @@ static inline int ublk_completed_tgt_io(struct ublk_queue *q, unsigned tag)
{
struct ublk_io *io = ublk_get_io(q, tag);
- q->io_inflight--;
+ io->t->io_inflight--;
return --io->tgt_ios == 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v6 6/8] selftests: ublk: kublk: move per-thread data out of ublk_queue
2025-05-07 21:49 ` [PATCH v6 6/8] selftests: ublk: kublk: move per-thread data out of ublk_queue Uday Shankar
@ 2025-05-09 8:14 ` Ming Lei
0 siblings, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-05-09 8:14 UTC (permalink / raw)
To: Uday Shankar
Cc: Jens Axboe, Caleb Sander Mateos, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Wed, May 07, 2025 at 03:49:40PM -0600, Uday Shankar wrote:
> Towards the goal of decoupling ublk_queues from ublk server threads,
> move resources/data that should be per-thread rather than per-queue out
> of ublk_queue and into a new struct ublk_thread.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
> tools/testing/selftests/ublk/kublk.c | 225 ++++++++++++++++++-----------------
> tools/testing/selftests/ublk/kublk.h | 38 ++++--
> 2 files changed, 145 insertions(+), 118 deletions(-)
>
> diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
> index 3ad9e162816c3a10e9928f9d530908cda7595530..313689f94cd6361a9a0f4b9257085b2a62bc8b8c 100644
> --- a/tools/testing/selftests/ublk/kublk.c
> +++ b/tools/testing/selftests/ublk/kublk.c
> @@ -324,8 +324,8 @@ static void ublk_ctrl_dump(struct ublk_dev *dev)
>
> for (i = 0; i < info->nr_hw_queues; i++) {
> ublk_print_cpu_set(&affinity[i], buf, sizeof(buf));
> - printf("\tqueue %u: tid %d affinity(%s)\n",
> - i, dev->q[i].tid, buf);
> + printf("\tqueue %u: affinity(%s)\n",
> + i, buf);
> }
> free(affinity);
> }
> @@ -395,18 +395,16 @@ static void ublk_queue_deinit(struct ublk_queue *q)
> free(q->ios[i].buf_addr);
> }
>
> -static void ublk_thread_deinit(struct ublk_queue *q)
> +static void ublk_thread_deinit(struct ublk_thread *t)
> {
> - q->tid = 0;
> + io_uring_unregister_buffers(&t->ring);
>
> - io_uring_unregister_buffers(&q->ring);
> + io_uring_unregister_ring_fd(&t->ring);
>
> - io_uring_unregister_ring_fd(&q->ring);
> -
> - if (q->ring.ring_fd > 0) {
> - io_uring_unregister_files(&q->ring);
> - close(q->ring.ring_fd);
> - q->ring.ring_fd = -1;
> + if (t->ring.ring_fd > 0) {
> + io_uring_unregister_files(&t->ring);
> + close(t->ring.ring_fd);
> + t->ring.ring_fd = -1;
> }
> }
>
> @@ -421,7 +419,6 @@ static int ublk_queue_init(struct ublk_queue *q)
> q->tgt_ops = dev->tgt.ops;
> q->state = 0;
> q->q_depth = depth;
> - q->cmd_inflight = 0;
>
> if (dev->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY) {
> q->state |= UBLKSRV_NO_BUF;
> @@ -443,6 +440,7 @@ static int ublk_queue_init(struct ublk_queue *q)
> q->ios[i].buf_addr = NULL;
> q->ios[i].flags = UBLKSRV_NEED_FETCH_RQ | UBLKSRV_IO_FREE;
> q->ios[i].q = q;
> + q->ios[i].tag = i;
>
> if (q->state & UBLKSRV_NO_BUF)
> continue;
> @@ -463,47 +461,46 @@ static int ublk_queue_init(struct ublk_queue *q)
> return -ENOMEM;
> }
>
> -static int ublk_thread_init(struct ublk_queue *q)
> +static int ublk_thread_init(struct ublk_thread *t)
> {
> - struct ublk_dev *dev = q->dev;
> + struct ublk_dev *dev = t->dev;
> int ring_depth = dev->tgt.sq_depth, cq_depth = dev->tgt.cq_depth;
> int ret;
>
> - q->tid = gettid();
> -
> - ret = ublk_setup_ring(&q->ring, ring_depth, cq_depth,
> + ret = ublk_setup_ring(&t->ring, ring_depth, cq_depth,
> IORING_SETUP_COOP_TASKRUN |
> IORING_SETUP_SINGLE_ISSUER |
> IORING_SETUP_DEFER_TASKRUN);
> if (ret < 0) {
> - ublk_err("ublk dev %d queue %d setup io_uring failed %d\n",
> - q->dev->dev_info.dev_id, q->q_id, ret);
> + ublk_err("ublk dev %d thread %d setup io_uring failed %d\n",
> + dev->dev_info.dev_id, t->idx, ret);
> goto fail;
> }
>
> if (dev->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY) {
> - ret = io_uring_register_buffers_sparse(&q->ring, q->q_depth);
> + ret = io_uring_register_buffers_sparse(
> + &t->ring, dev->dev_info.queue_depth);
> if (ret) {
> - ublk_err("ublk dev %d queue %d register spare buffers failed %d",
> - dev->dev_info.dev_id, q->q_id, ret);
> + ublk_err("ublk dev %d thread %d register spare buffers failed %d",
> + dev->dev_info.dev_id, t->idx, ret);
> goto fail;
> }
> }
>
> - io_uring_register_ring_fd(&q->ring);
> + io_uring_register_ring_fd(&t->ring);
>
> - ret = io_uring_register_files(&q->ring, dev->fds, dev->nr_fds);
> + ret = io_uring_register_files(&t->ring, dev->fds, dev->nr_fds);
> if (ret) {
> - ublk_err("ublk dev %d queue %d register files failed %d\n",
> - q->dev->dev_info.dev_id, q->q_id, ret);
> + ublk_err("ublk dev %d thread %d register files failed %d\n",
> + t->dev->dev_info.dev_id, t->idx, ret);
> goto fail;
> }
>
> return 0;
> fail:
> - ublk_thread_deinit(q);
> - ublk_err("ublk dev %d queue %d thread init failed\n",
> - dev->dev_info.dev_id, q->q_id);
> + ublk_thread_deinit(t);
> + ublk_err("ublk dev %d thread %d init failed\n",
> + dev->dev_info.dev_id, t->idx);
> return -ENOMEM;
> }
>
> @@ -545,8 +542,9 @@ static void ublk_dev_unprep(struct ublk_dev *dev)
> close(dev->fds[0]);
> }
>
> -int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
> +int ublk_queue_io_cmd(struct ublk_io *io)
> {
> + struct ublk_thread *t = io->t;
> struct ublksrv_io_cmd *cmd;
> struct io_uring_sqe *sqe[1];
> unsigned int cmd_op = 0;
> @@ -571,13 +569,13 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
> else if (io->flags & UBLKSRV_NEED_FETCH_RQ)
> cmd_op = UBLK_U_IO_FETCH_REQ;
>
> - if (io_uring_sq_space_left(&q->ring) < 1)
> - io_uring_submit(&q->ring);
> + if (io_uring_sq_space_left(&t->ring) < 1)
> + io_uring_submit(&t->ring);
>
> - ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 1);
> + ublk_io_alloc_sqes(io, sqe, 1);
> if (!sqe[0]) {
> - ublk_err("%s: run out of sqe %d, tag %d\n",
> - __func__, q->q_id, tag);
> + ublk_err("%s: run out of sqe. thread %u, tag %d\n",
> + __func__, t->idx, io->tag);
> return -1;
> }
>
> @@ -592,42 +590,51 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
> sqe[0]->opcode = IORING_OP_URING_CMD;
> sqe[0]->flags = IOSQE_FIXED_FILE;
> sqe[0]->rw_flags = 0;
> - cmd->tag = tag;
> - cmd->q_id = q->q_id;
> - if (!(q->state & UBLKSRV_NO_BUF))
> + cmd->tag = io->tag;
> + cmd->q_id = io->q->q_id;
> + if (!(io->q->state & UBLKSRV_NO_BUF))
> cmd->addr = (__u64) (uintptr_t) io->buf_addr;
> else
> cmd->addr = 0;
>
> - user_data = build_user_data(tag, _IOC_NR(cmd_op), 0, q->q_id, 0);
> + user_data = build_user_data(io->tag, _IOC_NR(cmd_op), 0, io->q->q_id, 0);
> io_uring_sqe_set_data64(sqe[0], user_data);
>
> io->flags = 0;
>
> - q->cmd_inflight += 1;
> + t->cmd_inflight += 1;
>
> - ublk_dbg(UBLK_DBG_IO_CMD, "%s: (qid %d tag %u cmd_op %u) iof %x stopping %d\n",
> - __func__, q->q_id, tag, cmd_op,
> - io->flags, !!(q->state & UBLKSRV_QUEUE_STOPPING));
> + ublk_dbg(UBLK_DBG_IO_CMD, "%s: (thread %u qid %d tag %u cmd_op %u) iof %x stopping %d\n",
> + __func__, t->idx, io->q->q_id, io->tag, cmd_op,
> + io->flags, !!(t->state & UBLKSRV_THREAD_STOPPING));
> return 1;
> }
>
> -static void ublk_submit_fetch_commands(struct ublk_queue *q)
> +static void ublk_submit_fetch_commands(struct ublk_thread *t)
> {
> + /*
> + * Service exclusively the queue whose q_id matches our thread
> + * index. This may change in the future.
> + */
> + struct ublk_queue *q = &t->dev->q[t->idx];
> + struct ublk_io *io;
> int i = 0;
>
> - for (i = 0; i < q->q_depth; i++)
> - ublk_queue_io_cmd(q, &q->ios[i], i);
> + for (i = 0; i < q->q_depth; i++) {
> + io = &q->ios[i];
> + io->t = t;
> + ublk_queue_io_cmd(io);
> + }
> }
>
> -static int ublk_queue_is_idle(struct ublk_queue *q)
> +static int ublk_thread_is_idle(struct ublk_thread *t)
> {
> - return !io_uring_sq_ready(&q->ring) && !q->io_inflight;
> + return !io_uring_sq_ready(&t->ring) && !t->io_inflight;
> }
>
> -static int ublk_queue_is_done(struct ublk_queue *q)
> +static int ublk_thread_is_done(struct ublk_thread *t)
> {
> - return (q->state & UBLKSRV_QUEUE_STOPPING) && ublk_queue_is_idle(q);
> + return (t->state & UBLKSRV_THREAD_STOPPING) && ublk_thread_is_idle(t);
> }
>
> static inline void ublksrv_handle_tgt_cqe(struct ublk_queue *q,
> @@ -645,15 +652,16 @@ static inline void ublksrv_handle_tgt_cqe(struct ublk_queue *q,
> q->tgt_ops->tgt_io_done(q, tag, cqe);
> }
>
> -static void ublk_handle_cqe(struct ublk_dev *dev,
> +static void ublk_handle_cqe(struct ublk_thread *t,
> struct io_uring_cqe *cqe, void *data)
> {
> + struct ublk_dev *dev = t->dev;
> unsigned q_id = user_data_to_q_id(cqe->user_data);
> struct ublk_queue *q = &dev->q[q_id];
> unsigned tag = user_data_to_tag(cqe->user_data);
> unsigned cmd_op = user_data_to_op(cqe->user_data);
> int fetch = (cqe->res != UBLK_IO_RES_ABORT) &&
> - !(q->state & UBLKSRV_QUEUE_STOPPING);
> + !(t->state & UBLKSRV_THREAD_STOPPING);
> struct ublk_io *io;
>
> if (cqe->res < 0 && cqe->res != -ENODEV)
> @@ -664,7 +672,7 @@ static void ublk_handle_cqe(struct ublk_dev *dev,
> __func__, cqe->res, q->q_id, tag, cmd_op,
> is_target_io(cqe->user_data),
> user_data_to_tgt_data(cqe->user_data),
> - (q->state & UBLKSRV_QUEUE_STOPPING));
> + (t->state & UBLKSRV_THREAD_STOPPING));
>
> /* Don't retrieve io in case of target io */
> if (is_target_io(cqe->user_data)) {
> @@ -673,10 +681,10 @@ static void ublk_handle_cqe(struct ublk_dev *dev,
> }
>
> io = &q->ios[tag];
> - q->cmd_inflight--;
> + t->cmd_inflight--;
>
> if (!fetch) {
> - q->state |= UBLKSRV_QUEUE_STOPPING;
> + t->state |= UBLKSRV_THREAD_STOPPING;
> io->flags &= ~UBLKSRV_NEED_FETCH_RQ;
> }
>
> @@ -686,7 +694,7 @@ static void ublk_handle_cqe(struct ublk_dev *dev,
> q->tgt_ops->queue_io(q, tag);
> } else if (cqe->res == UBLK_IO_RES_NEED_GET_DATA) {
> io->flags |= UBLKSRV_NEED_GET_DATA | UBLKSRV_IO_FREE;
> - ublk_queue_io_cmd(q, io, tag);
> + ublk_queue_io_cmd(io);
> } else {
> /*
> * COMMIT_REQ will be completed immediately since no fetching
> @@ -700,87 +708,92 @@ static void ublk_handle_cqe(struct ublk_dev *dev,
> }
> }
>
> -static int ublk_reap_events_uring(struct ublk_queue *q)
> +static int ublk_reap_events_uring(struct ublk_thread *t)
> {
> struct io_uring_cqe *cqe;
> unsigned head;
> int count = 0;
>
> - io_uring_for_each_cqe(&q->ring, head, cqe) {
> - ublk_handle_cqe(q->dev, cqe, NULL);
> + io_uring_for_each_cqe(&t->ring, head, cqe) {
> + ublk_handle_cqe(t, cqe, NULL);
> count += 1;
> }
> - io_uring_cq_advance(&q->ring, count);
> + io_uring_cq_advance(&t->ring, count);
>
> return count;
> }
>
> -static int ublk_process_io(struct ublk_queue *q)
> +static int ublk_process_io(struct ublk_thread *t)
> {
> int ret, reapped;
>
> - ublk_dbg(UBLK_DBG_QUEUE, "dev%d-q%d: to_submit %d inflight cmd %u stopping %d\n",
> - q->dev->dev_info.dev_id,
> - q->q_id, io_uring_sq_ready(&q->ring),
> - q->cmd_inflight,
> - (q->state & UBLKSRV_QUEUE_STOPPING));
> + ublk_dbg(UBLK_DBG_THREAD, "dev%d-t%u: to_submit %d inflight cmd %u stopping %d\n",
> + t->dev->dev_info.dev_id,
> + t->idx, io_uring_sq_ready(&t->ring),
> + t->cmd_inflight,
> + (t->state & UBLKSRV_THREAD_STOPPING));
>
> - if (ublk_queue_is_done(q))
> + if (ublk_thread_is_done(t))
> return -ENODEV;
>
> - ret = io_uring_submit_and_wait(&q->ring, 1);
> - reapped = ublk_reap_events_uring(q);
> + ret = io_uring_submit_and_wait(&t->ring, 1);
> + reapped = ublk_reap_events_uring(t);
>
> - ublk_dbg(UBLK_DBG_QUEUE, "submit result %d, reapped %d stop %d idle %d\n",
> - ret, reapped, (q->state & UBLKSRV_QUEUE_STOPPING),
> - (q->state & UBLKSRV_QUEUE_IDLE));
> + ublk_dbg(UBLK_DBG_THREAD, "submit result %d, reapped %d stop %d idle %d\n",
> + ret, reapped, (t->state & UBLKSRV_THREAD_STOPPING),
> + (t->state & UBLKSRV_THREAD_IDLE));
>
> return reapped;
> }
>
> -static void ublk_queue_set_sched_affinity(const struct ublk_queue *q,
> +static void ublk_thread_set_sched_affinity(const struct ublk_thread *t,
> cpu_set_t *cpuset)
> {
> if (sched_setaffinity(0, sizeof(*cpuset), cpuset) < 0)
> - ublk_err("ublk dev %u queue %u set affinity failed",
> - q->dev->dev_info.dev_id, q->q_id);
> + ublk_err("ublk dev %u thread %u set affinity failed",
> + t->dev->dev_info.dev_id, t->idx);
> }
>
> -struct ublk_queue_info {
> - struct ublk_queue *q;
> - sem_t *queue_sem;
> +struct ublk_thread_info {
> + struct ublk_dev *dev;
> + unsigned idx;
> + sem_t *ready;
> cpu_set_t *affinity;
> };
>
> static void *ublk_io_handler_fn(void *data)
> {
> - struct ublk_queue_info *info = data;
> - struct ublk_queue *q = info->q;
> - int dev_id = q->dev->dev_info.dev_id;
> + struct ublk_thread_info *info = data;
> + struct ublk_thread *t = &info->dev->threads[info->idx];
> + int dev_id = info->dev->dev_info.dev_id;
> int ret;
>
> - ret = ublk_thread_init(q);
> + t->dev = info->dev;
> + t->idx = info->idx;
> +
> + ret = ublk_thread_init(t);
> if (ret) {
> - ublk_err("ublk dev %d queue %d thread init failed\n",
> - dev_id, q->q_id);
> + ublk_err("ublk dev %d thread %u init failed\n",
> + dev_id, t->idx);
> return NULL;
> }
> /* IO perf is sensitive with queue pthread affinity on NUMA machine*/
> - ublk_queue_set_sched_affinity(q, info->affinity);
> - sem_post(info->queue_sem);
> + ublk_thread_set_sched_affinity(t, info->affinity);
> + sem_post(info->ready);
>
> - ublk_dbg(UBLK_DBG_QUEUE, "tid %d: ublk dev %d queue %d started\n",
> - q->tid, dev_id, q->q_id);
> + ublk_dbg(UBLK_DBG_THREAD, "tid %d: ublk dev %d thread %u started\n",
> + gettid(), dev_id, t->idx);
>
> /* submit all io commands to ublk driver */
> - ublk_submit_fetch_commands(q);
> + ublk_submit_fetch_commands(t);
> do {
> - if (ublk_process_io(q) < 0)
> + if (ublk_process_io(t) < 0)
> break;
> } while (1);
>
> - ublk_dbg(UBLK_DBG_QUEUE, "ublk dev %d queue %d exited\n", dev_id, q->q_id);
> - ublk_thread_deinit(q);
> + ublk_dbg(UBLK_DBG_THREAD, "tid %d: ublk dev %d thread %d exiting\n",
> + gettid(), dev_id, t->idx);
> + ublk_thread_deinit(t);
> return NULL;
> }
>
> @@ -823,20 +836,19 @@ static int ublk_send_dev_event(const struct dev_ctx *ctx, struct ublk_dev *dev,
> static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
> {
> const struct ublksrv_ctrl_dev_info *dinfo = &dev->dev_info;
> - struct ublk_queue_info *qinfo;
> + struct ublk_thread_info *tinfo;
> cpu_set_t *affinity_buf;
> void *thread_ret;
> - sem_t queue_sem;
> + sem_t ready;
> int ret, i;
>
> ublk_dbg(UBLK_DBG_DEV, "%s enter\n", __func__);
>
> - qinfo = (struct ublk_queue_info *)calloc(sizeof(struct ublk_queue_info),
> - dinfo->nr_hw_queues);
> - if (!qinfo)
> + tinfo = calloc(sizeof(struct ublk_thread_info), dinfo->nr_hw_queues);
> + if (!tinfo)
> return -ENOMEM;
>
> - sem_init(&queue_sem, 0, 0);
> + sem_init(&ready, 0, 0);
> ret = ublk_dev_prep(ctx, dev);
> if (ret)
> return ret;
> @@ -856,17 +868,18 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
> goto fail;
> }
>
> - qinfo[i].q = &dev->q[i];
> - qinfo[i].queue_sem = &queue_sem;
> - qinfo[i].affinity = &affinity_buf[i];
> - pthread_create(&dev->q[i].thread, NULL,
> + tinfo[i].dev = dev;
> + tinfo[i].idx = i;
> + tinfo[i].ready = &ready;
> + tinfo[i].affinity = &affinity_buf[i];
> + pthread_create(&dev->threads[i].thread, NULL,
> ublk_io_handler_fn,
> - &qinfo[i]);
> + &tinfo[i]);
> }
>
> for (i = 0; i < dinfo->nr_hw_queues; i++)
> - sem_wait(&queue_sem);
> - free(qinfo);
> + sem_wait(&ready);
> + free(tinfo);
> free(affinity_buf);
>
> /* everything is fine now, start us */
> @@ -889,7 +902,7 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
>
> /* wait until we are terminated */
> for (i = 0; i < dinfo->nr_hw_queues; i++)
> - pthread_join(dev->q[i].thread, &thread_ret);
> + pthread_join(dev->threads[i].thread, &thread_ret);
> fail:
> for (i = 0; i < dinfo->nr_hw_queues; i++)
> ublk_queue_deinit(&dev->q[i]);
> diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
> index 7c912116606429215af7dbc2a8ce6b40ef89bfbd..9eb2207fcebe96d34488d057c881db262b9767b3 100644
> --- a/tools/testing/selftests/ublk/kublk.h
> +++ b/tools/testing/selftests/ublk/kublk.h
> @@ -51,10 +51,12 @@
> #define UBLK_IO_MAX_BYTES (1 << 20)
> #define UBLK_MAX_QUEUES_SHIFT 5
> #define UBLK_MAX_QUEUES (1 << UBLK_MAX_QUEUES_SHIFT)
> +#define UBLK_MAX_THREADS_SHIFT 5
> +#define UBLK_MAX_THREADS (1 << UBLK_MAX_THREADS_SHIFT)
> #define UBLK_QUEUE_DEPTH 1024
>
> #define UBLK_DBG_DEV (1U << 0)
> -#define UBLK_DBG_QUEUE (1U << 1)
> +#define UBLK_DBG_THREAD (1U << 1)
> #define UBLK_DBG_IO_CMD (1U << 2)
> #define UBLK_DBG_IO (1U << 3)
> #define UBLK_DBG_CTRL_CMD (1U << 4)
> @@ -62,6 +64,7 @@
>
> struct ublk_dev;
> struct ublk_queue;
> +struct ublk_thread;
>
> struct stripe_ctx {
> /* stripe */
> @@ -120,6 +123,8 @@ struct ublk_io {
> unsigned short refs; /* used by target code only */
>
> struct ublk_queue *q;
> + struct ublk_thread *t;
Given you have to take static mapping between queue/tag and thread,
'struct ublk_thread' should have been figured out runtime easily,
then we can save 8 bytes, also avoid memory indirect dereference.
sizeof(struct ublk_io) need to be held in single L1 cacheline.
But it can be one followup.
Reviewed-by: Ming Lei <ming.lei@redhat.com>
thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6 7/8] selftests: ublk: kublk: decouple ublk_queues from ublk server threads
2025-05-07 21:49 [PATCH v6 0/8] ublk: decouple server threads from hctxs Uday Shankar
` (5 preceding siblings ...)
2025-05-07 21:49 ` [PATCH v6 6/8] selftests: ublk: kublk: move per-thread data out of ublk_queue Uday Shankar
@ 2025-05-07 21:49 ` Uday Shankar
2025-05-09 8:31 ` Ming Lei
2025-05-09 8:46 ` Ming Lei
2025-05-07 21:49 ` [PATCH v6 8/8] Documentation: ublk: document UBLK_F_RR_TAGS Uday Shankar
7 siblings, 2 replies; 22+ messages in thread
From: Uday Shankar @ 2025-05-07 21:49 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Caleb Sander Mateos, Andrew Morton,
Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc,
Uday Shankar
Add support in kublk for decoupled ublk_queues and ublk server threads.
kublk now has two modes of operation:
- (preexisting mode) threads and queues are paired 1:1, and each thread
services all the I/Os of one queue
- (new mode) thread and queue counts are independently configurable.
threads service I/Os in a way that balances load across threads even
if load is not balanced over queues. requires passing --round_robin
The new mode of operation is exercised by the new test_generic_08, which
issues I/O against a single queue and verifies that each of the 8 ublk
server threads handles exactly 1/8 of the total I/O count. Under the old
mode of operation (i.e. without --round_robin), all I/O goes to one ublk
server thread, and the test fails.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
tools/testing/selftests/ublk/Makefile | 1 +
tools/testing/selftests/ublk/file_backed.c | 4 +-
tools/testing/selftests/ublk/kublk.c | 96 ++++++++++++++++++----
tools/testing/selftests/ublk/kublk.h | 3 +
tools/testing/selftests/ublk/null.c | 4 +-
tools/testing/selftests/ublk/stripe.c | 4 +-
tools/testing/selftests/ublk/test_generic_08.sh | 61 ++++++++++++++
.../selftests/ublk/trace/count_ios_per_tid.bt | 9 ++
8 files changed, 160 insertions(+), 22 deletions(-)
diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
index e2e7b1e52a06252f626df5606221d17e3106d0d3..f8579019f0d1f9185df098440611c3b75087073b 100644
--- a/tools/testing/selftests/ublk/Makefile
+++ b/tools/testing/selftests/ublk/Makefile
@@ -14,6 +14,7 @@ TEST_PROGS += test_generic_04.sh
TEST_PROGS += test_generic_05.sh
TEST_PROGS += test_generic_06.sh
TEST_PROGS += test_generic_07.sh
+TEST_PROGS += test_generic_08.sh
TEST_PROGS += test_null_01.sh
TEST_PROGS += test_null_02.sh
diff --git a/tools/testing/selftests/ublk/file_backed.c b/tools/testing/selftests/ublk/file_backed.c
index 563f11a21604bbf5b9531f69f806d09cdd785960..72e22f54f7fe60d26096ace4eb4001987fbe7b15 100644
--- a/tools/testing/selftests/ublk/file_backed.c
+++ b/tools/testing/selftests/ublk/file_backed.c
@@ -50,7 +50,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 3);
- io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
+ io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, ublk_get_io(q, tag)->buf_index);
sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
sqe[0]->user_data = build_user_data(tag,
ublk_cmd_op_nr(sqe[0]->cmd_op), 0, q->q_id, 1);
@@ -62,7 +62,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
sqe[1]->flags |= IOSQE_FIXED_FILE | IOSQE_IO_HARDLINK;
sqe[1]->user_data = build_user_data(tag, ublk_op, 0, q->q_id, 1);
- io_uring_prep_buf_unregister(sqe[2], 0, tag, q->q_id, tag);
+ io_uring_prep_buf_unregister(sqe[2], 0, tag, q->q_id, ublk_get_io(q, tag)->buf_index);
sqe[2]->user_data = build_user_data(tag, ublk_cmd_op_nr(sqe[2]->cmd_op), 0, q->q_id, 1);
return 2;
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
index 313689f94cd6361a9a0f4b9257085b2a62bc8b8c..27046bb6a13b99879ad164ff8eaabeba57e17387 100644
--- a/tools/testing/selftests/ublk/kublk.c
+++ b/tools/testing/selftests/ublk/kublk.c
@@ -478,8 +478,11 @@ static int ublk_thread_init(struct ublk_thread *t)
}
if (dev->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY) {
+ unsigned nr_ios = dev->dev_info.queue_depth * dev->dev_info.nr_hw_queues;
+ unsigned max_nr_ios_per_thread = nr_ios / dev->nthreads;
+ max_nr_ios_per_thread += !!(nr_ios % dev->nthreads);
ret = io_uring_register_buffers_sparse(
- &t->ring, dev->dev_info.queue_depth);
+ &t->ring, max_nr_ios_per_thread);
if (ret) {
ublk_err("ublk dev %d thread %d register spare buffers failed %d",
dev->dev_info.dev_id, t->idx, ret);
@@ -612,18 +615,42 @@ int ublk_queue_io_cmd(struct ublk_io *io)
static void ublk_submit_fetch_commands(struct ublk_thread *t)
{
- /*
- * Service exclusively the queue whose q_id matches our thread
- * index. This may change in the future.
- */
- struct ublk_queue *q = &t->dev->q[t->idx];
+ struct ublk_queue *q;
struct ublk_io *io;
- int i = 0;
+ int i = 0, j = 0;
- for (i = 0; i < q->q_depth; i++) {
- io = &q->ios[i];
- io->t = t;
- ublk_queue_io_cmd(io);
+ if (t->dev->dev_info.flags & UBLK_F_RR_TAGS) {
+ /*
+ * Lexicographically order all the (qid,tag) pairs, with
+ * qid taking priority, and give this thread every Nth
+ * entry, where N is the total number of threads. The
+ * offset is controlled by the thread index. This takes
+ * load which may be imbalanced across the queues and
+ * balances it across the threads.
+ */
+ const struct ublksrv_ctrl_dev_info *dinfo = &t->dev->dev_info;
+ int nr_ios = dinfo->nr_hw_queues * dinfo->queue_depth;
+ for (i = t->idx; i < nr_ios; i += t->dev->nthreads, j++) {
+ int q_id = i / dinfo->queue_depth;
+ int tag = i % dinfo->queue_depth;
+ q = &t->dev->q[q_id];
+ io = &q->ios[tag];
+ io->t = t;
+ io->buf_index = j;
+ ublk_queue_io_cmd(io);
+ }
+ } else {
+ /*
+ * Service exclusively the queue whose q_id matches our
+ * thread index.
+ */
+ struct ublk_queue *q = &t->dev->q[t->idx];
+ for (i = 0; i < q->q_depth; i++) {
+ io = &q->ios[i];
+ io->t = t;
+ io->buf_index = i;
+ ublk_queue_io_cmd(io);
+ }
}
}
@@ -778,7 +805,8 @@ static void *ublk_io_handler_fn(void *data)
return NULL;
}
/* IO perf is sensitive with queue pthread affinity on NUMA machine*/
- ublk_thread_set_sched_affinity(t, info->affinity);
+ if (info->affinity)
+ ublk_thread_set_sched_affinity(t, info->affinity);
sem_post(info->ready);
ublk_dbg(UBLK_DBG_THREAD, "tid %d: ublk dev %d thread %u started\n",
@@ -844,7 +872,7 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
ublk_dbg(UBLK_DBG_DEV, "%s enter\n", __func__);
- tinfo = calloc(sizeof(struct ublk_thread_info), dinfo->nr_hw_queues);
+ tinfo = calloc(sizeof(struct ublk_thread_info), dev->nthreads);
if (!tinfo)
return -ENOMEM;
@@ -867,17 +895,24 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
dinfo->dev_id, i);
goto fail;
}
+ }
+ for (i = 0; i < dev->nthreads; i++) {
tinfo[i].dev = dev;
tinfo[i].idx = i;
tinfo[i].ready = &ready;
- tinfo[i].affinity = &affinity_buf[i];
+ /*
+ * If threads are not tied to queues, setting thread
+ * affinity based on queue affinity makes no sense.
+ */
+ if (!(dinfo->flags & UBLK_F_RR_TAGS))
+ tinfo[i].affinity = &affinity_buf[i];
pthread_create(&dev->threads[i].thread, NULL,
ublk_io_handler_fn,
&tinfo[i]);
}
- for (i = 0; i < dinfo->nr_hw_queues; i++)
+ for (i = 0; i < dev->nthreads; i++)
sem_wait(&ready);
free(tinfo);
free(affinity_buf);
@@ -901,7 +936,7 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
ublk_send_dev_event(ctx, dev, dev->dev_info.dev_id);
/* wait until we are terminated */
- for (i = 0; i < dinfo->nr_hw_queues; i++)
+ for (i = 0; i < dev->nthreads; i++)
pthread_join(dev->threads[i].thread, &thread_ret);
fail:
for (i = 0; i < dinfo->nr_hw_queues; i++)
@@ -1011,6 +1046,7 @@ static int ublk_stop_io_daemon(const struct ublk_dev *dev)
static int __cmd_dev_add(const struct dev_ctx *ctx)
{
+ unsigned nthreads = ctx->nthreads;
unsigned nr_queues = ctx->nr_hw_queues;
const char *tgt_type = ctx->tgt_type;
unsigned depth = ctx->queue_depth;
@@ -1034,6 +1070,23 @@ static int __cmd_dev_add(const struct dev_ctx *ctx)
return -EINVAL;
}
+ /* default to 1:1 threads:queues if nthreads is unspecified */
+ if (nthreads == -1)
+ nthreads = nr_queues;
+
+ if (nthreads > UBLK_MAX_THREADS) {
+ ublk_err("%s: %u is too many threads (max %u)\n",
+ __func__, nthreads, UBLK_MAX_THREADS);
+ return -EINVAL;
+ }
+
+ if (nthreads != nr_queues && !(ctx->flags & UBLK_F_RR_TAGS)) {
+ ublk_err("%s: threads %u must be same as queues %u if "
+ "not using round robin\n",
+ __func__, nthreads, nr_queues);
+ return -EINVAL;
+ }
+
dev = ublk_ctrl_init();
if (!dev) {
ublk_err("%s: can't alloc dev id %d, type %s\n",
@@ -1054,6 +1107,7 @@ static int __cmd_dev_add(const struct dev_ctx *ctx)
info->nr_hw_queues = nr_queues;
info->queue_depth = depth;
info->flags = ctx->flags;
+ dev->nthreads = nthreads;
dev->tgt.ops = ops;
dev->tgt.sq_depth = depth;
dev->tgt.cq_depth = depth;
@@ -1249,6 +1303,7 @@ static int cmd_dev_get_features(void)
[const_ilog2(UBLK_F_USER_COPY)] = "USER_COPY",
[const_ilog2(UBLK_F_ZONED)] = "ZONED",
[const_ilog2(UBLK_F_USER_RECOVERY_FAIL_IO)] = "RECOVERY_FAIL_IO",
+ [const_ilog2(UBLK_F_RR_TAGS)] = "RR_TAGS",
};
struct ublk_dev *dev;
__u64 features = 0;
@@ -1290,8 +1345,10 @@ static void __cmd_create_help(char *exe, bool recovery)
exe, recovery ? "recover" : "add");
printf("\t[--foreground] [--quiet] [-z] [--debug_mask mask] [-r 0|1 ] [-g]\n");
printf("\t[-e 0|1 ] [-i 0|1]\n");
+ printf("\t[--nthreads threads] [--round_robin]\n");
printf("\t[target options] [backfile1] [backfile2] ...\n");
printf("\tdefault: nr_queues=2(max 32), depth=128(max 1024), dev_id=-1(auto allocation)\n");
+ printf("\tdefault: nthreads=nr_queues");
for (i = 0; i < sizeof(tgt_ops_list) / sizeof(tgt_ops_list[0]); i++) {
const struct ublk_tgt_ops *ops = tgt_ops_list[i];
@@ -1343,6 +1400,8 @@ int main(int argc, char *argv[])
{ "recovery_fail_io", 1, NULL, 'e'},
{ "recovery_reissue", 1, NULL, 'i'},
{ "get_data", 1, NULL, 'g'},
+ { "nthreads", 1, NULL, 0 },
+ { "round_robin", 0, NULL, 0 },
{ 0, 0, 0, 0 }
};
const struct ublk_tgt_ops *ops = NULL;
@@ -1351,6 +1410,7 @@ int main(int argc, char *argv[])
struct dev_ctx ctx = {
.queue_depth = 128,
.nr_hw_queues = 2,
+ .nthreads = -1,
.dev_id = -1,
.tgt_type = "unknown",
};
@@ -1411,6 +1471,10 @@ int main(int argc, char *argv[])
ublk_dbg_mask = 0;
if (!strcmp(longopts[option_idx].name, "foreground"))
ctx.fg = 1;
+ if (!strcmp(longopts[option_idx].name, "nthreads"))
+ ctx.nthreads = strtol(optarg, NULL, 10);
+ if (!strcmp(longopts[option_idx].name, "round_robin"))
+ ctx.flags |= UBLK_F_RR_TAGS;
break;
case '?':
/*
diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
index 9eb2207fcebe96d34488d057c881db262b9767b3..c51c01d8be43293fc0760f3b5ebac927afbc1fe3 100644
--- a/tools/testing/selftests/ublk/kublk.h
+++ b/tools/testing/selftests/ublk/kublk.h
@@ -80,6 +80,7 @@ struct dev_ctx {
char tgt_type[16];
unsigned long flags;
unsigned nr_hw_queues;
+ unsigned nthreads;
unsigned queue_depth;
int dev_id;
int nr_files;
@@ -125,6 +126,7 @@ struct ublk_io {
struct ublk_queue *q;
struct ublk_thread *t;
int tag;
+ int buf_index;
int result;
@@ -194,6 +196,7 @@ struct ublk_dev {
struct ublksrv_ctrl_dev_info dev_info;
struct ublk_queue q[UBLK_MAX_QUEUES];
struct ublk_thread threads[UBLK_MAX_THREADS];
+ unsigned nthreads;
int fds[MAX_BACK_FILES + 1]; /* fds[0] points to /dev/ublkcN */
int nr_fds;
diff --git a/tools/testing/selftests/ublk/null.c b/tools/testing/selftests/ublk/null.c
index 7323fe61212f6041ef5a04758d30e62376ac9c6b..776da92306d8449b5b1ab89375ea4078a7948390 100644
--- a/tools/testing/selftests/ublk/null.c
+++ b/tools/testing/selftests/ublk/null.c
@@ -50,7 +50,7 @@ static int null_queue_zc_io(struct ublk_queue *q, int tag)
ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 3);
- io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
+ io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, ublk_get_io(q, tag)->buf_index);
sqe[0]->user_data = build_user_data(tag,
ublk_cmd_op_nr(sqe[0]->cmd_op), 0, q->q_id, 1);
sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
@@ -62,7 +62,7 @@ static int null_queue_zc_io(struct ublk_queue *q, int tag)
sqe[1]->len = iod->nr_sectors << 9; /* injected result */
sqe[1]->user_data = build_user_data(tag, ublk_op, 0, q->q_id, 1);
- io_uring_prep_buf_unregister(sqe[2], 0, tag, q->q_id, tag);
+ io_uring_prep_buf_unregister(sqe[2], 0, tag, q->q_id, ublk_get_io(q, tag)->buf_index);
sqe[2]->user_data = build_user_data(tag, ublk_cmd_op_nr(sqe[2]->cmd_op), 0, q->q_id, 1);
// buf register is marked as IOSQE_CQE_SKIP_SUCCESS
diff --git a/tools/testing/selftests/ublk/stripe.c b/tools/testing/selftests/ublk/stripe.c
index d569f62c9b7b5a6b9d82506c4aaab74b77a10305..4c87dc76712f18acd73834c87081a44fb87b7956 100644
--- a/tools/testing/selftests/ublk/stripe.c
+++ b/tools/testing/selftests/ublk/stripe.c
@@ -139,7 +139,7 @@ static int stripe_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_
ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, s->nr + extra);
if (zc) {
- io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
+ io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, io->buf_index);
sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
sqe[0]->user_data = build_user_data(tag,
ublk_cmd_op_nr(sqe[0]->cmd_op), 0, q->q_id, 1);
@@ -166,7 +166,7 @@ static int stripe_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_
if (zc) {
struct io_uring_sqe *unreg = sqe[s->nr + 1];
- io_uring_prep_buf_unregister(unreg, 0, tag, q->q_id, tag);
+ io_uring_prep_buf_unregister(unreg, 0, tag, q->q_id, io->buf_index);
unreg->user_data = build_user_data(
tag, ublk_cmd_op_nr(unreg->cmd_op), 0, q->q_id, 1);
}
diff --git a/tools/testing/selftests/ublk/test_generic_08.sh b/tools/testing/selftests/ublk/test_generic_08.sh
new file mode 100755
index 0000000000000000000000000000000000000000..8dfb11606610b263810f28ce501aedbed0d87a86
--- /dev/null
+++ b/tools/testing/selftests/ublk/test_generic_08.sh
@@ -0,0 +1,61 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+. "$(cd "$(dirname "$0")" && pwd)"/test_common.sh
+
+TID="generic_08"
+ERR_CODE=0
+
+if ! _have_program bpftrace; then
+ exit "$UBLK_SKIP_CODE"
+fi
+
+_prep_test "null" "do imbalanced load, it should be balanced over I/O threads"
+
+NTHREADS=8
+dev_id=$(_add_ublk_dev -t null -q 4 -d 16 --nthreads $NTHREADS --round_robin)
+_check_add_dev $TID $?
+
+dev_t=$(_get_disk_dev_t "$dev_id")
+bpftrace trace/count_ios_per_tid.bt "$dev_t" > "$UBLK_TMP" 2>&1 &
+btrace_pid=$!
+sleep 2
+
+if ! kill -0 "$btrace_pid" > /dev/null 2>&1; then
+ _cleanup_test "null"
+ exit "$UBLK_SKIP_CODE"
+fi
+
+# do imbalanced I/O on the ublk device
+# single-threaded because while tags are assigned round-robin, I/O
+# completions can come in any order, and this can cause imperfect
+# balance (in practice, balance is close to perfect, with less than 0.1%
+# error, but prefer perfection/determinism for automated tests)
+# pin to cpu 0 to prevent migration/only target one queue
+IOS_PER_THREAD=1024
+TOTAL_IOS=$(($IOS_PER_THREAD * $NTHREADS))
+taskset -c 0 dd if=/dev/urandom of=/dev/ublkb"${dev_id}" \
+ oflag=direct bs=4k count=${TOTAL_IOS} > /dev/null 2>&1
+ERR_CODE=$?
+kill "$btrace_pid"
+wait
+
+# check for perfectly balanced I/O
+# note that this depends on few things:
+# - $NTHREADS should divide the queue depth
+# - the queue depth should divide $IOS_PER_THREAD
+# - no I/O on the device should happen while the bpftrace script is
+# running, besides the dd in this script
+# the check below could be made more sophisticated to relax the first
+# two constraints above
+grep '@' < ${UBLK_TMP} | cut -d ' ' -f2 | while read ios; do
+ if [ "${ios}" -ne "${IOS_PER_THREAD}" ]; then
+ echo "imbalanced i/o detected!"
+ cat "$UBLK_TMP"
+ exit 255
+ fi
+done
+ERR_CODE=$?
+
+_cleanup_test "null"
+_show_result $TID $ERR_CODE
diff --git a/tools/testing/selftests/ublk/trace/count_ios_per_tid.bt b/tools/testing/selftests/ublk/trace/count_ios_per_tid.bt
new file mode 100644
index 0000000000000000000000000000000000000000..a62ef428a3dff1db33b128868baa62533d4066aa
--- /dev/null
+++ b/tools/testing/selftests/ublk/trace/count_ios_per_tid.bt
@@ -0,0 +1,9 @@
+/*
+ $1: dev_t
+*/
+tracepoint:block:block_rq_complete
+{
+ if ((int64)args.dev == $1 && !strncmp(args.rwbs, "W", 1)) {
+ @[tid] = count();
+ }
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v6 7/8] selftests: ublk: kublk: decouple ublk_queues from ublk server threads
2025-05-07 21:49 ` [PATCH v6 7/8] selftests: ublk: kublk: decouple ublk_queues from ublk server threads Uday Shankar
@ 2025-05-09 8:31 ` Ming Lei
2025-05-09 8:46 ` Ming Lei
1 sibling, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-05-09 8:31 UTC (permalink / raw)
To: Uday Shankar
Cc: Jens Axboe, Caleb Sander Mateos, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Wed, May 07, 2025 at 03:49:41PM -0600, Uday Shankar wrote:
> Add support in kublk for decoupled ublk_queues and ublk server threads.
> kublk now has two modes of operation:
>
> - (preexisting mode) threads and queues are paired 1:1, and each thread
> services all the I/Os of one queue
> - (new mode) thread and queue counts are independently configurable.
> threads service I/Os in a way that balances load across threads even
> if load is not balanced over queues. requires passing --round_robin
>
> The new mode of operation is exercised by the new test_generic_08, which
> issues I/O against a single queue and verifies that each of the 8 ublk
> server threads handles exactly 1/8 of the total I/O count. Under the old
> mode of operation (i.e. without --round_robin), all I/O goes to one ublk
> server thread, and the test fails.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
> tools/testing/selftests/ublk/Makefile | 1 +
> tools/testing/selftests/ublk/file_backed.c | 4 +-
> tools/testing/selftests/ublk/kublk.c | 96 ++++++++++++++++++----
> tools/testing/selftests/ublk/kublk.h | 3 +
> tools/testing/selftests/ublk/null.c | 4 +-
> tools/testing/selftests/ublk/stripe.c | 4 +-
> tools/testing/selftests/ublk/test_generic_08.sh | 61 ++++++++++++++
> .../selftests/ublk/trace/count_ios_per_tid.bt | 9 ++
> 8 files changed, 160 insertions(+), 22 deletions(-)
>
> diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
> index e2e7b1e52a06252f626df5606221d17e3106d0d3..f8579019f0d1f9185df098440611c3b75087073b 100644
> --- a/tools/testing/selftests/ublk/Makefile
> +++ b/tools/testing/selftests/ublk/Makefile
> @@ -14,6 +14,7 @@ TEST_PROGS += test_generic_04.sh
> TEST_PROGS += test_generic_05.sh
> TEST_PROGS += test_generic_06.sh
> TEST_PROGS += test_generic_07.sh
> +TEST_PROGS += test_generic_08.sh
>
> TEST_PROGS += test_null_01.sh
> TEST_PROGS += test_null_02.sh
> diff --git a/tools/testing/selftests/ublk/file_backed.c b/tools/testing/selftests/ublk/file_backed.c
> index 563f11a21604bbf5b9531f69f806d09cdd785960..72e22f54f7fe60d26096ace4eb4001987fbe7b15 100644
> --- a/tools/testing/selftests/ublk/file_backed.c
> +++ b/tools/testing/selftests/ublk/file_backed.c
> @@ -50,7 +50,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
>
> ublk_io_alloc_sqes(ublk_get_io(q, tag), sqe, 3);
>
> - io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, tag);
> + io_uring_prep_buf_register(sqe[0], 0, tag, q->q_id, ublk_get_io(q, tag)->buf_index);
->buf_index can be calculated runtime by adding helper of ublk_io_buf_idx(io), so
the extra field can be avoided, CPU is fast than memory.
> sqe[0]->flags |= IOSQE_CQE_SKIP_SUCCESS | IOSQE_IO_HARDLINK;
> sqe[0]->user_data = build_user_data(tag,
> ublk_cmd_op_nr(sqe[0]->cmd_op), 0, q->q_id, 1);
> @@ -62,7 +62,7 @@ static int loop_queue_tgt_rw_io(struct ublk_queue *q, const struct ublksrv_io_de
> sqe[1]->flags |= IOSQE_FIXED_FILE | IOSQE_IO_HARDLINK;
> sqe[1]->user_data = build_user_data(tag, ublk_op, 0, q->q_id, 1);
>
> - io_uring_prep_buf_unregister(sqe[2], 0, tag, q->q_id, tag);
> + io_uring_prep_buf_unregister(sqe[2], 0, tag, q->q_id, ublk_get_io(q, tag)->buf_index);
> sqe[2]->user_data = build_user_data(tag, ublk_cmd_op_nr(sqe[2]->cmd_op), 0, q->q_id, 1);
>
> return 2;
> diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
> index 313689f94cd6361a9a0f4b9257085b2a62bc8b8c..27046bb6a13b99879ad164ff8eaabeba57e17387 100644
> --- a/tools/testing/selftests/ublk/kublk.c
> +++ b/tools/testing/selftests/ublk/kublk.c
> @@ -478,8 +478,11 @@ static int ublk_thread_init(struct ublk_thread *t)
> }
>
> if (dev->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY) {
> + unsigned nr_ios = dev->dev_info.queue_depth * dev->dev_info.nr_hw_queues;
> + unsigned max_nr_ios_per_thread = nr_ios / dev->nthreads;
> + max_nr_ios_per_thread += !!(nr_ios % dev->nthreads);
> ret = io_uring_register_buffers_sparse(
> - &t->ring, dev->dev_info.queue_depth);
> + &t->ring, max_nr_ios_per_thread);
> if (ret) {
> ublk_err("ublk dev %d thread %d register spare buffers failed %d",
> dev->dev_info.dev_id, t->idx, ret);
> @@ -612,18 +615,42 @@ int ublk_queue_io_cmd(struct ublk_io *io)
>
> static void ublk_submit_fetch_commands(struct ublk_thread *t)
> {
> - /*
> - * Service exclusively the queue whose q_id matches our thread
> - * index. This may change in the future.
> - */
> - struct ublk_queue *q = &t->dev->q[t->idx];
> + struct ublk_queue *q;
> struct ublk_io *io;
> - int i = 0;
> + int i = 0, j = 0;
>
> - for (i = 0; i < q->q_depth; i++) {
> - io = &q->ios[i];
> - io->t = t;
> - ublk_queue_io_cmd(io);
> + if (t->dev->dev_info.flags & UBLK_F_RR_TAGS) {
You shouldn't depend on the generic feature of UBLK_F_RR_TAGS, which can be
used for non-io-task too.
Here you need one helper of ublk_use_io_task(), or sort of flags.
> + /*
> + * Lexicographically order all the (qid,tag) pairs, with
> + * qid taking priority, and give this thread every Nth
> + * entry, where N is the total number of threads. The
> + * offset is controlled by the thread index. This takes
> + * load which may be imbalanced across the queues and
> + * balances it across the threads.
> + */
> + const struct ublksrv_ctrl_dev_info *dinfo = &t->dev->dev_info;
> + int nr_ios = dinfo->nr_hw_queues * dinfo->queue_depth;
> + for (i = t->idx; i < nr_ios; i += t->dev->nthreads, j++) {
> + int q_id = i / dinfo->queue_depth;
> + int tag = i % dinfo->queue_depth;
I understand UBLK_F_RR_TAGS means that IO with adjacent tag should be
handled locally, but the above actually does the opposite, only IOs with
non-adjacent tags are handled in same pthread, can you explain a bit
why UBLK_F_RR_TAGS helps for this way?
> + q = &t->dev->q[q_id];
> + io = &q->ios[tag];
> + io->t = t;
> + io->buf_index = j;
> + ublk_queue_io_cmd(io);
> + }
> + } else {
> + /*
> + * Service exclusively the queue whose q_id matches our
> + * thread index.
> + */
> + struct ublk_queue *q = &t->dev->q[t->idx];
> + for (i = 0; i < q->q_depth; i++) {
> + io = &q->ios[i];
> + io->t = t;
> + io->buf_index = i;
> + ublk_queue_io_cmd(io);
> + }
> }
> }
>
> @@ -778,7 +805,8 @@ static void *ublk_io_handler_fn(void *data)
> return NULL;
> }
> /* IO perf is sensitive with queue pthread affinity on NUMA machine*/
> - ublk_thread_set_sched_affinity(t, info->affinity);
> + if (info->affinity)
> + ublk_thread_set_sched_affinity(t, info->affinity);
> sem_post(info->ready);
>
> ublk_dbg(UBLK_DBG_THREAD, "tid %d: ublk dev %d thread %u started\n",
> @@ -844,7 +872,7 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
>
> ublk_dbg(UBLK_DBG_DEV, "%s enter\n", __func__);
>
> - tinfo = calloc(sizeof(struct ublk_thread_info), dinfo->nr_hw_queues);
> + tinfo = calloc(sizeof(struct ublk_thread_info), dev->nthreads);
> if (!tinfo)
> return -ENOMEM;
>
> @@ -867,17 +895,24 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
> dinfo->dev_id, i);
> goto fail;
> }
> + }
>
> + for (i = 0; i < dev->nthreads; i++) {
> tinfo[i].dev = dev;
> tinfo[i].idx = i;
> tinfo[i].ready = &ready;
> - tinfo[i].affinity = &affinity_buf[i];
> + /*
> + * If threads are not tied to queues, setting thread
> + * affinity based on queue affinity makes no sense.
> + */
> + if (!(dinfo->flags & UBLK_F_RR_TAGS))
> + tinfo[i].affinity = &affinity_buf[i];
> pthread_create(&dev->threads[i].thread, NULL,
> ublk_io_handler_fn,
> &tinfo[i]);
> }
>
> - for (i = 0; i < dinfo->nr_hw_queues; i++)
> + for (i = 0; i < dev->nthreads; i++)
> sem_wait(&ready);
> free(tinfo);
> free(affinity_buf);
> @@ -901,7 +936,7 @@ static int ublk_start_daemon(const struct dev_ctx *ctx, struct ublk_dev *dev)
> ublk_send_dev_event(ctx, dev, dev->dev_info.dev_id);
>
> /* wait until we are terminated */
> - for (i = 0; i < dinfo->nr_hw_queues; i++)
> + for (i = 0; i < dev->nthreads; i++)
> pthread_join(dev->threads[i].thread, &thread_ret);
> fail:
> for (i = 0; i < dinfo->nr_hw_queues; i++)
> @@ -1011,6 +1046,7 @@ static int ublk_stop_io_daemon(const struct ublk_dev *dev)
>
> static int __cmd_dev_add(const struct dev_ctx *ctx)
> {
> + unsigned nthreads = ctx->nthreads;
> unsigned nr_queues = ctx->nr_hw_queues;
> const char *tgt_type = ctx->tgt_type;
> unsigned depth = ctx->queue_depth;
> @@ -1034,6 +1070,23 @@ static int __cmd_dev_add(const struct dev_ctx *ctx)
> return -EINVAL;
> }
>
> + /* default to 1:1 threads:queues if nthreads is unspecified */
> + if (nthreads == -1)
> + nthreads = nr_queues;
Maybe we can start 1:1 for nrthreads == 0, which looks more readable,
and you needn't to set -1 default.
> +
> + if (nthreads > UBLK_MAX_THREADS) {
> + ublk_err("%s: %u is too many threads (max %u)\n",
> + __func__, nthreads, UBLK_MAX_THREADS);
> + return -EINVAL;
> + }
> +
> + if (nthreads != nr_queues && !(ctx->flags & UBLK_F_RR_TAGS)) {
> + ublk_err("%s: threads %u must be same as queues %u if "
> + "not using round robin\n",
> + __func__, nthreads, nr_queues);
> + return -EINVAL;
> + }
> +
> dev = ublk_ctrl_init();
> if (!dev) {
> ublk_err("%s: can't alloc dev id %d, type %s\n",
> @@ -1054,6 +1107,7 @@ static int __cmd_dev_add(const struct dev_ctx *ctx)
> info->nr_hw_queues = nr_queues;
> info->queue_depth = depth;
> info->flags = ctx->flags;
> + dev->nthreads = nthreads;
> dev->tgt.ops = ops;
> dev->tgt.sq_depth = depth;
> dev->tgt.cq_depth = depth;
> @@ -1249,6 +1303,7 @@ static int cmd_dev_get_features(void)
> [const_ilog2(UBLK_F_USER_COPY)] = "USER_COPY",
> [const_ilog2(UBLK_F_ZONED)] = "ZONED",
> [const_ilog2(UBLK_F_USER_RECOVERY_FAIL_IO)] = "RECOVERY_FAIL_IO",
> + [const_ilog2(UBLK_F_RR_TAGS)] = "RR_TAGS",
> };
> struct ublk_dev *dev;
> __u64 features = 0;
> @@ -1290,8 +1345,10 @@ static void __cmd_create_help(char *exe, bool recovery)
> exe, recovery ? "recover" : "add");
> printf("\t[--foreground] [--quiet] [-z] [--debug_mask mask] [-r 0|1 ] [-g]\n");
> printf("\t[-e 0|1 ] [-i 0|1]\n");
> + printf("\t[--nthreads threads] [--round_robin]\n");
> printf("\t[target options] [backfile1] [backfile2] ...\n");
> printf("\tdefault: nr_queues=2(max 32), depth=128(max 1024), dev_id=-1(auto allocation)\n");
> + printf("\tdefault: nthreads=nr_queues");
>
> for (i = 0; i < sizeof(tgt_ops_list) / sizeof(tgt_ops_list[0]); i++) {
> const struct ublk_tgt_ops *ops = tgt_ops_list[i];
> @@ -1343,6 +1400,8 @@ int main(int argc, char *argv[])
> { "recovery_fail_io", 1, NULL, 'e'},
> { "recovery_reissue", 1, NULL, 'i'},
> { "get_data", 1, NULL, 'g'},
> + { "nthreads", 1, NULL, 0 },
> + { "round_robin", 0, NULL, 0 },
> { 0, 0, 0, 0 }
> };
> const struct ublk_tgt_ops *ops = NULL;
> @@ -1351,6 +1410,7 @@ int main(int argc, char *argv[])
> struct dev_ctx ctx = {
> .queue_depth = 128,
> .nr_hw_queues = 2,
> + .nthreads = -1,
> .dev_id = -1,
> .tgt_type = "unknown",
> };
> @@ -1411,6 +1471,10 @@ int main(int argc, char *argv[])
> ublk_dbg_mask = 0;
> if (!strcmp(longopts[option_idx].name, "foreground"))
> ctx.fg = 1;
> + if (!strcmp(longopts[option_idx].name, "nthreads"))
> + ctx.nthreads = strtol(optarg, NULL, 10);
> + if (!strcmp(longopts[option_idx].name, "round_robin"))
> + ctx.flags |= UBLK_F_RR_TAGS;
maybe `--io_task` is more readable, and you can always enable UBLK_F_RR_TAGS
for this '--io_task' ublk server feature.
Thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v6 7/8] selftests: ublk: kublk: decouple ublk_queues from ublk server threads
2025-05-07 21:49 ` [PATCH v6 7/8] selftests: ublk: kublk: decouple ublk_queues from ublk server threads Uday Shankar
2025-05-09 8:31 ` Ming Lei
@ 2025-05-09 8:46 ` Ming Lei
1 sibling, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-05-09 8:46 UTC (permalink / raw)
To: Uday Shankar
Cc: Jens Axboe, Caleb Sander Mateos, Andrew Morton, Shuah Khan,
Jonathan Corbet, linux-block, linux-kernel, linux-kselftest,
linux-doc
On Wed, May 07, 2025 at 03:49:41PM -0600, Uday Shankar wrote:
> Add support in kublk for decoupled ublk_queues and ublk server threads.
> kublk now has two modes of operation:
>
> - (preexisting mode) threads and queues are paired 1:1, and each thread
> services all the I/Os of one queue
> - (new mode) thread and queue counts are independently configurable.
> threads service I/Os in a way that balances load across threads even
> if load is not balanced over queues. requires passing --round_robin
>
> The new mode of operation is exercised by the new test_generic_08, which
> issues I/O against a single queue and verifies that each of the 8 ublk
> server threads handles exactly 1/8 of the total I/O count. Under the old
> mode of operation (i.e. without --round_robin), all I/O goes to one ublk
> server thread, and the test fails.
Hi Uday,
I just setup two ublk devices in the following way:
[root@ktest-40 ublk]# ./kublk add -t null -q 2 --nthreads=4 --round_robin
dev id 0: nr_hw_queues 2 queue_depth 128 block size 512 dev_capacity 524288000
max rq size 1048576 daemon pid 8516 flags 0x842 state LIVE
queue 0: affinity(0 )
queue 1: affinity(8 )
[root@ktest-40 ublk]# ./kublk add -t null -q 2
dev id 1: nr_hw_queues 2 queue_depth 128 block size 512 dev_capacity 524288000
max rq size 1048576 daemon pid 8539 flags 0x42 state LIVE
queue 0: affinity(0 )
queue 1: affinity(8 )
Then run 'fio/t/io_uring -p0 /dev/ublkb[0|1]', and IOPS of /dev/ublkb0 is
just 1/3 of /dev/ublkb1.
For ublk/loop, '-q 2 --nthreads=4 --round_robin' is still a bit slower
than '-q 2', but the gap isn't too big(<10%).
Thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6 8/8] Documentation: ublk: document UBLK_F_RR_TAGS
2025-05-07 21:49 [PATCH v6 0/8] ublk: decouple server threads from hctxs Uday Shankar
` (6 preceding siblings ...)
2025-05-07 21:49 ` [PATCH v6 7/8] selftests: ublk: kublk: decouple ublk_queues from ublk server threads Uday Shankar
@ 2025-05-07 21:49 ` Uday Shankar
2025-05-07 23:08 ` Randy Dunlap
2025-05-08 2:08 ` Bagas Sanjaya
7 siblings, 2 replies; 22+ messages in thread
From: Uday Shankar @ 2025-05-07 21:49 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Caleb Sander Mateos, Andrew Morton,
Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc,
Uday Shankar
Document the new flag UBLK_F_RR_TAGS along with its intended use case.
Also describe the new restrictions on threading model imposed by
ublk_drv (one (qid,tag) pair is can be served by only one thread), and
remove references to ubq_daemon/per-queue threads, since such a concept
no longer exists.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
Documentation/block/ublk.rst | 83 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 72 insertions(+), 11 deletions(-)
diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
index 854f823b46c2add01d0b65ba36aecd26c45bb65d..e9cbabdd69c5539a02463780ba5e51de0416c3f6 100644
--- a/Documentation/block/ublk.rst
+++ b/Documentation/block/ublk.rst
@@ -115,15 +115,15 @@ managing and controlling ublk devices with help of several control commands:
- ``UBLK_CMD_START_DEV``
- After the server prepares userspace resources (such as creating per-queue
- pthread & io_uring for handling ublk IO), this command is sent to the
+ After the server prepares userspace resources (such as creating I/O handler
+ threads & io_uring for handling ublk IO), this command is sent to the
driver for allocating & exposing ``/dev/ublkb*``. Parameters set via
``UBLK_CMD_SET_PARAMS`` are applied for creating the device.
- ``UBLK_CMD_STOP_DEV``
Halt IO on ``/dev/ublkb*`` and remove the device. When this command returns,
- ublk server will release resources (such as destroying per-queue pthread &
+ ublk server will release resources (such as destroying I/O handler threads &
io_uring).
- ``UBLK_CMD_DEL_DEV``
@@ -208,15 +208,15 @@ managing and controlling ublk devices with help of several control commands:
modify how I/O is handled while the ublk server is dying/dead (this is called
the ``nosrv`` case in the driver code).
- With just ``UBLK_F_USER_RECOVERY`` set, after one ubq_daemon(ublk server's io
- handler) is dying, ublk does not delete ``/dev/ublkb*`` during the whole
+ With just ``UBLK_F_USER_RECOVERY`` set, after the ublk server exits,
+ ublk does not delete ``/dev/ublkb*`` during the whole
recovery stage and ublk device ID is kept. It is ublk server's
responsibility to recover the device context by its own knowledge.
Requests which have not been issued to userspace are requeued. Requests
which have been issued to userspace are aborted.
- With ``UBLK_F_USER_RECOVERY_REISSUE`` additionally set, after one ubq_daemon
- (ublk server's io handler) is dying, contrary to ``UBLK_F_USER_RECOVERY``,
+ With ``UBLK_F_USER_RECOVERY_REISSUE`` additionally set, after the ublk server
+ exits, contrary to ``UBLK_F_USER_RECOVERY``,
requests which have been issued to userspace are requeued and will be
re-issued to the new process after handling ``UBLK_CMD_END_USER_RECOVERY``.
``UBLK_F_USER_RECOVERY_REISSUE`` is designed for backends who tolerate
@@ -241,10 +241,11 @@ can be controlled/accessed just inside this container.
Data plane
----------
-ublk server needs to create per-queue IO pthread & io_uring for handling IO
-commands via io_uring passthrough. The per-queue IO pthread
-focuses on IO handling and shouldn't handle any control & management
-tasks.
+The ublk server should create dedicated threads for handling I/O. Each
+thread should have its own io_uring through which it is notified of new
+I/O, and through which it can complete I/O. These dedicated threads
+should focus on IO handling and shouldn't handle any control &
+management tasks.
The's IO is assigned by a unique tag, which is 1:1 mapping with IO
request of ``/dev/ublkb*``.
@@ -265,6 +266,13 @@ with specified IO tag in the command data:
destined to ``/dev/ublkb*``. This command is sent only once from the server
IO pthread for ublk driver to setup IO forward environment.
+ Once a thread issues this command against a given (qid,tag) pair, the thread
+ registers itself as that I/O's daemon. In the future, only that I/O's daemon
+ is allowed to issue commands against the I/O. If any other thread attempts
+ to issue a command against a (qid,tag) pair for which the thread is not the
+ daemon, the command will fail. Daemons can be reset only be going through
+ recovery.
+
- ``UBLK_IO_COMMIT_AND_FETCH_REQ``
When an IO request is destined to ``/dev/ublkb*``, the driver stores
@@ -309,6 +317,59 @@ with specified IO tag in the command data:
``UBLK_IO_COMMIT_AND_FETCH_REQ`` to the server, ublkdrv needs to copy
the server buffer (pages) read to the IO request pages.
+Load balancing
+--------------
+
+A simple approach to designing a ublk server might involve selecting a
+number of I/O handler threads N, creating devices with N queues, and
+pairing up I/O handler threads with queues, so that each thread gets a
+unique qid, and it issues ``FETCH_REQ``s against all tags for that qid.
+Indeed, before the introduction of the ``UBLK_F_RR_TAGS`` feature, this
+was essentially the only option (*)
+
+This approach can run into performance issues under imbalanced load.
+This architecture taken together with the `blk-mq architecture
+<https://docs.kernel.org/block/blk-mq.html>`_ implies that there is a
+fixed mapping from I/O submission CPU to the ublk server thread that
+handles it. If the workload is CPU-bottlenecked, only allowing one ublk
+server thread to handle all the I/O generated from a single CPU can
+limit peak bandwidth.
+
+To address this issue, two changes were made:
+
+- ublk servers can now pair up threads with I/Os (i.e. (qid,tag) pairs)
+ arbitrarily. In particular, the preexisting restriction that all I/Os
+ in one queue must be served by the same thread is lifted.
+- ublk servers can now specify ``UBLK_F_RR_TAGS`` when creating a ublk
+ device to get round-robin tag allocation on each queue
+
+The ublk server can check for the presence of these changes by testing
+for the ``UBLK_F_RR_TAGS`` feature.
+
+With these changes, a ublk server can balance load as follows:
+
+- create the device with ``UBLK_F_RR_TAGS`` set in
+ ``ublksrv_ctrl_dev_info::flags`` when issuing the ``ADD_DEV`` command
+- issue ``FETCH_REQ``s from ublk server threads to (qid,tag) pairs in
+ a round-robin manner. For example, for a device configured with
+ ``nr_hw_queues=2`` and ``queue_depth=4``, and a ublk server having 4
+ I/O handling threads, ``FETCH_REQ``s could be issued as follows, where
+ each entry in the table is the pair (``ublksrv_io_cmd::q_id``,
+ ``ublksrv_io_cmd::tag``) in the payload of the ``FETCH_REQ``.
+
+ ======== ======== ======== ========
+ thread 0 thread 1 thread 2 thread 3
+ ======== ======== ======== ========
+ (0, 0) (0, 1) (0, 2) (0, 3)
+ (1, 3) (1, 0) (1, 1) (1, 2)
+
+With this setup, I/O submitted on a CPU which maps to queue 0 will be
+balanced across all threads instead of all landing on the same thread.
+Thus, a potential bottleneck is avoided.
+
+(*) technically, one I/O handling thread could service multiple queues
+if it wanted to, but that doesn't help with imbalanced load
+
Zero copy
---------
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v6 8/8] Documentation: ublk: document UBLK_F_RR_TAGS
2025-05-07 21:49 ` [PATCH v6 8/8] Documentation: ublk: document UBLK_F_RR_TAGS Uday Shankar
@ 2025-05-07 23:08 ` Randy Dunlap
2025-05-08 2:08 ` Bagas Sanjaya
1 sibling, 0 replies; 22+ messages in thread
From: Randy Dunlap @ 2025-05-07 23:08 UTC (permalink / raw)
To: Uday Shankar, Ming Lei, Jens Axboe, Caleb Sander Mateos,
Andrew Morton, Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc
Hi,
On 5/7/25 2:49 PM, Uday Shankar wrote:
> Document the new flag UBLK_F_RR_TAGS along with its intended use case.
> Also describe the new restrictions on threading model imposed by
> ublk_drv (one (qid,tag) pair is can be served by only one thread), and
> remove references to ubq_daemon/per-queue threads, since such a concept
> no longer exists.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
> Documentation/block/ublk.rst | 83 ++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 72 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
> index 854f823b46c2add01d0b65ba36aecd26c45bb65d..e9cbabdd69c5539a02463780ba5e51de0416c3f6 100644
> --- a/Documentation/block/ublk.rst
> +++ b/Documentation/block/ublk.rst
> @@ -115,15 +115,15 @@ managing and controlling ublk devices with help of several control commands:
>
> - ``UBLK_CMD_START_DEV``
>
> - After the server prepares userspace resources (such as creating per-queue
> - pthread & io_uring for handling ublk IO), this command is sent to the
> + After the server prepares userspace resources (such as creating I/O handler
> + threads & io_uring for handling ublk IO), this command is sent to the
> driver for allocating & exposing ``/dev/ublkb*``. Parameters set via
> ``UBLK_CMD_SET_PARAMS`` are applied for creating the device.
>
> - ``UBLK_CMD_STOP_DEV``
>
> Halt IO on ``/dev/ublkb*`` and remove the device. When this command returns,
> - ublk server will release resources (such as destroying per-queue pthread &
> + ublk server will release resources (such as destroying I/O handler threads &
> io_uring).
>
> - ``UBLK_CMD_DEL_DEV``
> @@ -208,15 +208,15 @@ managing and controlling ublk devices with help of several control commands:
> modify how I/O is handled while the ublk server is dying/dead (this is called
> the ``nosrv`` case in the driver code).
>
> - With just ``UBLK_F_USER_RECOVERY`` set, after one ubq_daemon(ublk server's io
> - handler) is dying, ublk does not delete ``/dev/ublkb*`` during the whole
> + With just ``UBLK_F_USER_RECOVERY`` set, after the ublk server exits,
> + ublk does not delete ``/dev/ublkb*`` during the whole
> recovery stage and ublk device ID is kept. It is ublk server's
> responsibility to recover the device context by its own knowledge.
> Requests which have not been issued to userspace are requeued. Requests
> which have been issued to userspace are aborted.
>
> - With ``UBLK_F_USER_RECOVERY_REISSUE`` additionally set, after one ubq_daemon
> - (ublk server's io handler) is dying, contrary to ``UBLK_F_USER_RECOVERY``,
> + With ``UBLK_F_USER_RECOVERY_REISSUE`` additionally set, after the ublk server
> + exits, contrary to ``UBLK_F_USER_RECOVERY``,
> requests which have been issued to userspace are requeued and will be
> re-issued to the new process after handling ``UBLK_CMD_END_USER_RECOVERY``.
> ``UBLK_F_USER_RECOVERY_REISSUE`` is designed for backends who tolerate
> @@ -241,10 +241,11 @@ can be controlled/accessed just inside this container.
> Data plane
> ----------
>
> -ublk server needs to create per-queue IO pthread & io_uring for handling IO
> -commands via io_uring passthrough. The per-queue IO pthread
> -focuses on IO handling and shouldn't handle any control & management
> -tasks.
> +The ublk server should create dedicated threads for handling I/O. Each
> +thread should have its own io_uring through which it is notified of new
> +I/O, and through which it can complete I/O. These dedicated threads
> +should focus on IO handling and shouldn't handle any control &
> +management tasks.
>
> The's IO is assigned by a unique tag, which is 1:1 mapping with IO
???
> request of ``/dev/ublkb*``.
> @@ -265,6 +266,13 @@ with specified IO tag in the command data:
> destined to ``/dev/ublkb*``. This command is sent only once from the server
> IO pthread for ublk driver to setup IO forward environment.
>
> + Once a thread issues this command against a given (qid,tag) pair, the thread
> + registers itself as that I/O's daemon. In the future, only that I/O's daemon
> + is allowed to issue commands against the I/O. If any other thread attempts
> + to issue a command against a (qid,tag) pair for which the thread is not the
> + daemon, the command will fail. Daemons can be reset only be going through
> + recovery.
> +
> - ``UBLK_IO_COMMIT_AND_FETCH_REQ``
>
> When an IO request is destined to ``/dev/ublkb*``, the driver stores
> @@ -309,6 +317,59 @@ with specified IO tag in the command data:
> ``UBLK_IO_COMMIT_AND_FETCH_REQ`` to the server, ublkdrv needs to copy
> the server buffer (pages) read to the IO request pages.
>
> +Load balancing
> +--------------
> +
> +A simple approach to designing a ublk server might involve selecting a
> +number of I/O handler threads N, creating devices with N queues, and
> +pairing up I/O handler threads with queues, so that each thread gets a
> +unique qid, and it issues ``FETCH_REQ``s against all tags for that qid.
> +Indeed, before the introduction of the ``UBLK_F_RR_TAGS`` feature, this
> +was essentially the only option (*)
Add ending period (full stop), please.
> +
> +This approach can run into performance issues under imbalanced load.
> +This architecture taken together with the `blk-mq architecture
> +<https://docs.kernel.org/block/blk-mq.html>`_ implies that there is a
> +fixed mapping from I/O submission CPU to the ublk server thread that
> +handles it. If the workload is CPU-bottlenecked, only allowing one ublk
> +server thread to handle all the I/O generated from a single CPU can
> +limit peak bandwidth.
> +
> +To address this issue, two changes were made:
> +
> +- ublk servers can now pair up threads with I/Os (i.e. (qid,tag) pairs)
> + arbitrarily. In particular, the preexisting restriction that all I/Os
> + in one queue must be served by the same thread is lifted.
> +- ublk servers can now specify ``UBLK_F_RR_TAGS`` when creating a ublk
> + device to get round-robin tag allocation on each queue
Add ending period (full stop), please.
> +
> +The ublk server can check for the presence of these changes by testing
> +for the ``UBLK_F_RR_TAGS`` feature.
> +
> +With these changes, a ublk server can balance load as follows:
> +
> +- create the device with ``UBLK_F_RR_TAGS`` set in
> + ``ublksrv_ctrl_dev_info::flags`` when issuing the ``ADD_DEV`` command
> +- issue ``FETCH_REQ``s from ublk server threads to (qid,tag) pairs in
> + a round-robin manner. For example, for a device configured with
> + ``nr_hw_queues=2`` and ``queue_depth=4``, and a ublk server having 4
> + I/O handling threads, ``FETCH_REQ``s could be issued as follows, where
> + each entry in the table is the pair (``ublksrv_io_cmd::q_id``,
> + ``ublksrv_io_cmd::tag``) in the payload of the ``FETCH_REQ``.
> +
> + ======== ======== ======== ========
> + thread 0 thread 1 thread 2 thread 3
> + ======== ======== ======== ========
> + (0, 0) (0, 1) (0, 2) (0, 3)
> + (1, 3) (1, 0) (1, 1) (1, 2)
> +
> +With this setup, I/O submitted on a CPU which maps to queue 0 will be
> +balanced across all threads instead of all landing on the same thread.
> +Thus, a potential bottleneck is avoided.
> +
> +(*) technically, one I/O handling thread could service multiple queues
Technically,
> +if it wanted to, but that doesn't help with imbalanced load
Add ending period (full stop), please.
> +
> Zero copy
> ---------
>
>
--
~Randy
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v6 8/8] Documentation: ublk: document UBLK_F_RR_TAGS
2025-05-07 21:49 ` [PATCH v6 8/8] Documentation: ublk: document UBLK_F_RR_TAGS Uday Shankar
2025-05-07 23:08 ` Randy Dunlap
@ 2025-05-08 2:08 ` Bagas Sanjaya
1 sibling, 0 replies; 22+ messages in thread
From: Bagas Sanjaya @ 2025-05-08 2:08 UTC (permalink / raw)
To: Uday Shankar, Ming Lei, Jens Axboe, Caleb Sander Mateos,
Andrew Morton, Shuah Khan, Jonathan Corbet
Cc: linux-block, linux-kernel, linux-kselftest, linux-doc
[-- Attachment #1: Type: text/plain, Size: 4079 bytes --]
On Wed, May 07, 2025 at 03:49:42PM -0600, Uday Shankar wrote:
> +Load balancing
> +--------------
> +
> +A simple approach to designing a ublk server might involve selecting a
> +number of I/O handler threads N, creating devices with N queues, and
> +pairing up I/O handler threads with queues, so that each thread gets a
> +unique qid, and it issues ``FETCH_REQ``s against all tags for that qid.
``FETCH_REQ``\s (escape s)
> +Indeed, before the introduction of the ``UBLK_F_RR_TAGS`` feature, this
> +was essentially the only option (*)
Use reST footnotes syntax, i.e.:
---- >8 ----
diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
index 440b63be4ea8b6..b1d29fceff4e80 100644
--- a/Documentation/block/ublk.rst
+++ b/Documentation/block/ublk.rst
@@ -325,7 +325,7 @@ number of I/O handler threads N, creating devices with N queues, and
pairing up I/O handler threads with queues, so that each thread gets a
unique qid, and it issues ``FETCH_REQ``\s against all tags for that qid.
Indeed, before the introduction of the ``UBLK_F_RR_TAGS`` feature, this
-was essentially the only option (*)
+was essentially the only option [#]_
This approach can run into performance issues under imbalanced load.
This architecture taken together with the `blk-mq architecture
@@ -368,8 +368,8 @@ With this setup, I/O submitted on a CPU which maps to queue 0 will be
balanced across all threads instead of all landing on the same thread.
Thus, a potential bottleneck is avoided.
-(*) technically, one I/O handling thread could service multiple queues
-if it wanted to, but that doesn't help with imbalanced load
+.. [#] Technically, one I/O handling thread could service multiple queues
+ if it wanted to, but that doesn't help with imbalanced load
Zero copy
---------
> +
> +This approach can run into performance issues under imbalanced load.
> +This architecture taken together with the `blk-mq architecture
> +<https://docs.kernel.org/block/blk-mq.html>`_ implies that there is a
This architecture, taken together with the
:doc:`blk-mq architecture </block/blk-mq>`, implies that ...
> +fixed mapping from I/O submission CPU to the ublk server thread that
> +handles it. If the workload is CPU-bottlenecked, only allowing one ublk
> +server thread to handle all the I/O generated from a single CPU can
> +limit peak bandwidth.
> +
> <snipped>...
> +With these changes, a ublk server can balance load as follows:
> +
> +- create the device with ``UBLK_F_RR_TAGS`` set in
> + ``ublksrv_ctrl_dev_info::flags`` when issuing the ``ADD_DEV`` command
> +- issue ``FETCH_REQ``s from ublk server threads to (qid,tag) pairs in
> + a round-robin manner. For example, for a device configured with
> + ``nr_hw_queues=2`` and ``queue_depth=4``, and a ublk server having 4
> + I/O handling threads, ``FETCH_REQ``s could be issued as follows, where
> + each entry in the table is the pair (``ublksrv_io_cmd::q_id``,
> + ``ublksrv_io_cmd::tag``) in the payload of the ``FETCH_REQ``.
s/``FETCH_REQ``/``FETCH_REQ``\s/ (escape s after FETCH_REQ).
> +
> + ======== ======== ======== ========
> + thread 0 thread 1 thread 2 thread 3
> + ======== ======== ======== ========
> + (0, 0) (0, 1) (0, 2) (0, 3)
> + (1, 3) (1, 0) (1, 1) (1, 2)
Add table border in the bottom, i.e.:
---- >8 ----
diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
index e9cbabdd69c553..dc6fdfedba9ab4 100644
--- a/Documentation/block/ublk.rst
+++ b/Documentation/block/ublk.rst
@@ -362,6 +362,7 @@ With these changes, a ublk server can balance load as follows:
======== ======== ======== ========
(0, 0) (0, 1) (0, 2) (0, 3)
(1, 3) (1, 0) (1, 1) (1, 2)
+ ======== ======== ======== ========
With this setup, I/O submitted on a CPU which maps to queue 0 will be
balanced across all threads instead of all landing on the same thread.
Thanks.
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply related [flat|nested] 22+ messages in thread