[PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[Xiaoguang Wang]

In io_init_req(), if uers requires a new credentials, currently we'll
save it in req->work.creds, but indeed io_wq_work is designed to describe
needed running environment for requests that will go to io-wq, if one
request is going to be submitted inline, we'd better not touch io_wq_work.
Here add a new 'const struct cred *creds' in io_kiocb, if uers requires a
new credentials, inline requests can use it.

This patch is also a preparation for later patch.

Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
---
 fs/io_uring.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 2af87f73848e..788d960abc69 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -635,6 +635,7 @@ struct io_kiocb {
 	unsigned int		flags;
 	refcount_t		refs;
 	struct task_struct	*task;
+	const struct cred	*creds;
 	unsigned long		fsize;
 	u64			user_data;
 	u32			result;
@@ -1035,8 +1036,10 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
 		mmgrab(current->mm);
 		req->work.mm = current->mm;
 	}
-	if (!req->work.creds)
+	if (!req->creds)
 		req->work.creds = get_current_cred();
+	else
+		req->work.creds = get_cred(req->creds);
 	if (!req->work.fs && def->needs_fs) {
 		spin_lock(&current->fs->lock);
 		if (!current->fs->in_exec) {
@@ -1368,6 +1371,9 @@ static void __io_req_aux_free(struct io_kiocb *req)
 	if (req->flags & REQ_F_NEED_CLEANUP)
 		io_cleanup_req(req);
 
+	if (req->creds)
+		put_cred(req->creds);
+
 	kfree(req->io);
 	if (req->file)
 		io_put_file(req, req->file, (req->flags & REQ_F_FIXED_FILE));
@@ -5673,13 +5679,13 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 again:
 	linked_timeout = io_prep_linked_timeout(req);
 
-	if (req->work.creds && req->work.creds != current_cred()) {
+	if (req->creds && req->creds != current_cred()) {
 		if (old_creds)
 			revert_creds(old_creds);
-		if (old_creds == req->work.creds)
+		if (old_creds == req->creds)
 			old_creds = NULL; /* restored original creds */
 		else
-			old_creds = override_creds(req->work.creds);
+			old_creds = override_creds(req->creds);
 	}
 
 	ret = io_issue_sqe(req, sqe, true);
@@ -5970,11 +5976,12 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 
 	id = READ_ONCE(sqe->personality);
 	if (id) {
-		req->work.creds = idr_find(&ctx->personality_idr, id);
-		if (unlikely(!req->work.creds))
+		req->creds = idr_find(&ctx->personality_idr, id);
+		if (unlikely(!req->creds))
 			return -EINVAL;
-		get_cred(req->work.creds);
-	}
+		get_cred(req->creds);
+	} else
+		req->creds = NULL;
 
 	/* same numerical values with corresponding REQ_F_*, safe to copy */
 	req->flags |= sqe_flags;
-- 
2.17.2

[PATCH 2/3] io_uring: avoid whole io_wq_work copy for inline requests

[Xiaoguang Wang]

If requests can be submitted inline, we don't need to copy whole
io_wq_work in io_init_req(), which is an expensive operation. I
use my io_uring_nop_stress to evaluate performance improvement.

In my physical machine, before this patch:
$sudo taskset -c 60 ./io_uring_nop_stress -r 120
total ios: 749093872
IOPS:      6242448

$sudo taskset -c 60 ./io_uring_nop_stress -r 120
total ios: 786083712
IOPS:      6550697

About 4.9% improvement.

Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
---
 fs/io-wq.h    | 13 +++++++++----
 fs/io_uring.c | 33 ++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/fs/io-wq.h b/fs/io-wq.h
index 5ba12de7572f..11d981a67006 100644
--- a/fs/io-wq.h
+++ b/fs/io-wq.h
@@ -94,10 +94,15 @@ struct io_wq_work {
 	pid_t task_pid;
 };
 
-#define INIT_IO_WORK(work, _func)				\
-	do {							\
-		*(work) = (struct io_wq_work){ .func = _func };	\
-	} while (0)						\
+static inline void init_io_work(struct io_wq_work *work,
+		void (*func)(struct io_wq_work **))
+{
+	if (!work->func)
+		*(work) = (struct io_wq_work){ .func = func };
+	else
+		work->func = func;
+}
+
 
 static inline struct io_wq_work *wq_next_work(struct io_wq_work *work)
 {
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 788d960abc69..a54b21e6d921 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1056,6 +1056,13 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
 
 static inline void io_req_work_drop_env(struct io_kiocb *req)
 {
+	/*
+	 * Use io_wq_work.func as a flag to determine whether needs to
+	 * drop environment.
+	 */
+	if (!req->work.func)
+		return;
+
 	if (req->work.mm) {
 		mmdrop(req->work.mm);
 		req->work.mm = NULL;
@@ -1600,7 +1607,7 @@ static void io_wq_assign_next(struct io_wq_work **workptr, struct io_kiocb *nxt)
 	*workptr = &nxt->work;
 	link = io_prep_linked_timeout(nxt);
 	if (link)
-		nxt->work.func = io_link_work_cb;
+		init_io_work(&nxt->work, io_link_work_cb);
 }
 
 /*
@@ -2929,7 +2936,7 @@ static int io_fsync(struct io_kiocb *req, bool force_nonblock)
 {
 	/* fsync always requires a blocking context */
 	if (force_nonblock) {
-		req->work.func = io_fsync_finish;
+		init_io_work(&req->work, io_fsync_finish);
 		return -EAGAIN;
 	}
 	__io_fsync(req);
@@ -2977,7 +2984,7 @@ static int io_fallocate(struct io_kiocb *req, bool force_nonblock)
 {
 	/* fallocate always requiring blocking context */
 	if (force_nonblock) {
-		req->work.func = io_fallocate_finish;
+		init_io_work(&req->work, io_fallocate_finish);
 		return -EAGAIN;
 	}
 
@@ -3506,7 +3513,7 @@ static int io_close(struct io_kiocb *req, bool force_nonblock)
 		/* submission ref will be dropped, take it for async */
 		refcount_inc(&req->refs);
 
-		req->work.func = io_close_finish;
+		init_io_work(&req->work, io_close_finish);
 		/*
 		 * Do manual async queue here to avoid grabbing files - we don't
 		 * need the files, and it'll cause io_close_finish() to close
@@ -3569,7 +3576,7 @@ static int io_sync_file_range(struct io_kiocb *req, bool force_nonblock)
 {
 	/* sync_file_range always requires a blocking context */
 	if (force_nonblock) {
-		req->work.func = io_sync_file_range_finish;
+		init_io_work(&req->work, io_sync_file_range_finish);
 		return -EAGAIN;
 	}
 
@@ -4038,7 +4045,7 @@ static int io_accept(struct io_kiocb *req, bool force_nonblock)
 
 	ret = __io_accept(req, force_nonblock);
 	if (ret == -EAGAIN && force_nonblock) {
-		req->work.func = io_accept_finish;
+		init_io_work(&req->work, io_accept_finish);
 		return -EAGAIN;
 	}
 	return 0;
@@ -4254,6 +4261,7 @@ static void io_poll_task_handler(struct io_kiocb *req, struct io_kiocb **nxt)
 	}
 
 	hash_del(&req->hash_node);
+	req->work.func = NULL;
 	io_poll_complete(req, req->result, 0);
 	req->flags |= REQ_F_COMP_LOCKED;
 	io_put_req_find_next(req, nxt);
@@ -5038,6 +5046,9 @@ static int io_req_defer_prep(struct io_kiocb *req,
 	if (!sqe)
 		return 0;
 
+	if (!req->work.func)
+		init_io_work(&req->work, io_wq_submit_work);
+
 	if (io_op_defs[req->opcode].file_table) {
 		ret = io_grab_files(req);
 		if (unlikely(ret))
@@ -5702,6 +5713,9 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 			goto exit;
 		}
 punt:
+		if (!req->work.func)
+			init_io_work(&req->work, io_wq_submit_work);
+
 		if (io_op_defs[req->opcode].file_table) {
 			ret = io_grab_files(req);
 			if (ret)
@@ -5954,7 +5968,12 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 	refcount_set(&req->refs, 2);
 	req->task = NULL;
 	req->result = 0;
-	INIT_IO_WORK(&req->work, io_wq_submit_work);
+	/*
+	 * Use io_wq_work.func as a flag to determine whether req->work
+	 * is valid. If req->work.func is NULL, there is no need to drop
+	 * environment when freeing req.
+	 */
+	req->work.func = NULL;
 
 	if (unlikely(req->opcode >= IORING_OP_LAST))
 		return -EINVAL;
-- 
2.17.2

[PATCH 3/3] io_uring: avoid unnecessary io_wq_work copy for fast poll feature

[Xiaoguang Wang]

Basically IORING_OP_POLL_ADD command and async armed poll handlers
for regular commands don't touch io_wq_work, so there is no need to
always do io_wq_work copy. Here add a new flag 'REQ_F_WORK_NEED_RESTORE'
to control whether to do io_wq_work copy.

Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
---
 fs/io_uring.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index a54b21e6d921..6b9c79048962 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -535,6 +535,7 @@ enum {
 	REQ_F_POLLED_BIT,
 	REQ_F_BUFFER_SELECTED_BIT,
 	REQ_F_NO_FILE_TABLE_BIT,
+	REQ_F_WORK_NEED_RESTORE_BIT,
 
 	/* not a real bit, just to check we're not overflowing the space */
 	__REQ_F_LAST_BIT,
@@ -590,6 +591,8 @@ enum {
 	REQ_F_BUFFER_SELECTED	= BIT(REQ_F_BUFFER_SELECTED_BIT),
 	/* doesn't need file table for this request */
 	REQ_F_NO_FILE_TABLE	= BIT(REQ_F_NO_FILE_TABLE_BIT),
+	/* need restore io_wq_work */
+	REQ_F_WORK_NEED_RESTORE = BIT(REQ_F_WORK_NEED_RESTORE_BIT),
 };
 
 struct async_poll {
@@ -4390,7 +4393,10 @@ static void io_async_task_func(struct callback_head *cb)
 	spin_unlock_irq(&ctx->completion_lock);
 
 	/* restore ->work in case we need to retry again */
-	memcpy(&req->work, &apoll->work, sizeof(req->work));
+	if (req->flags & REQ_F_WORK_NEED_RESTORE)
+		memcpy(&req->work, &apoll->work, sizeof(req->work));
+	else
+		req->work.func = NULL;
 	kfree(apoll);
 
 	if (!canceled) {
@@ -4487,7 +4493,10 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
 		return false;
 
 	req->flags |= REQ_F_POLLED;
-	memcpy(&apoll->work, &req->work, sizeof(req->work));
+	if (req->work.func) {
+		req->flags |= REQ_F_WORK_NEED_RESTORE;
+		memcpy(&apoll->work, &req->work, sizeof(req->work));
+	}
 	had_io = req->io != NULL;
 
 	get_task_struct(current);
@@ -4512,7 +4521,10 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
 		if (!had_io)
 			io_poll_remove_double(req);
 		spin_unlock_irq(&ctx->completion_lock);
-		memcpy(&req->work, &apoll->work, sizeof(req->work));
+		if (req->flags & REQ_F_WORK_NEED_RESTORE)
+			memcpy(&req->work, &apoll->work, sizeof(req->work));
+		else
+			req->work.func = NULL;
 		kfree(apoll);
 		return false;
 	}
@@ -4557,7 +4569,11 @@ static bool io_poll_remove_one(struct io_kiocb *req)
 			 * io_req_work_drop_env below when dropping the
 			 * final reference.
 			 */
-			memcpy(&req->work, &apoll->work, sizeof(req->work));
+			if (req->flags & REQ_F_WORK_NEED_RESTORE)
+				memcpy(&req->work, &apoll->work,
+				       sizeof(req->work));
+			else
+				req->work.func = NULL;
 			kfree(apoll);
 		}
 	}
-- 
2.17.2

Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[Pavel Begunkov]

On 26/05/2020 09:43, Xiaoguang Wang wrote:
> In io_init_req(), if uers requires a new credentials, currently we'll
> save it in req->work.creds, but indeed io_wq_work is designed to describe
> needed running environment for requests that will go to io-wq, if one
> request is going to be submitted inline, we'd better not touch io_wq_work.
> Here add a new 'const struct cred *creds' in io_kiocb, if uers requires a
> new credentials, inline requests can use it.
> 
> This patch is also a preparation for later patch.

What's the difference from keeping only one creds field in io_kiocb (i.e.
req->work.creds), but handling it specially (i.e. always initialising)? It will
be a lot easier than tossing it around.

Also, the patch doubles {get,put}_creds() for sqe->personality case, and that's
extra atomics without a good reason.

> 
> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> ---
>  fs/io_uring.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 2af87f73848e..788d960abc69 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -635,6 +635,7 @@ struct io_kiocb {
>  	unsigned int		flags;
>  	refcount_t		refs;
>  	struct task_struct	*task;
> +	const struct cred	*creds;
>  	unsigned long		fsize;
>  	u64			user_data;
>  	u32			result;
> @@ -1035,8 +1036,10 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
>  		mmgrab(current->mm);
>  		req->work.mm = current->mm;
>  	}
> -	if (!req->work.creds)
> +	if (!req->creds)
>  		req->work.creds = get_current_cred();
> +	else
> +		req->work.creds = get_cred(req->creds);
>  	if (!req->work.fs && def->needs_fs) {
>  		spin_lock(&current->fs->lock);
>  		if (!current->fs->in_exec) {
> @@ -1368,6 +1371,9 @@ static void __io_req_aux_free(struct io_kiocb *req)
>  	if (req->flags & REQ_F_NEED_CLEANUP)
>  		io_cleanup_req(req);
>  
> +	if (req->creds)
> +		put_cred(req->creds);
> +
>  	kfree(req->io);
>  	if (req->file)
>  		io_put_file(req, req->file, (req->flags & REQ_F_FIXED_FILE));
> @@ -5673,13 +5679,13 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  again:
>  	linked_timeout = io_prep_linked_timeout(req);
>  
> -	if (req->work.creds && req->work.creds != current_cred()) {
> +	if (req->creds && req->creds != current_cred()) {
>  		if (old_creds)
>  			revert_creds(old_creds);
> -		if (old_creds == req->work.creds)
> +		if (old_creds == req->creds)
>  			old_creds = NULL; /* restored original creds */
>  		else
> -			old_creds = override_creds(req->work.creds);
> +			old_creds = override_creds(req->creds);
>  	}
>  
>  	ret = io_issue_sqe(req, sqe, true);
> @@ -5970,11 +5976,12 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
>  
>  	id = READ_ONCE(sqe->personality);
>  	if (id) {
> -		req->work.creds = idr_find(&ctx->personality_idr, id);
> -		if (unlikely(!req->work.creds))
> +		req->creds = idr_find(&ctx->personality_idr, id);
> +		if (unlikely(!req->creds))
>  			return -EINVAL;
> -		get_cred(req->work.creds);
> -	}
> +		get_cred(req->creds);
> +	} else
> +		req->creds = NULL;
>  
>  	/* same numerical values with corresponding REQ_F_*, safe to copy */
>  	req->flags |= sqe_flags;
> 

-- 
Pavel Begunkov

Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[Xiaoguang Wang]

hi,

> On 26/05/2020 09:43, Xiaoguang Wang wrote:
>> In io_init_req(), if uers requires a new credentials, currently we'll
>> save it in req->work.creds, but indeed io_wq_work is designed to describe
>> needed running environment for requests that will go to io-wq, if one
>> request is going to be submitted inline, we'd better not touch io_wq_work.
>> Here add a new 'const struct cred *creds' in io_kiocb, if uers requires a
>> new credentials, inline requests can use it.
>>
>> This patch is also a preparation for later patch.
> 
> What's the difference from keeping only one creds field in io_kiocb (i.e.
> req->work.creds), but handling it specially (i.e. always initialising)? It will
> be a lot easier than tossing it around.
> 
> Also, the patch doubles {get,put}_creds() for sqe->personality case, and that's
> extra atomics without a good reason.
You're right, thanks.
The original motivation for this patch is that it's just a preparation later patch
"io_uring: avoid whole io_wq_work copy for inline requests", I can use io_wq_work.func
to determine whether to drop io_wq_work in io_req_work_drop_env(), so if io_wq_work.func
is NULL, I don't want io_wq_work has a valid creds.
I'll look into whether we can just assign req->creds's pointer to io_wq_work.creds to
reduce the atomic operations.

Regards,
Xiaoguang Wang

> 
>>
>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
>> ---
>>   fs/io_uring.c | 23 +++++++++++++++--------
>>   1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 2af87f73848e..788d960abc69 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -635,6 +635,7 @@ struct io_kiocb {
>>   	unsigned int		flags;
>>   	refcount_t		refs;
>>   	struct task_struct	*task;
>> +	const struct cred	*creds;
>>   	unsigned long		fsize;
>>   	u64			user_data;
>>   	u32			result;
>> @@ -1035,8 +1036,10 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
>>   		mmgrab(current->mm);
>>   		req->work.mm = current->mm;
>>   	}
>> -	if (!req->work.creds)
>> +	if (!req->creds)
>>   		req->work.creds = get_current_cred();
>> +	else
>> +		req->work.creds = get_cred(req->creds);
>>   	if (!req->work.fs && def->needs_fs) {
>>   		spin_lock(&current->fs->lock);
>>   		if (!current->fs->in_exec) {
>> @@ -1368,6 +1371,9 @@ static void __io_req_aux_free(struct io_kiocb *req)
>>   	if (req->flags & REQ_F_NEED_CLEANUP)
>>   		io_cleanup_req(req);
>>   
>> +	if (req->creds)
>> +		put_cred(req->creds);
>> +
>>   	kfree(req->io);
>>   	if (req->file)
>>   		io_put_file(req, req->file, (req->flags & REQ_F_FIXED_FILE));
>> @@ -5673,13 +5679,13 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>   again:
>>   	linked_timeout = io_prep_linked_timeout(req);
>>   
>> -	if (req->work.creds && req->work.creds != current_cred()) {
>> +	if (req->creds && req->creds != current_cred()) {
>>   		if (old_creds)
>>   			revert_creds(old_creds);
>> -		if (old_creds == req->work.creds)
>> +		if (old_creds == req->creds)
>>   			old_creds = NULL; /* restored original creds */
>>   		else
>> -			old_creds = override_creds(req->work.creds);
>> +			old_creds = override_creds(req->creds);
>>   	}
>>   
>>   	ret = io_issue_sqe(req, sqe, true);
>> @@ -5970,11 +5976,12 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>   
>>   	id = READ_ONCE(sqe->personality);
>>   	if (id) {
>> -		req->work.creds = idr_find(&ctx->personality_idr, id);
>> -		if (unlikely(!req->work.creds))
>> +		req->creds = idr_find(&ctx->personality_idr, id);
>> +		if (unlikely(!req->creds))
>>   			return -EINVAL;
>> -		get_cred(req->work.creds);
>> -	}
>> +		get_cred(req->creds);
>> +	} else
>> +		req->creds = NULL;
>>   
>>   	/* same numerical values with corresponding REQ_F_*, safe to copy */
>>   	req->flags |= sqe_flags;
>>
> 

Re: [PATCH 2/3] io_uring: avoid whole io_wq_work copy for inline requests

[Pavel Begunkov]

On 26/05/2020 09:43, Xiaoguang Wang wrote:
> If requests can be submitted inline, we don't need to copy whole
> io_wq_work in io_init_req(), which is an expensive operation. I
> use my io_uring_nop_stress to evaluate performance improvement.
> 
> In my physical machine, before this patch:
> $sudo taskset -c 60 ./io_uring_nop_stress -r 120
> total ios: 749093872
> IOPS:      6242448
> 
> $sudo taskset -c 60 ./io_uring_nop_stress -r 120
> total ios: 786083712
> IOPS:      6550697
> 
> About 4.9% improvement.

Interesting, what's the contribution of fast check in *drop_env() separately
from not zeroing req->work.

> 
> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> ---
>  fs/io-wq.h    | 13 +++++++++----
>  fs/io_uring.c | 33 ++++++++++++++++++++++++++-------
>  2 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/io-wq.h b/fs/io-wq.h
> index 5ba12de7572f..11d981a67006 100644
> --- a/fs/io-wq.h
> +++ b/fs/io-wq.h
> @@ -94,10 +94,15 @@ struct io_wq_work {
>  	pid_t task_pid;
>  };
>  
> -#define INIT_IO_WORK(work, _func)				\
> -	do {							\
> -		*(work) = (struct io_wq_work){ .func = _func };	\
> -	} while (0)						\
> +static inline void init_io_work(struct io_wq_work *work,
> +		void (*func)(struct io_wq_work **))
> +{
> +	if (!work->func)

Not really a good name for a function, which expects some of the fields to be
already initialised, too subtle. Unfortunately, it'll break at some point.

I think, a flag in req->flags for that would be better.
e.g. REQ_F_WORK_INITIALIZED

And it'd be better to somehow separate field of struct io_wq_work,
1. always initialised ones like ->func (and maybe ->creds).
2. lazy initialised controlled by the mentioned flag.


> +		*(work) = (struct io_wq_work){ .func = func };
> +	else
> +		work->func = func;
> +}
> +
>
-- 
Pavel Begunkov

Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[Pavel Begunkov]

On 26/05/2020 17:59, Xiaoguang Wang wrote:
> hi,
> 
>> On 26/05/2020 09:43, Xiaoguang Wang wrote:
>>> In io_init_req(), if uers requires a new credentials, currently we'll
>>> save it in req->work.creds, but indeed io_wq_work is designed to describe
>>> needed running environment for requests that will go to io-wq, if one
>>> request is going to be submitted inline, we'd better not touch io_wq_work.
>>> Here add a new 'const struct cred *creds' in io_kiocb, if uers requires a
>>> new credentials, inline requests can use it.
>>>
>>> This patch is also a preparation for later patch.
>>
>> What's the difference from keeping only one creds field in io_kiocb (i.e.
>> req->work.creds), but handling it specially (i.e. always initialising)? It will
>> be a lot easier than tossing it around.
>>
>> Also, the patch doubles {get,put}_creds() for sqe->personality case, and that's
>> extra atomics without a good reason.
> You're right, thanks.
> The original motivation for this patch is that it's just a preparation later patch
> "io_uring: avoid whole io_wq_work copy for inline requests", I can use
> io_wq_work.func
> to determine whether to drop io_wq_work in io_req_work_drop_env(), so if
> io_wq_work.func
> is NULL, I don't want io_wq_work has a valid creds.
> I'll look into whether we can just assign req->creds's pointer to
> io_wq_work.creds to
> reduce the atomic operations.

See a comment for the [2/3], can spark some ideas.

It's a bit messy and makes it more difficult to keep in mind -- all that extra
state (i.e. initialised or not) + caring whether func was already set. IMHO, the
nop-test do not really justifies extra complexity, unless the whole stuff is
pretty and clear. Can you benchmark something more realistic? at least
reads/writes to null_blk (completion_nsec=0).

-- 
Pavel Begunkov

Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[Xiaoguang Wang]

hi Pavel,

> On 26/05/2020 17:59, Xiaoguang Wang wrote:
>> hi,
>>
>>> On 26/05/2020 09:43, Xiaoguang Wang wrote:
>>>> In io_init_req(), if uers requires a new credentials, currently we'll
>>>> save it in req->work.creds, but indeed io_wq_work is designed to describe
>>>> needed running environment for requests that will go to io-wq, if one
>>>> request is going to be submitted inline, we'd better not touch io_wq_work.
>>>> Here add a new 'const struct cred *creds' in io_kiocb, if uers requires a
>>>> new credentials, inline requests can use it.
>>>>
>>>> This patch is also a preparation for later patch.
>>>
>>> What's the difference from keeping only one creds field in io_kiocb (i.e.
>>> req->work.creds), but handling it specially (i.e. always initialising)? It will
>>> be a lot easier than tossing it around.
>>>
>>> Also, the patch doubles {get,put}_creds() for sqe->personality case, and that's
>>> extra atomics without a good reason.
>> You're right, thanks.
>> The original motivation for this patch is that it's just a preparation later patch
>> "io_uring: avoid whole io_wq_work copy for inline requests", I can use
>> io_wq_work.func
>> to determine whether to drop io_wq_work in io_req_work_drop_env(), so if
>> io_wq_work.func
>> is NULL, I don't want io_wq_work has a valid creds.
>> I'll look into whether we can just assign req->creds's pointer to
>> io_wq_work.creds to
>> reduce the atomic operations.
> 
> See a comment for the [2/3], can spark some ideas.
> 
> It's a bit messy and makes it more difficult to keep in mind -- all that extra
> state (i.e. initialised or not) + caring whether func was already set. IMHO, the
> nop-test do not really justifies extra complexity, unless the whole stuff is
> pretty and clear. Can you benchmark something more realistic? at least
> reads/writes to null_blk (completion_nsec=0).
Indeed for this patch set, I also don't expect any obvious performance improvement,
just think current codes are not good, so try to improve it.
I will send a v2 version later, in which I'll use null_blk to evaluate performance,
please have a check.

Regards,
Xiaoguang Wang


> 

Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[Pavel Begunkov]

On 27/05/2020 19:41, Xiaoguang Wang wrote:
>>
>> See a comment for the [2/3], can spark some ideas.
>>
>> It's a bit messy and makes it more difficult to keep in mind -- all that extra
>> state (i.e. initialised or not) + caring whether func was already set. IMHO, the
>> nop-test do not really justifies extra complexity, unless the whole stuff is
>> pretty and clear. Can you benchmark something more realistic? at least
>> reads/writes to null_blk (completion_nsec=0).
> Indeed for this patch set, I also don't expect any obvious performance improvement,
> just think current codes are not good, so try to improve it.

I'm sure, we'll figure out something good in the process!
There are shaky places where io_uring can use having a more orderly workflow.


> I will send a v2 version later, in which I'll use null_blk to evaluate performance,
> please have a check.
> 

-- 
Pavel Begunkov