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

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

[John Ogness]

Hello,

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 is against next-20210607.

v1 can be found here [1].

Changes since v1:

- Keep cpu lock functionality even if !CONFIG_PRINTK. Only make it a
  NOP for !CONFIG_SMP.

- Rename the lock/unlock functions to reflect irq disabling/enabling.

- Change first argument of lock/unlock functions to bool. There is
  no need to track the previous @printk_cpulock_owner value.

- Change the series to first move over the cpu lock implementation,
  then fix ordering in a separate patch.

- Preserve the style from the dump_stack() cpu lock (i.e. goto-retry
  instead of for-loop and cmpxchg() instead of try_cmpxchg()).

- Drop the patch that adds serialization to show_regs().

- Fix various typos in comments.

John Ogness

[0] https://lore.kernel.org/lkml/YGW63%2FelFr%2FgYW1u@alley
[1] https://lore.kernel.org/lkml/20210531162051.2325-1-john.ogness@linutronix.de

John Ogness (2):
  dump_stack: move cpu lock to printk.c
  printk: fix cpu lock ordering

 include/linux/printk.h |  13 ++++++
 kernel/printk/printk.c | 101 +++++++++++++++++++++++++++++++++++++++++
 lib/dump_stack.c       |  41 ++---------------
 3 files changed, 118 insertions(+), 37 deletions(-)

-- 
2.20.1

[PATCH next v2 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_irqsave()/printk_cpu_unlock_irqrestore()
so that it is available for others as well. For !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 | 75 ++++++++++++++++++++++++++++++++++++++++++
 lib/dump_stack.c       | 41 +++--------------------
 3 files changed, 92 insertions(+), 37 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index f589b8b60806..b84e0c59220f 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_SMP)
