lkml.org 
[lkml]   [2022]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3] printk: ringbuffer: Improve prb_next_seq() performance
From
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@quicinc.com> wrote:
>> From: Petr Mladek <pmladek@suse.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@mediatek.com>
>> Signed-off-by: Petr Mladek <pmladek@suse.com>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.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@linutronix.de>

\
 
 \ /
  Last update: 2022-01-26 07:56    [W:0.063 / U:0.708 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site