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

[John Ogness <john.ogness@linutronix.de>]

On 2021-06-16, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> On (21/06/15 23:39), John Ogness wrote:
>> On 2021-06-15, John Ogness <john.ogness@linutronix.de> wrote:
>> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> > index 114e9963f903..5369d8f33299 100644
>> > --- a/kernel/printk/printk.c
>> > +++ b/kernel/printk/printk.c
>> > @@ -3532,3 +3532,70 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
>> >  EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>> >  
>> >  #endif
>> > +
>> > +#ifdef CONFIG_SMP
>> > +static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1);
>> > +static bool printk_cpulock_nested;
>> 
>> I just realized that @printk_cpulock_nested will need to be an atomic_t
>> counter to allow multiple nested levels since nesting can also occur
>
> Strictly speaking, this is not nested printk, right? printk recursion is
> handled in printk separately. This one is more like "nested dump_stack()-s",
> or nested "error reporinting".
>
> Because the original code has never limited nested error reporting
> contexts.

It isn't about limiting. It is about tracking. The current dump_stack()
handles it correctly because the tracking is done in the stack frame of
the caller (in @was_locked of dump_stack_lvl()). My previous versions
also handled it correctly by using the same technique.

With this series version I moved the tracking into a global variable
@printk_cpulock_nested, which is fine, except that a boolean is not
capable of tracking more than 1 nesting. Which means that
__printk_cpu_unlock() would release cpu lock ownership too soon.

Doing this correctly is a simple change:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e67dc510fa1b..5376216e4f3d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3535,7 +3535,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
 
 #ifdef CONFIG_SMP
 static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1);
-static bool printk_cpulock_nested;
+static atomic_t printk_cpulock_nested = ATOMIC_INIT(0);
 
 /**
  * __printk_wait_on_cpu_lock() - Busy wait until the printk cpu-reentrant
@@ -3596,7 +3598,7 @@ int __printk_cpu_trylock(void)
 
 	} else if (old == cpu) {
 		/* This CPU is already the owner. */
-		printk_cpulock_nested = true;
+		atomic_inc(&printk_cpulock_nested);
 		return 1;
 	}
 
@@ -3613,8 +3615,8 @@ EXPORT_SYMBOL(__printk_cpu_trylock);
  */
 void __printk_cpu_unlock(void)
 {
-	if (printk_cpulock_nested) {
-		printk_cpulock_nested = false;
+	if (atomic_read(&printk_cpulock_nested)) {
+		atomic_dec(&printk_cpulock_nested);
 		return;
 	}

> Shall this be a separate patch?

I would prefer a v4 because I also noticed that this patch accidentally
implements atomic_set_release() instead of moving over the atomit_set()
from dump_stack(). That also needs to be corrected, otherwise the next
patch in the series makes no sense.

John Ogness