Re: [PATCH 1/2] kdb: Properly synchronize vkdb_printf() calls with other CPUs

[Petr Mladek]

On Mon 2016-11-07 10:07:36, Daniel Thompson wrote:
> On 21/10/16 13:50, Petr Mladek wrote:
> >kdb_printf_lock does not prevent other CPUs from entering the critical
> >section because it is ignored when KDB_STATE_PRINTF_LOCK is set.
> >
> >The problematic situation might look like:
> >
> > ...
> >
> >Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
> >---
> > kernel/debug/kdb/kdb_io.c      | 36 +++++++++++++++++-------------------
> > kernel/debug/kdb/kdb_private.h |  1 -
> > 2 files changed, 17 insertions(+), 20 deletions(-)
> >
> >diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> >index fc1ef736253c..227b59ec7dbe 100644
> >--- a/kernel/debug/kdb/kdb_io.c
> >+++ b/kernel/debug/kdb/kdb_io.c
> >@@ -555,16 +555,16 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> > 	int colcount;
> > 	int logging, saved_loglevel = 0;
> > 	int saved_trap_printk;
> >-	int got_printf_lock = 0;
> > 	int retlen = 0;
> > 	int fnd, len;
> >+	int this_cpu, old_cpu;
> >+	static int kdb_printf_cpu = -1;
> > 	char *cp, *cp2, *cphold = NULL, replaced_byte = ' ';
> > 	char *moreprompt = "more> ";
> > 	struct console *c = console_drivers;
> >-	static DEFINE_SPINLOCK(kdb_printf_lock);
> > 	unsigned long uninitialized_var(flags);
> >
> >-	preempt_disable();
> >+	local_irq_save(flags);
> > 	saved_trap_printk = kdb_trap_printk;
> > 	kdb_trap_printk = 0;
> >
> >@@ -572,13 +572,14 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> > 	 * But if any cpu goes recursive in kdb, just print the output,
> > 	 * even if it is interleaved with any other text.
> > 	 */
> >-	if (!KDB_STATE(PRINTF_LOCK)) {
> >-		KDB_STATE_SET(PRINTF_LOCK);
> >-		spin_lock_irqsave(&kdb_printf_lock, flags);
> >-		got_printf_lock = 1;
> >-		atomic_inc(&kdb_event);
> >-	} else {
> >-		__acquire(kdb_printf_lock);
> >+	this_cpu = smp_processor_id();
> >+	atomic_inc(&kdb_event);
> 
> When reviewing I noticed that, when we recursively enter, kdb_event
> is handled differently after this patch so I wanted to figure out if
> this alternative handling of kdb_event was safe.
> 
> In the end I concluded it is safe but that's mostly because the
> *only* thing we ever seem to do with kdb_event is increment and
> decrement it. So perhaps adding another patch at the front of this
> series to nuke this variable would be worthwhile (whilst making this
> patch easier to read).

Yeah, I was curious about kdb_event as well. I wondered if it might
be used by some 3rd party stuff.

I can find only one usage. It was supposed to affect
WARN_CONSOLE_UNLOCKED(), see
http://www.spinics.net/lists/kdb/msg01733.html
But this was never upstreamed, so kdb_event really looks like an
historical artifact that might get removed. I am going to so in v2.

> However, your choice and, either way:
> Reviewed-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>

Thanks a lot for review.

Best Regards,
Petr