On 01/22/22 at 10:49am, Guilherme G. Piccoli wrote:
> 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.
Missed this one.
Above code will allow any of _crash_kexec_post_notifiers and panic_print
to execute, then crash dump in L11.
L5 -> L11
Since you have posted v4, let's ignore it anyway.
>
> 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.
I am glad to see any improvement from refactory. As for panic_notifier,
I have expressed my concern and worry about the plan. So, if no any
new action added before kdump switching, it's welcomed.
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
