[PATCH next v1 0/2] introduce printk cpu lock

[PATCH next v1 0/2] introduce printk cpu lock

[John Ogness]

While working on removing the safe buffers for printk [0] we
stumbled on a cpu-reentrant spinning lock used by dump_stack(). This
type of lock (dubbed a cpu lock) will cause deadlock risks once we
introduce atomic consoles because atomic consoles also need such a
lock.

Although we are not yet ready to introduce atomic consoles, this is
an appropriate time to provide an official cpu lock to be used for
all things relating to printk (including the atomic consoles, once
they are introduced).

This series also extends the new printk cpu lock to cover
show_regs().

This series is against next-20210528.

John Ogness

[0] https://lore.kernel.org/lkml/YGW63%2FelFr%2FgYW1u@alley

John Ogness (2):
  dump_stack: move cpu lock to printk.c
  nmi_backtrace: use the printk cpu lock for show_regs()

 include/linux/printk.h | 13 ++++++
 kernel/printk/printk.c | 92 ++++++++++++++++++++++++++++++++++++++++++
 lib/dump_stack.c       | 43 ++------------------
 lib/nmi_backtrace.c    |  4 ++
 4 files changed, 112 insertions(+), 40 deletions(-)

-- 
2.20.1

[PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[John Ogness]

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.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/printk.h | 13 ++++++
 kernel/printk/printk.c | 92 ++++++++++++++++++++++++++++++++++++++++++
 lib/dump_stack.c       | 43 ++------------------
 3 files changed, 108 insertions(+), 40 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index f589b8b60806..2f2d89b9e728 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -287,6 +287,19 @@ static inline void printk_safe_flush_on_panic(void)
 }
 #endif
 
+#if defined(CONFIG_PRINTK) && defined(CONFIG_SMP)
+extern void printk_cpu_lock(unsigned int *cpu_store, unsigned long *flags);
+extern void printk_cpu_unlock(unsigned int cpu_store, unsigned long flags);
+#else
+static inline void printk_cpu_lock(unsigned int *cpu_store, unsigned long *flags)
+{
+}
+
+static inline void printk_cpu_unlock(unsigned int cpu_store, unsigned long flags)
+{
+}
+#endif
+
 extern int kptr_restrict;
 
 /**
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.
+			 *
+			 * 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
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 6e7ca3d67710..88f13250f29d 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -93,52 +93,15 @@ static void __dump_stack(const char *log_lvl)
  *
  * Architectures can override this implementation by implementing its own.
  */
-#ifdef CONFIG_SMP
-static atomic_t dump_lock = ATOMIC_INIT(-1);
-
 asmlinkage __visible void dump_stack_lvl(const char *log_lvl)
 {
+	unsigned int cpu_store;
 	unsigned long flags;
-	int was_locked;
-	int old;
-	int cpu;
-
-	/*
-	 * Permit this cpu to perform nested stack dumps while serialising
-	 * against other CPUs
-	 */
-retry:
-	local_irq_save(flags);
-	cpu = smp_processor_id();
-	old = atomic_cmpxchg(&dump_lock, -1, cpu);
-	if (old == -1) {
-		was_locked = 0;
-	} else if (old == cpu) {
-		was_locked = 1;
-	} else {
-		local_irq_restore(flags);
-		/*
-		 * Wait for the lock to release before jumping to
-		 * atomic_cmpxchg() in order to mitigate the thundering herd
-		 * problem.
-		 */
-		do { cpu_relax(); } while (atomic_read(&dump_lock) != -1);
-		goto retry;
-	}
-
-	__dump_stack(log_lvl);
-
-	if (!was_locked)
-		atomic_set(&dump_lock, -1);
 
-	local_irq_restore(flags);
-}
-#else
-asmlinkage __visible void dump_stack_lvl(const char *log_lvl)
-{
+	printk_cpu_lock(&cpu_store, &flags);
 	__dump_stack(log_lvl);
+	printk_cpu_unlock(cpu_store, flags);
 }
-#endif
 EXPORT_SYMBOL(dump_stack_lvl);
 
 asmlinkage __visible void dump_stack(void)
-- 
2.20.1

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[John Ogness]

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

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[kernel test robot]

[-- 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 --]

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[kernel test robot]

[-- 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 --]

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[Sergey Senozhatsky]

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();

?

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[John Ogness]

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

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[John Ogness]

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

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[Petr Mladek]

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

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[Petr Mladek]

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

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[John Ogness]

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

Re: [PATCH next v1 1/2] dump_stack: move cpu lock to printk.c

[Petr Mladek]

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

[PATCH next v1 2/2] nmi_backtrace: use the printk cpu lock for show_regs()

[John Ogness]

dump_stack() uses the printk cpu lock to synchronize the stacktrace,
but this can also be used for dumping the banner and regs.

Since the cpu lock allows recursive locking, it is not an issue to
call dump_stack() with the printk cpu lock held.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 lib/nmi_backtrace.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/nmi_backtrace.c b/lib/nmi_backtrace.c
index 8abe1870dba4..9ed02c2d77be 100644
--- a/lib/nmi_backtrace.c
+++ b/lib/nmi_backtrace.c
@@ -92,17 +92,21 @@ module_param(backtrace_idle, bool, 0644);
 bool nmi_cpu_backtrace(struct pt_regs *regs)
 {
 	int cpu = smp_processor_id();
+	unsigned int cpu_store;
+	unsigned long flags;
 
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		if (!READ_ONCE(backtrace_idle) && regs && cpu_in_idle(instruction_pointer(regs))) {
 			pr_warn("NMI backtrace for cpu %d skipped: idling at %pS\n",
 				cpu, (void *)instruction_pointer(regs));
 		} else {
+			printk_cpu_lock(&cpu_store, &flags);
 			pr_warn("NMI backtrace for cpu %d\n", cpu);
 			if (regs)
 				show_regs(regs);
 			else
 				dump_stack();
+			printk_cpu_unlock(cpu_store, flags);
 		}
 		cpumask_clear_cpu(cpu, to_cpumask(backtrace_mask));
 		return true;
-- 
2.20.1

Re: [PATCH next v1 2/2] nmi_backtrace: use the printk cpu lock for show_regs()

[Petr Mladek]

On Mon 2021-05-31 18:20:51, John Ogness wrote:
> dump_stack() uses the printk cpu lock to synchronize the stacktrace,
> but this can also be used for dumping the banner and regs.
> 
> Since the cpu lock allows recursive locking, it is not an issue to
> call dump_stack() with the printk cpu lock held.

This does not explain why it is serialized only in nmi_cpu_backtrace().

It would be better to serialize all show_regs() calls. But it would
require updating all the arch-specific implementations.

I know that this patch is a pre-requisite for another patchset
removing printk_safe(). Where printk_safe() is serializing
the backtraces from all CPUs at the moment.

IMHO, it is perfectly fine synchronize only nmi_cpu_backtrace() for
now. It is the most important use-case where show_regs() calls could
get messed easily. But I suggest to do it in the patchset removing
printk_safe(). Also we need to explain in the commit message the relation
to printk_safe().

Best Regards,
Petr