+extern void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags);
+extern void printk_cpu_unlock_irqrestore(bool lock_flag, unsigned long irq_flags);
+#else
+static inline void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
+{
+}
+
+static inline void printk_cpu_unlock_irqrestore(bool lock_flag, unsigned long irq_flags)
+{
+}
+#endif
+
 extern int kptr_restrict;
 
 /**
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 114e9963f903..f94babb38493 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3532,3 +3532,78 @@ 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);
+
+/*
+ * printk_cpu_lock_irqsave: Acquire the printk cpu-reentrant spinning lock
+ *                          and disable interrupts.
+ * @lock_flag: A buffer to store lock state.
+ * @irq_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 held by another
+ * processor, this function spins until the calling processor becomes the
+ * owner. This function returns with interrupts disabled.
+ *
+ * It is safe to call this function from any context and state.
+ */
+void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
+{
+	int old;
+	int cpu;
+
+retry:
+	local_irq_save(*irq_flags);
+
+	cpu = smp_processor_id();
+
+	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
+	if (old == -1) {
+		/* This CPU is now the owner. */
+
+		*lock_flag = true;
+
+	} else if (old == cpu) {
+		/* This CPU is already the owner. */
+
+		*lock_flag = false;
+
+	} else {
+		local_irq_restore(*irq_flags);
+
+		/*
+		 * Wait for the lock to release before jumping to cmpxchg()
+		 * in order to mitigate the thundering herd problem.
+		 */
+		do {
+			cpu_relax();
+		} while (atomic_read(&printk_cpulock_owner) != -1);
+
+		goto retry;
+	}
+}
+EXPORT_SYMBOL(printk_cpu_lock_irqsave);
+
+/*
+ * printk_cpu_unlock_irqrestore: Release the printk cpu-reentrant spinning
+ *                               lock and restore interrupts.
+ * @lock_flag: The current lock state.
+ * @irq_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_irqrestore(bool lock_flag, unsigned long irq_flags)
+{
+	if (lock_flag) {
+		atomic_set(&printk_cpulock_owner, -1);
+
+		local_irq_restore(irq_flags);
+	}
+}
+EXPORT_SYMBOL(printk_cpu_unlock_irqrestore);
+#endif /* CONFIG_SMP */
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 6e7ca3d67710..84c68bad94c7 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -93,52 +93,19 @@ 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 long flags;
-	int was_locked;
-	int old;
-	int cpu;
+	unsigned long irq_flags;
+	bool lock_flag;
 
 	/*
 	 * 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_irqsave(&lock_flag, &irq_flags);
 	__dump_stack(log_lvl);
+	printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
 }
-#endif
 EXPORT_SYMBOL(dump_stack_lvl);
 
 asmlinkage __visible void dump_stack(void)
-- 
2.20.1

[PATCH next v2 2/2] printk: fix cpu lock ordering

[John Ogness]

The cpu lock implementation uses a full memory barrier to take
the lock, but no memory barriers when releasing the lock. This
means that changes performed by a lock owner may not be seen by
the next lock owner. This may have been "good enough" for use
by dump_stack() as a serialization mechanism, but it is not
enough to provide proper protection for a critical section.

Correct this problem by using acquire/release memory barriers
for lock/unlock, respectively.

Note that it is not necessary for a cpu lock to disable
interrupts. However, in upcoming work this cpu lock will be used
for emergency tasks (for example, atomic consoles during kernel
crashes) and any interruptions should be avoided if possible.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f94babb38493..8c870581cfb4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3560,10 +3560,29 @@ void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
 
 	cpu = smp_processor_id();
 
-	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
+	/*
+	 * 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.
+	 *
+	 * 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
+	 */
+	old = atomic_cmpxchg_acquire(&printk_cpulock_owner,
+				     -1, cpu); /* LMM(cpu_lock:A) */
 	if (old == -1) {
 		/* This CPU is now the owner. */
 
+		/* This CPU begins loading/storing data: LMM(cpu_lock:B) */
+
 		*lock_flag = true;
 
 	} else if (old == cpu) {
@@ -3600,7 +3619,14 @@ EXPORT_SYMBOL(printk_cpu_lock_irqsave);
 void printk_cpu_unlock_irqrestore(bool lock_flag, unsigned long irq_flags)
 {
 	if (lock_flag) {
-		atomic_set(&printk_cpulock_owner, -1);
+		/* This CPU is finished loading/storing data: LMM(cpu_unlock:A) */
+
+		/*
+		 * Guarantee loads and stores from this CPU when it was the
+		 * lock owner are visible to the next lock owner. This pairs
+		 * with cpu_lock:A.
+		 */
+		atomic_set_release(&printk_cpulock_owner, -1); /* LMM(cpu_unlock:B) */
 
 		local_irq_restore(irq_flags);
 	}
-- 
2.20.1

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

Hi John,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20210607]

url:    https://github.com/0day-ci/linux/commits/John-Ogness/introduce-printk-cpu-lock/20210608-040454
base:    7f09e895a7f3e0af63bf9ec6c7c22893ec7e6c8e
config: mips-randconfig-r036-20210607 (attached as .config)
compiler: mips-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/136bcc2980e636b2ae156ca63fbe95c713e44c1b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review John-Ogness/introduce-printk-cpu-lock/20210608-040454
        git checkout 136bcc2980e636b2ae156ca63fbe95c713e44c1b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

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: In function 'dump_stack_lvl':
>> lib/dump_stack.c:107:2: warning: 'lock_flag' is used uninitialized in this function [-Wuninitialized]
     107 |  printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/lock_flag +107 lib/dump_stack.c

    90	
    91	/**
    92	 * dump_stack - dump the current task information and its stack trace
    93	 *
    94	 * Architectures can override this implementation by implementing its own.
    95	 */
    96	asmlinkage __visible void dump_stack_lvl(const char *log_lvl)
    97	{
    98		unsigned long irq_flags;
    99		bool lock_flag;
   100	
   101		/*
   102		 * Permit this cpu to perform nested stack dumps while serialising
   103		 * against other CPUs
   104		 */
   105		printk_cpu_lock_irqsave(&lock_flag, &irq_flags);
   106		__dump_stack(log_lvl);
 > 107		printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
   108	}
   109	EXPORT_SYMBOL(dump_stack_lvl);
   110	

---
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: 26602 bytes --]

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

[Petr Mladek]

On Mon 2021-06-07 22:02:31, 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_irqsave()/printk_cpu_unlock_irqrestore()
> so that it is available for others as well. For !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>

There are some nits below but the patch looks fine to me as it.

