Received-SPF: pass (google.com: domain of linux-kernel+bounces-13503-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <7967c7a9-3d17-44de-a170-2b5354460126@gmail.com>
Date: Sat, 30 Dec 2023 16:27:17 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6] io_uring: Statistics of the true utilization of sq
 threads.
Content-Language: en-US
To: Jens Axboe <axboe@kernel.dk>, Xiaobing Li <xiaobing.li@samsung.com>
Cc: 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,
 cliang01.li@samsung.com, xue01.he@samsung.com
References: <CGME20231225055252epcas5p43ae8016d329b160f688def7b4f9d4ddb@epcas5p4.samsung.com>
 <20231225054438.44581-1-xiaobing.li@samsung.com>
 <170360833542.1229482.7687326255574388809.b4-ty@kernel.dk>
From: Pavel Begunkov <asml.silence@gmail.com>
In-Reply-To: <170360833542.1229482.7687326255574388809.b4-ty@kernel.dk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 12/26/23 16:32, Jens Axboe wrote:
> 
> On Mon, 25 Dec 2023 13:44:38 +0800, Xiaobing Li wrote:
>> Count the running time and actual IO processing time of the sqpoll
>> thread, and output the statistical data to fdinfo.
>>
>> Variable description:
>> "work_time" in the code represents the sum of the jiffies of the sq
>> thread actually processing IO, that is, how many milliseconds it
>> actually takes to process IO. "total_time" represents the total time
>> that the sq thread has elapsed from the beginning of the loop to the
>> current time point, that is, how many milliseconds it has spent in
>> total.
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] io_uring: Statistics of the true utilization of sq threads.
>        commit: 9f7e5872eca81d7341e3ec222ebdc202ff536655

I don't believe the patch is near complete, there are still
pending question that the author ignored (see replies to
prev revisions).

Why it uses jiffies instead of some task run time?
Consequently, why it's fine to account irq time and other
preemption? (hint, it's not)

Why it can't be done with userspace and/or bpf? Why
can't it be estimated by checking and tracking
IORING_SQ_NEED_WAKEUP in userspace?

What's the use case in particular? Considering that
one of the previous revisions was uapi-less, something
is really fishy here. Again, it's a procfs file nobody
but a few would want to parse to use the feature.

Why it just keeps aggregating stats for the whole
life time of the ring? If the workload changes,
that would either totally screw the stats or would make
it too inert to be useful. That's especially relevant
for long running (days) processes. There should be a
way to reset it so it starts counting anew.


I say the patch has to be removed until all that is
figured, but otherwise I'll just leave a NACK for
history.

-- 
Pavel Begunkov