From: Xiaobing Li <xiaobing.li@samsung.com>
To: axboe@kernel.dk
Cc: asml.silence@gmail.com, linux-kernel@vger.kernel.org,
io-uring@vger.kernel.org, kun.dou@samsung.com,
peiwei.li@samsung.com, joshi.k@samsung.com,
kundan.kumar@samsung.com, wenwen.chen@samsung.com,
ruyi.zhang@samsung.com, xiaobing.li@samsung.com
Subject: Re: Re: [PATCH] liburing: add script for statistics sqpoll running time.
Date: Mon, 19 Feb 2024 11:12:32 +0800 [thread overview]
Message-ID: <20240219031232.203025-1-xiaobing.li@samsung.com> (raw)
In-Reply-To: <522c03d9-a8ba-459d-9f7c-dfbf461dcf6b@kernel.dk>
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
prev parent reply other threads:[~2024-02-19 7:22 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20240206024726epcas5p1d90e29244e62ede67813da5fcd582151@epcas5p1.samsung.com>
2024-02-06 2:39 ` [PATCH v8] io_uring: Statistics of the true utilization of sq threads Xiaobing Li
[not found] ` <CGME20240218055523epcas5p390fe990f970cf2b9b1f96edd5d7bc9b5@epcas5p3.samsung.com>
2024-02-18 5:55 ` Xiaobing Li
2024-02-18 12:45 ` Jens Axboe
2024-02-18 12:55 ` Jens Axboe
2024-02-18 13:00 ` Jens Axboe
[not found] ` <CGME20240219031238epcas5p3aa330855093314a2c5768cf83971599c@epcas5p3.samsung.com>
2024-02-19 3:12 ` Xiaobing Li [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240219031232.203025-1-xiaobing.li@samsung.com \
--to=xiaobing.li@samsung.com \
--cc=asml.silence@gmail.com \
--cc=axboe@kernel.dk \
--cc=io-uring@vger.kernel.org \
--cc=joshi.k@samsung.com \
--cc=kun.dou@samsung.com \
--cc=kundan.kumar@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=peiwei.li@samsung.com \
--cc=ruyi.zhang@samsung.com \
--cc=wenwen.chen@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).