Re: [PATCH v3] printk: ringbuffer: Improve prb_next_seq() performance

[Mukesh Ojha]

Thanks John for the review.

@petr : does it looks fine from your side ?


Regards,
-Mukesh

On 1/21/2022 7:38 PM, John Ogness wrote:
> 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@xxxxxxxxxxx> wrote:
> > From: Petr Mladek <pmladek@xxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
> > Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx>