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 4FD0BC28CFA for ; Mon, 1 Mar 2021 23:46:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2C68860232 for ; Mon, 1 Mar 2021 23:46:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346263AbhCAXkP (ORCPT ); Mon, 1 Mar 2021 18:40:15 -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-kernel@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 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=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 D85B6C433E0 for ; Mon, 1 Mar 2021 18:08:14 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3B8D865272 for ; Mon, 1 Mar 2021 18:08:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3B8D865272 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4Dq7Xr6kfzz3cl5 for ; Tue, 2 Mar 2021 05:08:12 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=suse.com header.i=@suse.com header.a=rsa-sha256 header.s=susede1 header.b=aH1+7x/W; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=suse.com (client-ip=195.135.220.15; helo=mx2.suse.de; envelope-from=pmladek@suse.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=suse.com header.i=@suse.com header.a=rsa-sha256 header.s=susede1 header.b=aH1+7x/W; dkim-atps=neutral Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4Dq7XN1ddfz30L3 for ; Tue, 2 Mar 2021 05:07:47 +1100 (AEDT) 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 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> X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-hyperv@vger.kernel.org, Sergey Senozhatsky , Douglas Anderson , Paul Mackerras , Miquel Raynal , "K. Y. Srinivasan" , Thomas Meyer , Vignesh Raghavendra , Wei Liu , Madhavan Srinivasan , Stephen Hemminger , Anton Vorontsov , Joel Stanley , Jason Wessel , Anton Ivanov , Wei Li , Haiyang Zhang , Ravi Bangoria , Kees Cook , Alistair Popple , Jeff Dike , Colin Cross , linux-um@lists.infradead.org, Daniel Thompson , Steven Rostedt , Davidlohr Bueso , Nicholas Piggin , Oleg Nesterov , Thomas Gleixner , Andy Shevchenko , Jordan Niethe , Michael Kelley , Christophe Leroy , Tony Luck , Pavel Tatashin , linux-kernel@vger.kernel.org, Sergey Senozhatsky , Richard Weinberger , kgdb-bugreport@lists.sourceforge.net, linux-mtd@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, Mike Rapoport Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" 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 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=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,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 B2715C433E0 for ; Mon, 1 Mar 2021 18:08:27 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6285665084 for ; Mon, 1 Mar 2021 18:08:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6285665084 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=bMpYA4HvJAEyTzIDjlMJnghalJE5XFW+34h/BrVSVrc=; b=z1jZ91LJ6tjc2LcRZSoBG9f/r kVBS5v/0sW6JpRVIBZGNCjPK+O5c+XsiSRh+zE/5MXuJXP9Oxcs6NqcV00X698iUHefWe4zXifv9T T2qpy2pQ0f6W7UczYpx0BBeSKLTRFsxOX08Mhvdi3sRacFmqsTopL/cF1jnMrzaviayt6MabM+qgJ t3W588IqA9WAV5I2ywNottLEF6sQPvQBiibFj9IvoRdQ8qFr3CNCulot94StuMkY7DpovY7sQ0nqN 0NnwcoT/iSsuhz3qOFLfuZ+lLD3wnhcfpEkTG5kx1KMnTPvdcM1Kz1TbuQqqE+v2ejLkBqLCeQzVz m1vKy97AA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lGmxP-0005xU-CQ; Mon, 01 Mar 2021 18:07:47 +0000 Received: from mx2.suse.de ([195.135.220.15]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lGmxN-0005wz-I3; Mon, 01 Mar 2021 18:07:46 +0000 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 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-Disposition: inline In-Reply-To: <20210225202438.28985-13-john.ogness@linutronix.de> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210301_130745_845260_B7163EA0 X-CRM114-Status: GOOD ( 32.11 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-hyperv@vger.kernel.org, Sergey Senozhatsky , Benjamin Herrenschmidt , Douglas Anderson , Paul Mackerras , Miquel Raynal , "K. Y. Srinivasan" , Thomas Meyer , Vignesh Raghavendra , Wei Liu , Madhavan Srinivasan , Stephen Hemminger , Michael Ellerman , Anton Vorontsov , Joel Stanley , Jason Wessel , Anton Ivanov , Wei Li , Haiyang Zhang , Ravi Bangoria , Kees Cook , Alistair Popple , Jeff Dike , Colin Cross , linux-um@lists.infradead.org, Daniel Thompson , Steven Rostedt , Davidlohr Bueso , Nicholas Piggin , Oleg Nesterov , Thomas Gleixner , Andy Shevchenko , Jordan Niethe , Michael Kelley , Christophe Leroy , Tony Luck , Pavel Tatashin , linux-kernel@vger.kernel.org, Sergey Senozhatsky , Richard Weinberger , kgdb-bugreport@lists.sourceforge.net, linux-mtd@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, Mike Rapoport Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 1 Mar 2021 19:07:41 +0100 From: Petr Mladek 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-Disposition: inline In-Reply-To: <20210225202438.28985-13-john.ogness@linutronix.de> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-um" Errors-To: linux-um-bounces+geert=linux-m68k.org@lists.infradead.org To: John Ogness Cc: linux-hyperv@vger.kernel.org, Sergey Senozhatsky , Benjamin Herrenschmidt , Douglas Anderson , Paul Mackerras , Miquel Raynal , "K. Y. Srinivasan" , Thomas Meyer , Vignesh Raghavendra , Wei Liu , Madhavan Srinivasan , Stephen Hemminger , Michael Ellerman , Anton Vorontsov , Joel Stanley , Jason Wessel , Anton Ivanov , Wei Li , Haiyang Zhang , Ravi Bangoria , Kees Cook , Alistair Popple , Jeff Dike , Colin Cross , linux-um@lists.infradead.org, Daniel Thompson , Steven Rostedt , Davidlohr Bueso , Nicholas Piggin , Oleg Nesterov , Thomas Gleixner , Andy Shevchenko , Jordan Niethe , Michael Kelley , Christophe Leroy , Tony Luck , Pavel Tatashin , linux-kernel@vger.kernel.org, Sergey Senozhatsky , Richard Weinberger , kgdb-bugreport@lists.sourceforge.net, linux-mtd@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, Mike Rapoport 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 _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um