On 2/18/24 06:00 AM, Jens Axboe wrote:
>And since your Signed-off-by is here, it also does not go into the
>commit message, which it must.
>
>> index 976e9500f651..18c6f4aa4a48 100644
>> --- a/io_uring/fdinfo.c
>> +++ b/io_uring/fdinfo.c
>> @@ -64,6 +64,7 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *f)
>> unsigned int sq_shift = 0;
>> unsigned int sq_entries, cq_entries;
>> int sq_pid = -1, sq_cpu = -1;
>> + u64 sq_total_time = 0, sq_work_time = 0;
>> bool has_lock;
>> unsigned int i;
>>
>> @@ -147,10 +148,17 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *f)
>>
>> sq_pid = sq->task_pid;
>> sq_cpu = sq->sq_cpu;
>> + struct rusage r;
>
>Here, and in one other spot, you're mixing variable declarations and
>code. Don't do that, they need to be top of that scope and before any
>code.
>
>> diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
>> index 65b5dbe3c850..9155fc0b5eee 100644
>> --- a/io_uring/sqpoll.c
>> +++ b/io_uring/sqpoll.c
>> @@ -251,6 +251,9 @@ static int io_sq_thread(void *data)
>> }
>>
>> cap_entries = !list_is_singular(&sqd->ctx_list);
>> + struct rusage start, end;
>> +
>> + getrusage(current, RUSAGE_SELF, &start);
>
>Ditto, move the variables to the top of the scope.
>
>> list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
>> int ret = __io_sq_thread(ctx, cap_entries);
>>
>> @@ -260,6 +263,11 @@ static int io_sq_thread(void *data)
>> if (io_run_task_work())
>> sqt_spin = true;
>>
>> + getrusage(current, RUSAGE_SELF, &end);
>> + if (sqt_spin == true)
>> + sqd->work_time += (end.ru_stime.tv_sec - start.ru_stime.tv_sec) *
>> + 1000000 + (end.ru_stime.tv_usec - start.ru_stime.tv_usec);
>> +
>
>and this should go in a helper instead. It's trivial code, but the way
>too long lines makes it hard to read. Compare the above to eg:
>
>static void io_sq_update_worktime(struct io_sq_data *sqd, struct rusage *start)
>{
> struct rusage end;
>
> getrusage(current, RUSAGE_SELF, &end);
> end.ru_stime.tv_sec -= start->ru_stime.tv_sec;
> end_ru_stime.tv_usec -= start->ru_stime.tv_usec;
>
> sqd->work_time += end.ru_stime.tv_usec + end.ru_stime.tv_sec * 1000000;
>}
>
>which is so much nicer to look at.
>
>We're already doing an sqt_spin == true check right below, here:
>
>> if (sqt_spin || !time_after(jiffies, timeout)) {
>> if (sqt_spin)
>> timeout = jiffies + sqd->sq_thread_idle;
>
>why not just put io_sq_update_worktime(sqd, &start); inside this check?
>
ok, I got it, I will send out a v9.
--
Xiaobing Li