Reviewed-by: Petr Mladek <pmladek@suse.com>

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3532,3 +3532,78 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
> +void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
> +{
> +	int old;
> +	int cpu;
> +
> +retry:
> +	local_irq_save(*irq_flags);
> +
> +	cpu = smp_processor_id();
> +
> +	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
> +	if (old == -1) {
> +		/* This CPU is now the owner. */
> +

Superfluous space?

> +		*lock_flag = true;

The original name name "was_locked" was more descriptive. I agree that
it was not good for an API. What about keeping the inverted logic and
calling it "lock_nested" ?

I do not resist on any change. The logic is trivial so...

> +
> +	} else if (old == cpu) {
> +		/* This CPU is already the owner. */
> +
> +		*lock_flag = false;
> +

Even more superfluous spaces?

> +	} else {
> +		local_irq_restore(*irq_flags);
> +
> +		/*
> +		 * Wait for the lock to release before jumping to cmpxchg()
> +		 * in order to mitigate the thundering herd problem.
> +		 */
> +		do {
> +			cpu_relax();
> +		} while (atomic_read(&printk_cpulock_owner) != -1);
> +
> +		goto retry;
> +	}
> +}
> +EXPORT_SYMBOL(printk_cpu_lock_irqsave);
> +
> +/*
> + * printk_cpu_unlock_irqrestore: Release the printk cpu-reentrant spinning
> + *                               lock and restore interrupts.
> + * @lock_flag: The current lock state.
> + * @irq_flags: The current irq state.

"The current" is a bit misleading. Both values actually describe
the state before the related printk_cpu_lock_irqsave().
What about something like?

  * @lock_nested: Lock state set when the lock was taken.
  * @irq_flags: IRQ flags stored when the lock was taken.


> + *
> + * 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_irqrestore(bool lock_flag, unsigned long irq_flags)
> +{
> +	if (lock_flag) {
> +		atomic_set(&printk_cpulock_owner, -1);
> +
> +		local_irq_restore(irq_flags);
> +	}
> +}
> +EXPORT_SYMBOL(printk_cpu_unlock_irqrestore);
> +#endif /* CONFIG_SMP */

 Best Regards,
 Petr
 

Re: [PATCH next v2 2/2] printk: fix cpu lock ordering

[Petr Mladek]

On Mon 2021-06-07 22:02:32, John Ogness wrote:
> The cpu lock implementation uses a full memory barrier to take
> the lock, but no memory barriers when releasing the lock. This
> means that changes performed by a lock owner may not be seen by
> the next lock owner. This may have been "good enough" for use
> by dump_stack() as a serialization mechanism, but it is not
> enough to provide proper protection for a critical section.
> 
> Correct this problem by using acquire/release memory barriers
> for lock/unlock, respectively.
> 
> Note that it is not necessary for a cpu lock to disable
> interrupts. However, in upcoming work this cpu lock will be used
> for emergency tasks (for example, atomic consoles during kernel
> crashes) and any interruptions should be avoided if possible.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

The change makes perfect sense and the code looks correct.
But I am not sure about the description of the memory barriers.

