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]

Message-ID: <e5fb98ca-70f0-4b30-deb3-103e6a9915c4@quicinc.com>
Date:   Wed, 26 Jan 2022 12:25:07 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     John Ogness <john.ogness@...utronix.de>,
        <linux-kernel@...r.kernel.org>
CC:     <pmladek@...e.com>, <senozhatsky@...omium.org>,
        <rostedt@...dmis.org>
Subject: Re: [PATCH v3] printk: ringbuffer: Improve prb_next_seq() performance

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@...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>