Re: [PATCH v3 1/2] io_uring: avoid whole io_wq_work copy for requests completed inline

Pavel Begunkov
2020-05-29T08:59:52Z

On 28/05/2020 12:15, Xiaoguang Wang wrote:
> If requests can be submitted and completed inline, we don't need to
> initialize whole io_wq_work in io_init_req(), which is an expensive
> operation, add a new 'REQ_F_WORK_INITIALIZED' to control whether
> io_wq_work is initialized.

It looks nicer. Especially if you'd add a helper as Jens supposed.

The other thing, even though I hate treating a part of the fields differently
from others, I don't like ->creds tossing either.

Did you consider trying using only ->work.creds without adding req->creds? like
in the untested incremental below. init_io_work() there is misleading, should be
somehow played around better.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 4dd3295d74f6..4086561ce444 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -643,7 +643,6 @@ 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;
@@ -894,8 +893,16 @@ static const struct file_operations io_uring_fops;
static inline void init_io_work(struct io_kiocb *req,
void (*func)(struct io_wq_work **))
{
- req->work = (struct io_wq_work){ .func = func };
- req->flags |= REQ_F_WORK_INITIALIZED;
+ struct io_wq_work *work = &req->work;
+
+ /* work->creds are already initialised by a user */
+ work->list.next = NULL;
+ work->func = func;
+ work->files = NULL;
+ work->mm = NULL;
+ work->fs = NULL;
+ work->flags = REQ_F_WORK_INITIALIZED;
+ work->task_pid = 0;
}
struct sock *io_uring_get_socket(struct file *file)
{
@@ -1019,15 +1026,9 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
mmgrab(current->mm);
req->work.mm = current->mm;
}
+ if (!req->work.creds)
+ req->work.creds = get_current_cred();

- if (!req->work.creds) {
- if (!req->creds)
- req->work.creds = get_current_cred();
- else {
- req->work.creds = req->creds;
- req->creds = NULL;
- }
- }
if (!req->work.fs && def->needs_fs) {
spin_lock(&current->fs->lock);
if (!current->fs->in_exec) {
@@ -1044,6 +1045,12 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,

static inline void io_req_work_drop_env(struct io_kiocb *req)
{
+ /* always init'ed, put before REQ_F_WORK_INITIALIZED check */
+ if (req->work.creds) {
+ put_cred(req->work.creds);
+ req->work.creds = NULL;
+ }
+
if (!(req->flags & REQ_F_WORK_INITIALIZED))
return;

@@ -1051,10 +1058,6 @@ static inline void io_req_work_drop_env(struct io_kiocb *req)
mmdrop(req->work.mm);
req->work.mm = NULL;
}
- if (req->work.creds) {
- put_cred(req->work.creds);
- req->work.creds = NULL;
- }
if (req->work.fs) {
struct fs_struct *fs = req->work.fs;

@@ -5901,12 +5904,12 @@ static int io_init_req(struct io_ring_ctx *ctx, struct
io_kiocb *req,

id = READ_ONCE(sqe->personality);
if (id) {
- req->creds = idr_find(&ctx->personality_idr, id);
- if (unlikely(!req->creds))
+ req->work.creds = idr_find(&ctx->personality_idr, id);
+ if (unlikely(!req->work.creds))
return -EINVAL;
- get_cred(req->creds);
+ get_cred(req->work.creds);
} else
- req->creds = NULL;
+ req->work.creds = NULL;

/* same numerical values with corresponding REQ_F_*, safe to copy */
req->flags |= sqe_flags;

--
Pavel Begunkov