Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 8 Apr 2022 09:15:14 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Saravana Kannan <saravanak@google.com>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        Chunlei.wang@mediatek.com, john.ogness@linutronix.de,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mediatek@lists.infradead.org, rostedt@goodmis.org,
        senozhatsky@chromium.org, kernel-team@android.com
Subject: Re: [PATCH v2] printk: ringbuffer: Improve prb_next_seq() performance
Message-ID: <Yk/hAubbZeV43ejA@alley>
References: <YXlddJxLh77DKfIO@alley>
 <20220408004821.4010969-1-saravanak@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220408004821.4010969-1-saravanak@google.com>
Precedence: bulk

On Thu 2022-04-07 17:48:21, Saravana Kannan wrote:
> 
> > Petr Mladek <pmladek@suse.com> wrote:
> > 
> > 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
> > 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.
> > 
> > --- a/kernel/printk/printk_ringbuffer.h
> > +++ b/kernel/printk/printk_ringbuffer.h
> > @@ -75,6 +75,7 @@ struct prb_desc_ring {
> >  	struct printk_info	*infos;
> >  	atomic_long_t		head_id;
> >  	atomic_long_t		tail_id;
> > +	atomic_long_t		last_finalized_id;
> >  };
> 
> 
> I really know nothing about this code, but while looking around
> kernel/printk/ I noticed kernel/printk/printk.c has these lines in
> log_buf_vmcoreinfo_setup().
> 
> 	VMCOREINFO_STRUCT_SIZE(prb_desc_ring);
> 	VMCOREINFO_OFFSET(prb_desc_ring, count_bits);
> 	VMCOREINFO_OFFSET(prb_desc_ring, descs);
> 	VMCOREINFO_OFFSET(prb_desc_ring, infos);
> 	VMCOREINFO_OFFSET(prb_desc_ring, head_id);
> 	VMCOREINFO_OFFSET(prb_desc_ring, tail_id);
> 
> Would this header file change also require a change to that location?
> Something like
> 
> 	VMCOREINFO_OFFSET(prb_desc_ring, head_id);
> 	VMCOREINFO_OFFSET(prb_desc_ring, tail_id);
> +	VMCOREINFO_OFFSET(prb_desc_ring, last_finalized_id);

It is actually not needed. VMCOREINFO_*() macros define some data
that are used by the "crash" tool when reading crashdumps. The field
"last_finalized_id" is used only at runtime. It is not needed
for reading the log in the crashdump.

Anyway, thanks for the question. It is great to know that there are
more people checking our changes.

Best Regards,
Petr