Re: [PATCH] io_uring: save repeated issue_flags

[Pavel Begunkov <asml.silence@gmail.com>]

On 12/5/23 22:00, Jens Axboe wrote:
> On 12/5/23 2:55 PM, Keith Busch wrote:
>> From: Keith Busch <kbusch@kernel.org>
>>
>> No need to rebuild the issue_flags on every IO: they're always the same.
>>
>> Suggested-by: Jens Axboe <axboe@kernel.dk>
>> Signed-off-by: Keith Busch <kbusch@kernel.org>
>> ---
[...]
>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>> index 8a38b9f75d841..dbc0bfbfd0f05 100644
>> --- a/io_uring/uring_cmd.c
>> +++ b/io_uring/uring_cmd.c
>> @@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>   	if (ret)
>>   		return ret;
>>   
>> -	if (ctx->flags & IORING_SETUP_SQE128)
>> -		issue_flags |= IO_URING_F_SQE128;
>> -	if (ctx->flags & IORING_SETUP_CQE32)
>> -		issue_flags |= IO_URING_F_CQE32;
>> -	if (ctx->compat)
>> -		issue_flags |= IO_URING_F_COMPAT;
>>   	if (ctx->flags & IORING_SETUP_IOPOLL) {
>>   		if (!file->f_op->uring_cmd_iopoll)
>>   			return -EOPNOTSUPP;
>> -		issue_flags |= IO_URING_F_IOPOLL;
>>   		req->iopoll_completed = 0;
>>   	}
>>   
>> +	issue_flags |= ctx->issue_flags;
>>   	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>>   	if (ret == -EAGAIN) {
>>   		if (!req_has_async_data(req)) {
> 
> I obviously like this idea, but it should be accompanied by getting rid
> of ->compat and ->syscall_iopoll in the ctx as well?

This all piggy backing cmd specific bits onto core io_uring issue_flags
business is pretty nasty. Apart from that, it mixes constant io_uring
flags and "execution context" issue_flags. And we're dancing around it
not really addressing the problem.

IMHO, cmds should be testing for IORING_SETUP flags directly via
helpers, not renaming them and abusing core io_uring flags. E.g. I had
a patch like below but didn't care enough to send:


diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 909377068a87..1a82a0633f16 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2874,7 +2874,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
  
  	ublk_ctrl_cmd_dump(cmd);
  
-	if (!(issue_flags & IO_URING_F_SQE128))
+	if (!(io_uring_cmd_get_ctx_flags(cmd) & IORING_SETUP_SQE128))
  		goto out;
  
  	ret = ublk_check_cmd_op(cmd_op);
diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index d69b4038aa3e..8a18a705ff31 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -79,4 +79,11 @@ static inline struct task_struct *io_uring_cmd_get_task(struct io_uring_cmd *cmd
  	return cmd_to_io_kiocb(cmd)->task;
  }
  
+static inline unsigned io_uring_cmd_get_ctx_flags(struct io_uring_cmd *cmd)
+{
+	struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
+
+	return ctx->flags;
+}
+
  #endif /* _LINUX_IO_URING_CMD_H */

-- 
Pavel Begunkov