Date:   Thu, 13 Jan 2022 15:22:29 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc:     linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
        kernel@gpiccoli.net, senozhatsky@chromium.org, rostedt@goodmis.org,
        john.ogness@linutronix.de, feng.tang@intel.com,
        kexec@lists.infradead.org, dyoung@redhat.com,
        keescook@chromium.org, anton@enomsg.org, ccross@android.com,
        tony.luck@intel.com
Subject: Re: [PATCH V2] panic: Move panic_print before kmsg dumpers
Message-ID: <YeA1pXz7f0wqSnah@alley>
References: <20220106212835.119409-1-gpiccoli@igalia.com>
 <Yd/0K1x7ILw3Qa46@alley>
 <ba0e29ba-0e08-df6e-ade5-eb58ae2495e3@igalia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ba0e29ba-0e08-df6e-ade5-eb58ae2495e3@igalia.com>
Precedence: bulk

On Thu 2022-01-13 09:34:01, Guilherme G. Piccoli wrote:
> On 13/01/2022 08:50, Petr Mladek wrote:
> >> @@ -249,7 +252,7 @@ void panic(const char *fmt, ...)
> >>  	 * show some extra information on kernel log if it was set...
> >>  	 */
> >>  	if (kexec_crash_loaded())
> >> -		panic_print_sys_info();
> >> +		panic_print_sys_info(false);
> > 
> > panic_print_sys_info(false) will be called twice when both
> > kexec_crash_loaded() and _crash_kexec_post_notifiers are true.
> > 
> > Do we really need to call panic_print_sys_info() here? All information
> > provided by panic_print_sys_info(false) can be found also in
> > the crash dump.
> > 
> >>  	/*
> >>  	 * If we have crashed and we have a crash kernel loaded let it handle
> >> @@ -283,6 +286,8 @@ void panic(const char *fmt, ...)
> >>  	 */
> >>  	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
> >>  
> >> +	panic_print_sys_info(false);
> > 
> > This is where the info might be printed 2nd time.
> > 
> >> +
> >>  	kmsg_dump(KMSG_DUMP_PANIC);

> Thanks for catching this issue - indeed, if
> "_crash_kexec_post_notifiers" is true, with this patch we print stuff
> twice. I will submit a V3 that guards against that, using a bool, makes
> sense to you?

It might be possible to check kexec_crash_loaded() on the two
locations. But I think about even easier solution, see below.


> The interesting question here is:
> > Do we really need to call panic_print_sys_info() here? All information
> > provided by panic_print_sys_info(false) can be found also in
> > the crash dump.
> 
> So, we indeed need that in our use case. Crash is meant to be used
> post-mortem, i.e., you made a full vmcore collection and then, of
> course, you have basically all the data you need accessible though the
> crash tool.
> 
> Problem is: in our use case, we want more data than a regular dmesg in a
> panic event (hence we use panic_print), but we don't collect a full
> crash dump, due to its big size. Also, as you can imagine, we do favor
> pstore over kdump, but it might fail due to a variety of reasons (like
> not having a free RAM buffer for ramoops), so kdump is our fallback.
> Hence, we'd like to be able to use panic_print with both kdump and
> pstore, and for that, both patches are needed.

Fair enough.

OK, do we have any specific reason why panic_print_sys_info()
should get called right before kmsg_dump() when this code patch
is used?

Alternative solution would be to remove the check of
kexec_crash_loaded() and always call panic_print_sys_info(false)
at the beginning (after kgdb_panic(buf)).

The advantage is that panic_print_sys_info(false) will be always
called on the same location. It will give the same results
in all code paths so that it will be easier to interpret them.
And it will have the same problems so it should be easier
to debug and maintain.

It is possible that it will not work for some users. Also it is
possible that it might cause some problems. But it is hard to
guess at least for me.

I think that we might try it and see if anyone complains.
Honestly, I think that only few people use panic_printk_sys_info().
And your use-case makes sense.

Best Regards,
Petr