From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 18164C433E6 for ; Mon, 1 Mar 2021 18:10:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E3491651C1 for ; Mon, 1 Mar 2021 18:10:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238760AbhCASJl (ORCPT ); Mon, 1 Mar 2021 13:09:41 -0500 Received: from mx2.suse.de ([195.135.220.15]:46240 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239339AbhCASIb (ORCPT ); Mon, 1 Mar 2021 13:08:31 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1614622064; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=qp4HJtwIT4bWIf+9Fj02wgzG00aspAv7NDE1H9XsKw8=; b=aH1+7x/WqqqBgwoXsxJWiMMfixqBzqQo+CHJ0FMCUdP0kn8yxVawZhEsqiOr+QSi7cVRUm cH6dvttkUBHzyuxGaLPn0bQ3ilhyaCz7ZY6y77iScN5BUKhqdowr9f4TAygKPghH0VyeHu hCUXGbUSrWDF6mn/T5TxstesPyTwCSg= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id E550DAE3C; Mon, 1 Mar 2021 18:07:43 +0000 (UTC) Date: Mon, 1 Mar 2021 19:07:41 +0100 From: Petr Mladek To: John Ogness Cc: Sergey Senozhatsky , Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , linux-kernel@vger.kernel.org, Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Jeff Dike , Richard Weinberger , Anton Ivanov , "K. Y. Srinivasan" , Haiyang Zhang , Stephen Hemminger , Wei Liu , Miquel Raynal , Vignesh Raghavendra , Kees Cook , Anton Vorontsov , Colin Cross , Tony Luck , Jason Wessel , Daniel Thompson , Douglas Anderson , Pavel Tatashin , Joel Stanley , Christophe Leroy , Jordan Niethe , Alistair Popple , Ravi Bangoria , Nicholas Piggin , Mike Rapoport , Madhavan Srinivasan , Thomas Meyer , Andy Shevchenko , Davidlohr Bueso , Oleg Nesterov , Wei Li , Michael Kelley , linuxppc-dev@lists.ozlabs.org, linux-um@lists.infradead.org, linux-hyperv@vger.kernel.org, linux-mtd@lists.infradead.org, kgdb-bugreport@lists.sourceforge.net Subject: Re: [PATCH next v3 12/15] printk: introduce a kmsg_dump iterator Message-ID: References: <20210225202438.28985-1-john.ogness@linutronix.de> <20210225202438.28985-13-john.ogness@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210225202438.28985-13-john.ogness@linutronix.de> Precedence: bulk List-ID: X-Mailing-List: linux-hyperv@vger.kernel.org On Thu 2021-02-25 21:24:35, John Ogness wrote: > Rather than storing the iterator information in the registered > kmsg_dumper structure, create a separate iterator structure. The > kmsg_dump_iter structure can reside on the stack of the caller, thus > allowing lockless use of the kmsg_dump functions. > > This change also means that the kmsg_dumper dump() callback no > longer needs to pass in the kmsg_dumper as an argument. If > kmsg_dumpers want to access the kernel logs, they can use the new > iterator. > > Update the kmsg_dumper callback prototype. Update code that accesses > the kernel logs using the kmsg_dumper structure to use the new > kmsg_dump_iter structure. For kmsg_dumpers, this also means adding a > call to kmsg_dump_rewind() to initialize the iterator. > > All this is in preparation for removal of @logbuf_lock. > > Signed-off-by: John Ogness > --- > arch/powerpc/kernel/nvram_64.c | 14 +++--- > arch/powerpc/platforms/powernv/opal-kmsg.c | 3 +- > arch/powerpc/xmon/xmon.c | 6 +-- > arch/um/kernel/kmsg_dump.c | 8 +-- > drivers/hv/vmbus_drv.c | 7 +-- > drivers/mtd/mtdoops.c | 8 +-- > fs/pstore/platform.c | 8 +-- > include/linux/kmsg_dump.h | 38 ++++++++------- > kernel/debug/kdb/kdb_main.c | 10 ++-- > kernel/printk/printk.c | 57 ++++++++++------------ > 10 files changed, 81 insertions(+), 78 deletions(-) > > diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c > index 532f22637783..5a64b24a91c2 100644 > --- a/arch/powerpc/kernel/nvram_64.c > +++ b/arch/powerpc/kernel/nvram_64.c > @@ -72,8 +72,7 @@ static const char *nvram_os_partitions[] = { > NULL > }; > > -static void oops_to_nvram(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason); > +static void oops_to_nvram(enum kmsg_dump_reason reason); > > static struct kmsg_dumper nvram_kmsg_dumper = { > .dump = oops_to_nvram > @@ -642,11 +641,11 @@ void __init nvram_init_oops_partition(int rtas_partition_exists) > * that we think will compress sufficiently to fit in the lnx,oops-log > * partition. If that's too much, go back and capture uncompressed text. > */ > -static void oops_to_nvram(struct kmsg_dumper *dumper, > - enum kmsg_dump_reason reason) > +static void oops_to_nvram(enum kmsg_dump_reason reason) > { > struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf; > static unsigned int oops_count = 0; > + static struct kmsg_dump_iter iter; > static bool panicking = false; > static DEFINE_SPINLOCK(lock); > unsigned long flags; > @@ -681,13 +680,14 @@ static void oops_to_nvram(struct kmsg_dumper *dumper, > return; > > if (big_oops_buf) { > - kmsg_dump_get_buffer(dumper, false, > + kmsg_dump_rewind(&iter); It would be nice to get rid of the kmsg_dump_rewind(&iter) calls in all callers. A solution might be to create the following in include/linux/kmsg_dump.h #define KMSG_DUMP_ITER_INIT(iter) { \ .cur_seq = 0, \ .next_seq = U64_MAX, \ } #define DEFINE_KMSG_DUMP_ITER(iter) \ struct kmsg_dump_iter iter = KMSG_DUMP_ITER_INIT(iter) Then we could do the following at the beginning of both kmsg_dump_get_buffer() and kmsg_dump_get_line(): u64 clear_seq = latched_seq_read_nolock(&clear_seq); if (iter->cur_seq < clear_seq) cur_seq = clear_seq; I am not completely sure about next_seq: + kmsg_dump_get_buffer() will set it for the next call anyway. It reads the blocks of messages from the newest. + kmsg_dump_get_line() wants to read the entire buffer anyway. But there is a small risk of an infinite loop when new messages are printed when dumping each line. It might be better to avoid the infinite loop. We could do the following: static void check_and_set_iter(struct kmsg_dump_iter) { if (iter->cur_seq == 0 && iter->next_seq == U64_MAX) { kmsg_dump_rewind(iter); } and call this at the beginning of both kmsg_dump_get_buffer() and kmsg_dump_get_line() What do you think? Note that I do not resist on it. But it might make the API easier to use from my POV. Otherwise the patch looks good to me. Best Regards, Petr