| Message ID | 20210225202438.28985-3-john.ogness@linutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | printk: remove logbuf_lock | expand |
On Thu 2021-02-25 21:24:25, John Ogness wrote: > The kmsg_dumper can be called from any context and CPU, possibly > from multiple CPUs simultaneously. Since the writing of the buffer > can occur from a later scheduled work queue, the oops buffer must > be protected against simultaneous dumping. > > Use an atomic bit to mark when the buffer is protected. Release the > protection in between setting the buffer and the actual writing in > order for a possible panic (immediate write) to be written during > the scheduling of a previous oops (delayed write). Just to be sure. You did not use spin lock to prevent problems with eventual double unlock in panic(). Do I get it correctly, please? > Signed-off-by: John Ogness <john.ogness@linutronix.de> Anyway, it looks good to me. Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On 2021-03-01, Petr Mladek <pmladek@suse.com> wrote: >> The kmsg_dumper can be called from any context and CPU, possibly >> from multiple CPUs simultaneously. Since the writing of the buffer >> can occur from a later scheduled work queue, the oops buffer must >> be protected against simultaneous dumping. >> >> Use an atomic bit to mark when the buffer is protected. Release the >> protection in between setting the buffer and the actual writing in >> order for a possible panic (immediate write) to be written during >> the scheduling of a previous oops (delayed write). > > Just to be sure. You did not use spin lock to prevent problems > with eventual double unlock in panic(). Do I get it correctly, > please? I do not understand what possible double unlock you are referring to. I chose not to use spinlocks because I wanted something that does not cause any scheduling or preemption side-effects for mtd. The mtd dumper sometimes dumps directly, sometimes delayed (via scheduled work), and they use different mtd callbacks in different contexts. mtd_write() expects to be called in a non-atomic context. The callbacks can take a mutex. John Ogness
On Tue 2021-03-02 11:45:27, John Ogness wrote: > On 2021-03-01, Petr Mladek <pmladek@suse.com> wrote: > >> The kmsg_dumper can be called from any context and CPU, possibly > >> from multiple CPUs simultaneously. Since the writing of the buffer > >> can occur from a later scheduled work queue, the oops buffer must > >> be protected against simultaneous dumping. > >> > >> Use an atomic bit to mark when the buffer is protected. Release the > >> protection in between setting the buffer and the actual writing in > >> order for a possible panic (immediate write) to be written during > >> the scheduling of a previous oops (delayed write). > > > > Just to be sure. You did not use spin lock to prevent problems > > with eventual double unlock in panic(). Do I get it correctly, > > please? > > I do not understand what possible double unlock you are referring to. I was wrong. I meant the tricks that are under in console drivrers, for example: static void mvebu_uart_console_write(struct console *co, const char *s, unsigned int count) { int locked = 1; if (oops_in_progress) locked = spin_trylock_irqsave(&port->lock, flags); else spin_lock_irqsave(&port->lock, flags); /* do the job */ if (locked) spin_unlock_irqrestore(&port->lock, flags); } But this is not a problem here because the kmsg dumper bails out when the lock could not be taken. > I chose not to use spinlocks because I wanted something that does not > cause any scheduling or preemption side-effects for mtd. The mtd dumper > sometimes dumps directly, sometimes delayed (via scheduled work), and > they use different mtd callbacks in different contexts. > > mtd_write() expects to be called in a non-atomic context. The callbacks > can take a mutex. Makes sense. Could you please mention this in the commit message? Best Regards, Petr
diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c index 774970bfcf85..8bbfba40a554 100644 --- a/drivers/mtd/mtdoops.c +++ b/drivers/mtd/mtdoops.c @@ -52,6 +52,7 @@ static struct mtdoops_context { int nextcount; unsigned long *oops_page_used; + unsigned long oops_buf_busy; void *oops_buf; } oops_cxt; @@ -180,6 +181,9 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic) u32 *hdr; int ret; + if (test_and_set_bit(0, &cxt->oops_buf_busy)) + return; + /* Add mtdoops header to the buffer */ hdr = cxt->oops_buf; hdr[0] = cxt->nextcount; @@ -190,7 +194,7 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic) record_size, &retlen, cxt->oops_buf); if (ret == -EOPNOTSUPP) { printk(KERN_ERR "mtdoops: Cannot write from panic without panic_write\n"); - return; + goto out; } } else ret = mtd_write(mtd, cxt->nextpage * record_size, @@ -203,6 +207,8 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic) memset(cxt->oops_buf, 0xff, record_size); mtdoops_inc_counter(cxt); +out: + clear_bit(0, &cxt->oops_buf_busy); } static void mtdoops_workfunc_write(struct work_struct *work) @@ -276,8 +282,11 @@ static void mtdoops_do_dump(struct kmsg_dumper *dumper, if (reason == KMSG_DUMP_OOPS && !dump_oops) return; + if (test_and_set_bit(0, &cxt->oops_buf_busy)) + return; kmsg_dump_get_buffer(dumper, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE, record_size - MTDOOPS_HEADER_SIZE, NULL); + clear_bit(0, &cxt->oops_buf_busy); if (reason != KMSG_DUMP_OOPS) { /* Panics must be written immediately */ @@ -394,6 +403,7 @@ static int __init mtdoops_init(void) return -ENOMEM; } memset(cxt->oops_buf, 0xff, record_size); + cxt->oops_buf_busy = 0; INIT_WORK(&cxt->work_erase, mtdoops_workfunc_erase); INIT_WORK(&cxt->work_write, mtdoops_workfunc_write);
The kmsg_dumper can be called from any context and CPU, possibly from multiple CPUs simultaneously. Since the writing of the buffer can occur from a later scheduled work queue, the oops buffer must be protected against simultaneous dumping. Use an atomic bit to mark when the buffer is protected. Release the protection in between setting the buffer and the actual writing in order for a possible panic (immediate write) to be written during the scheduling of a previous oops (delayed write). Signed-off-by: John Ogness <john.ogness@linutronix.de> --- drivers/mtd/mtdoops.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)