> ---
>  kernel/printk/printk.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index f94babb38493..8c870581cfb4 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3560,10 +3560,29 @@ void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
>  
>  	cpu = smp_processor_id();
>  
> -	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
> +	/*
> +	 * 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.

These things are not easy to describe. It took me quite some time to
understand the above description. And think that it does not say
the full storry.

IMHO, the lock should work the way that:

   + The new owner see all writes done or seen by the previous owner(s).
   + The previous owner(s) never see writes done by the new owner.

It is actually a full barrier. I see this in include/linux/atomic.h

#define __atomic_acquire_fence		smp_mb__after_atomic
#define __atomic_release_fence		smp_mb__before_atomic


Well, I am not sure if my description is correct.
Documentation/memory-barriers.txt describes the acquire operation from
another angle:

 (5) ACQUIRE operations.

     This acts as a one-way permeable barrier.  It guarantees that all memory
     operations after the ACQUIRE operation will appear to happen after the
     ACQUIRE operation with respect to the other components of the system.
     ACQUIRE operations include LOCK operations and both smp_load_acquire()
     and smp_cond_load_acquire() operations.

     Memory operations that occur before an ACQUIRE operation may appear to
     happen after it completes.

     An ACQUIRE operation should almost always be paired with a RELEASE
     operation.

> +	 *
> +	 * Memory barrier involvement:
> +	 *
> +	 * If cpu_lock:A reads from cpu_unlock:B, then cpu_lock:B
> +	 * reads from cpu_unlock:A.

IMHO, this seems to describe even smaller part of the picture.
It is just about reads.

*
> +	 * Relies on:
> +	 *
> +	 * RELEASE from cpu_unlock:A to cpu_unlock:B
> +	 *    matching
> +	 * ACQUIRE from cpu_lock:A to cpu_lock:B
> +	 */
> +	old = atomic_cmpxchg_acquire(&printk_cpulock_owner,
> +				     -1, cpu); /* LMM(cpu_lock:A) */
>  	if (old == -1) {
>  		/* This CPU is now the owner. */
>  
> +		/* This CPU begins loading/storing data: LMM(cpu_lock:B) */
> +
>  		*lock_flag = true;
>  
>  	} else if (old == cpu) {
> @@ -3600,7 +3619,14 @@ EXPORT_SYMBOL(printk_cpu_lock_irqsave);
>  void printk_cpu_unlock_irqrestore(bool lock_flag, unsigned long irq_flags)
>  {
>  	if (lock_flag) {
> -		atomic_set(&printk_cpulock_owner, -1);
> +		/* This CPU is finished loading/storing data: LMM(cpu_unlock:A) */
> +
> +		/*
> +		 * Guarantee loads and stores from this CPU when it was the
> +		 * lock owner are visible to the next lock owner. This pairs
> +		 * with cpu_lock:A.

Yup, this seems to better match the formulation in
Documentation/memory-barriers.txt:

 (6) RELEASE operations.

     This also acts as a one-way permeable barrier.  It guarantees that all
     memory operations before the RELEASE operation will appear to happen
     before the RELEASE operation with respect to the other components of the
     system. RELEASE operations include UNLOCK operations and
     smp_store_release() operations.

     Memory operations that occur after a RELEASE operation may appear to
     happen before it completes.


Except that the acquire description does not mention "this CPU".

> +		 */
> +		atomic_set_release(&printk_cpulock_owner, -1); /* LMM(cpu_unlock:B) */
>  
>  		local_irq_restore(irq_flags);
>  	}

Honestly, I am not sure if we could describe the barriers correctly
and effectively at the same time.

IMHO, the following sentence, in the commit message, says everything:

   Correct this problem by using acquire/release memory barriers
   for lock/unlock, respectively.


In the code, I would use something similar:

	/* Try to take the lock with the appropriate memory barriers. */
	old = atomic_cmpxchg_acquire(&printk_cpulock_owner, -1, cpu);

and

		/* Release the lock with the appropriate memory barriers. */
		atomic_set_release(&printk_cpulock_owner, -1);


After all, the _acquire() and _release() variants were introduced
to implement locking operations correctly and effectively.
AFAIK, there were long discussions about the implementation and
documentation. IMHO, we do not need to duplicate it.

Best Regards,
Petr

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

[Petr Mladek]

On Tue 2021-06-08 10:43:46, kernel test robot wrote:
> Hi John,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on next-20210607]
> 
> url:    https://github.com/0day-ci/linux/commits/John-Ogness/introduce-printk-cpu-lock/20210608-040454
> base:    7f09e895a7f3e0af63bf9ec6c7c22893ec7e6c8e
> config: mips-randconfig-r036-20210607 (attached as .config)
> compiler: mips-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/136bcc2980e636b2ae156ca63fbe95c713e44c1b
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review John-Ogness/introduce-printk-cpu-lock/20210608-040454
>         git checkout 136bcc2980e636b2ae156ca63fbe95c713e44c1b
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 
> 
> 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: In function 'dump_stack_lvl':
> >> lib/dump_stack.c:107:2: warning: 'lock_flag' is used uninitialized in this function [-Wuninitialized]
>      107 |  printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
>          |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Interesting. I am curious that it does not complain also about
irq_flags. But it is possible the it reports only the first problem.

Anyway, we will likely need to do some trickery via #define to tell
the compiler that the value is set. I mean to do similar thing as:

#define raw_local_irq_save(flags)			\
	do {						\
		typecheck(unsigned long, flags);	\
		flags = arch_local_irq_save();		\
	} while (0)


In our case, it might look like:

#define printk_cpu_lock_irqsave(lock_nested, irq_flags)		\
	do {							\
		local_irq_save(irq_flags);			\
		typecheck(bool, lock_nested);			\
		lock_nested = __printk_cpu_lock(irq_flags);	\
	} while (0)


then we would need to do in __prink_cpu_lock(unsigned long irq_flags)


	} else {
		local_irq_restore(irq_flags);

		/*
		 * Wait for the lock to release before jumping to cmpxchg()
		 * in order to mitigate the thundering herd problem.
		 */
		do {
			cpu_relax();
		} while (atomic_read(&printk_cpulock_owner) != -1);

		local_irq_save(irq_flags)
		goto retry;
	}

