linux-kernel - Re: [PATCH v3] printk: Add console owner and waiter logic to loadbalance console writes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 4 Nov 2017 17:32:52 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     jhubbard@...dia.com, rostedt@...dmis.org
Cc:     vbabka@...e.cz, linux-kernel@...r.kernel.org, peterz@...radead.org,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        xiyou.wangcong@...il.com, dave.hansen@...el.com,
        hannes@...xchg.org, mgorman@...e.de, mhocko@...nel.org,
        pmladek@...e.com, sergey.senozhatsky@...il.com,
        yuwang.yuwang@...baba-inc.com, torvalds@...ux-foundation.org,
        jack@...e.cz, mathieu.desnoyers@...icios.com
Subject: Re: [PATCH v3] printk: Add console owner and waiter logic to loadbalance console writes

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@...dmis.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