Message-ID: <20170701015043.GA889@tigerII.localdomain>
Date:   Sat, 1 Jul 2017 10:50:43 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Jan Kara <jack@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Eric Biederman <ebiederm@...ssion.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>, Pavel Machek <pavel@....cz>,
        Andreas Mohr <andi@...as.de>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread

On (06/30/17 10:45), Steven Rostedt wrote:
> On Fri, 30 Jun 2017 23:28:51 +0900
> Sergey Senozhatsky <sergey.senozhatsky.work@...il.com> wrote:
> 
> > I guess the question was, a knob that would determine what happens after
> > current wakes up printk_kthread -- does it stay in console_unlock() and
> > wait for new console_sem owner, printing the messages in the meantime,
> > or goes all in and expects printk_kthread or anything else to lock
> > console_sem at some point and flush the remaining messages. is that
> > correct? we can do this (well, I'm absolutely not in position to say
> > "we can't do this" :) ). I guess the sort of a problem we have now is
> > that we can't guarantee that wake_up() will actually wake_up printk_kthread.
> > but if user requests it, then well... it might be easier to adjust watchdog
> > timeout value ;) just kidding. or may be I misunderstood your question.
> 
> Actually, what I was thinking of was just setting a hard limit at how
> much printk() can write without pushing it off to a helper thread. Kind
> of like how softirq works. If there's too much work, it then pushes off
> the rest to ksoftirqd.

oh, OK. this is exactly the way it works now. we've got a sysfs knob

 echo 150 > /sys/module/printk/parameters/atomic_print_limit

which sets the limit on the number of lines we print in direct (old
printk mode). as soon as current prints more than atomic_print_limit
lines it wakes up printk_kthread.

the default value for atomic_print_limit is 0, which means no limit.
IOW, no offloading at all, aka the old behaviour.

> If we have a kprintd, have a knob that says how much printk can write
> under console_lock, and after it hits a limit (default being no limit),
> then to simply wake up kprintd to do the rest. This should allow those
> that care to limit how much time printk can stop a CPU by, with the
> knowledge that if the kernel were to crash, there's a good chance that
> the necessary debug info will not be printed.

yes. this is how it's done in the patch set. with some additional tricks.

> Now as I have said, the default would be to keep things as is, and let
> printk take up as much of the CPU it needs till it gets all the pending
> prints out.

yes. this is the way it's done in the patch set.

> I would also like (in my wish list), a mechanism, that when an oops in
> progress happens, to set an emergency flag. If something already has
> console_lock, then we can let it do what it does today, and have the
> critical thread write its data to the logbuf, and let the current
> printer hopefully output it (I think that's what we do today). But if
> there's no holder of console lock, the critical thread takes control
> and dumps the buffer to get everything out. Once that emergency flag is
> set, there will be no more offloading. All prints will be in critical
> mode, and try to get the data out if possible.

yes. this is what we are trying to do in the patch set.

when we decide if we need to offload printing, we call this guy first

static inline bool printk_offloading_enabled(void)
{
	return atomic_print_limit &&
		printk_enforce_emergency == 0 &&
		printk_kthread &&
		atomic_read(&printk_emergency) == 0;
}

which checks printk internal emergency flags, etc.. for example,
printk_enforce_emergency is set automatically when we printk()
LOGLEVEL_EMERG message the first time.


then we check the system wide flags

	if (system_state != SYSTEM_RUNNING || oops_in_progress)
		return false;


if all checks pass (no oops, no emergency, nothing) then we continue
with the offloading thing. increment the counter which tracks the number
of lines printed already printed and offload printing if that counter
goes above user defined atomic_print_limit threshold.

otherwise, we just stay and continue printing, because we are in oops or
emergency, or whatever.

> While I'm giving you my Christmas list, I would also like a way to have
> an NMI safe lock in printk (like Peter Zijlstra and I have patches for
> with early-printk). Which checks if the current CPU owns the spin lock,
> and if it doesn't it is safe to take the lock even in NMI mode. But if
> the current CPU has the lock, then the NMI handle does the printing
> without locking, not caring if it screws up the thread that is currently
> printing.
> 
> Of course we need to be careful about various consoles that have their
> own locks. Maybe make an NMI lock primitive that all consoles use?
> 
> This is just wishful thinking, but hopefully we can come up with
> something that can solve all issues reasonably.

ok, the NMI thing is not in the current patch set. we can do it a bit
later. the early-printk patch set is also interesting. and I hope that
Petr will take a look on it once we settle down the offloading.

	-ss

Powered by blists - more mailing lists