Best Regards,
Petr

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

[John Ogness]

On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -3532,3 +3532,78 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
>> +void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
>> +{
>> +	int old;
>> +	int cpu;
>> +
>> +retry:
>> +	local_irq_save(*irq_flags);
>> +
>> +	cpu = smp_processor_id();
>> +
>> +	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
>> +	if (old == -1) {
>> +		/* This CPU is now the owner. */
>> +
>
> Superfluous space?

I was concerned that people may associate the comment with the following
line of code. Especially in the next patch when many more lines are
added. The comment is for the whole conditional block.

>> +		*lock_flag = true;
>
> The original name name "was_locked" was more descriptive. I agree that
> it was not good for an API. What about keeping the inverted logic and
> calling it "lock_nested" ?
>
> I do not resist on any change. The logic is trivial so...

I wanted it to be an opaque variable, which is why it is named so. But I
can rename it for v3. There is no need to debate naming here.

>> +
>> +	} else if (old == cpu) {
>> +		/* This CPU is already the owner. */
>> +
>> +		*lock_flag = false;
>> +
>
> Even more superfluous spaces?

This code is not trivial and it is absolutely critical when we introduce
atomic consoles. My goal is clarity rather than compactness
(particularly after proper memory barrier comments are added).

>> +	} else {
>> +		local_irq_restore(*irq_flags);
>> +
>> +		/*
>> +		 * Wait for the lock to release before jumping to cmpxchg()
>> +		 * in order to mitigate the thundering herd problem.
>> +		 */
>> +		do {
>> +			cpu_relax();
>> +		} while (atomic_read(&printk_cpulock_owner) != -1);
>> +
>> +		goto retry;
>> +	}
>> +}
>> +EXPORT_SYMBOL(printk_cpu_lock_irqsave);
>> +
>> +/*
>> + * printk_cpu_unlock_irqrestore: Release the printk cpu-reentrant spinning
>> + *                               lock and restore interrupts.
>> + * @lock_flag: The current lock state.
>> + * @irq_flags: The current irq state.
>
> "The current" is a bit misleading. Both values actually describe
> the state before the related printk_cpu_lock_irqsave().
> What about something like?
>
>   * @lock_nested: Lock state set when the lock was taken.
>   * @irq_flags: IRQ flags stored when the lock was taken.

OK. I will make this change for v3.

John Ogness

Re: [PATCH next v2 2/2] printk: fix cpu lock ordering

[John Ogness]

On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
> The change makes perfect sense and the code looks correct.
> But I am not sure about the description of the memory barriers.

OK.

>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index f94babb38493..8c870581cfb4 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -3560,10 +3560,29 @@ void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
>>  
>>  	cpu = smp_processor_id();
>>  
>> -	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
>> +	/*
>> +	 * 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.
>
> These things are not easy to describe. It took me quite some time to
> understand the above description. And think that it does not say
> the full storry.
>
> IMHO, the lock should work the way that:
>
>    + The new owner see all writes done or seen by the previous owner(s).
>    + The previous owner(s) never see writes done by the new owner.

You are right. I can describe those independently.

> Honestly, I am not sure if we could describe the barriers correctly
> and effectively at the same time.

For v3 I would describe the 2 cases separately. For lock/acquire:

	/*
	 * Guarantee loads and stores from this CPU when it is the lock owner
	 * are _not_ visible to the previous lock owner. This pairs with
	 * cpu_unlock:B.
	 *
	 * Memory barrier involvement:
	 *
	 * If cpu_lock:A reads from cpu_unlock:B, then cpu_unlock:A can never
	 * read from cpu_lock:B.
	 *
	 * Relies on:
	 *
	 * RELEASE from cpu_unlock:A to cpu_unlock:B
	 *    matching
	 * ACQUIRE from cpu_lock:A to cpu_lock:B
	 */

