Re: Re: [PATCH] liburing: add script for statistics sqpoll running time.

[Xiaobing Li <xiaobing.li@samsung.com>]

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