| Message ID | 20210225202438.28985-13-john.ogness@linutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | printk: remove logbuf_lock | expand |
On Thu, Feb 25, 2021 at 09:24:35PM +0100, John Ogness wrote: > Rather than storing the iterator information in the registered > kmsg_dumper structure, create a separate iterator structure. The > kmsg_dump_iter structure can reside on the stack of the caller, thus > allowing lockless use of the kmsg_dump functions. > > This change also means that the kmsg_dumper dump() callback no > longer needs to pass in the kmsg_dumper as an argument. If > kmsg_dumpers want to access the kernel logs, they can use the new > iterator. > > Update the kmsg_dumper callback prototype. Update code that accesses > the kernel logs using the kmsg_dumper structure to use the new > kmsg_dump_iter structure. For kmsg_dumpers, this also means adding a > call to kmsg_dump_rewind() to initialize the iterator. > > All this is in preparation for removal of @logbuf_lock. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> > --- > arch/powerpc/kernel/nvram_64.c | 14 +++--- > arch/powerpc/platforms/powernv/opal-kmsg.c | 3 +- > arch/powerpc/xmon/xmon.c | 6 +-- > arch/um/kernel/kmsg_dump.c | 8 +-- > drivers/hv/vmbus_drv.c | 7 +-- > drivers/mtd/mtdoops.c | 8 +-- > fs/pstore/platform.c | 8 +-- Reviewed-by: Kees Cook <keescook@chromium.org> # pstore -Kees > include/linux/kmsg_dump.h | 38 ++++++++------- > kernel/debug/kdb/kdb_main.c | 10 ++-- > kernel/printk/printk.c | 57 ++++++++++------------ > 10 files changed, 81 insertions(+), 78 deletions(-) > > diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c > index 532f22637783..5a64b24a91c2 100644 > --- a/arch/powerpc/kernel/nvram_64.c > +++ b/arch/powerpc/kernel/nvram_64.c > @@ -72,8 +72,7 @@ static const char *nvram_os_partitions[] = { > NULL > }; > > -static void oops_to_nvram(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason); > +static void oops_to_nvram(enum kmsg_dump_reason reason); > > static struct kmsg_dumper nvram_kmsg_dumper = { > .dump = oops_to_nvram > @@ -642,11 +641,11 @@ void __init nvram_init_oops_partition(int rtas_partition_exists) > * that we think will compress sufficiently to fit in the lnx,oops-log > * partition. If that's too much, go back and capture uncompressed text. > */ > -static void oops_to_nvram(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason) > +static void oops_to_nvram(enum kmsg_dump_reason reason) > { > struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf; > static unsigned int oops_count = 0; > + static struct kmsg_dump_iter iter; > static bool panicking = false; > static DEFINE_SPINLOCK(lock); > unsigned long flags; > @@ -681,13 +680,14 @@ static void oops_to_nvram(struct kmsg_dumper *dumper, > return; > > if (big_oops_buf) { > - kmsg_dump_get_buffer(dumper, false, > + kmsg_dump_rewind(&iter); > + kmsg_dump_get_buffer(&iter, false, > big_oops_buf, big_oops_buf_sz, &text_len); > rc = zip_oops(text_len); > } > if (rc != 0) { > - kmsg_dump_rewind(dumper); > - kmsg_dump_get_buffer(dumper, false, > + kmsg_dump_rewind(&iter); > + kmsg_dump_get_buffer(&iter, false, > oops_data, oops_data_sz, &text_len); > err_type = ERR_TYPE_KERNEL_PANIC; > oops_hdr->version = cpu_to_be16(OOPS_HDR_VERSION); > diff --git a/arch/powerpc/platforms/powernv/opal-kmsg.c b/arch/powerpc/platforms/powernv/opal-kmsg.c > index 6c3bc4b4da98..a7bd6ac681f4 100644 > --- a/arch/powerpc/platforms/powernv/opal-kmsg.c > +++ b/arch/powerpc/platforms/powernv/opal-kmsg.c > @@ -19,8 +19,7 @@ > * may not be completely printed. This function does not actually dump the > * message, it just ensures that OPAL completely flushes the console buffer. > */ > -static void kmsg_dump_opal_console_flush(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason) > +static void kmsg_dump_opal_console_flush(enum kmsg_dump_reason reason) > { > /* > * Outside of a panic context the pollers will continue to run, > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index 80ed3e1becf9..5978b90a885f 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -3001,7 +3001,7 @@ print_address(unsigned long addr) > static void > dump_log_buf(void) > { > - struct kmsg_dumper dumper; > + struct kmsg_dump_iter iter; > unsigned char buf[128]; > size_t len; > > @@ -3013,9 +3013,9 @@ dump_log_buf(void) > catch_memory_errors = 1; > sync(); > > - kmsg_dump_rewind_nolock(&dumper); > + kmsg_dump_rewind_nolock(&iter); > xmon_start_pagination(); > - while (kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf), &len)) { > + while (kmsg_dump_get_line_nolock(&iter, false, buf, sizeof(buf), &len)) { > buf[len] = '\0'; > printf("%s", buf); > } > diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c > index 4869e2cc787c..9fbc5e5b1023 100644 > --- a/arch/um/kernel/kmsg_dump.c > +++ b/arch/um/kernel/kmsg_dump.c > @@ -7,9 +7,9 @@ > #include <shared/kern.h> > #include <os.h> > > -static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason) > +static void kmsg_dumper_stdout(enum kmsg_dump_reason reason) > { > + static struct kmsg_dump_iter iter; > static DEFINE_SPINLOCK(lock); > static char line[1024]; > struct console *con; > @@ -34,8 +34,10 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, > if (!spin_trylock(&lock)) > return; > > + kmsg_dump_rewind(&iter); > + > printf("kmsg_dump:\n"); > - while (kmsg_dump_get_line(dumper, true, line, sizeof(line), &len)) { > + while (kmsg_dump_get_line(&iter, true, line, sizeof(line), &len)) { > line[len] = '\0'; > printf("%s", line); > } > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 10dce9f91216..1b858f280e22 100644 > --- a/drivers/hv/vmbus_drv.c > +++ b/drivers/hv/vmbus_drv.c > @@ -1388,9 +1388,9 @@ static void vmbus_isr(void) > * Callback from kmsg_dump. Grab as much as possible from the end of the kmsg > * buffer and call into Hyper-V to transfer the data. > */ > -static void hv_kmsg_dump(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason) > +static void hv_kmsg_dump(enum kmsg_dump_reason reason) > { > + struct kmsg_dump_iter iter; > size_t bytes_written; > phys_addr_t panic_pa; > > @@ -1404,7 +1404,8 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper, > * Write dump contents to the page. No need to synchronize; panic should > * be single-threaded. > */ > - kmsg_dump_get_buffer(dumper, false, hv_panic_page, HV_HYP_PAGE_SIZE, > + kmsg_dump_rewind(&iter); > + kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE, > &bytes_written); > if (bytes_written) > hyperv_report_panic_msg(panic_pa, bytes_written); > diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c > index 8bbfba40a554..d179b726a1c9 100644 > --- a/drivers/mtd/mtdoops.c > +++ b/drivers/mtd/mtdoops.c > @@ -272,19 +272,21 @@ static void find_next_position(struct mtdoops_context *cxt) > mtdoops_inc_counter(cxt); > } > > -static void mtdoops_do_dump(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason) > +static void mtdoops_do_dump(enum kmsg_dump_reason reason) > { > struct mtdoops_context *cxt = container_of(dumper, > struct mtdoops_context, dump); > + struct kmsg_dump_iter iter; > > /* Only dump oopses if dump_oops is set */ > if (reason == KMSG_DUMP_OOPS && !dump_oops) > return; > > + kmsg_dump_rewind(&iter); > + > if (test_and_set_bit(0, &cxt->oops_buf_busy)) > return; > - kmsg_dump_get_buffer(dumper, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE, > + kmsg_dump_get_buffer(&iter, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE, > record_size - MTDOOPS_HEADER_SIZE, NULL); > clear_bit(0, &cxt->oops_buf_busy); > > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c > index d963ae7902f9..edfc9504e024 100644 > --- a/fs/pstore/platform.c > +++ b/fs/pstore/platform.c > @@ -382,9 +382,9 @@ void pstore_record_init(struct pstore_record *record, > * callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the > * end of the buffer. > */ > -static void pstore_dump(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason) > +static void pstore_dump(enum kmsg_dump_reason reason) > { > + struct kmsg_dump_iter iter; > unsigned long total = 0; > const char *why; > unsigned int part = 1; > @@ -405,6 +405,8 @@ static void pstore_dump(struct kmsg_dumper *dumper, > } > } > > + kmsg_dump_rewind(&iter); > + > oopscount++; > while (total < kmsg_bytes) { > char *dst; > @@ -435,7 +437,7 @@ static void pstore_dump(struct kmsg_dumper *dumper, > dst_size -= header_size; > > /* Write dump contents. */ > - if (!kmsg_dump_get_buffer(dumper, true, dst + header_size, > + if (!kmsg_dump_get_buffer(&iter, true, dst + header_size, > dst_size, &dump_size)) > break; > > diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h > index 84eaa2090efa..5d3bf20f9f0a 100644 > --- a/include/linux/kmsg_dump.h > +++ b/include/linux/kmsg_dump.h > @@ -29,6 +29,16 @@ enum kmsg_dump_reason { > KMSG_DUMP_MAX > }; > > +/** > + * struct kmsg_dump_iter - iterator for retrieving kernel messages > + * @cur_seq: Points to the oldest message to dump > + * @next_seq: Points after the newest message to dump > + */ > +struct kmsg_dump_iter { > + u64 cur_seq; > + u64 next_seq; > +}; > + > /** > * struct kmsg_dumper - kernel crash message dumper structure > * @list: Entry in the dumper list (private) > @@ -36,35 +46,29 @@ enum kmsg_dump_reason { > * through the record iterator > * @max_reason: filter for highest reason number that should be dumped > * @registered: Flag that specifies if this is already registered > - * @cur_seq: Points to the oldest message to dump > - * @next_seq: Points after the newest message to dump > */ > struct kmsg_dumper { > struct list_head list; > - void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason); > + void (*dump)(enum kmsg_dump_reason reason); > enum kmsg_dump_reason max_reason; > bool registered; > - > - /* private state of the kmsg iterator */ > - u64 cur_seq; > - u64 next_seq; > }; > > #ifdef CONFIG_PRINTK > void kmsg_dump(enum kmsg_dump_reason reason); > > -bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, > +bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog, > char *line, size_t size, size_t *len); > > -bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog, > +bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog, > char *line, size_t size, size_t *len); > > -bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, > +bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog, > char *buf, size_t size, size_t *len_out); > > -void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper); > +void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter); > > -void kmsg_dump_rewind(struct kmsg_dumper *dumper); > +void kmsg_dump_rewind(struct kmsg_dump_iter *iter); > > int kmsg_dump_register(struct kmsg_dumper *dumper); > > @@ -76,30 +80,30 @@ static inline void kmsg_dump(enum kmsg_dump_reason reason) > { > } > > -static inline bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, > +static inline bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, > bool syslog, const char *line, > size_t size, size_t *len) > { > return false; > } > > -static inline bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog, > +static inline bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog, > const char *line, size_t size, size_t *len) > { > return false; > } > > -static inline bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, > +static inline bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog, > char *buf, size_t size, size_t *len) > { > return false; > } > > -static inline void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper) > +static inline void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter) > { > } > > -static inline void kmsg_dump_rewind(struct kmsg_dumper *dumper) > +static inline void kmsg_dump_rewind(struct kmsg_dump_iter *iter) > { > } > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 315169d5e119..8544d7a55a57 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -2101,7 +2101,7 @@ static int kdb_dmesg(int argc, const char **argv) > int adjust = 0; > int n = 0; > int skip = 0; > - struct kmsg_dumper dumper; > + struct kmsg_dump_iter iter; > size_t len; > char buf[201]; > > @@ -2126,8 +2126,8 @@ static int kdb_dmesg(int argc, const char **argv) > kdb_set(2, setargs); > } > > - kmsg_dump_rewind_nolock(&dumper); > - while (kmsg_dump_get_line_nolock(&dumper, 1, NULL, 0, NULL)) > + kmsg_dump_rewind_nolock(&iter); > + while (kmsg_dump_get_line_nolock(&iter, 1, NULL, 0, NULL)) > n++; > > if (lines < 0) { > @@ -2159,8 +2159,8 @@ static int kdb_dmesg(int argc, const char **argv) > if (skip >= n || skip < 0) > return 0; > > - kmsg_dump_rewind_nolock(&dumper); > - while (kmsg_dump_get_line_nolock(&dumper, 1, buf, sizeof(buf), &len)) { > + kmsg_dump_rewind_nolock(&iter); > + while (kmsg_dump_get_line_nolock(&iter, 1, buf, sizeof(buf), &len)) { > if (skip) { > skip--; > continue; > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 45cb3e9c62c5..e58ccc368348 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -3390,7 +3390,6 @@ EXPORT_SYMBOL_GPL(kmsg_dump_reason_str); > void kmsg_dump(enum kmsg_dump_reason reason) > { > struct kmsg_dumper *dumper; > - unsigned long flags; > > rcu_read_lock(); > list_for_each_entry_rcu(dumper, &dump_list, list) { > @@ -3407,21 +3406,15 @@ void kmsg_dump(enum kmsg_dump_reason reason) > if (reason > max_reason) > continue; > > - /* initialize iterator with data about the stored records */ > - logbuf_lock_irqsave(flags); > - dumper->cur_seq = latched_seq_read_nolock(&clear_seq); > - dumper->next_seq = prb_next_seq(prb); > - logbuf_unlock_irqrestore(flags); > - > /* invoke dumper which will iterate over records */ > - dumper->dump(dumper, reason); > + dumper->dump(reason); > } > rcu_read_unlock(); > } > > /** > * kmsg_dump_get_line_nolock - retrieve one kmsg log line (unlocked version) > - * @dumper: registered kmsg dumper > + * @iter: kmsg dump iterator > * @syslog: include the "<4>" prefixes > * @line: buffer to copy the line to > * @size: maximum size of the buffer > @@ -3438,7 +3431,7 @@ void kmsg_dump(enum kmsg_dump_reason reason) > * > * The function is similar to kmsg_dump_get_line(), but grabs no locks. > */ > -bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, > +bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog, > char *line, size_t size, size_t *len) > { > struct printk_info info; > @@ -3451,11 +3444,11 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, > > /* Read text or count text lines? */ > if (line) { > - if (!prb_read_valid(prb, dumper->cur_seq, &r)) > + if (!prb_read_valid(prb, iter->cur_seq, &r)) > goto out; > l = record_print_text(&r, syslog, printk_time); > } else { > - if (!prb_read_valid_info(prb, dumper->cur_seq, > + if (!prb_read_valid_info(prb, iter->cur_seq, > &info, &line_count)) { > goto out; > } > @@ -3464,7 +3457,7 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, > > } > > - dumper->cur_seq = r.info->seq + 1; > + iter->cur_seq = r.info->seq + 1; > ret = true; > out: > if (len) > @@ -3474,7 +3467,7 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, > > /** > * kmsg_dump_get_line - retrieve one kmsg log line > - * @dumper: registered kmsg dumper > + * @iter: kmsg dump iterator > * @syslog: include the "<4>" prefixes > * @line: buffer to copy the line to > * @size: maximum size of the buffer > @@ -3489,14 +3482,14 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, > * A return value of FALSE indicates that there are no more records to > * read. > */ > -bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog, > +bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog, > char *line, size_t size, size_t *len) > { > unsigned long flags; > bool ret; > > logbuf_lock_irqsave(flags); > - ret = kmsg_dump_get_line_nolock(dumper, syslog, line, size, len); > + ret = kmsg_dump_get_line_nolock(iter, syslog, line, size, len); > logbuf_unlock_irqrestore(flags); > > return ret; > @@ -3505,7 +3498,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_line); > > /** > * kmsg_dump_get_buffer - copy kmsg log lines > - * @dumper: registered kmsg dumper > + * @iter: kmsg dump iterator > * @syslog: include the "<4>" prefixes > * @buf: buffer to copy the line to > * @size: maximum size of the buffer > @@ -3522,7 +3515,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_line); > * A return value of FALSE indicates that there are no more records to > * read. > */ > -bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, > +bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog, > char *buf, size_t size, size_t *len_out) > { > struct printk_info info; > @@ -3538,15 +3531,15 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, > goto out; > > logbuf_lock_irqsave(flags); > - if (prb_read_valid_info(prb, dumper->cur_seq, &info, NULL)) { > - if (info.seq != dumper->cur_seq) { > + if (prb_read_valid_info(prb, iter->cur_seq, &info, NULL)) { > + if (info.seq != iter->cur_seq) { > /* messages are gone, move to first available one */ > - dumper->cur_seq = info.seq; > + iter->cur_seq = info.seq; > } > } > > /* last entry */ > - if (dumper->cur_seq >= dumper->next_seq) { > + if (iter->cur_seq >= iter->next_seq) { > logbuf_unlock_irqrestore(flags); > goto out; > } > @@ -3557,7 +3550,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, > * because this function (by way of record_print_text()) will > * not write more than size-1 bytes of text into @buf. > */ > - seq = find_first_fitting_seq(dumper->cur_seq, dumper->next_seq, > + seq = find_first_fitting_seq(iter->cur_seq, iter->next_seq, > size - 1, syslog, time); > > /* > @@ -3570,7 +3563,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, > > len = 0; > prb_for_each_record(seq, prb, seq, &r) { > - if (r.info->seq >= dumper->next_seq) > + if (r.info->seq >= iter->next_seq) > break; > > len += record_print_text(&r, syslog, time); > @@ -3579,7 +3572,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, > prb_rec_init_rd(&r, &info, buf + len, size - len); > } > > - dumper->next_seq = next_seq; > + iter->next_seq = next_seq; > ret = true; > logbuf_unlock_irqrestore(flags); > out: > @@ -3591,7 +3584,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer); > > /** > * kmsg_dump_rewind_nolock - reset the iterator (unlocked version) > - * @dumper: registered kmsg dumper > + * @iter: kmsg dump iterator > * > * Reset the dumper's iterator so that kmsg_dump_get_line() and > * kmsg_dump_get_buffer() can be called again and used multiple > @@ -3599,26 +3592,26 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer); > * > * The function is similar to kmsg_dump_rewind(), but grabs no locks. > */ > -void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper) > +void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter) > { > - dumper->cur_seq = latched_seq_read_nolock(&clear_seq); > - dumper->next_seq = prb_next_seq(prb); > + iter->cur_seq = latched_seq_read_nolock(&clear_seq); > + iter->next_seq = prb_next_seq(prb); > } > > /** > * kmsg_dump_rewind - reset the iterator > - * @dumper: registered kmsg dumper > + * @iter: kmsg dump iterator > * > * Reset the dumper's iterator so that kmsg_dump_get_line() and > * kmsg_dump_get_buffer() can be called again and used multiple > * times within the same dumper.dump() callback. > */ > -void kmsg_dump_rewind(struct kmsg_dumper *dumper) > +void kmsg_dump_rewind(struct kmsg_dump_iter *iter) > { > unsigned long flags; > > logbuf_lock_irqsave(flags); > - kmsg_dump_rewind_nolock(dumper); > + kmsg_dump_rewind_nolock(iter); > logbuf_unlock_irqrestore(flags); > } > EXPORT_SYMBOL_GPL(kmsg_dump_rewind); > -- > 2.20.1 >
Hello, Thank you kernel test robot! Despite all of my efforts to carefully construct and test this series, somehome I managed to miss a compile test with CONFIG_MTD_OOPS. That kmsg_dumper does require the dumper parameter so that it can use container_of(). I will discuss this with the printk team. But most likely we will just re-instate the dumper parameter in the callback. I apologize for the lack of care on my part. John Ogness On 2021-02-26, kernel test robot <lkp@intel.com> wrote: > Hi John, > > I love your patch! Yet something to improve: > > [auto build test ERROR on next-20210225] > > url: https://github.com/0day-ci/linux/commits/John-Ogness/printk-remove-logbuf_lock/20210226-043457 > base: 7f206cf3ec2bee4621325cfacb2588e5085c07f5 > config: arm-randconfig-r024-20210225 (attached as .config) > compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project a921aaf789912d981cbb2036bdc91ad7289e1523) > 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 > # install arm cross compiling tool for clang build > # apt-get install binutils-arm-linux-gnueabi > # https://github.com/0day-ci/linux/commit/fc7f655cded40fc98ba5304c200e3a01e8291fb4 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review John-Ogness/printk-remove-logbuf_lock/20210226-043457 > git checkout fc7f655cded40fc98ba5304c200e3a01e8291fb4 > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > >>> drivers/mtd/mtdoops.c:277:45: error: use of undeclared identifier 'dumper' > struct mtdoops_context *cxt = container_of(dumper, > ^ >>> drivers/mtd/mtdoops.c:277:45: error: use of undeclared identifier 'dumper' >>> drivers/mtd/mtdoops.c:277:45: error: use of undeclared identifier 'dumper' > 3 errors generated. > > > vim +/dumper +277 drivers/mtd/mtdoops.c > > 4b23aff083649e Richard Purdie 2007-05-29 274 > fc7f655cded40f John Ogness 2021-02-25 275 static void mtdoops_do_dump(enum kmsg_dump_reason reason) > 2e386e4bac9055 Simon Kagstrom 2009-11-03 276 { > 2e386e4bac9055 Simon Kagstrom 2009-11-03 @277 struct mtdoops_context *cxt = container_of(dumper, > 2e386e4bac9055 Simon Kagstrom 2009-11-03 278 struct mtdoops_context, dump); > fc7f655cded40f John Ogness 2021-02-25 279 struct kmsg_dump_iter iter; > fc2d557c74dc58 Seiji Aguchi 2011-01-12 280 > 2e386e4bac9055 Simon Kagstrom 2009-11-03 281 /* Only dump oopses if dump_oops is set */ > 2e386e4bac9055 Simon Kagstrom 2009-11-03 282 if (reason == KMSG_DUMP_OOPS && !dump_oops) > 2e386e4bac9055 Simon Kagstrom 2009-11-03 283 return; > 2e386e4bac9055 Simon Kagstrom 2009-11-03 284 > fc7f655cded40f John Ogness 2021-02-25 285 kmsg_dump_rewind(&iter); > fc7f655cded40f John Ogness 2021-02-25 286 > df92cad8a03e83 John Ogness 2021-02-25 287 if (test_and_set_bit(0, &cxt->oops_buf_busy)) > df92cad8a03e83 John Ogness 2021-02-25 288 return; > fc7f655cded40f John Ogness 2021-02-25 289 kmsg_dump_get_buffer(&iter, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE, > e2ae715d66bf4b Kay Sievers 2012-06-15 290 record_size - MTDOOPS_HEADER_SIZE, NULL); > df92cad8a03e83 John Ogness 2021-02-25 291 clear_bit(0, &cxt->oops_buf_busy); > 2e386e4bac9055 Simon Kagstrom 2009-11-03 292 > c1cf1d57d14922 Mark Tomlinson 2020-09-03 293 if (reason != KMSG_DUMP_OOPS) { > 2e386e4bac9055 Simon Kagstrom 2009-11-03 294 /* Panics must be written immediately */ > 2e386e4bac9055 Simon Kagstrom 2009-11-03 295 mtdoops_write(cxt, 1); > c1cf1d57d14922 Mark Tomlinson 2020-09-03 296 } else { > 2e386e4bac9055 Simon Kagstrom 2009-11-03 297 /* For other cases, schedule work to write it "nicely" */ > 2e386e4bac9055 Simon Kagstrom 2009-11-03 298 schedule_work(&cxt->work_write); > 2e386e4bac9055 Simon Kagstrom 2009-11-03 299 } > c1cf1d57d14922 Mark Tomlinson 2020-09-03 300 } > 4b23aff083649e Richard Purdie 2007-05-29 301 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu 2021-02-25 21:24:35, John Ogness wrote: > Rather than storing the iterator information in the registered > kmsg_dumper structure, create a separate iterator structure. The > kmsg_dump_iter structure can reside on the stack of the caller, thus > allowing lockless use of the kmsg_dump functions. > > This change also means that the kmsg_dumper dump() callback no > longer needs to pass in the kmsg_dumper as an argument. If > kmsg_dumpers want to access the kernel logs, they can use the new > iterator. > > Update the kmsg_dumper callback prototype. Update code that accesses > the kernel logs using the kmsg_dumper structure to use the new > kmsg_dump_iter structure. For kmsg_dumpers, this also means adding a > call to kmsg_dump_rewind() to initialize the iterator. > > All this is in preparation for removal of @logbuf_lock. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> > --- > arch/powerpc/kernel/nvram_64.c | 14 +++--- > arch/powerpc/platforms/powernv/opal-kmsg.c | 3 +- > arch/powerpc/xmon/xmon.c | 6 +-- > arch/um/kernel/kmsg_dump.c | 8 +-- > drivers/hv/vmbus_drv.c | 7 +-- > drivers/mtd/mtdoops.c | 8 +-- > fs/pstore/platform.c | 8 +-- > include/linux/kmsg_dump.h | 38 ++++++++------- > kernel/debug/kdb/kdb_main.c | 10 ++-- > kernel/printk/printk.c | 57 ++++++++++------------ > 10 files changed, 81 insertions(+), 78 deletions(-) > > diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c > index 532f22637783..5a64b24a91c2 100644 > --- a/arch/powerpc/kernel/nvram_64.c > +++ b/arch/powerpc/kernel/nvram_64.c > @@ -72,8 +72,7 @@ static const char *nvram_os_partitions[] = { > NULL > }; > > -static void oops_to_nvram(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason); > +static void oops_to_nvram(enum kmsg_dump_reason reason); > > static struct kmsg_dumper nvram_kmsg_dumper = { > .dump = oops_to_nvram > @@ -642,11 +641,11 @@ void __init nvram_init_oops_partition(int rtas_partition_exists) > * that we think will compress sufficiently to fit in the lnx,oops-log > * partition. If that's too much, go back and capture uncompressed text. > */ > -static void oops_to_nvram(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason) > +static void oops_to_nvram(enum kmsg_dump_reason reason) > { > struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf; > static unsigned int oops_count = 0; > + static struct kmsg_dump_iter iter; > static bool panicking = false; > static DEFINE_SPINLOCK(lock); > unsigned long flags; > @@ -681,13 +680,14 @@ static void oops_to_nvram(struct kmsg_dumper *dumper, > return; > > if (big_oops_buf) { > - kmsg_dump_get_buffer(dumper, false, > + kmsg_dump_rewind(&iter); It would be nice to get rid of the kmsg_dump_rewind(&iter) calls in all callers. A solution might be to create the following in include/linux/kmsg_dump.h #define KMSG_DUMP_ITER_INIT(iter) { \ .cur_seq = 0, \ .next_seq = U64_MAX, \ } #define DEFINE_KMSG_DUMP_ITER(iter) \ struct kmsg_dump_iter iter = KMSG_DUMP_ITER_INIT(iter) Then we could do the following at the beginning of both kmsg_dump_get_buffer() and kmsg_dump_get_line(): u64 clear_seq = latched_seq_read_nolock(&clear_seq); if (iter->cur_seq < clear_seq) cur_seq = clear_seq; I am not completely sure about next_seq: + kmsg_dump_get_buffer() will set it for the next call anyway. It reads the blocks of messages from the newest. + kmsg_dump_get_line() wants to read the entire buffer anyway. But there is a small risk of an infinite loop when new messages are printed when dumping each line. It might be better to avoid the infinite loop. We could do the following: static void check_and_set_iter(struct kmsg_dump_iter) { if (iter->cur_seq == 0 && iter->next_seq == U64_MAX) { kmsg_dump_rewind(iter); } and call this at the beginning of both kmsg_dump_get_buffer() and kmsg_dump_get_line() What do you think? Note that I do not resist on it. But it might make the API easier to use from my POV. Otherwise the patch looks good to me. Best Regards, Petr
On 2021-03-01, Petr Mladek <pmladek@suse.com> wrote: >> diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c >> index 532f22637783..5a64b24a91c2 100644 >> --- a/arch/powerpc/kernel/nvram_64.c >> +++ b/arch/powerpc/kernel/nvram_64.c >> @@ -72,8 +72,7 @@ static const char *nvram_os_partitions[] = { >> NULL >> }; >> >> -static void oops_to_nvram(struct kmsg_dumper *dumper, >> - enum kmsg_dump_reason reason); >> +static void oops_to_nvram(enum kmsg_dump_reason reason); >> >> static struct kmsg_dumper nvram_kmsg_dumper = { >> .dump = oops_to_nvram >> @@ -642,11 +641,11 @@ void __init nvram_init_oops_partition(int rtas_partition_exists) >> * that we think will compress sufficiently to fit in the lnx,oops-log >> * partition. If that's too much, go back and capture uncompressed text. >> */ >> -static void oops_to_nvram(struct kmsg_dumper *dumper, >> - enum kmsg_dump_reason reason) >> +static void oops_to_nvram(enum kmsg_dump_reason reason) >> { >> struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf; >> static unsigned int oops_count = 0; >> + static struct kmsg_dump_iter iter; >> static bool panicking = false; >> static DEFINE_SPINLOCK(lock); >> unsigned long flags; >> @@ -681,13 +680,14 @@ static void oops_to_nvram(struct kmsg_dumper *dumper, >> return; >> >> if (big_oops_buf) { >> - kmsg_dump_get_buffer(dumper, false, >> + kmsg_dump_rewind(&iter); > > It would be nice to get rid of the kmsg_dump_rewind(&iter) calls > in all callers. > > A solution might be to create the following in include/linux/kmsg_dump.h > > #define KMSG_DUMP_ITER_INIT(iter) { \ > .cur_seq = 0, \ > .next_seq = U64_MAX, \ > } > > #define DEFINE_KMSG_DUMP_ITER(iter) \ > struct kmsg_dump_iter iter = KMSG_DUMP_ITER_INIT(iter) For this caller (arch/powerpc/kernel/nvram_64.c) and for (kernel/debug/kdb/kdb_main.c), kmsg_dump_rewind() is called twice within the dumper. So rewind will still be used there. > Then we could do the following at the beginning of both > kmsg_dump_get_buffer() and kmsg_dump_get_line(): > > u64 clear_seq = latched_seq_read_nolock(&clear_seq); > > if (iter->cur_seq < clear_seq) > cur_seq = clear_seq; I suppose we need to add this part anyway, if we want to enforce that records before @clear_seq are not to be available for dumpers. > I am not completely sure about next_seq: > > + kmsg_dump_get_buffer() will set it for the next call anyway. > It reads the blocks of messages from the newest. > > + kmsg_dump_get_line() wants to read the entire buffer anyway. > But there is a small risk of an infinite loop when new messages > are printed when dumping each line. > > It might be better to avoid the infinite loop. We could do the following: > > static void check_and_set_iter(struct kmsg_dump_iter) > { > if (iter->cur_seq == 0 && iter->next_seq == U64_MAX) { > kmsg_dump_rewind(iter); > } > > and call this at the beginning of both kmsg_dump_get_buffer() > and kmsg_dump_get_line() > > What do you think? On a technical level, it does not make any difference. It is pure cosmetic. Personally, I prefer the rewind directly before the kmsg_dump_get calls because it puts the initializer directly next to the user. As an example to illustrate my view, I prefer: for (i = 0; i < n; i++) ...; instead of: int i = 0; ... for (; i < n; i++) ...; Also, I do not really like the special use of 0/U64_MAX to identify special actions of the kmsg_dump_get functions. > Note that I do not resist on it. But it might make the API easier to > use from my POV. Since you do not resist, I will keep the API the same for v4. But I will add the @clear_seq check to the kmsg_dump_get functions. John Ogness
On Tue 2021-03-02 14:20:51, John Ogness wrote: > On 2021-03-01, Petr Mladek <pmladek@suse.com> wrote: > >> diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c > >> index 532f22637783..5a64b24a91c2 100644 > >> --- a/arch/powerpc/kernel/nvram_64.c > >> +++ b/arch/powerpc/kernel/nvram_64.c > >> @@ -681,13 +680,14 @@ static void oops_to_nvram(struct kmsg_dumper *dumper, > >> return; > >> > >> if (big_oops_buf) { > >> - kmsg_dump_get_buffer(dumper, false, > >> + kmsg_dump_rewind(&iter); > > > > It would be nice to get rid of the kmsg_dump_rewind(&iter) calls > > in all callers. > > > > A solution might be to create the following in include/linux/kmsg_dump.h > > > > Then we could do the following at the beginning of both > > kmsg_dump_get_buffer() and kmsg_dump_get_line(): > > > > u64 clear_seq = latched_seq_read_nolock(&clear_seq); > > > > if (iter->cur_seq < clear_seq) > > cur_seq = clear_seq; > > I suppose we need to add this part anyway, if we want to enforce that > records before @clear_seq are not to be available for dumpers. Yup. > > It might be better to avoid the infinite loop. We could do the following: > > > > static void check_and_set_iter(struct kmsg_dump_iter) > > { > > if (iter->cur_seq == 0 && iter->next_seq == U64_MAX) { > > kmsg_dump_rewind(iter); > > } > > > > and call this at the beginning of both kmsg_dump_get_buffer() > > and kmsg_dump_get_line() > > > > What do you think? > > On a technical level, it does not make any difference. It is pure > cosmetic. Yup. > Personally, I prefer the rewind directly before the kmsg_dump_get calls > because it puts the initializer directly next to the user. > > As an example to illustrate my view, I prefer: > > for (i = 0; i < n; i++) > ...; > > instead of: > > int i = 0; > > ... > > for (; i < n; i++) > ...; > > Also, I do not really like the special use of 0/U64_MAX to identify > special actions of the kmsg_dump_get functions. Fair enough. > > Note that I do not resist on it. But it might make the API easier to > > use from my POV. > > Since you do not resist, I will keep the API the same for v4. But I will > add the @clear_seq check to the kmsg_dump_get functions. Go for it. Best Regards, Petr
diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c index 532f22637783..5a64b24a91c2 100644 --- a/arch/powerpc/kernel/nvram_64.c +++ b/arch/powerpc/kernel/nvram_64.c @@ -72,8 +72,7 @@ static const char *nvram_os_partitions[] = { NULL }; -static void oops_to_nvram(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason); +static void oops_to_nvram(enum kmsg_dump_reason reason); static struct kmsg_dumper nvram_kmsg_dumper = { .dump = oops_to_nvram @@ -642,11 +641,11 @@ void __init nvram_init_oops_partition(int rtas_partition_exists) * that we think will compress sufficiently to fit in the lnx,oops-log * partition. If that's too much, go back and capture uncompressed text. */ -static void oops_to_nvram(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) +static void oops_to_nvram(enum kmsg_dump_reason reason) { struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf; static unsigned int oops_count = 0; + static struct kmsg_dump_iter iter; static bool panicking = false; static DEFINE_SPINLOCK(lock); unsigned long flags; @@ -681,13 +680,14 @@ static void oops_to_nvram(struct kmsg_dumper *dumper, return; if (big_oops_buf) { - kmsg_dump_get_buffer(dumper, false, + kmsg_dump_rewind(&iter); + kmsg_dump_get_buffer(&iter, false, big_oops_buf, big_oops_buf_sz, &text_len); rc = zip_oops(text_len); } if (rc != 0) { - kmsg_dump_rewind(dumper); - kmsg_dump_get_buffer(dumper, false, + kmsg_dump_rewind(&iter); + kmsg_dump_get_buffer(&iter, false, oops_data, oops_data_sz, &text_len); err_type = ERR_TYPE_KERNEL_PANIC; oops_hdr->version = cpu_to_be16(OOPS_HDR_VERSION); diff --git a/arch/powerpc/platforms/powernv/opal-kmsg.c b/arch/powerpc/platforms/powernv/opal-kmsg.c index 6c3bc4b4da98..a7bd6ac681f4 100644 --- a/arch/powerpc/platforms/powernv/opal-kmsg.c +++ b/arch/powerpc/platforms/powernv/opal-kmsg.c @@ -19,8 +19,7 @@ * may not be completely printed. This function does not actually dump the * message, it just ensures that OPAL completely flushes the console buffer. */ -static void kmsg_dump_opal_console_flush(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) +static void kmsg_dump_opal_console_flush(enum kmsg_dump_reason reason) { /* * Outside of a panic context the pollers will continue to run, diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index 80ed3e1becf9..5978b90a885f 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -3001,7 +3001,7 @@ print_address(unsigned long addr) static void dump_log_buf(void) { - struct kmsg_dumper dumper; + struct kmsg_dump_iter iter; unsigned char buf[128]; size_t len; @@ -3013,9 +3013,9 @@ dump_log_buf(void) catch_memory_errors = 1; sync(); - kmsg_dump_rewind_nolock(&dumper); + kmsg_dump_rewind_nolock(&iter); xmon_start_pagination(); - while (kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf), &len)) { + while (kmsg_dump_get_line_nolock(&iter, false, buf, sizeof(buf), &len)) { buf[len] = '\0'; printf("%s", buf); } diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c index 4869e2cc787c..9fbc5e5b1023 100644 --- a/arch/um/kernel/kmsg_dump.c +++ b/arch/um/kernel/kmsg_dump.c @@ -7,9 +7,9 @@ #include <shared/kern.h> #include <os.h> -static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) +static void kmsg_dumper_stdout(enum kmsg_dump_reason reason) { + static struct kmsg_dump_iter iter; static DEFINE_SPINLOCK(lock); static char line[1024]; struct console *con; @@ -34,8 +34,10 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, if (!spin_trylock(&lock)) return; + kmsg_dump_rewind(&iter); + printf("kmsg_dump:\n"); - while (kmsg_dump_get_line(dumper, true, line, sizeof(line), &len)) { + while (kmsg_dump_get_line(&iter, true, line, sizeof(line), &len)) { line[len] = '\0'; printf("%s", line); } diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c index 10dce9f91216..1b858f280e22 100644 --- a/drivers/hv/vmbus_drv.c +++ b/drivers/hv/vmbus_drv.c @@ -1388,9 +1388,9 @@ static void vmbus_isr(void) * Callback from kmsg_dump. Grab as much as possible from the end of the kmsg * buffer and call into Hyper-V to transfer the data. */ -static void hv_kmsg_dump(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) +static void hv_kmsg_dump(enum kmsg_dump_reason reason) { + struct kmsg_dump_iter iter; size_t bytes_written; phys_addr_t panic_pa; @@ -1404,7 +1404,8 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper, * Write dump contents to the page. No need to synchronize; panic should * be single-threaded. */ - kmsg_dump_get_buffer(dumper, false, hv_panic_page, HV_HYP_PAGE_SIZE, + kmsg_dump_rewind(&iter); + kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE, &bytes_written); if (bytes_written) hyperv_report_panic_msg(panic_pa, bytes_written); diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c index 8bbfba40a554..d179b726a1c9 100644 --- a/drivers/mtd/mtdoops.c +++ b/drivers/mtd/mtdoops.c @@ -272,19 +272,21 @@ static void find_next_position(struct mtdoops_context *cxt) mtdoops_inc_counter(cxt); } -static void mtdoops_do_dump(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) +static void mtdoops_do_dump(enum kmsg_dump_reason reason) { struct mtdoops_context *cxt = container_of(dumper, struct mtdoops_context, dump); + struct kmsg_dump_iter iter; /* Only dump oopses if dump_oops is set */ if (reason == KMSG_DUMP_OOPS && !dump_oops) return; + kmsg_dump_rewind(&iter); + if (test_and_set_bit(0, &cxt->oops_buf_busy)) return; - kmsg_dump_get_buffer(dumper, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE, + kmsg_dump_get_buffer(&iter, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE, record_size - MTDOOPS_HEADER_SIZE, NULL); clear_bit(0, &cxt->oops_buf_busy); diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index d963ae7902f9..edfc9504e024 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -382,9 +382,9 @@ void pstore_record_init(struct pstore_record *record, * callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the * end of the buffer. */ -static void pstore_dump(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) +static void pstore_dump(enum kmsg_dump_reason reason) { + struct kmsg_dump_iter iter; unsigned long total = 0; const char *why; unsigned int part = 1; @@ -405,6 +405,8 @@ static void pstore_dump(struct kmsg_dumper *dumper, } } + kmsg_dump_rewind(&iter); + oopscount++; while (total < kmsg_bytes) { char *dst; @@ -435,7 +437,7 @@ static void pstore_dump(struct kmsg_dumper *dumper, dst_size -= header_size; /* Write dump contents. */ - if (!kmsg_dump_get_buffer(dumper, true, dst + header_size, + if (!kmsg_dump_get_buffer(&iter, true, dst + header_size, dst_size, &dump_size)) break; diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h index 84eaa2090efa..5d3bf20f9f0a 100644 --- a/include/linux/kmsg_dump.h +++ b/include/linux/kmsg_dump.h @@ -29,6 +29,16 @@ enum kmsg_dump_reason { KMSG_DUMP_MAX }; +/** + * struct kmsg_dump_iter - iterator for retrieving kernel messages + * @cur_seq: Points to the oldest message to dump + * @next_seq: Points after the newest message to dump + */ +struct kmsg_dump_iter { + u64 cur_seq; + u64 next_seq; +}; + /** * struct kmsg_dumper - kernel crash message dumper structure * @list: Entry in the dumper list (private) @@ -36,35 +46,29 @@ enum kmsg_dump_reason { * through the record iterator * @max_reason: filter for highest reason number that should be dumped * @registered: Flag that specifies if this is already registered - * @cur_seq: Points to the oldest message to dump - * @next_seq: Points after the newest message to dump */ struct kmsg_dumper { struct list_head list; - void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason); + void (*dump)(enum kmsg_dump_reason reason); enum kmsg_dump_reason max_reason; bool registered; - - /* private state of the kmsg iterator */ - u64 cur_seq; - u64 next_seq; }; #ifdef CONFIG_PRINTK void kmsg_dump(enum kmsg_dump_reason reason); -bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, +bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog, char *line, size_t size, size_t *len); -bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog, +bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog, char *line, size_t size, size_t *len); -bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, +bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog, char *buf, size_t size, size_t *len_out); -void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper); +void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter); -void kmsg_dump_rewind(struct kmsg_dumper *dumper); +void kmsg_dump_rewind(struct kmsg_dump_iter *iter); int kmsg_dump_register(struct kmsg_dumper *dumper); @@ -76,30 +80,30 @@ static inline void kmsg_dump(enum kmsg_dump_reason reason) { } -static inline bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, +static inline bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog, const char *line, size_t size, size_t *len) { return false; } -static inline bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog, +static inline bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog, const char *line, size_t size, size_t *len) { return false; } -static inline bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, +static inline bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog, char *buf, size_t size, size_t *len) { return false; } -static inline void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper) +static inline void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter) { } -static inline void kmsg_dump_rewind(struct kmsg_dumper *dumper) +static inline void kmsg_dump_rewind(struct kmsg_dump_iter *iter) { } diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 315169d5e119..8544d7a55a57 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -2101,7 +2101,7 @@ static int kdb_dmesg(int argc, const char **argv) int adjust = 0; int n = 0; int skip = 0; - struct kmsg_dumper dumper; + struct kmsg_dump_iter iter; size_t len; char buf[201]; @@ -2126,8 +2126,8 @@ static int kdb_dmesg(int argc, const char **argv) kdb_set(2, setargs); } - kmsg_dump_rewind_nolock(&dumper); - while (kmsg_dump_get_line_nolock(&dumper, 1, NULL, 0, NULL)) + kmsg_dump_rewind_nolock(&iter); + while (kmsg_dump_get_line_nolock(&iter, 1, NULL, 0, NULL)) n++; if (lines < 0) { @@ -2159,8 +2159,8 @@ static int kdb_dmesg(int argc, const char **argv) if (skip >= n || skip < 0) return 0; - kmsg_dump_rewind_nolock(&dumper); - while (kmsg_dump_get_line_nolock(&dumper, 1, buf, sizeof(buf), &len)) { + kmsg_dump_rewind_nolock(&iter); + while (kmsg_dump_get_line_nolock(&iter, 1, buf, sizeof(buf), &len)) { if (skip) { skip--; continue; diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 45cb3e9c62c5..e58ccc368348 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -3390,7 +3390,6 @@ EXPORT_SYMBOL_GPL(kmsg_dump_reason_str); void kmsg_dump(enum kmsg_dump_reason reason) { struct kmsg_dumper *dumper; - unsigned long flags; rcu_read_lock(); list_for_each_entry_rcu(dumper, &dump_list, list) { @@ -3407,21 +3406,15 @@ void kmsg_dump(enum kmsg_dump_reason reason) if (reason > max_reason) continue; - /* initialize iterator with data about the stored records */ - logbuf_lock_irqsave(flags); - dumper->cur_seq = latched_seq_read_nolock(&clear_seq); - dumper->next_seq = prb_next_seq(prb); - logbuf_unlock_irqrestore(flags); - /* invoke dumper which will iterate over records */ - dumper->dump(dumper, reason); + dumper->dump(reason); } rcu_read_unlock(); } /** * kmsg_dump_get_line_nolock - retrieve one kmsg log line (unlocked version) - * @dumper: registered kmsg dumper + * @iter: kmsg dump iterator * @syslog: include the "<4>" prefixes * @line: buffer to copy the line to * @size: maximum size of the buffer @@ -3438,7 +3431,7 @@ void kmsg_dump(enum kmsg_dump_reason reason) * * The function is similar to kmsg_dump_get_line(), but grabs no locks. */ -bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, +bool kmsg_dump_get_line_nolock(struct kmsg_dump_iter *iter, bool syslog, char *line, size_t size, size_t *len) { struct printk_info info; @@ -3451,11 +3444,11 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, /* Read text or count text lines? */ if (line) { - if (!prb_read_valid(prb, dumper->cur_seq, &r)) + if (!prb_read_valid(prb, iter->cur_seq, &r)) goto out; l = record_print_text(&r, syslog, printk_time); } else { - if (!prb_read_valid_info(prb, dumper->cur_seq, + if (!prb_read_valid_info(prb, iter->cur_seq, &info, &line_count)) { goto out; } @@ -3464,7 +3457,7 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, } - dumper->cur_seq = r.info->seq + 1; + iter->cur_seq = r.info->seq + 1; ret = true; out: if (len) @@ -3474,7 +3467,7 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, /** * kmsg_dump_get_line - retrieve one kmsg log line - * @dumper: registered kmsg dumper + * @iter: kmsg dump iterator * @syslog: include the "<4>" prefixes * @line: buffer to copy the line to * @size: maximum size of the buffer @@ -3489,14 +3482,14 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog, * A return value of FALSE indicates that there are no more records to * read. */ -bool kmsg_dump_get_line(struct kmsg_dumper *dumper, bool syslog, +bool kmsg_dump_get_line(struct kmsg_dump_iter *iter, bool syslog, char *line, size_t size, size_t *len) { unsigned long flags; bool ret; logbuf_lock_irqsave(flags); - ret = kmsg_dump_get_line_nolock(dumper, syslog, line, size, len); + ret = kmsg_dump_get_line_nolock(iter, syslog, line, size, len); logbuf_unlock_irqrestore(flags); return ret; @@ -3505,7 +3498,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_line); /** * kmsg_dump_get_buffer - copy kmsg log lines - * @dumper: registered kmsg dumper + * @iter: kmsg dump iterator * @syslog: include the "<4>" prefixes * @buf: buffer to copy the line to * @size: maximum size of the buffer @@ -3522,7 +3515,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_line); * A return value of FALSE indicates that there are no more records to * read. */ -bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, +bool kmsg_dump_get_buffer(struct kmsg_dump_iter *iter, bool syslog, char *buf, size_t size, size_t *len_out) { struct printk_info info; @@ -3538,15 +3531,15 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, goto out; logbuf_lock_irqsave(flags); - if (prb_read_valid_info(prb, dumper->cur_seq, &info, NULL)) { - if (info.seq != dumper->cur_seq) { + if (prb_read_valid_info(prb, iter->cur_seq, &info, NULL)) { + if (info.seq != iter->cur_seq) { /* messages are gone, move to first available one */ - dumper->cur_seq = info.seq; + iter->cur_seq = info.seq; } } /* last entry */ - if (dumper->cur_seq >= dumper->next_seq) { + if (iter->cur_seq >= iter->next_seq) { logbuf_unlock_irqrestore(flags); goto out; } @@ -3557,7 +3550,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, * because this function (by way of record_print_text()) will * not write more than size-1 bytes of text into @buf. */ - seq = find_first_fitting_seq(dumper->cur_seq, dumper->next_seq, + seq = find_first_fitting_seq(iter->cur_seq, iter->next_seq, size - 1, syslog, time); /* @@ -3570,7 +3563,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, len = 0; prb_for_each_record(seq, prb, seq, &r) { - if (r.info->seq >= dumper->next_seq) + if (r.info->seq >= iter->next_seq) break; len += record_print_text(&r, syslog, time); @@ -3579,7 +3572,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, prb_rec_init_rd(&r, &info, buf + len, size - len); } - dumper->next_seq = next_seq; + iter->next_seq = next_seq; ret = true; logbuf_unlock_irqrestore(flags); out: @@ -3591,7 +3584,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer); /** * kmsg_dump_rewind_nolock - reset the iterator (unlocked version) - * @dumper: registered kmsg dumper + * @iter: kmsg dump iterator * * Reset the dumper's iterator so that kmsg_dump_get_line() and * kmsg_dump_get_buffer() can be called again and used multiple @@ -3599,26 +3592,26 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer); * * The function is similar to kmsg_dump_rewind(), but grabs no locks. */ -void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper) +void kmsg_dump_rewind_nolock(struct kmsg_dump_iter *iter) { - dumper->cur_seq = latched_seq_read_nolock(&clear_seq); - dumper->next_seq = prb_next_seq(prb); + iter->cur_seq = latched_seq_read_nolock(&clear_seq); + iter->next_seq = prb_next_seq(prb); } /** * kmsg_dump_rewind - reset the iterator - * @dumper: registered kmsg dumper + * @iter: kmsg dump iterator * * Reset the dumper's iterator so that kmsg_dump_get_line() and * kmsg_dump_get_buffer() can be called again and used multiple * times within the same dumper.dump() callback. */ -void kmsg_dump_rewind(struct kmsg_dumper *dumper) +void kmsg_dump_rewind(struct kmsg_dump_iter *iter) { unsigned long flags; logbuf_lock_irqsave(flags); - kmsg_dump_rewind_nolock(dumper); + kmsg_dump_rewind_nolock(iter); logbuf_unlock_irqrestore(flags); } EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
Rather than storing the iterator information in the registered kmsg_dumper structure, create a separate iterator structure. The kmsg_dump_iter structure can reside on the stack of the caller, thus allowing lockless use of the kmsg_dump functions. This change also means that the kmsg_dumper dump() callback no longer needs to pass in the kmsg_dumper as an argument. If kmsg_dumpers want to access the kernel logs, they can use the new iterator. Update the kmsg_dumper callback prototype. Update code that accesses the kernel logs using the kmsg_dumper structure to use the new kmsg_dump_iter structure. For kmsg_dumpers, this also means adding a call to kmsg_dump_rewind() to initialize the iterator. All this is in preparation for removal of @logbuf_lock. Signed-off-by: John Ogness <john.ogness@linutronix.de> --- arch/powerpc/kernel/nvram_64.c | 14 +++--- arch/powerpc/platforms/powernv/opal-kmsg.c | 3 +- arch/powerpc/xmon/xmon.c | 6 +-- arch/um/kernel/kmsg_dump.c | 8 +-- drivers/hv/vmbus_drv.c | 7 +-- drivers/mtd/mtdoops.c | 8 +-- fs/pstore/platform.c | 8 +-- include/linux/kmsg_dump.h | 38 ++++++++------- kernel/debug/kdb/kdb_main.c | 10 ++-- kernel/printk/printk.c | 57 ++++++++++------------ 10 files changed, 81 insertions(+), 78 deletions(-)