And for unlock/release:

	/*
	 * Guarantee loads and stores from this CPU when it was the
	 * lock owner are visible to the next 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
	 */

I know you are not a fan of these drawn out memory barrier comments. But
it really simplifies verification and translation to litmus
tests. Without such comments, I would be lost looking back at
printk_ringbuffer.c.

If the previous dump_stack() cpu lock implementation had such comments,
we would know if the missing memory barriers were by design.

John Ogness

Re: [PATCH next v2 2/2] printk: fix cpu lock ordering

[Petr Mladek]

On Tue 2021-06-08 16:18:51, John Ogness wrote:
> On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
> > The change makes perfect sense and the code looks correct.
> > But I am not sure about the description of the memory barriers.
> 
> OK.
> 
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index f94babb38493..8c870581cfb4 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -3560,10 +3560,29 @@ void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
> >>  
> >>  	cpu = smp_processor_id();
> >>  
> >> -	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
> >> +	/*
> >> +	 * 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.
> >
> > These things are not easy to describe. It took me quite some time to
> > understand the above description. And think that it does not say
> > the full storry.
> >
> > IMHO, the lock should work the way that:
> >
> >    + The new owner see all writes done or seen by the previous owner(s).
> >    + The previous owner(s) never see writes done by the new owner.
> 
> You are right. I can describe those independently.
> 
> > Honestly, I am not sure if we could describe the barriers correctly
> > and effectively at the same time.
> 
> For v3 I would describe the 2 cases separately. For lock/acquire:
> 
> 	/*
> 	 * Guarantee loads and stores from this CPU when it is the lock owner
> 	 * are _not_ visible to the previous lock owner. This pairs with
> 	 * cpu_unlock:B.

Sounds better to me.

*
> 	 * Memory barrier involvement:
> 	 *
> 	 * If cpu_lock:A reads from cpu_unlock:B, then cpu_unlock:A can never
> 	 * read from cpu_lock:B.
> 	 *
> 	 * Relies on:
> 	 *
> 	 * RELEASE from cpu_unlock:A to cpu_unlock:B
> 	 *    matching
> 	 * ACQUIRE from cpu_lock:A to cpu_lock:B
> 	 */

I can't check this so I believe you ;-)

> And for unlock/release:
> 
> 	/*
> 	 * Guarantee loads and stores from this CPU when it was the
> 	 * lock owner are visible to the next lock owner. This pairs
> 	 * with cpu_lock:A.

Sounds good.

*
> 	 * 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
> 	 */

Same as for acquire ;-)

> I know you are not a fan of these drawn out memory barrier comments. But
> it really simplifies verification and translation to litmus
> tests. Without such comments, I would be lost looking back at
> printk_ringbuffer.c.
> 
> If the previous dump_stack() cpu lock implementation had such comments,
> we would know if the missing memory barriers were by design.

Sure. I am fine with the comments as long as there is also some human
readable description of the barrier intention.

Best Regards,
Petr

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

[Petr Mladek]

