Re: [External] Re: [PATCH v2] io_uring: add IORING_SETUP_SQTHREAD_STATS flag to enable sqthread stats collection

[Fengnan Chang <changfengnan@bytedance.com>]

Gabriel Krisman Bertazi <krisman@suse.de> 于2025年10月20日周一 22:59写道：
>
> Fengnan Chang <changfengnan@bytedance.com> writes:
>
> > In previous versions, getrusage was always called in sqrthread
> > to count work time, but this could incur some overhead.
> > This patch turn off stats by default, and introduces a new flag
> > IORING_SETUP_SQTHREAD_STATS that allows user to enable the
> > collection of statistics in the sqthread.
> >
> > ./t/io_uring -p1 -d128 -b4096 -s32 -c1 -F1 -B1 -R1 -X1 -n1 ./testfile
> > IOPS base: 570K, patch: 590K
> >
> > ./t/io_uring -p1 -d128 -b4096 -s32 -c1 -F1 -B1 -R1 -X1 -n1 /dev/nvme1n1
> > IOPS base: 826K, patch: 889K
> >
> > Signed-off-by: Fengnan Chang <changfengnan@bytedance.com>
> > Reviewed-by: Diangang Li <lidiangang@bytedance.com>
> > ---
> >  include/uapi/linux/io_uring.h |  5 +++++
> >  io_uring/fdinfo.c             | 15 ++++++++++-----
> >  io_uring/io_uring.h           |  3 ++-
> >  io_uring/sqpoll.c             | 10 +++++++---
> >  io_uring/sqpoll.h             |  1 +
> >  5 files changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> > index 263bed13473e..8c5cb9533950 100644
> > --- a/include/uapi/linux/io_uring.h
> > +++ b/include/uapi/linux/io_uring.h
> > @@ -231,6 +231,11 @@ enum io_uring_sqe_flags_bit {
> >   */
> >  #define IORING_SETUP_CQE_MIXED               (1U << 18)
> >
> > +/*
> > + * Enable SQPOLL thread stats collection
> > + */
> > +#define IORING_SETUP_SQTHREAD_STATS  (1U << 19)
> > +
> >  enum io_uring_op {
> >       IORING_OP_NOP,
> >       IORING_OP_READV,
> > diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c
> > index ff3364531c77..4c532e414255 100644
> > --- a/io_uring/fdinfo.c
> > +++ b/io_uring/fdinfo.c
> > @@ -154,13 +154,16 @@ static void __io_uring_show_fdinfo(struct io_ring_ctx *ctx, struct seq_file *m)
> >               if (tsk) {
> >                       get_task_struct(tsk);
> >                       rcu_read_unlock();
> > -                     getrusage(tsk, RUSAGE_SELF, &sq_usage);
> > +                     if (sq->enable_work_time_stat)
> > +                             getrusage(tsk, RUSAGE_SELF, &sq_usage);
> >                       put_task_struct(tsk);
>
> If the usage statistics are disabled, you don't need to acquire and drop
> the task_struct reference any longer.  you can move the get/put_task_struct
> into the if.

I'll fix this in next version.

>
> >                       sq_pid = sq->task_pid;
> >                       sq_cpu = sq->sq_cpu;
> > -                     sq_total_time = (sq_usage.ru_stime.tv_sec * 1000000
> > +                     if (sq->enable_work_time_stat) {
> > +                             sq_total_time = (sq_usage.ru_stime.tv_sec * 1000000
> >                                        + sq_usage.ru_stime.tv_usec);
> > -                     sq_work_time = sq->work_time;
> > +                             sq_work_time = sq->work_time;
> > +                     }
> >               } else {
> >                       rcu_read_unlock();
> >               }
> > @@ -168,8 +171,10 @@ static void __io_uring_show_fdinfo(struct io_ring_ctx *ctx, struct seq_file *m)
> >
> >       seq_printf(m, "SqThread:\t%d\n", sq_pid);
> >       seq_printf(m, "SqThreadCpu:\t%d\n", sq_cpu);
> > -     seq_printf(m, "SqTotalTime:\t%llu\n", sq_total_time);
> > -     seq_printf(m, "SqWorkTime:\t%llu\n", sq_work_time);
> > +     if (ctx->flags & IORING_SETUP_SQTHREAD_STATS) {
> > +             seq_printf(m, "SqTotalTime:\t%llu\n", sq_total_time);
> > +             seq_printf(m, "SqWorkTime:\t%llu\n", sq_work_time);
> > +     }
>
> It works, but it is weird that you gate the writing of sq_total_time on
> (sq->enable_work_time_stat) and then, the display of it on (ctx->flags &
> IORING_SETUP_SQTHREAD_STATS).  Since a sqpoll can attend to more than
> one ctx, I'd just check ctx->flags & IORING_SETUP_SQTHREAD_STATS
> in both places in this function.
Yeh, there's a bit of a problem here, I was going to use check
ctx->flags & IORING_SETUP_SQTHREAD_STATS, but in io_sq_thread, getrusage
is try to account multi ctx, and can't get ctx before getrusage, so I use
enable_work_time_stat, and I missed something. I would like to use
enable_work_time_stat in both places in this function.

>
> >       seq_printf(m, "UserFiles:\t%u\n", ctx->file_table.data.nr);
> >       for (i = 0; i < ctx->file_table.data.nr; i++) {
> >               struct file *f = NULL;
> > diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
> > index 46d9141d772a..949dc7cba111 100644
> > --- a/io_uring/io_uring.h
> > +++ b/io_uring/io_uring.h
> > @@ -54,7 +54,8 @@
> >                       IORING_SETUP_REGISTERED_FD_ONLY |\
> >                       IORING_SETUP_NO_SQARRAY |\
> >                       IORING_SETUP_HYBRID_IOPOLL |\
> > -                     IORING_SETUP_CQE_MIXED)
> > +                     IORING_SETUP_CQE_MIXED |\
> > +                     IORING_SETUP_SQTHREAD_STATS)
> >
> >  #define IORING_ENTER_FLAGS (IORING_ENTER_GETEVENTS |\
> >                       IORING_ENTER_SQ_WAKEUP |\
> > diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
> > index a3f11349ce06..46bcd4854abc 100644
> > --- a/io_uring/sqpoll.c
> > +++ b/io_uring/sqpoll.c
> > @@ -161,6 +161,7 @@ static struct io_sq_data *io_get_sq_data(struct io_uring_params *p,
> >       mutex_init(&sqd->lock);
> >       init_waitqueue_head(&sqd->wait);
> >       init_completion(&sqd->exited);
> > +     sqd->enable_work_time_stat = false;
> >       return sqd;
> >  }
> >
> > @@ -317,7 +318,8 @@ static int io_sq_thread(void *data)
> >               }
> >
> >               cap_entries = !list_is_singular(&sqd->ctx_list);
> > -             getrusage(current, RUSAGE_SELF, &start);
> > +             if (sqd->enable_work_time_stat)
> > +                     getrusage(current, RUSAGE_SELF, &start);
> >               list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
> >                       int ret = __io_sq_thread(ctx, cap_entries);
> >
> > @@ -333,7 +335,8 @@ static int io_sq_thread(void *data)
> >
> >               if (sqt_spin || !time_after(jiffies, timeout)) {
> >                       if (sqt_spin) {
> > -                             io_sq_update_worktime(sqd, &start);
> > +                             if (sqd->enable_work_time_stat)
> > +                                     io_sq_update_worktime(sqd, &start);
> >                               timeout = jiffies + sqd->sq_thread_idle;
> >                       }
> >                       if (unlikely(need_resched())) {
> > @@ -445,7 +448,8 @@ __cold int io_sq_offload_create(struct io_ring_ctx *ctx,
> >                       ret = PTR_ERR(sqd);
> >                       goto err;
> >               }
> > -
> > +             if (ctx->flags & IORING_SETUP_SQTHREAD_STATS)
> > +                     sqd->enable_work_time_stat = true;
> >               ctx->sq_creds = get_current_cred();
> >               ctx->sq_data = sqd;
> >               ctx->sq_thread_idle = msecs_to_jiffies(p->sq_thread_idle);
> > diff --git a/io_uring/sqpoll.h b/io_uring/sqpoll.h
> > index b83dcdec9765..55f2e4d46d54 100644
> > --- a/io_uring/sqpoll.h
> > +++ b/io_uring/sqpoll.h
> > @@ -19,6 +19,7 @@ struct io_sq_data {
> >       u64                     work_time;
> >       unsigned long           state;
> >       struct completion       exited;
> > +     bool                    enable_work_time_stat;
> >  };
> >
> >  int io_sq_offload_create(struct io_ring_ctx *ctx, struct io_uring_params *p);
>
> --
> Gabriel Krisman Bertazi