* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-05-31 16:20 ` [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c John Ogness
@ 2021-05-31 16:30 ` John Ogness
2021-05-31 20:04 ` kernel test robot
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: John Ogness @ 2021-05-31 16:30 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt,
Thomas Gleixner, linux-kernel, Sergey Senozhatsky, Andrew Morton,
Stephen Rothwell, Dmitry Safonov, Valentin Schneider,
Daniel Bristot de Oliveira, Peter Zijlstra, Stephen Boyd,
Alexander Potapenko, Paul E. McKenney
On 2021-05-31, John Ogness <john.ogness@linutronix.de> wrote:
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 114e9963f903..98feead621ff 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3531,4 +3531,96 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
> }
> EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>
> +#ifdef CONFIG_SMP
> +static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1);
> +
> +/*
> + * printk_cpu_lock: Acquire the printk cpu-reentrant spinning lock.
> + * @cpu_store: A buffer to store lock state.
> + * @flags: A buffer to store irq state.
> + *
> + * If no processor has the lock, the calling processor takes the lock and
> + * becomes the owner. If the calling processor is already the owner of the
> + * lock, this function succeeds immediately. If the lock is locked by another
> + * processor, that function spins until the calling processor becomes the
> + * owner.
> + *
> + * It is safe to call this function from any context and state.
> + */
> +void printk_cpu_lock(unsigned int *cpu_store, unsigned long *flags)
> +{
> + unsigned int cpu;
> +
> + for (;;) {
> + cpu = get_cpu();
> +
> + *cpu_store = atomic_read(&printk_cpulock_owner);
> +
> + if (*cpu_store == -1) {
> + local_irq_save(*flags);
> +
> + /*
> + * Guarantee loads an stores from the previous lock
> + * owner are visible to this CPU once it is the lock
> + * owner. This pairs with cpu_lock:A.
The above paragraph is totally broken. It should read:
+ * Guarantee loads and stores from the previous lock
+ * owner are visible to this CPU once it is the lock
+ * owner. This pairs with cpu_unlock:B.
John Ogness
> + *
> + * Memory barrier involvement:
> + *
> + * If cpu_lock:A reads from cpu_unlock:B, then
> + * cpu_lock:B reads from cpu_unlock:A.
> + *
> + * Relies on:
> + *
> + * RELEASE from cpu_unlock:A to cpu_unlock:B
> + * matching
> + * ACQUIRE from cpu_lock:A to cpu_lock:B
> + */
> + if (atomic_try_cmpxchg_acquire(&printk_cpulock_owner,
> + cpu_store, cpu)) { /* LMM(cpu_lock:A) */
> +
> + /* This CPU begins loading/storing data: LMM(cpu_lock:B) */
> + break;
> + }
> +
> + local_irq_restore(*flags);
> +
> + } else if (*cpu_store == cpu) {
> + break;
> + }
> +
> + put_cpu();
> + cpu_relax();
> + }
> +}
> +EXPORT_SYMBOL(printk_cpu_lock);
> +
> +/*
> + * printk_cpu_unlock: Release the printk cpu-reentrant spinning lock.
> + * @cpu_store: The current lock state.
> + * @flags: The current irq state.
> + *
> + * Release the lock. The calling processor must be the owner of the lock.
> + *
> + * It is safe to call this function from any context and state.
> + */
> +void printk_cpu_unlock(unsigned int cpu_store, unsigned long flags)
> +{
> + if (cpu_store == -1) {
> + /* This CPU is finished loading/storing data: LMM(cpu_unlock:A) */
> +
> + /*
> + * Guarantee loads an stores from this CPU when it is the lock
> + * owner are visible to the next lock owner. This pairs with
> + * cpu_lock:A.
> + */
> + atomic_set_release(&printk_cpulock_owner, cpu_store); /* LMM(cpu_unlock:B) */
> +
> + local_irq_restore(flags);
> + }
> +
> + put_cpu();
> +}
> +EXPORT_SYMBOL(printk_cpu_unlock);
> +#endif /* CONFIG_SMP */
> +
> #endif
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-05-31 16:20 ` [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c John Ogness
2021-05-31 16:30 ` John Ogness
@ 2021-05-31 20:04 ` kernel test robot
2021-05-31 21:49 ` kernel test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-05-31 20:04 UTC (permalink / raw)
To: John Ogness, Petr Mladek
Cc: kbuild-all, Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner,
linux-kernel, Andrew Morton, Linux Memory Management List,
Dmitry Safonov, Valentin Schneider, Daniel Bristot de Oliveira
[-- Attachment #1: Type: text/plain, Size: 2771 bytes --]
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20210528]
url: https://github.com/0day-ci/linux/commits/John-Ogness/introduce-printk-cpu-lock/20210601-013122
base: 3e029760e6f8ce90c122c267a039ae73b3f1f5a4
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/528abf3d0ab80471d02f4f16cbe97b2eaa918a11
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Ogness/introduce-printk-cpu-lock/20210601-013122
git checkout 528abf3d0ab80471d02f4f16cbe97b2eaa918a11
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
lib/dump_stack.c:97: warning: Function parameter or member 'log_lvl' not described in 'dump_stack_lvl'
>> lib/dump_stack.c:97: warning: expecting prototype for dump_stack(). Prototype was for dump_stack_lvl() instead
vim +97 lib/dump_stack.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 90
196779b9b4ce19 Tejun Heo 2013-04-30 91 /**
196779b9b4ce19 Tejun Heo 2013-04-30 92 * dump_stack - dump the current task information and its stack trace
196779b9b4ce19 Tejun Heo 2013-04-30 93 *
196779b9b4ce19 Tejun Heo 2013-04-30 94 * Architectures can override this implementation by implementing its own.
196779b9b4ce19 Tejun Heo 2013-04-30 95 */
d542aa86de8ad0 Alexander Potapenko 2021-05-26 96 asmlinkage __visible void dump_stack_lvl(const char *log_lvl)
^1da177e4c3f41 Linus Torvalds 2005-04-16 @97 {
528abf3d0ab804 John Ogness 2021-05-31 98 unsigned int cpu_store;
d7ce36924344ac Eric Dumazet 2016-02-05 99 unsigned long flags;
b58d977432c80e Alex Thorlton 2013-07-03 100
528abf3d0ab804 John Ogness 2021-05-31 101 printk_cpu_lock(&cpu_store, &flags);
d542aa86de8ad0 Alexander Potapenko 2021-05-26 102 __dump_stack(log_lvl);
528abf3d0ab804 John Ogness 2021-05-31 103 printk_cpu_unlock(cpu_store, flags);
b58d977432c80e Alex Thorlton 2013-07-03 104 }
d542aa86de8ad0 Alexander Potapenko 2021-05-26 105 EXPORT_SYMBOL(dump_stack_lvl);
d542aa86de8ad0 Alexander Potapenko 2021-05-26 106
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60755 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-05-31 16:20 ` [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c John Ogness
2021-05-31 16:30 ` John Ogness
2021-05-31 20:04 ` kernel test robot
@ 2021-05-31 21:49 ` kernel test robot
2021-06-01 2:55 ` Sergey Senozhatsky
2021-06-01 13:59 ` Petr Mladek
4 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-05-31 21:49 UTC (permalink / raw)
To: John Ogness, Petr Mladek
Cc: kbuild-all, clang-built-linux, Sergey Senozhatsky,
Steven Rostedt, Thomas Gleixner, linux-kernel, Andrew Morton,
Linux Memory Management List, Dmitry Safonov, Valentin Schneider,
Daniel Bristot de Oliveira
[-- Attachment #1: Type: text/plain, Size: 2988 bytes --]
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20210528]
url: https://github.com/0day-ci/linux/commits/John-Ogness/introduce-printk-cpu-lock/20210601-013122
base: 3e029760e6f8ce90c122c267a039ae73b3f1f5a4
config: x86_64-randconfig-a014-20210531 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project bc6799f2f79f0ae87e9f1ebf9d25ba799fbd25a9)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/528abf3d0ab80471d02f4f16cbe97b2eaa918a11
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Ogness/introduce-printk-cpu-lock/20210601-013122
git checkout 528abf3d0ab80471d02f4f16cbe97b2eaa918a11
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
lib/dump_stack.c:97: warning: Function parameter or member 'log_lvl' not described in 'dump_stack_lvl'
>> lib/dump_stack.c:97: warning: expecting prototype for dump_stack(). Prototype was for dump_stack_lvl() instead
vim +97 lib/dump_stack.c
^1da177e4c3f415 Linus Torvalds 2005-04-16 90
196779b9b4ce192 Tejun Heo 2013-04-30 91 /**
196779b9b4ce192 Tejun Heo 2013-04-30 92 * dump_stack - dump the current task information and its stack trace
196779b9b4ce192 Tejun Heo 2013-04-30 93 *
196779b9b4ce192 Tejun Heo 2013-04-30 94 * Architectures can override this implementation by implementing its own.
196779b9b4ce192 Tejun Heo 2013-04-30 95 */
d542aa86de8ad0c Alexander Potapenko 2021-05-26 96 asmlinkage __visible void dump_stack_lvl(const char *log_lvl)
^1da177e4c3f415 Linus Torvalds 2005-04-16 @97 {
528abf3d0ab8047 John Ogness 2021-05-31 98 unsigned int cpu_store;
d7ce36924344ace Eric Dumazet 2016-02-05 99 unsigned long flags;
b58d977432c80ee Alex Thorlton 2013-07-03 100
528abf3d0ab8047 John Ogness 2021-05-31 101 printk_cpu_lock(&cpu_store, &flags);
d542aa86de8ad0c Alexander Potapenko 2021-05-26 102 __dump_stack(log_lvl);
528abf3d0ab8047 John Ogness 2021-05-31 103 printk_cpu_unlock(cpu_store, flags);
b58d977432c80ee Alex Thorlton 2013-07-03 104 }
d542aa86de8ad0c Alexander Potapenko 2021-05-26 105 EXPORT_SYMBOL(dump_stack_lvl);
d542aa86de8ad0c Alexander Potapenko 2021-05-26 106
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30124 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-05-31 16:20 ` [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c John Ogness
` (2 preceding siblings ...)
2021-05-31 21:49 ` kernel test robot
@ 2021-06-01 2:55 ` Sergey Senozhatsky
2021-06-01 6:58 ` John Ogness
2021-06-01 13:59 ` Petr Mladek
4 siblings, 1 reply; 14+ messages in thread
From: Sergey Senozhatsky @ 2021-06-01 2:55 UTC (permalink / raw)
To: John Ogness
Cc: Petr Mladek, Steven Rostedt, Thomas Gleixner, linux-kernel,
Sergey Senozhatsky, Andrew Morton, Stephen Rothwell,
Dmitry Safonov, Valentin Schneider, Daniel Bristot de Oliveira,
Peter Zijlstra, Stephen Boyd, Alexander Potapenko,
Paul E. McKenney
On (21/05/31 18:20), John Ogness wrote:
> +void printk_cpu_lock(unsigned int *cpu_store, unsigned long *flags)
> +{
> + unsigned int cpu;
> +
> + for (;;) {
> + cpu = get_cpu();
> +
> + *cpu_store = atomic_read(&printk_cpulock_owner);
> +
> + if (*cpu_store == -1) {
> + local_irq_save(*flags);
Is there any particular reason this does
preempt_disable();
cpu = smp_processor_id();
local_irq_safe();
instead of
local_irq_safe();
cpu = raw_smp_processor_id();
?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-06-01 2:55 ` Sergey Senozhatsky
@ 2021-06-01 6:58 ` John Ogness
2021-06-01 7:37 ` John Ogness
0 siblings, 1 reply; 14+ messages in thread
From: John Ogness @ 2021-06-01 6:58 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Steven Rostedt, Thomas Gleixner, linux-kernel,
Sergey Senozhatsky, Andrew Morton, Stephen Rothwell,
Dmitry Safonov, Valentin Schneider, Daniel Bristot de Oliveira,
Peter Zijlstra, Stephen Boyd, Alexander Potapenko,
Paul E. McKenney
On 2021-06-01, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> On (21/05/31 18:20), John Ogness wrote:
>> +void printk_cpu_lock(unsigned int *cpu_store, unsigned long *flags)
>> +{
>> + unsigned int cpu;
>> +
>> + for (;;) {
>> + cpu = get_cpu();
>> +
>> + *cpu_store = atomic_read(&printk_cpulock_owner);
>> +
>> + if (*cpu_store == -1) {
>> + local_irq_save(*flags);
>
> Is there any particular reason this does
>
> preempt_disable();
> cpu = smp_processor_id();
> local_irq_safe();
>
> instead of
>
> local_irq_safe();
> cpu = raw_smp_processor_id();
>
> ?
If the lock is owned by another CPU, there is no need to disable
interrupts for this CPU. (The local_irq_save() is conditional.)
John Ogness
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-06-01 6:58 ` John Ogness
@ 2021-06-01 7:37 ` John Ogness
2021-06-01 13:29 ` Petr Mladek
0 siblings, 1 reply; 14+ messages in thread
From: John Ogness @ 2021-06-01 7:37 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Steven Rostedt, Thomas Gleixner, linux-kernel,
Sergey Senozhatsky, Andrew Morton, Stephen Rothwell,
Dmitry Safonov, Valentin Schneider, Daniel Bristot de Oliveira,
Peter Zijlstra, Stephen Boyd, Alexander Potapenko,
Paul E. McKenney
On 2021-06-01, John Ogness <john.ogness@linutronix.de> wrote:
>> Is there any particular reason this does
>>
>> preempt_disable();
>> cpu = smp_processor_id();
>> local_irq_safe();
>>
>> instead of
>>
>> local_irq_safe();
>> cpu = raw_smp_processor_id();
>>
>> ?
>
> If the lock is owned by another CPU, there is no need to disable
> interrupts for this CPU. (The local_irq_save() is conditional.)
The cpu lock implementation from dump_stack() also keeps preemption
continually enabled while spinning. I used the cpu lock implementation
from PREEMPT_RT. But for my v2 I will adopt the same ordering from
dump_stack(), as you are suggesting.
Thanks for pointing that out.
John Ogness
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-06-01 7:37 ` John Ogness
@ 2021-06-01 13:29 ` Petr Mladek
0 siblings, 0 replies; 14+ messages in thread
From: Petr Mladek @ 2021-06-01 13:29 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner,
linux-kernel, Andrew Morton, Stephen Rothwell, Dmitry Safonov,
Valentin Schneider, Daniel Bristot de Oliveira, Peter Zijlstra,
Stephen Boyd, Alexander Potapenko, Paul E. McKenney
On Tue 2021-06-01 09:37:08, John Ogness wrote:
> On 2021-06-01, John Ogness <john.ogness@linutronix.de> wrote:
> >> Is there any particular reason this does
> >>
> >> preempt_disable();
> >> cpu = smp_processor_id();
> >> local_irq_safe();
> >>
> >> instead of
> >>
> >> local_irq_safe();
> >> cpu = raw_smp_processor_id();
> >>
> >> ?
> >
> > If the lock is owned by another CPU, there is no need to disable
> > interrupts for this CPU. (The local_irq_save() is conditional.)
>
> The cpu lock implementation from dump_stack() also keeps preemption
> continually enabled while spinning.
I wonder if this might reduce some noise on the CPU cache lines
when disable_preemption()/enable_preemption() actually does something.
But the problem might be only with cmpxchg() in a busy loop.
Peter Zijlstra might know more.
> I used the cpu lock implementation from PREEMPT_RT. But for my v2
> I will adopt the same ordering from dump_stack(), as you are suggesting.
Anyway, please document any changes in the ordering if there are any.
The current commit message sounds like a code move without any
functional changes.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-05-31 16:20 ` [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c John Ogness
` (3 preceding siblings ...)
2021-06-01 2:55 ` Sergey Senozhatsky
@ 2021-06-01 13:59 ` Petr Mladek
2021-06-01 14:21 ` John Ogness
4 siblings, 1 reply; 14+ messages in thread
From: Petr Mladek @ 2021-06-01 13:59 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt,
Thomas Gleixner, linux-kernel, Sergey Senozhatsky, Andrew Morton,
Stephen Rothwell, Dmitry Safonov, Valentin Schneider,
Daniel Bristot de Oliveira, Peter Zijlstra, Stephen Boyd,
Alexander Potapenko, Paul E. McKenney
On Mon 2021-05-31 18:20:50, John Ogness wrote:
> dump_stack() implements its own cpu-reentrant spinning lock to
> best-effort serialize stack traces in the printk log. However,
> there are other functions (such as show_regs()) that can also
> benefit from this serialization.
>
> Move the cpu-reentrant spinning lock (cpu lock) into new helper
> functions printk_cpu_lock()/printk_cpu_unlock() so that it is
> available for others as well. For !CONFIG_PRINTK or !CONFIG_SMP
> the cpu lock is a NOP.
>
> Note that having multiple cpu locks in the system can easily
> lead to deadlock. Code needing a cpu lock should use the
> printk cpu lock, since the printk cpu lock could be acquired
> from any code and any context.
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 114e9963f903..98feead621ff 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3531,4 +3531,96 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
> }
> EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>
> +#ifdef CONFIG_SMP
> +static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1);
> +
> +/*
> + * printk_cpu_lock: Acquire the printk cpu-reentrant spinning lock.
> + * @cpu_store: A buffer to store lock state.
> + * @flags: A buffer to store irq state.
> + *
> + * If no processor has the lock, the calling processor takes the lock and
> + * becomes the owner. If the calling processor is already the owner of the
> + * lock, this function succeeds immediately. If the lock is locked by another
> + * processor, that function spins until the calling processor becomes the
> + * owner.
> + *
> + * It is safe to call this function from any context and state.
> + */
> +void printk_cpu_lock(unsigned int *cpu_store, unsigned long *flags)
I think about calling this printk_cpu_lock_irqsave() to make it clear
that it disables interrupts.
Strictly speaking, it should be enough to disable preemption. If it is
safe when interrupted by NMI, it must be safe also when interrupted
by a normal interrupt.
I guess that the interrupts are disabled because it reduces the risk
of nested (messed) backtraces.
Anyway, I would keep the current approach (disabled irqs) unless we
have a good reason to change it. Well, enabled irqs might be better
for RT.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-06-01 13:59 ` Petr Mladek
@ 2021-06-01 14:21 ` John Ogness
2021-06-03 6:33 ` Petr Mladek
0 siblings, 1 reply; 14+ messages in thread
From: John Ogness @ 2021-06-01 14:21 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt,
Thomas Gleixner, linux-kernel, Sergey Senozhatsky, Andrew Morton,
Stephen Rothwell, Dmitry Safonov, Valentin Schneider,
Daniel Bristot de Oliveira, Peter Zijlstra, Stephen Boyd,
Alexander Potapenko, Paul E. McKenney
On 2021-06-01, Petr Mladek <pmladek@suse.com> wrote:
>> +void printk_cpu_lock(unsigned int *cpu_store, unsigned long *flags)
>
> I think about calling this printk_cpu_lock_irqsave() to make it clear
> that it disables interrupts.
Agreed.
> Strictly speaking, it should be enough to disable preemption. If it is
> safe when interrupted by NMI, it must be safe also when interrupted
> by a normal interrupt.
>
> I guess that the interrupts are disabled because it reduces the risk
> of nested (messed) backtraces.
If it was just about synchronizing output triggered by sysreq, then it
probably would be acceptable to leave interrupts active. But when atomic
consoles are involved, we are talking about a crashing machine that is
trying to get log messages out. Any interrupt is a risk that the machine
may not survive long enough to return from that interruption.
John Ogness
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c
2021-06-01 14:21 ` John Ogness
@ 2021-06-03 6:33 ` Petr Mladek
0 siblings, 0 replies; 14+ messages in thread
From: Petr Mladek @ 2021-06-03 6:33 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt,
Thomas Gleixner, linux-kernel, Sergey Senozhatsky, Andrew Morton,
Stephen Rothwell, Dmitry Safonov, Valentin Schneider,
Daniel Bristot de Oliveira, Peter Zijlstra, Stephen Boyd,
Alexander Potapenko, Paul E. McKenney
On Tue 2021-06-01 16:21:52, John Ogness wrote:
> On 2021-06-01, Petr Mladek <pmladek@suse.com> wrote:
> >> +void printk_cpu_lock(unsigned int *cpu_store, unsigned long *flags)
> >
> > I think about calling this printk_cpu_lock_irqsave() to make it clear
> > that it disables interrupts.
>
> Agreed.
>
> > Strictly speaking, it should be enough to disable preemption. If it is
> > safe when interrupted by NMI, it must be safe also when interrupted
> > by a normal interrupt.
> >
> > I guess that the interrupts are disabled because it reduces the risk
> > of nested (messed) backtraces.
>
> If it was just about synchronizing output triggered by sysreq, then it
> probably would be acceptable to leave interrupts active. But when atomic
> consoles are involved, we are talking about a crashing machine that is
> trying to get log messages out. Any interrupt is a risk that the machine
> may not survive long enough to return from that interruption.
Fair enough. It might be good to mention this motivation in the commit
message or in a code commentary. IMHO, it is always good to know
whether these things a must to have or if there is another reason.
Anyway, it was not obvious to me ;-)
Best Regards,
Petr
^ permalink raw reply [flat|nested] 14+ messages in thread