On Tue 2021-06-08 15:55:35, John Ogness wrote:
> On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> >
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -3532,3 +3532,78 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
> >> +void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
> >> +{
> >> +	int old;
> >> +	int cpu;
> >> +
> >> +retry:
> >> +	local_irq_save(*irq_flags);
> >> +
> >> +	cpu = smp_processor_id();
> >> +
> >> +	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
> >> +	if (old == -1) {
> >> +		/* This CPU is now the owner. */
> >> +
> >
> > Superfluous space?
> 
> I was concerned that people may associate the comment with the following
> line of code. Especially in the next patch when many more lines are
> added. The comment is for the whole conditional block.
> 
> >> +		*lock_flag = true;
> >
> > The original name name "was_locked" was more descriptive. I agree that
> > it was not good for an API. What about keeping the inverted logic and
> > calling it "lock_nested" ?
> >
> > I do not resist on any change. The logic is trivial so...
> 
> I wanted it to be an opaque variable, which is why it is named so. But I
> can rename it for v3. There is no need to debate naming here.

Yup. I didn't want to block the patch because of this. I mentioned it
just for case v3 was needed and you agreed. Feel free to keep your
preferred names and spacing. I am not going to fight over it.

Best Regards,
Petr

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

[John Ogness]

On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
>>    lib/dump_stack.c: In function 'dump_stack_lvl':
>> >> lib/dump_stack.c:107:2: warning: 'lock_flag' is used uninitialized in this function [-Wuninitialized]
>>      107 |  printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
>>          |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Interesting. I am curious that it does not complain also about
> irq_flags. But it is possible the it reports only the first problem.

Strangely enough, if I set a value for @lock_flag, it is happy and does
not complain about @irq_flags. Probably a compiler oversight.

> Anyway, we will likely need to do some trickery via #define to tell
> the compiler that the value is set.

This is on ARCH=mips and !CONFIG_SMP. So the value is _not_ getting
set. (The static inline function does nothing.)

By changing printk_cpu_unlock_irqrestore() to use pointers:

    static inline void printk_cpu_unlock_irqrestore(bool *lock_flag, unsigned long *irq_flags)

then the warning disappears. Indeed, by not using pointers on unlock,
technically data is copied that was never initialized. I thought maybe
the compiler would optimize all that out, but it seems that it does not.

I have no problems using pointers for unlock(). It was strange using
pointers for lock(), but not for unlock() anyway.

Or would you prefer something else?

John Ogness

Re: [PATCH next v2 2/2] printk: fix cpu lock ordering

[John Ogness]

On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
>> 	/*
>> 	 * Guarantee loads and stores from this CPU when it is the lock owner
>> 	 * are _not_ visible to the previous lock owner. This pairs with
>> 	 * cpu_unlock:B.
>> 	 *
>> 	 * Memory barrier involvement:
>> 	 *
>> 	 * If cpu_lock:A reads from cpu_unlock:B, then cpu_unlock:A can never
>> 	 * read from cpu_lock:B.
>> 	 *
>> 	 * Relies on:
>> 	 *
>> 	 * RELEASE from cpu_unlock:A to cpu_unlock:B
>> 	 *    matching
>> 	 * ACQUIRE from cpu_lock:A to cpu_lock:B
>> 	 */
>
> I can't check this so I believe you ;-)

I appreciate your confidence in me, but for the record, we should
operate on proofs. Here is a litmus test for this case that is only
using the 2 memory barriers described in the coments. I also added
commented out non-memory-barrier variants so you can quickly check what
happens if either memory barrier is removed.

-------- BEGIN prevent_backwards_leak.litmus --------
C prevent_backwards_leak

(*
 * Guarantee a previous CPU (P0) in the critical section cannot
 * see data stored by the next CPU (P1) in the critical section.
 *
 * RELEASE in P0 matches ACQUIRE in P1
 *)

{ }

P0(int *lck, int *var)
{
	int old;
	int val;

	old = atomic_cmpxchg_relaxed(lck, 0, 1);
	if (old == 0) {
		val = *var;
		atomic_set_release(lck, 2);
		//atomic_set(lck, 2);
	}
}

P1(int *lck, int *var)
{
	int old;

	old = atomic_cmpxchg_acquire(lck, 2, 3);
	//old = atomic_cmpxchg_relaxed(lck, 2, 3);
	if (old == 2) {
		*var = 1;
		atomic_set(lck, 3);
	}
}

exists (1:old=2 /\ 0:val=1)
-------- END prevent_backwards_leak.litmus --------

And running it shows:

$ herd7 -conf linux-kernel.cfg prevent_backwards_leak.litmus
Test prevent_backwards_leak Allowed
States 3
0:val=0; 1:old=0;
0:val=0; 1:old=1;
0:val=0; 1:old=2;
No
Witnesses
Positive: 0 Negative: 3
Condition exists (1:old=2 /\ 0:val=1)
Observation prevent_backwards_leak Never 0 3
Time prevent_backwards_leak 0.01
Hash=a83f3a63382111d7f61810639fa38ad4

If either of the two memory barriers are removed, the results will show
that @val in first CPU (P0) can be 1 (0:val=1), which was the value set
by the following CPU (P1) in the critical section.

>> 	/*
>> 	 * Guarantee loads and stores from this CPU when it was the
>> 	 * lock owner are visible to the next 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
>> 	 */
>
> Same as for acquire ;-)

And here is the litmus test for this case, also with extra commented out
non-memory-barrier variants.

-------- BEGIN guarantee_forward_visibility.litmus --------
C guarantee_forward_visibility

(*
 * Guarantee data stored by a previous CPU (P0) in the critical
 * section is always visible to the next CPU (P1) in the critical
 * section.
 *
 * RELEASE in P0 matches ACQUIRE in P1
 *)

{ }

P0(int *lck, int *var)
{
	int old;

	old = atomic_cmpxchg_relaxed(lck, 0, 1);
	if (old == 0) {
		*var = 1;
		atomic_set_release(lck, 2);
		//atomic_set(lck, 2);
	}
}

P1(int *lck, int *var)
{
	int old;
	int val;

	old = atomic_cmpxchg_acquire(lck, 2, 3);
	//old = atomic_cmpxchg_relaxed(lck, 2, 3);
	if (old == 2) {
		val = *var;
		atomic_set(lck, 3);
	}
}

exists (1:old=2 /\ 1:val=0 )
-------- END guarantee_forward_visibility.litmus --------

$ herd7 -conf linux-kernel.cfg guarantee_forward_visibility.litmus
Test guarantee_forward_visibility Allowed
States 3
1:old=0; 1:val=0;
1:old=1; 1:val=0;
1:old=2; 1:val=1;
No
Witnesses
Positive: 0 Negative: 3
Condition exists (1:old=2 /\ 1:val=0)
Observation guarantee_forward_visibility Never 0 3
Time guarantee_forward_visibility 0.00
Hash=fad189f07da06da99b97a7ab1215a5dc

Also here, if either of the memory barriers are removed, @val in the
later CPU (P1) can be 0 (1:val=0). Meaning that the a following CPU in
the critical section would not see a value set by the previous CPU in
the critical section.

John Ogness

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

[Petr Mladek]

On Thu 2021-06-10 15:26:15, John Ogness wrote:
> On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
> >>    lib/dump_stack.c: In function 'dump_stack_lvl':
> >> >> lib/dump_stack.c:107:2: warning: 'lock_flag' is used uninitialized in this function [-Wuninitialized]
> >>      107 |  printk_cpu_unlock_irqrestore(lock_flag, irq_flags);
> >>          |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Interesting. I am curious that it does not complain also about
> > irq_flags. But it is possible the it reports only the first problem.
> 
> Strangely enough, if I set a value for @lock_flag, it is happy and does
> not complain about @irq_flags. Probably a compiler oversight.

Yeah, it is strange.

> > Anyway, we will likely need to do some trickery via #define to tell
> > the compiler that the value is set.
> 
> This is on ARCH=mips and !CONFIG_SMP. So the value is _not_ getting
> set. (The static inline function does nothing.)
> 
> By changing printk_cpu_unlock_irqrestore() to use pointers:
> 
>     static inline void printk_cpu_unlock_irqrestore(bool *lock_flag, unsigned long *irq_flags)
> 
> then the warning disappears. Indeed, by not using pointers on unlock,
> technically data is copied that was never initialized. I thought maybe
> the compiler would optimize all that out, but it seems that it does not.
> 
> I have no problems using pointers for unlock(). It was strange using
> pointers for lock(), but not for unlock() anyway.
> 
> Or would you prefer something else?

I would actually prefer to introduce the macros and pass the flags
without referencing.

I was about to write that I did not mind. But then it came to me that
it might be worth being compatible with the other
irqsafe()/irqrestore() APIs. It seems that people are pretty used
to pass flags directly:

$> git grep irqsave.*flags | wc -l
17084
$> git grep irqsave.*\&flags | wc -l
15

That said, I do not resist on it. It will not block the patchset
if you decided to used the pointers. The lock should not be used
widely...

Best Regards,
Petr