On Fri 2020-06-26 17:02:48, John Ogness wrote: > On 2020-06-25, Petr Mladek <pmladek@xxxxxxxx> wrote: > >> --- a/kernel/printk/printk.c > >> +++ b/kernel/printk/printk.c > >> + free = __LOG_BUF_LEN; > >> + prb_for_each_record(0, &printk_rb_static, seq, &r) > >> + free -= add_to_rb(&printk_rb_dynamic, &r); > >> + > >> + prb = &printk_rb_dynamic; > > > > This might deserve a comment that this is safe (no lost message) > > because it is called early enough when everything is still running > > on the boot CPU. > > I will add a comment and an extra check to make sure. > > Once everything is lockless, new messages could appear (for example, due > to NMI messages). The simple check should probably change to a loop. But > let us not worry about that at this point. Yup. > Below is a new version of the relevant patch snippets against mainline > just to show you how I plan to make it look for the next version. > > John Ogness > > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ @@ > +#define PRB_AVGBITS 5 /* 32 character average length */ > + > +#if CONFIG_LOG_BUF_SHIFT <= PRB_AVGBITS > +#error CONFIG_LOG_BUF_SHIFT value too small. > +#endif > +_DEFINE_PRINTKRB(printk_rb_static, CONFIG_LOG_BUF_SHIFT - PRB_AVGBITS, > + PRB_AVGBITS, PRB_AVGBITS, &__log_buf[0]); > + > @@ @@ > void __init setup_log_buf(int early) > { > + unsigned int new_descs_count; > + struct prb_desc *new_descs; > + struct printk_info info; > + struct printk_record r; > + size_t new_descs_size; > unsigned long flags; > + char *new_dict_buf; > char *new_log_buf; > unsigned int free; > + u64 seq; > > /* > * Some archs call setup_log_buf() multiple times - first is very > @@ @@ void __init setup_log_buf(int early) > if (!new_log_buf_len) > return; > > + new_descs_count = new_log_buf_len >> PRB_AVGBITS; > + if (new_descs_count == 0) { > + pr_err("new_log_buf_len: %lu too small\n", new_log_buf_len); > + return; > + } > + > new_log_buf = memblock_alloc(new_log_buf_len, LOG_ALIGN); > if (unlikely(!new_log_buf)) { > - pr_err("log_buf_len: %lu bytes not available\n", > - new_log_buf_len); > + pr_err("log_buf_len: %lu text bytes not available\n", > + new_log_buf_len); > + return; > + } > + > + new_dict_buf = memblock_alloc(new_log_buf_len, LOG_ALIGN); > + if (unlikely(!new_dict_buf)) { > + pr_err("log_buf_len: %lu dict bytes not available\n", > + new_log_buf_len); > + memblock_free(__pa(new_log_buf), new_log_buf_len); > return; > } > > + new_descs_size = new_descs_count * sizeof(struct prb_desc); > + new_descs = memblock_alloc(new_descs_size, LOG_ALIGN); > + if (unlikely(!new_descs)) { > + pr_err("log_buf_len: %lu desc bytes not available\n", > + new_descs_size); > + memblock_free(__pa(new_dict_buf), new_log_buf_len); > + memblock_free(__pa(new_log_buf), new_log_buf_len); > + return; > + } > + > + prb_rec_init_rd(&r, &info, > + &setup_text_buf[0], sizeof(setup_text_buf), > + &setup_dict_buf[0], sizeof(setup_dict_buf)); > + > + prb_init(&printk_rb_dynamic, > + new_log_buf, order_base_2(new_log_buf_len), > + new_dict_buf, order_base_2(new_log_buf_len), > + new_descs, order_base_2(new_descs_count)); order_base_2() is safe. But the result might be tat some allocated space is not used. I would prefer to make sure that new_log_buf_len is rounded, e.g. by roundup_pow_of_two(), at the beginning of the function. Then we could use ilog2() here. Otherwise, it looks fine to me. Best Regards, Petr _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec