Re: [PATCH next v1 2/3] printk: remove safe buffers
On 2021-03-29, John Ogness wrote: >> Will you call console write() callback with irq enabled from the >> kthread? > > No. That defeats the fundamental purpose of this entire rework > excercise. ;-) Sorry, I misread your question. The answer is "yes". We want to avoid a local_irq_save() when calling into console->write(). John Ogness
Re: [PATCH next v1 2/3] printk: remove safe buffers
On 2021-03-29, Petr Mladek wrote: > I wonder if some console drivers rely on the fact that the write() > callback is called with interrupts disabled. > > IMHO, it would be a bug when any write() callback expects that > callers disabled the interrupts. Agreed. > Do you plan to remove the console-spinning stuff after offloading > consoles to the kthreads? Yes. Although a similar concept will be introduced to allow the threaded printers and the atomic consoles to compete. > Will you call console write() callback with irq enabled from the > kthread? No. That defeats the fundamental purpose of this entire rework excercise. ;-) > Anyway, we should at least add a comment why the interrupts are > disabled. I decided to move the local_irq_save/restore inside the console-spinning functions and added a comment for v2. John Ogness
Re: [PATCH next v1 2/3] printk: remove safe buffers
On Fri 2021-03-26 12:12:37, John Ogness wrote:
> On 2021-03-23, Petr Mladek wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> -
> >>if (seq != prb_next_seq(_rb_static)) {
> >>pr_err("dropped %llu messages\n",
> >> prb_next_seq(_rb_static) - seq);
> >> @@ -2666,7 +2631,6 @@ void console_unlock(void)
> >>size_t ext_len = 0;
> >>size_t len;
> >>
> >> - printk_safe_enter_irqsave(flags);
> >> skip:
> >>if (!prb_read_valid(prb, console_seq, ))
> >>break;
> >> @@ -2711,6 +2675,8 @@ void console_unlock(void)
> >>printk_time);
> >>console_seq++;
> >>
> >> + printk_safe_enter_irqsave(flags);
> >
> > What is the purpose of the printk_safe context here, please?
>
> console_lock_spinning_enable() needs to be called with interrupts
> disabled. I should have just used local_irq_save().
>
> I could add local_irq_save() to console_lock_spinning_enable() and
> restore them at the end of console_lock_spinning_disable_and_check(),
> but then I would need to add a @flags argument to both functions. I
> think it is simpler to just do the disable/enable from the caller,
> console_unlock().
I see. I have missed it that all this code have to be called with
interrupts disabled.
OK, it is a must-to-have because of the spinning. But I wonder if some
console drivers rely on the fact that the write() callback is
called with interrupts disabled.
IMHO, it would be a bug when any write() callback expects that
callers disabled the interrupts.
Do you plan to remove the console-spinning stuff after offloading
consoles to the kthreads?
Will you call console write() callback with irq enabled from
the kthread?
Anyway, we should at least add a comment why the interrupts are
disabled.
> BTW, I could not find any sane way of disabling interrupts via a
> raw_spin_lock_irqsave() of @console_owner_lock because of the how it is
> used with lockdep. In particular for
> console_lock_spinning_disable_and_check().
I see. IMHO, we would need to explicitly call local_irq_save()/restore()
if we moved them to console_lock_spinning_enable()/disable_and_check().
I mean to do:
static void console_lock_spinning_enable(unsigned long *flags)
{
local_irq_save(*flags);
raw_spin_lock(_owner_lock);
console_owner = current;
raw_spin_unlock(_owner_lock);
/* The waiter may spin on us after setting console_owner */
spin_acquire(_owner_dep_map, 0, 0, _THIS_IP_);
}
...
Best Regards,
Petr
Re: [PATCH next v1 2/3] printk: remove safe buffers
On 2021-03-23, Petr Mladek wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1142,8 +1126,6 @@ void __init setup_log_buf(int early)
>> new_descs, ilog2(new_descs_count),
>> new_infos);
>>
>> -printk_safe_enter_irqsave(flags);
>> -
>> log_buf_len = new_log_buf_len;
>> log_buf = new_log_buf;
>> new_log_buf_len = 0;
>> @@ -1159,8 +1141,6 @@ void __init setup_log_buf(int early)
>> */
>> prb = _rb_dynamic;
>>
>> -printk_safe_exit_irqrestore(flags);
>
> This will allow to add new messages from the IRQ context when we
> are copying them to the new buffer. They might get lost in
> the small race window.
>
> Also the messages from NMI might get lost because they are not
> longer stored in the per-CPU buffer.
>
> A possible solution might be to do something like this:
>
> prb_for_each_record(0, _rb_static, seq, )
> free -= add_to_rb(_rb_dynamic, );
>
> prb = _rb_dynamic;
>
> /*
>* Copy the remaining messages that might have appeared
>* from IRQ or NMI context after we ended copying and
>* before we switched the buffers. They must be finalized
>* because only one CPU is up at this stage.
>*/
> prb_for_each_record(seq, _rb_static, seq, )
> free -= add_to_rb(_rb_dynamic, );
OK. I'll probably rework it some and combine it with the "dropped" test
so that we can identify if messages were dropped during the transition
(because of static ringbuffer overrun).
>> -
>> if (seq != prb_next_seq(_rb_static)) {
>> pr_err("dropped %llu messages\n",
>> prb_next_seq(_rb_static) - seq);
>> @@ -2666,7 +2631,6 @@ void console_unlock(void)
>> size_t ext_len = 0;
>> size_t len;
>>
>> -printk_safe_enter_irqsave(flags);
>> skip:
>> if (!prb_read_valid(prb, console_seq, ))
>> break;
>> @@ -2711,6 +2675,8 @@ void console_unlock(void)
>> printk_time);
>> console_seq++;
>>
>> +printk_safe_enter_irqsave(flags);
>
> What is the purpose of the printk_safe context here, please?
console_lock_spinning_enable() needs to be called with interrupts
disabled. I should have just used local_irq_save().
I could add local_irq_save() to console_lock_spinning_enable() and
restore them at the end of console_lock_spinning_disable_and_check(),
but then I would need to add a @flags argument to both functions. I
think it is simpler to just do the disable/enable from the caller,
console_unlock().
BTW, I could not find any sane way of disabling interrupts via a
raw_spin_lock_irqsave() of @console_owner_lock because of the how it is
used with lockdep. In particular for
console_lock_spinning_disable_and_check().
John Ogness
Re: [PATCH next v1 2/3] printk: remove safe buffers
On Wed 2021-03-17 00:33:25, John Ogness wrote:
> With @logbuf_lock removed, the high level printk functions for
> storing messages are lockless. Messages can be stored from any
> context, so there is no need for the NMI and safe buffers anymore.
> Remove the NMI and safe buffers.
>
> Although the safe buffers are removed, the NMI and safe context
> tracking is still in place. In these contexts, store the message
> immediately but still use irq_work to defer the console printing.
>
> Since printk recursion tracking is in place, safe context tracking
> for most of printk is not needed. Remove it. Only safe context
> tracking relating to the console lock is left in place. This is
> because the console lock is needed for the actual printing.
I have two more questions after actually checking the entire patch.
See below.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1084,7 +1069,6 @@ void __init setup_log_buf(int early)
> struct printk_record r;
> size_t new_descs_size;
> size_t new_infos_size;
> - unsigned long flags;
> char *new_log_buf;
> unsigned int free;
> u64 seq;
> @@ -1142,8 +1126,6 @@ void __init setup_log_buf(int early)
>new_descs, ilog2(new_descs_count),
>new_infos);
>
> - printk_safe_enter_irqsave(flags);
> -
> log_buf_len = new_log_buf_len;
> log_buf = new_log_buf;
> new_log_buf_len = 0;
> @@ -1159,8 +1141,6 @@ void __init setup_log_buf(int early)
>*/
> prb = _rb_dynamic;
>
> - printk_safe_exit_irqrestore(flags);
This will allow to add new messages from the IRQ context when we
are copying them to the new buffer. They might get lost in
the small race window.
Also the messages from NMI might get lost because they are not
longer stored in the per-CPU buffer.
A possible solution might be to do something like this:
prb_for_each_record(0, _rb_static, seq, )
free -= add_to_rb(_rb_dynamic, );
prb = _rb_dynamic;
/*
* Copy the remaining messages that might have appeared
* from IRQ or NMI context after we ended copying and
* before we switched the buffers. They must be finalized
* because only one CPU is up at this stage.
*/
prb_for_each_record(seq, _rb_static, seq, )
free -= add_to_rb(_rb_dynamic, );
> -
> if (seq != prb_next_seq(_rb_static)) {
> pr_err("dropped %llu messages\n",
> prb_next_seq(_rb_static) - seq);
> @@ -2666,7 +2631,6 @@ void console_unlock(void)
> size_t ext_len = 0;
> size_t len;
>
> - printk_safe_enter_irqsave(flags);
> skip:
> if (!prb_read_valid(prb, console_seq, ))
> break;
> @@ -2711,6 +2675,8 @@ void console_unlock(void)
> printk_time);
> console_seq++;
>
> + printk_safe_enter_irqsave(flags);
What is the purpose of the printk_safe context here, please?
I guess that you wanted to prevent calling console drivers
recursively. But it is already serialized by console_lock().
IMHO, the only risk is when manipulating console_sem->lock
or console_owner_lock. But they are already guarded by
printk_safe context, for example, in console_lock() or
console_lock_spinning_enable().
Do I miss something, please?
> +
> /*
>* While actively printing out messages, if another printk()
>* were to occur on another CPU, it may wait for this one to
> @@ -2745,8 +2711,6 @@ void console_unlock(void)
>* flush, no worries.
>*/
> retry = prb_read_valid(prb, console_seq, NULL);
> - printk_safe_exit_irqrestore(flags);
> -
> if (retry && console_trylock())
> goto again;
> }
Heh, all these patches feels like stripping printk of an armour. I hope
that we trained it enough to be flexible and avoid any damage.
Best Regards,
Petr
Re: [PATCH next v1 2/3] printk: remove safe buffers
On Mon 2021-03-22 22:58:47, John Ogness wrote:
> On 2021-03-22, Petr Mladek wrote:
> > On Mon 2021-03-22 12:16:15, John Ogness wrote:
> >> On 2021-03-21, Sergey Senozhatsky wrote:
> >> >> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt,
> >> >> va_list args)
> >> >> * Use the main logbuf even in NMI. But avoid calling console
> >> >> * drivers that might have their own locks.
> >> >> */
> >> >> - if ((this_cpu_read(printk_context) &
> >> >> PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
> >> >> + if (this_cpu_read(printk_context) &
> >> >> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
> >> >> +PRINTK_NMI_CONTEXT_MASK |
> >> >> +PRINTK_SAFE_CONTEXT_MASK)) {
> >> >
> >> But I suppose I could switch
> >> the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that
> >> PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a
> >> 4th patch of the series.
> >
> > Yes, please unify the PRINTK_NMI_CONTEXT. One is enough.
>
> Agreed. (But I'll go even further. See below.)
>
> > I wonder if it would make sense to go even further at this stage.
> > What is possible?
> >
> > 1. We could get rid of printk_nmi_enter()/exit() and
> >PRINTK_NMI_CONTEXT completely already now. It is enough
> >to check in_nmi() in printk_func().
> >
>
> Agreed. in_nmi() within vprintk_emit() is enough to detect if the
> console code should be skipped:
>
> if (!in_sched && !in_nmi()) {
> ...
> }
Well, we also need to make sure that the irq work is scheduled to
call console later. We should keep this dicision in
printk_func(). I mean to replace the current
if (this_cpu_read(printk_context) &
(PRINTK_NMI_DIRECT_CONTEXT_MASK |
PRINTK_NMI_CONTEXT_MASK |
PRINTK_SAFE_CONTEXT_MASK)) {
with
/*
* Avoid calling console drivers in recursive printk()
* and in NMI context.
*/
if (this_cpu_read(printk_context) || in_nmi() {
That said, I am not sure how this fits your further rework.
I do not want to complicate it too much.
I am just afraid that the discussion about console rework might
take some time. And this would remove some complexity before we
started the more complicated or controversial changes.
> > 2. I thought about unifying printk_safe_enter()/exit() and
> >printk_enter()/exit(). They both count recursion with
> >IRQs disabled, have similar name. But they are used
> >different way.
> >
> >But better might be to rename printk_safe_enter()/exit() to
> >console_enter()/exit() or to printk_deferred_enter()/exit().
> >It would make more clear what it does now. And it might help
> >to better distinguish it from the new printk_enter()/exit().
> >
> >I am not sure if it is worth it.
>
> I am also not sure if it is worth the extra "noise" just to give the
> function a more appropriate name. The plan is to remove it completely
> soon anyway. My vote is to leave the name as it is.
OK, let's keep printk_safe() name. It was just an idea. I wrote it
primary to sort my thoughts.
Best Regards,
Petr
Re: [PATCH next v1 2/3] printk: remove safe buffers
On 2021-03-22, Petr Mladek wrote:
> On Mon 2021-03-22 12:16:15, John Ogness wrote:
>> On 2021-03-21, Sergey Senozhatsky wrote:
>> >> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt,
>> >> va_list args)
>> >>* Use the main logbuf even in NMI. But avoid calling console
>> >>* drivers that might have their own locks.
>> >>*/
>> >> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
>> >> + if (this_cpu_read(printk_context) &
>> >> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
>> >> + PRINTK_NMI_CONTEXT_MASK |
>> >> + PRINTK_SAFE_CONTEXT_MASK)) {
>> >
>> > Do we need printk_nmi_direct_enter/exit() and
>> > PRINTK_NMI_DIRECT_CONTEXT_MASK? Seems like all printk_safe() paths
>> > are now DIRECT - we store messages to the prb, but don't call console
>> > drivers.
>>
>> I was planning on waiting until the kthreads are introduced, in which
>> case printk_safe.c is completely removed.
>
> You want to keep printk_safe() context because it prevents calling
> consoles even in normal context. Namely, it prevents deadlock by
> recursively taking, for example, sem->lock in console_lock() or
> console_owner_lock in console_trylock_spinning(). Am I right?
Correct.
>> But I suppose I could switch
>> the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that
>> PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a
>> 4th patch of the series.
>
> Yes, please unify the PRINTK_NMI_CONTEXT. One is enough.
Agreed. (But I'll go even further. See below.)
> I wonder if it would make sense to go even further at this stage.
> There will still be 4 contexts that modify the printk behavior after
> this patchset:
>
> + printk_count set by printk_enter()/exit()
> + prevents: infinite recursion
> + context: any context
> + action: skips entire printk at 3rd recursion level
>
> + prink_context set by printk_safe_enter()/exit()
> + prevents: dead lock caused by recursion into some
> console code in any context
> + context: any
> + action: skips console call at 1st recursion level
Technically, at this point printk_safe_enter() behavior is identical to
printk_nmi_enter(). Namely, prevent any recursive printk calls from
calling into the console code.
> + printk_context set by printk_nmi_enter()/exit()
> + prevents: dead lock caused by any console lock recursion
> + context: NMI
> + action: skips console calls at 0th recursion level
>
> + kdb_trap_printk
> + redirects printk() to kdb_printk() in kdb context
>
>
> What is possible?
>
> 1. We could get rid of printk_nmi_enter()/exit() and
>PRINTK_NMI_CONTEXT completely already now. It is enough
>to check in_nmi() in printk_func().
>
>printk_nmi_enter() was added by the commit 42a0bb3f71383b457a7db362
>("printk/nmi: generic solution for safe printk in NMI"). It was
>really needed to modify @printk_func pointer.
>
>We did not remove it later when printk_function became a real
>function. The idea was to track all printk contexts in a single
>variable. But we never added kdb context.
>
>It might make sense to remove it now. Peter Zijstra would be happy.
>There already were some churns with tracking printk_context in NMI.
>For example, see
>https://lore.kernel.org/r/20200219150744.428764...@infradead.org
>
>IMHO, it does not make sense to wait until the entire console-stuff
>rework is done in this case.
Agreed. in_nmi() within vprintk_emit() is enough to detect if the
console code should be skipped:
if (!in_sched && !in_nmi()) {
...
}
> 2. I thought about unifying printk_safe_enter()/exit() and
>printk_enter()/exit(). They both count recursion with
>IRQs disabled, have similar name. But they are used
>different way.
>
>But better might be to rename printk_safe_enter()/exit() to
>console_enter()/exit() or to printk_deferred_enter()/exit().
>It would make more clear what it does now. And it might help
>to better distinguish it from the new printk_enter()/exit().
>
>This patchset actually splits the original printk_safe()
>functionality into two:
>
>+ printk_count prevents infinite recursion
>+ printk_deferred_enter() deffers console handling.
>
>I am not sure if it is worth it. But it might help people (even me)
>when digging into the printk history. Different name will help to
>understand the functionality at the given time.
I am also not sure if it is worth the extra "noise" just to give the
function a more appropriate name. The plan is to remove it completely
soon anyway. My vote is to leave the name as it is.
But I am willing to do the rename in an addtional patch if you
want. printk_deferred_enter() sounds fine to me. Please confirm if you
want me to do this.
John Ogness
Re: [PATCH next v1 2/3] printk: remove safe buffers
On Mon 2021-03-22 12:16:15, John Ogness wrote:
> On 2021-03-21, Sergey Senozhatsky wrote:
> >> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt,
> >> va_list args)
> >> * Use the main logbuf even in NMI. But avoid calling console
> >> * drivers that might have their own locks.
> >> */
> >> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
> >> + if (this_cpu_read(printk_context) &
> >> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
> >> + PRINTK_NMI_CONTEXT_MASK |
> >> + PRINTK_SAFE_CONTEXT_MASK)) {
> >
> > Do we need printk_nmi_direct_enter/exit() and
> > PRINTK_NMI_DIRECT_CONTEXT_MASK? Seems like all printk_safe() paths
> > are now DIRECT - we store messages to the prb, but don't call console
> > drivers.
>
> I was planning on waiting until the kthreads are introduced, in which
> case printk_safe.c is completely removed.
You want to keep printk_safe() context because it prevents calling
consoles even in normal context. Namely, it prevents deadlock by
recursively taking, for example, sem->lock in console_lock() or
console_owner_lock in console_trylock_spinning(). Am I right?
> But I suppose I could switch
> the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that
> PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a
> 4th patch of the series.
Yes, please unify the PRINTK_NMI_CONTEXT. One is enough.
I wonder if it would make sense to go even further at this stage.
There will still be 4 contexts that modify the printk behavior after
this patchset:
+ printk_count set by printk_enter()/exit()
+ prevents: infinite recursion
+ context: any context
+ action: skips entire printk at 3rd recursion level
+ prink_context set by printk_safe_enter()/exit()
+ prevents: dead lock caused by recursion into some
console code in any context
+ context: any
+ action: skips console call at 1st recursion level
+ printk_context set by printk_nmi_enter()/exit()
+ prevents: dead lock caused by any console lock recursion
+ context: NMI
+ action: skips console calls at 0th recursion level
+ kdb_trap_printk
+ redirects printk() to kdb_printk() in kdb context
What is possible?
1. We could get rid of printk_nmi_enter()/exit() and
PRINTK_NMI_CONTEXT completely already now. It is enough
to check in_nmi() in printk_func().
printk_nmi_enter() was added by the commit 42a0bb3f71383b457a7db362
("printk/nmi: generic solution for safe printk in NMI"). It was
really needed to modify @printk_func pointer.
We did not remove it later when printk_function became a real
function. The idea was to track all printk contexts in a single
variable. But we never added kdb context.
It might make sense to remove it now. Peter Zijstra would be happy.
There already were some churns with tracking printk_context in NMI.
For example, see
https://lore.kernel.org/r/20200219150744.428764...@infradead.org
IMHO, it does not make sense to wait until the entire console-stuff
rework is done in this case.
2. I thought about unifying printk_safe_enter()/exit() and
printk_enter()/exit(). They both count recursion with
IRQs disabled, have similar name. But they are used
different way.
But better might be to rename printk_safe_enter()/exit() to
console_enter()/exit() or to printk_deferred_enter()/exit().
It would make more clear what it does now. And it might help
to better distinguish it from the new printk_enter()/exit().
This patchset actually splits the original printk_safe()
functionality into two:
+ printk_count prevents infinite recursion
+ printk_deferred_enter() deffers console handling.
I am not sure if it is worth it. But it might help people (even me)
when digging into the printk history. Different name will help to
understand the functionality at the given time.
What do you think, please?
Best Regards,
Petr
Re: [PATCH next v1 2/3] printk: remove safe buffers
On 2021-03-21, Sergey Senozhatsky wrote:
>> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list
>> args)
>> * Use the main logbuf even in NMI. But avoid calling console
>> * drivers that might have their own locks.
>> */
>> -if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
>> +if (this_cpu_read(printk_context) &
>> +(PRINTK_NMI_DIRECT_CONTEXT_MASK |
>> + PRINTK_NMI_CONTEXT_MASK |
>> + PRINTK_SAFE_CONTEXT_MASK)) {
>
> Do we need printk_nmi_direct_enter/exit() and
> PRINTK_NMI_DIRECT_CONTEXT_MASK? Seems like all printk_safe() paths
> are now DIRECT - we store messages to the prb, but don't call console
> drivers.
I was planning on waiting until the kthreads are introduced, in which
case printk_safe.c is completely removed. But I suppose I could switch
the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that
PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a
4th patch of the series.
John Ogness
Re: [PATCH next v1 2/3] printk: remove safe buffers
On (21/03/17 00:33), John Ogness wrote:
[..]
> void printk_nmi_direct_enter(void)
> {
> @@ -324,27 +44,8 @@ void printk_nmi_direct_exit(void)
> this_cpu_and(printk_context, ~PRINTK_NMI_DIRECT_CONTEXT_MASK);
> }
>
> -#else
> -
> -static __printf(1, 0) int vprintk_nmi(const char *fmt, va_list args)
> -{
> - return 0;
> -}
> -
> #endif /* CONFIG_PRINTK_NMI */
>
> -/*
> - * Lock-less printk(), to avoid deadlocks should the printk() recurse
> - * into itself. It uses a per-CPU buffer to store the message, just like
> - * NMI.
> - */
> -static __printf(1, 0) int vprintk_safe(const char *fmt, va_list args)
> -{
> - struct printk_safe_seq_buf *s = this_cpu_ptr(_print_seq);
> -
> - return printk_safe_log_store(s, fmt, args);
> -}
> -
> /* Can be preempted by NMI. */
> void __printk_safe_enter(void)
> {
> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list
> args)
>* Use the main logbuf even in NMI. But avoid calling console
>* drivers that might have their own locks.
>*/
> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
> + if (this_cpu_read(printk_context) &
> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
> + PRINTK_NMI_CONTEXT_MASK |
> + PRINTK_SAFE_CONTEXT_MASK)) {
Do we need printk_nmi_direct_enter/exit() and PRINTK_NMI_DIRECT_CONTEXT_MASK?
Seems like all printk_safe() paths are now DIRECT - we store messages to the
prb, but don't call console drivers.
-ss
[PATCH next v1 2/3] printk: remove safe buffers
With @logbuf_lock removed, the high level printk functions for
storing messages are lockless. Messages can be stored from any
context, so there is no need for the NMI and safe buffers anymore.
Remove the NMI and safe buffers.
Although the safe buffers are removed, the NMI and safe context
tracking is still in place. In these contexts, store the message
immediately but still use irq_work to defer the console printing.
Since printk recursion tracking is in place, safe context tracking
for most of printk is not needed. Remove it. Only safe context
tracking relating to the console lock is left in place. This is
because the console lock is needed for the actual printing.
Signed-off-by: John Ogness
---
arch/powerpc/kernel/traps.c| 1 -
arch/powerpc/kernel/watchdog.c | 5 -
include/linux/printk.h | 10 -
kernel/kexec_core.c| 1 -
kernel/panic.c | 3 -
kernel/printk/internal.h | 2 -
kernel/printk/printk.c | 81 ++--
kernel/printk/printk_safe.c| 332 +
lib/nmi_backtrace.c| 6 -
9 files changed, 18 insertions(+), 423 deletions(-)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index a44a30b0688c..5828c83eaca6 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -171,7 +171,6 @@ extern void panic_flush_kmsg_start(void)
extern void panic_flush_kmsg_end(void)
{
- printk_safe_flush_on_panic();
kmsg_dump(KMSG_DUMP_PANIC);
bust_spinlocks(0);
debug_locks_off();
diff --git a/arch/powerpc/kernel/watchdog.c b/arch/powerpc/kernel/watchdog.c
index c9a8f4781a10..dc17d8903d4f 100644
--- a/arch/powerpc/kernel/watchdog.c
+++ b/arch/powerpc/kernel/watchdog.c
@@ -183,11 +183,6 @@ static void watchdog_smp_panic(int cpu, u64 tb)
wd_smp_unlock();
- printk_safe_flush();
- /*
-* printk_safe_flush() seems to require another print
-* before anything actually goes out to console.
-*/
if (sysctl_hardlockup_all_cpu_backtrace)
trigger_allbutself_cpu_backtrace();
diff --git a/include/linux/printk.h b/include/linux/printk.h
index fe7eb2351610..2476796c1150 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -207,8 +207,6 @@ __printf(1, 2) void dump_stack_set_arch_desc(const char
*fmt, ...);
void dump_stack_print_info(const char *log_lvl);
void show_regs_print_info(const char *log_lvl);
extern asmlinkage void dump_stack(void) __cold;
-extern void printk_safe_flush(void);
-extern void printk_safe_flush_on_panic(void);
#else
static inline __printf(1, 0)
int vprintk(const char *s, va_list args)
@@ -272,14 +270,6 @@ static inline void show_regs_print_info(const char
*log_lvl)
static inline void dump_stack(void)
{
}
-
-static inline void printk_safe_flush(void)
-{
-}
-
-static inline void printk_safe_flush_on_panic(void)
-{
-}
#endif
extern int kptr_restrict;
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index f04d04d1b855..64bf5d5cdd06 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -977,7 +977,6 @@ void crash_kexec(struct pt_regs *regs)
old_cpu = atomic_cmpxchg(_cpu, PANIC_CPU_INVALID, this_cpu);
if (old_cpu == PANIC_CPU_INVALID) {
/* This is the 1st CPU which comes here, so go ahead. */
- printk_safe_flush_on_panic();
__crash_kexec(regs);
/*
diff --git a/kernel/panic.c b/kernel/panic.c
index 332736a72a58..1f0df42f8d0c 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -247,7 +247,6 @@ void panic(const char *fmt, ...)
* Bypass the panic_cpu check and call __crash_kexec directly.
*/
if (!_crash_kexec_post_notifiers) {
- printk_safe_flush_on_panic();
__crash_kexec(NULL);
/*
@@ -271,8 +270,6 @@ void panic(const char *fmt, ...)
*/
atomic_notifier_call_chain(_notifier_list, 0, buf);
- /* Call flush even twice. It tries harder with a single online CPU */
- printk_safe_flush_on_panic();
kmsg_dump(KMSG_DUMP_PANIC);
/*
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index e7acc2888c8e..e108b2ece8c7 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -23,7 +23,6 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list
args);
void __printk_safe_enter(void);
void __printk_safe_exit(void);
-void printk_safe_init(void);
bool printk_percpu_data_ready(void);
#define printk_safe_enter_irqsave(flags) \
@@ -67,6 +66,5 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list
args) { return 0; }
#define printk_safe_enter_irq() local_irq_disable()
#define printk_safe_exit_irq() local_irq_enable()
-static inline void printk_safe_init(void) { }
static inline bool printk_percpu_data_ready(void) { return false; }
#endif /* CONFIG_PRINTK */
diff --git a/kernel/printk/printk.c
