On 12/30/23 9:27 AM, Pavel Begunkov wrote: > 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). We can drop and defer, that's not an issue. It's still sitting top of branch. Can you elaborate on the pending questions? > 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) Yeah that's a good point, might be better to use task run time. Jiffies is also an annoying metric to expose, as you'd need to then get the tick rate as well. Though I suspect the ratio is the interesting bit here. > 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? Asking people to integrate bpf for this is a bit silly imho. Tracking NEED_WAKEUP is also quite cumbersome and would most likely be higher overhead as well. > 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. I brought this up earlier too, fdinfo is not a great API. For anything, really. > 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 don't see a problem there with the current revision, as the app can just remember the previous two numbers and do the appropriate math "since last time". > I say the patch has to be removed until all that is > figured, but otherwise I'll just leave a NACK for > history. That's fine, I can drop it for now and we can get the rest addressed. -- Jens Axboe
