Re: buffer allocation: was: [PATCH v3 3/3] printk: use the lockless ringbuffer

[Petr Mladek]

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