Re: [PATCH v2 RESEND] io_uring/fdinfo: add timeout_list to fdinfo

[Jens Axboe]

On Sat, Oct 12, 2024 at 3:30?AM Ruyi Zhang <ruyi.zhang@xxxxxxxxxxx> wrote:
>
> ---
> On 2024-10-10 15:35 Pavel Begunkov wrote:
> >> Two questions:
> >>
> >> 1. I agree with you, we shouldn't walk a potentially very
> >> long list under spinlock. but i can't find any other way
> >> to get all the timeout
>
> > If only it's just under the spin, but with disabled irqs...
>
> >> information than to walk the timeout_list. Do you have any
> >> good ideas?
>
> > In the long run it'd be great to replace the spinlock
> > with a mutex, i.e. just ->uring_lock, but that would might be
> > a bit involving as need to move handling to the task context.
>
>  Yes, it makes more sense to replace spin_lock, but that would
>  require other related logic to be modified, and I don't think
>  it's wise to do that for the sake of a piece of debugging
>  information.
>
> >> 2. I also agree seq_printf heavier, if we use
> >> seq_put_decimal_ull and seq_puts to concatenate strings,
> >> I haven't tested whether it's more efficient or not, but
> >> the code is certainly not as readable as the former. It's
> >> also possible that I don't fully understand what you mean
> >> and want to hear your opinion.
>
> > I don't think there is any difference, it'd be a matter of
> > doubling the number of in flight timeouts to achieve same
> > timings. Tell me, do you really have a good case where you
> > need that (pretty verbose)? Why not drgn / bpftrace it out
> > of the kernel instead?
>
>  Of course, this information is available through existing tools.
>  But I think that most of the io_uring metadata has been exported
>  from the fdinfo file, and the purpose of adding the timeout
>  information is the same as before, easier to use. This way,
>  I don't have to write additional scripts to get all kinds of data.
>
>  And as far as I know, the io_uring_show_fdinfo function is
>  only called once when the user is viewing the
>  /proc/xxx/fdinfo/x file once. I don't think we normally need to
>  look at this file as often, and only look at it when the program
>  is abnormal, and the timeout_list is very long in the extreme case,
>  so I think the performance impact of adding this code is limited.

I do think it's useful, sometimes the only thing you have to poke at
after-the-fact is the fdinfo information. At the same time, would it be
more useful to dump _some_ of the info, even if we can't get all of it?
Would not be too hard to just stop dumping if need_resched() is set, and
even note that - you can always retry, as this info is generally grabbed
from the console anyway, not programmatically. That avoids the worst
possible scenario, which is a malicious setup with a shit ton of pending
timers, while still allowing it to be useful for a normal setup. And
this patch could just do that, rather than attempt to re-architect how
the timers are tracked and which locking it uses.

-- 
Jens Axboe