Re: [PATCH 0/6] tracing/ring-buffer: Fix wakeup of ring buffer waiters
On Fri, 8 Mar 2024 13:41:59 -0800 Linus Torvalds wrote: > On Fri, 8 Mar 2024 at 13:39, Linus Torvalds > wrote: > > > > So the above "complexity" is *literally* just changing the > > > > (new = atomic_read_acquire(>seq)) != old > > > > condition to > > > > should_exit || > > (new = atomic_read_acquire(>seq)) != old > > .. and obviously you'll need to add the exit condition to the actual > "deal with events" loop too. I haven't had a chance to rework this part of the patches, but I have some other fixes to push to you from earlier this week, and I think the first three patches of this series are also fine. As the loop in ring_buffer_wait() isn't needed, and patch 2 and 3 are trivial bugs. I'll send you a pull request for that work and I'll work on this code later. -- Steve
Re: [PATCH 0/6] tracing/ring-buffer: Fix wakeup of ring buffer waiters
On Fri, 8 Mar 2024 at 13:39, Linus Torvalds wrote: > > So the above "complexity" is *literally* just changing the > > (new = atomic_read_acquire(>seq)) != old > > condition to > > should_exit || > (new = atomic_read_acquire(>seq)) != old .. and obviously you'll need to add the exit condition to the actual "deal with events" loop too. Linus
Re: [PATCH 0/6] tracing/ring-buffer: Fix wakeup of ring buffer waiters
On Fri, 8 Mar 2024 at 13:33, Steven Rostedt wrote: > > 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. But that's the beauty of just using the wait_event() model. Just add that "exit" condition to the condition. So the above "complexity" is *literally* just changing the (new = atomic_read_acquire(>seq)) != old condition to should_exit || (new = atomic_read_acquire(>seq)) != old (replace "should_exit" with whatever that condition is, of course) and the wait_event() logic will take care of the rest. Linus
Re: [PATCH 0/6] tracing/ring-buffer: Fix wakeup of ring buffer waiters
On Fri, 8 Mar 2024 12:39:10 -0800
Linus Torvalds wrote:
> On Fri, 8 Mar 2024 at 10:38, Steven Rostedt 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(>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(>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(>seq);
> wake_up(>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
Re: [PATCH 0/6] tracing/ring-buffer: Fix wakeup of ring buffer waiters
On Fri, 8 Mar 2024 at 10:38, Steven Rostedt 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.
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(>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(>seq)) != old);
return new;
}
static inline void add_seq_event(struct xyz *my)
{
atomic_fetch_inc_release(>seq);
wake_up(>seq_wait);
}
Note how you don't need any new locks, and note how "wait_event()"
will do all the required optimistic stuff for you (ie it will check
that "has seq changed" before even bothering to add itself to the wait
queue etc).
So the above is not only short and sweet, it generates fairly good
code too, and doesn't it look really simple and fairly understandable?
And - AS ALWAYS - the above isn't actually tested in any way, shape or form.
Linus
