Re: [PATCH v2 1/6] panic: Add a notifier chain for pre-kdump callbacks

[Petr Mladek]

On Wed 2026-10-21 01:10:25, Zack Rusin wrote:
> On Tue, Oct 20, 2026 at 10:04 PM Baoquan He <baoquan.he@xxxxxxxxx> wrote:
> >
> > On 10/20/26 at 03:23pm, Zack Rusin wrote:
> > > Hi, Petr.
> > >
> > > First of all I really appreciate you going over this. Thanks!
> > >
> > > On Tue, Oct 20, 2026 at 12:37 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > > > Approach B: Three lists: platform, smp_safe, and smp_unsafe()
> > > > =============================================================
> > > >
> > > > It is similar to approach A. We would just add an extra:
> > > >
> > > >      + panic_notifiers_platform_list
> > > >      + disable_panic_notifiers_platform_list
> > > >
> > > > This list would be used for smp-safe notifiers which are
> > > > required be some platform. It would be the notifiers which
> > > > are crucial. Which are currently the reason to set
> > > > "_crash_kexec_post_notifiers".
> > > >
> > > > It might be called also when __crash_kexec() is called
> > > > outside panic().
> > > [...]
> > > > This patchset, adding vmware_log_notifier, might implement just
> > > > the panic_notifiers_platform_list. The rest might be
> > > > added by another volunteer.
> > > >
> > > > How does this sounds, please?
> > >
> > > That sounds good to me. For v3 I'd do just the platform list:
> > >
> > > - One call in vpanic(), right after kgdb_panic() and before the first
> > > __crash_kexec(), whatever crash_kexec_post_notifiers is set to. The
> > > list runs before kdump and before the other CPUs are stopped.
> > > - One call in __crash_kexec(), after it has taken the kexec lock,
> > > found a crash image and saved the registers, for paths that enter
> > > kdump without panic(), like oops_end() or Hyper-V's hv_crash_handle().
> > > Once the early panic call has run the list, the guard makes this one a
> > > no-op. That removes the dependence on crash_kexec_post_notifiers you
> > > pointed out before.
> > > - A once-only guard shared by both, so the list also runs only once
> > > when a callback panics.

Just to be sure. I think that a simple static bool or int should be
enough. The caller is responsible for the synchronization. I believe
that it should be done by the "panic_cpu".

> > > - disable_panic_notifiers_platform_list as a boot and runtime switch,
> > > checked in the same helper, replacing panic_pre_kdump_postpone.
> > > - Kernel-doc with your definition: smp-safe callbacks a platform
> > > needs, which would otherwise be a reason to set
> > > crash_kexec_post_notifiers.
> > >
> > > Roughly:
> > >         kgdb_panic(buf);
> > >
> > >         panic_notify_platform();
> > >
> > >         if (!_crash_kexec_post_notifiers)
> > >                 __crash_kexec(NULL);
> > >
> > > and in __crash_kexec():
> > >
> > >         crash_setup_regs(&fixed_regs, regs);
> > >         /* Direct entry outside panic() */
> > >         panic_notify_platform();
> > >         crash_save_vmcoreinfo();
> > >
> > > I'd keep the second call in __crash_kexec() rather than crash_kexec(),
> > > because afaict crash_kexec() can return without a crash image, e.g.
> > > from the BPF kfunc, or from oops_end() before it panics. Calling the
> > > list there would use up the guard even when no kdump follows.
> > >
> > > crash_kexec_post_notifiers, the existing list and the kmsg dumpers
> > > stay as they are, and moving other platforms over can come later.
> > >
> > > The VMware log copy would be the only user, and it's still best-effort
> > > diagnostics: kdump and the reboot don't depend on it. It copies the
> > > log tail into an 8 KiB buffer. On panic() that happens after the panic
> > > message and any local backtrace; on direct entry from an oops, after
> > > the exception output. With the list enabled throughout, the copy
> > > precedes the later panic notifiers, sys_info() and the all-CPU
> > > backtrace step. The guest-crash report stays a late kmsg dumper that's
> > > skipped while a crash kernel is loaded, because the host can power off
> > > the VM inside that call.
> > >
> > > If that matches what you had in mind, I'll do it that way in v3.
> > >
> > > Guilherme, Stephen, Baoquan, does that work for you too?
> >
> > Hi Zack,
> >
> > Sorry I didn't check this series before, just trying to catch up.
> >
> > I like the Approach B direction: one panic_notifiers_platform_list for
> > platform callbacks that must run before kdump. While I have one concern
> > about the long term.
> >
> > Today the list has only the VMware log sender. Later, more handlers may join
> > (for example pvpanic, Hyper-V, Xen, gsmi, SNP). When the list grows, someone
> > will want to run only their handler, or to skip some handlers. The easy
> > answer is "add a new list for my handler". I think this will be a problem too
> > in the future. Every new list adds a new global ordering rule and a new disable
> > switch. Petr already said we should keep the number of lists small.

My hope is most of the potential users won't be registered at the same
time or will be a NOP in the real life. Because each particular system
will actively used only some technology.

But maybe I am naive and there will be selection and ordering problems.

> > Currently, atomic_notifier_call_chain() runs all registered callbacks. There
> > is no per-handler enable/disable and no filter. NOTIFY_STOP_MASK can stop the
> > chain, but we must not use it here, because one handler could hide all the
> > later handlers. The list-level disable_panic_notifiers_platform_list can
> > only disable the whole list.
> >
> > I have some rough though, just for reference:
> >
> > 1. Keep one panic_notifiers_platform_list.
> >
> > 2. Give each handler a small wrapper with a name and an enabled flag:
> >
> >    struct panic_pre_kdump_notifier {
> >            struct notifier_block nb;
> >            const char *name;
> >            bool enabled;
> >    };

This looks good to me.

> > 3. In the dispatch, skip a handler when enabled is false. Print the name
> >    before calling it. This also helps when a handler hangs and we want to
> >    know which one it was.
> >
> > 4. Add a boot parameter to disable handlers by name, for example:
> >
> >            panic_pre_kdump.disable=vmware_log,hyperv_crash

I like this. I think that we just would want to do s/_pre_dump/_notifiers_platform/.

> >    Parse it early. At panic time we only read the flag. We do not parse
> >    strings in the panic path.
> >
> > 5. Keep the list-level disable_panic_notifiers_platform_list. It is still
> >    useful to disable the whole list for debugging.
> >
> > 6. If the list is still too broad later, we can add a reason/class argument
> >    (for example PANIC, OOPS_DIRECT). A handler can then decide if it cares.
> >    This is better than adding a new list.
> 
> Thanks for looking at it. I agree we shouldn't add a separate list for
> each handler, and named handlers with their own switch on the one list
> sound like the right way to grow it.
> 
> For v3 I'd like to keep the series to the list itself. With VMware as
> the only callback, the list-wide boot switch already lets us disable
> platform logging. The sender also has its own sysctl,
> kernel.vmware_record_panic_msg. We can add per-handler selection with
> the next user, when there's an actual choice between handlers. I'm
> happy to help with that when it comes up.

Sound good to me. I would like to start simple. We could add the selection
later when it is needed in the real life.

> Two of your points do fit in v3 as documentation. I'll change the
> panic_notify_platform() kernel-doc to also say that:
> - callbacks must return NOTIFY_DONE, because NOTIFY_STOP would skip
> the callbacks after them
> - the notifier value is 0.

Great catch!

> The VMware callback ignores that value, which leaves room to add
> reason information later.
> 
> Petr, does keeping v3 to that sound right to you?

Sounds good to me. But you might want to wait few more days for
more potential feedback.

Best Regards,
Petr