Messages in this thread | | | Date | Tue, 22 Nov 2016 11:34:29 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH 1/2] kdb: Properly synchronize vkdb_printf() calls with other CPUs |
| |
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@suse.com> > >--- > > 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@linaro.org>
Thanks a lot for review.
Best Regards, Petr
| |