Re: [PATCH V3] panic: Move panic_print before kmsg dumpers

["Guilherme G. Piccoli" <gpiccoli@igalia.com>]

On 22/01/2022 07:31, Baoquan He wrote:
> [...]
> From my old POV, I took pstore as a necessity on handheld devices or
> embeded system, e.g on Andriod. In that case, reserving crashkernel
> memory to enable kdump to save kernel log, it sounds not so
> cost-effective, since memory on those systems is usually not big.
> I am also interested in any new use case where people deploy these
> and why it's needed, to widen my view.

Hi Baoquan, that's great to hear. Indeed, I feel pstore is unfortunately
not very used in non-embedded devices - if you see kdump/error-report
userspace tooling, like on Red Hat/Fedora, Debian/Ubuntu and so on, they
never rely on pstore. And the configuration is not straightforward for
the users...I think that's a good thing to change, since pstore is much
less resource consuming than kdump.
But of course, not a discussion related to this patch specifically, just
me thinking out loud heh


> [...] 
> It's my bad. My thought is panic_print and kmsg_dump can be coupled, but
> they should decouple with panic_notifier. When panic_print is enabled,
> we do not expect to execute panic_notifier? My personal opinion.
> 
> I missed the change at line 8, sorry for the caused misunderstanding. 
> Now the chance of holding C-programmer-prize of the year comes back
> again.
> 
>   void panic()
>   {
> 1         if (!_crash_kexec_post_notifiers && !panic_print) {
> 2                 __crash_kexec(NULL);
> 3                 smp_send_stop();
> 4         } else {
> 5                 crash_smp_send_stop();
> 6         }
>   
>   	if (_crash_kexec_post_notifiers)
> 8  		atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
> 9  	panic_print_sys_info(false);
> 10  	kmsg_dump(KMSG_DUMP_PANIC);
> 11  	if (_crash_kexec_post_notifiers || panic_print)
> 12                 __crash_kexec(NULL);
>   	...
>   	debug_locks_off();
>           console_flush_on_panic(CONSOLE_FLUSH_PENDING);
>           panic_print_sys_info(true);

Hmm, yeah, I still don't think I'm a brilliant C programmer heh
Again, in the code above, I can't see how we would reach
"__crash_kexec(NULL)" after printing the extra info of panic_print, if
we don't have panic notifiers enabled.

So, indeed the code currently don't really tightly couple "panic_print"
with the panic notifiers. We could change that in another patch series,
based on what Petr suggested in the filter thread (I know you're
following there as well, thanks bu the way!), but for now, they are
completely independent. My plan, following Petr suggestions here and if
you agree, is to re-submit this patch with some changes, but in the end
the code will allow users that have kdump enabled + panic_print
-"crash_kexec_post_notifiers" to have "panic_print_sys_info(false)"
executing before the "__crash_kexec(NULL)".

But also, we can add "crash_kexec_post_notifiers" and it will still
work; finally, pstore is gonna be able to collect the logs from
"panic_print" as well (the main purpose of this patch).

Once that's all resolved, my goal is to jump into the panic notifiers
refactor suggested in the other thread. Let me know if you agree with
these steps/plans, and I'll work them.
Cheers,


Guilherme