lkml.org 
[lkml]   [2017]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3] printk: Add console owner and waiter logic to loadbalance console writes
    From
    Date
    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@goodmis.org> 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

    \
     
     \ /
      Last update: 2017-11-04 09:34    [W:2.185 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site