Re: [PATCH 0/6] tracing/ring-buffer: Fix wakeup of ring buffer waiters

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 8 Mar 2024 12:39:10 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, 8 Mar 2024 at 10:38, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > A patch was sent to "fix" the wait_index variable that is used to help with
> > waking of waiters on the ring buffer. The patch was rejected, but I started
> > looking at associated code. Discussing it on IRC with Mathieu Desnoyers
> > we discovered a design flaw.  
> 
> Honestly, all of this seems excessively complicated.
> 
> And your new locking shouldn't be necessary if you just do things much
> more simply.

You mean to replace the wait_woken_*() code (that has the new locking)?

> 
> Here's what I *think* you should do:
> 
>   struct xyz {
>         ...
>         atomic_t seq;
>         struct wait_queue_head seq_wait;
>         ...
>   };
> 
> with the consumer doing something very simple like this:
> 
>         int seq = atomic_read_acquire(&my->seq);
>         for (;;) {
>                 .. consume outstanding events ..
>                 seq = wait_for_seq_change(seq, my);
>         }
> 
> and the producer being similarly trivial, just having a
> "add_seq_event()" at the end:
> 
>         ... add whatever event ..
>         add_seq_event(my);
> 
> And the helper functions for this are really darn simple:
> 
>   static inline int wait_for_seq_change(int old, struct xyz *my)
>   {
>         int new;
>         wait_event(my->seq_wait,
>                 (new = atomic_read_acquire(&my->seq)) != old);

But the index isn't the only condition for it to wake up to. If the file is
closing, it want's to know that too. Or if it's just being kicked out to
consume whatever is there and ignore the watermark.

>         return new;
>   }
> 
>   static inline void add_seq_event(struct xyz *my)
>   {
>         atomic_fetch_inc_release(&my->seq);
>         wake_up(&my->seq_wait);
>   }

But it's not only the producer that does the wakeup. That part wasn't
broken.

The broken part is a third party that comes along and wants to wake up the
consumer and tell them to just consume what's there and exit.

There's two layers:

1) the ring buffer has the above simple producer / consumer.
   Where the wake ups can happen at the point of where the buffer has
   the amount filled that the consumer wants to start consuming with.

2) The tracing layer; Here on close of a file, the consumers need to be
   woken up and not wait again. And just take whatever was there to finish
   reading.

   There's also another case that the ioctl() just kicks the current
   readers out, but doesn't care about new readers.

I'm not sure how the seq can handle both there being enough data to wake up
the consumer and the case that another task just wants the consume to wake
up and ignore the watermark.

The wake_woken_*() code was only for the second part (to wake up consumers
and tell them to no longer wait for the producer), and had nothing to do
with the produce/consumer part.

-- Steve