public inbox for [email protected]
 help / color / mirror / Atom feed
From: Xiaobing Li <[email protected]>
To: [email protected]
Cc: [email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected]
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: <[email protected]> (raw)
In-Reply-To: <[email protected]>

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

      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 \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /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