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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 19 Feb 2024 11:12:32 +0800
From: Xiaobing Li <xiaobing.li@...sung.com>
To: axboe@...nel.dk
Cc: asml.silence@...il.com, linux-kernel@...r.kernel.org,
	io-uring@...r.kernel.org, kun.dou@...sung.com, peiwei.li@...sung.com,
	joshi.k@...sung.com, kundan.kumar@...sung.com, wenwen.chen@...sung.com,
	ruyi.zhang@...sung.com, xiaobing.li@...sung.com
Subject: Re: Re: [PATCH] liburing: add script for statistics sqpoll running
 time.

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