Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk-rework 09/12] um: synchronize kmsg_dumper | Date | Mon, 01 Feb 2021 17:57:14 +0106 |
| |
(Added CC: Thomas Meyer, Richard Weinberger)
On 2021-02-01, Petr Mladek <pmladek@suse.com> wrote: >> In preparation for removing printk's @logbuf_lock, dumpers that have >> assumed to be protected against parallel calls must provide their own >> synchronization. Add a locally static spinlock to synchronize the >> kmsg_dump call and temporary buffer usage. >> >> Signed-off-by: John Ogness <john.ogness@linutronix.de> >> --- >> arch/um/kernel/kmsg_dump.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c >> index f38349ad00ea..173999422ed8 100644 >> --- a/arch/um/kernel/kmsg_dump.c >> +++ b/arch/um/kernel/kmsg_dump.c >> @@ -1,5 +1,6 @@ >> // SPDX-License-Identifier: GPL-2.0 >> #include <linux/kmsg_dump.h> >> +#include <linux/spinlock.h> >> #include <linux/console.h> >> #include <shared/init.h> >> #include <shared/kern.h> >> @@ -9,8 +10,10 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, >> enum kmsg_dump_reason reason, >> struct kmsg_dumper_iter *iter) >> { >> + static DEFINE_SPINLOCK(lock); >> static char line[1024]; >> struct console *con; >> + unsigned long flags; >> size_t len = 0; >> >> /* only dump kmsg when no console is available */ >> @@ -25,11 +28,16 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, >> if (con) >> return; >> >> + if (!spin_trylock_irqsave(&lock, flags)) >> + return; >> + >> printf("kmsg_dump:\n"); >> while (kmsg_dump_get_line(iter, true, line, sizeof(line), &len)) { >> line[len] = '\0'; >> printf("%s", line); >> } >> + >> + spin_unlock_irqrestore(&lock, flags); > > What exactly is synchronized here, please? > Access to @line buffer or @iter or both?
@line is being synchronized. @iter does not require synchronization.
> It looks to me that the access to @line buffer was not synchronized > before. kmsg_dump_get_line() used a lock internally but > it was not synchronized with the later printf("%s", line);
The line was previously synchronized for the kmsg_dump_get_line() call. But yes, it was not synchronized after the call, which is a bug if the dump is triggered on multiple CPUs simultaneously. The commit message should also mention that it is handling that bug.
> IMHO, this patch is not needed.
I am not familiar enough with ARCH=um to know if dumps can be triggered on multiple CPUs simultaneously. Perhaps ThomasM or Richard can chime in here.
John Ogness
| |