John Hubbard wrote: > On 11/03/2017 02:46 PM, John Hubbard wrote: > > On 11/03/2017 04:54 AM, Steven Rostedt wrote: > >> On Fri, 3 Nov 2017 07:21:21 -0400 > >> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: > [...] > >> > >> I'll condense the patch to show what I mean: > >> > >> To become a waiter, a task must do the following: > >> > >> + printk_safe_enter_irqsave(flags); > >> + > >> + raw_spin_lock(&console_owner_lock); > >> + owner = READ_ONCE(console_owner); > >> + waiter = READ_ONCE(console_waiter); When CPU0 is writing to consoles after "console_owner = current;", what prevents from CPU1 and CPU2 concurrently reached this line from seeing waiter == false && owner != NULL && owner != current (which will concurrently set console_waiter = true and spin = true) without using atomic instructions? > >> + if (!waiter && owner && owner != current) { > >> + WRITE_ONCE(console_waiter, true); > >> + spin = true; > >> + } > >> + raw_spin_unlock(&console_owner_lock); > >> > >> > >> The new waiter gets set only if there isn't already a waiter *and* > >> there is an owner that is not current (and with the printk_safe_enter I > >> don't think that is even needed). > >> > >> + while (!READ_ONCE(console_waiter)) > >> + cpu_relax(); > >> > >> The spin is outside the spin lock. But only the owner can clear it. > >> > >> Now the owner is doing a loop of this (with interrupts disabled) > >> > >> + raw_spin_lock(&console_owner_lock); > >> + console_owner = current; > >> + raw_spin_unlock(&console_owner_lock); > >> > >> Write to consoles. > >> > >> + raw_spin_lock(&console_owner_lock); > >> + waiter = READ_ONCE(console_waiter); > >> + console_owner = NULL; > >> + raw_spin_unlock(&console_owner_lock); > >> > >> + if (waiter) > >> + break; > >> > >> At this moment console_owner is NULL, and no new waiters can happen. > >> The next owner will be the waiter that is spinning. > >> > >> + if (waiter) { > >> + WRITE_ONCE(console_waiter, false); > >> > >> There is no possibility of another task sneaking in and becoming a > >> waiter at this moment. The console_owner was cleared under spin lock, > >> and a waiter is only set under the same spin lock if owner is set. > >> There will be no new owner sneaking in because to become the owner, you > >> must have the console lock. Since it is never released between the time > >> the owner clears console_waiter and the waiter takes the console lock, > >> there is no race. > > > > Yes, you are right of course. That does close the window. Sorry about > > missing that point. > > > > I'll try to quickly put together a small patch on top of this, that > > shows a simplification, to just use an atomic compare and swap between a > > global atomic value, and a local (on the stack) flag value, just in > > case that is of interest. > > > > thanks > > john h > > Just a follow-up: I was unable to simplify this; the atomic compare-and-swap > approach merely made it different, rather than smaller or simpler. Why no need to use [cmp]xchg() approach? > > So, after spending a fair amount of time with the patch, it looks good to me, > for whatever that's worth. :) Thanks again for explaining the locking details. > > thanks > john h > > > > >> > >> -- Steve -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>