* [PATCH 2/3] io_uring: avoid whole io_wq_work copy for inline requests
2020-05-26 6:43 [PATCH 1/3] io_uring: don't use req->work.creds for inline requests Xiaoguang Wang
@ 2020-05-26 6:43 ` Xiaoguang Wang
2020-05-26 15:08 ` Pavel Begunkov
2020-05-26 6:43 ` [PATCH 3/3] io_uring: avoid unnecessary io_wq_work copy for fast poll feature Xiaoguang Wang
2020-05-26 14:33 ` [PATCH 1/3] io_uring: don't use req->work.creds for inline requests Pavel Begunkov
2 siblings, 1 reply; 9+ messages in thread
From: Xiaoguang Wang @ 2020-05-26 6:43 UTC (permalink / raw)
To: io-uring; +Cc: axboe, asml.silence, joseph.qi, 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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] io_uring: avoid whole io_wq_work copy for inline requests
2020-05-26 6:43 ` [PATCH 2/3] io_uring: avoid whole io_wq_work copy " Xiaoguang Wang
@ 2020-05-26 15:08 ` Pavel Begunkov
0 siblings, 0 replies; 9+ messages in thread
From: Pavel Begunkov @ 2020-05-26 15:08 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
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 <[email protected]>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] io_uring: avoid unnecessary io_wq_work copy for fast poll feature
2020-05-26 6:43 [PATCH 1/3] io_uring: don't use req->work.creds for inline requests Xiaoguang Wang
2020-05-26 6:43 ` [PATCH 2/3] io_uring: avoid whole io_wq_work copy " Xiaoguang Wang
@ 2020-05-26 6:43 ` Xiaoguang Wang
2020-05-26 14:33 ` [PATCH 1/3] io_uring: don't use req->work.creds for inline requests Pavel Begunkov
2 siblings, 0 replies; 9+ messages in thread
From: Xiaoguang Wang @ 2020-05-26 6:43 UTC (permalink / raw)
To: io-uring; +Cc: axboe, asml.silence, joseph.qi, 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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests
2020-05-26 6:43 [PATCH 1/3] io_uring: don't use req->work.creds for inline requests Xiaoguang Wang
2020-05-26 6:43 ` [PATCH 2/3] io_uring: avoid whole io_wq_work copy " Xiaoguang Wang
2020-05-26 6:43 ` [PATCH 3/3] io_uring: avoid unnecessary io_wq_work copy for fast poll feature Xiaoguang Wang
@ 2020-05-26 14:33 ` Pavel Begunkov
2020-05-26 14:59 ` Xiaoguang Wang
2 siblings, 1 reply; 9+ messages in thread
From: Pavel Begunkov @ 2020-05-26 14:33 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
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 <[email protected]>
> ---
> 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(¤t->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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests
2020-05-26 14:33 ` [PATCH 1/3] io_uring: don't use req->work.creds for inline requests Pavel Begunkov
@ 2020-05-26 14:59 ` Xiaoguang Wang
2020-05-26 15:31 ` Pavel Begunkov
0 siblings, 1 reply; 9+ messages in thread
From: Xiaoguang Wang @ 2020-05-26 14:59 UTC (permalink / raw)
To: Pavel Begunkov, io-uring; +Cc: axboe, joseph.qi
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 <[email protected]>
>> ---
>> 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(¤t->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;
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests
2020-05-26 14:59 ` Xiaoguang Wang
@ 2020-05-26 15:31 ` Pavel Begunkov
2020-05-27 16:41 ` Xiaoguang Wang
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Begunkov @ 2020-05-26 15:31 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests
2020-05-26 15:31 ` Pavel Begunkov
@ 2020-05-27 16:41 ` Xiaoguang Wang
2020-05-29 8:16 ` Pavel Begunkov
0 siblings, 1 reply; 9+ messages in thread
From: Xiaoguang Wang @ 2020-05-27 16:41 UTC (permalink / raw)
To: Pavel Begunkov, io-uring; +Cc: axboe, joseph.qi
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests
2020-05-27 16:41 ` Xiaoguang Wang
@ 2020-05-29 8:16 ` Pavel Begunkov
0 siblings, 0 replies; 9+ messages in thread
From: Pavel Begunkov @ 2020-05-29 8:16 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
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
^ permalink raw reply [flat|nested] 9+ messages in thread