On Fri, 8 Mar 2024 12:39:10 -0800 Linus Torvalds <torva...@linux-foundation.org> wrote:
> On Fri, 8 Mar 2024 at 10:38, Steven Rostedt <rost...@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