linux-kernel - Re: [PATCH v6 4/8] selftests: ublk: kublk: tie sqe allocation to io instead of queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aB2xd4593ZkWuMbB@fedora>
Date: Fri, 9 May 2025 15:40:39 +0800
From: Ming Lei <ming.lei@...hat.com>
To: Uday Shankar <ushankar@...estorage.com>
Cc: Jens Axboe <axboe@...nel.dk>,
	Caleb Sander Mateos <csander@...estorage.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Shuah Khan <shuah@...nel.org>, Jonathan Corbet <corbet@....net>,
	linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v6 4/8] selftests: ublk: kublk: tie sqe allocation to io
 instead of queue

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@...estorage.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