linux-kernel - Re: [PATCH v3] printk: ringbuffer: Improve prb_next

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Jan 2022 15:14:41 +0106
From:   John Ogness <john.ogness@...utronix.de>
To:     Mukesh Ojha <quic_mojha@...cinc.com>, linux-kernel@...r.kernel.org
Cc:     pmladek@...e.com, senozhatsky@...omium.org, rostedt@...dmis.org,
        Mukesh Ojha <quic_mojha@...cinc.com>
Subject: Re: [PATCH v3] printk: ringbuffer: Improve prb_next_seq() performance

Hi Mukesh,

Thanks for pushing this. I think it got lost somewhere. I have a couple
very minor non-functional change requests.

On 2022-01-21, Mukesh Ojha <quic_mojha@...cinc.com> wrote:
> From: Petr Mladek <pmladek@...e.com>
>
> prb_next_seq() always iterates from the first known sequence number.
> In the worst case, it might loop 8k times for 256kB buffer,
> 15k times for 512kB buffer, and 64k times for 2MB buffer.
>
> It was reported that pooling and reading using syslog interface

                       ^^^^^^^ polling

> might occupy 50% of CPU.
>
> Speedup the search by storing @id of the last finalized descriptor.
>
> The loop is still needed because the @id is stored and read in the best
> effort way. An atomic variable is used to keep the @id consistent.
> But the stores and reads are not serialized against each other.
> The descriptor could get reused in the meantime. The related sequence
> number will be used only when it is still valid.
>
> An invalid value should be read _only_ when there is a flood of messages
> and the ringbuffer is rapidly reused. The performance is the least
> problem in this case.
>
> Link: https://lore.kernel.org/lkml/YXlddJxLh77DKfIO@alley/T/#m43062e8b2a17f8dbc8c6ccdb8851fb0dbaabbb14
> Reported-by: Chunlei Wang <chunlei.wang@...iatek.com>
> Signed-off-by: Petr Mladek <pmladek@...e.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@...cinc.com>
> ---
> Changes against v2:
>   Added the hunk suggested by John
>
> Changes against v1:
>   Read @seq by the last finalized @id directly in prb_next_seq() (John)
>
>  kernel/printk/printk_ringbuffer.c | 48 +++++++++++++++++++++++++++++++++++----
>  kernel/printk/printk_ringbuffer.h |  2 ++
>  2 files changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 8a7b736..297bc18 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -2005,8 +2014,38 @@ u64 prb_first_valid_seq(struct printk_ringbuffer *rb)
>   */
>  u64 prb_next_seq(struct printk_ringbuffer *rb)
>  {
> -	u64 seq = 0;
> +	struct prb_desc_ring *desc_ring = &rb->desc_ring;
> +	enum desc_state d_state;
> +	unsigned long id;
> +	u64 seq;
> +
> +	/* Check if the cached @id still points to a valid @seq. */
> +	id = atomic_long_read(&desc_ring->last_finalized_id);
> +	d_state = desc_read(desc_ring, id, NULL, &seq, NULL);
>  
> +	if (d_state == desc_finalized || d_state == desc_reusable) {
> +		/*
> +		 * Begin searching after the last finalized record.
> +		 * (On 0, the search must begin at 0 because of hack#2
> +		 * of the bootstrapping phase it is not known if a
> +		 * record at index 0 exists.)
> +		*/

               ^^^ whitespace
               
> +		if (seq != 0)
> +			seq++;
> +	} else {
> +		/*
> +		 * The information about the last finalized sequence number
> +		 * has gone. It should happen only when there is a flood of
> +		 * new messages and the ringbuffer is rapidly recycled.
> +		 * Give up and start from the beginning.
> +		 */
> +		seq = 0;
> +	}
> +
> +	/*
> +	 * The information about the last finalized @seq might be inaccurate.
> +	 * Search forward to find the current one.
> +	 */

It is fine to add this comment. But then the following comment should be
removed. It is redundant.

>  	/* Search forward from the oldest descriptor. */
>  	while (_prb_read_valid(rb, &seq, NULL, NULL))
>  		seq++;

Petr can probably just make the changes when committing. I am not
requesting a v4.

@Petr: Feel free to add:

Reviewed-by: John Ogness <john.ogness@...utronix.de>