On Fri 2020-06-26 17:02:48, John Ogness wrote:
> On 2020-06-25, Petr Mladek <[email protected]> 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
