Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <e2d39675-5df6-73fb-fa24-df906a97ee50@igalia.com>
Date:   Tue, 8 Feb 2022 15:51:13 -0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH V4] notifier/panic: Introduce panic_notifier_filter
Content-Language: en-US
To:     Petr Mladek <pmladek@suse.com>, "bhe@redhat.com" <bhe@redhat.com>,
        "dyoung@redhat.com" <dyoung@redhat.com>,
        "vgoyal@redhat.com" <vgoyal@redhat.com>
Cc:     "d.hatayama@fujitsu.com" <d.hatayama@fujitsu.com>,
        "kexec@lists.infradead.org" <kexec@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "stern@rowland.harvard.edu" <stern@rowland.harvard.edu>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "andriy.shevchenko@linux.intel.com" 
        <andriy.shevchenko@linux.intel.com>,
        "corbet@lwn.net" <corbet@lwn.net>,
        "halves@canonical.com" <halves@canonical.com>,
        "kernel@gpiccoli.net" <kernel@gpiccoli.net>
References: <20220108153451.195121-1-gpiccoli@igalia.com>
 <TYAPR01MB6507D06BA6D32218F6E88198955F9@TYAPR01MB6507.jpnprd01.prod.outlook.com>
 <fda509a5-ea0d-4d1d-a1c1-ca5e80010fc0@igalia.com>
 <TYAPR01MB6507D9747647685B554B8F8F955F9@TYAPR01MB6507.jpnprd01.prod.outlook.com>
 <fb5e66b6-049a-22ab-5913-a04cc302b629@igalia.com> <YfPxvzSzDLjO5ldp@alley>
From:   "Guilherme G. Piccoli" <gpiccoli@igalia.com>
In-Reply-To: <YfPxvzSzDLjO5ldp@alley>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28/01/2022 10:38, Petr Mladek wrote:
> [...] On Thu 2022-01-27 14:16:20, Guilherme G. Piccoli wrote:
> First, I am sorry for the very long mail. But the problem is really
> complicated. I did my best to describe it a clean way.
> 
> I have discussed these problems with a colleague and he had some good
> points. And my view evolved even further.

Thanks Petr for the very comprehensive and detailed email - this helps a
lot in shaping the future of panic notifier(s)!


> [...] 
> I think about the following solution:
> 
>     + split the notifiers into three lists:
> 
> 	+ info: stop watchdogs, provide extra info
> 	+ hypervisor: poke hypervisor
> 	+ reboot: actions needed only when crash dump did not happen
> 
>     + allow to call hypervisor notifiers before or after kdump
> 
>     + stop CPUs before kdump when either hypervisor notifiers or
>       kmsg_dump is enabled
> 
> Note that it still allows to call kdump as the first action when
> hypervisor notifiers are called after kdump and no kmsg dumper
> is registered.
> 
> 
> void panic(void)
> {
> 	[...]
> 
> 	if (crash_kexec_post_hypervisor || panic_print || enabled_kmsg_dump()) {
> 		/*
> 		 * Stop CPUs when some extra action is required before
> 		 * crash dump. We will need architecture dependent extra
> 		 * works in addition to stopping other CPUs.
> 		 */
> 		 crash_smp_send_stop();
> 		 cpus_stopped = true;
> 	}
> 
> 	if (crash_kexec_post_hypervisor) {
> 		  /* Tell hypervisor about the panic */
> 		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> 	}
> 
> 	if (enabled_kmsg_dump) {
> 		  /*
> 		   * Print extra info by notifiers.
> 		   * Prevent rumors, for example, by stopping watchdogs.
> 		   */
> 		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> 	}
> 
> 	/* Optional extra info */
> 	panic_printk_sys_info();
> 
> 	/* No dumper by default */
> 	kmsg_dump();
> 
> 	/* Used only when crash kernel loaded */
> 	__crash_kexec(NULL);
> 
> 	if (!cpus_stopped) {
> 		/*
> 		 * Note smp_send_stop is the usual smp shutdown function, which
> 		 * unfortunately means it may not be hardened to work in a
> 		 * panic situation.
> 		 */
> 		smp_send_stop();
> 	}
> 
> 	if (!crash_kexec_post_hypervisor) {
> 		  /* Tell hypervisor about the panic */
> 		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> 	}
> 
> 	if (!enabled_kmsg_dump) {
> 		  /*
> 		   * Print extra info by notifiers.
> 		   * Prevent rumors, for example, by stopping watchdogs.
> 		   */
> 		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> 	}
> 
> 	/*
> 	 * Help to reboot a safe way.
> 	 */
> 	atomic_notifier_call_chain(&panic_reboot_notifier_list, 0, buf);
> 
> 	[...]
> }
> 
> Any opinion?
> Do the notifier list names make sense?
> 

This was exposed very clearly, thanks. I agree with you, it's a good
approach, and we can evolve that during the implementation phase, like
"function A is not good in the hypervisor list because of this and
that", so we move it to the reboot list. Also, name of the lists is not
so relevant, might evolve in the implementation phase - I personally
liked them, specially the "info" and "hypervisor" ones (reboot seems
good but not great heh).

So, what are the opinions from kdump maintainers about this idea?
Baoquan / Vivek / Dave, does it make sense to you? Do you have any
suggestions/concerns to add on top of Petr draft?

I prefer this refactor than the filter, certainly. If nobody else
working on that, I can try implementing that - it's very interesting.
The only thing I'd like to have first is an ACK from the kdump
maintainers about the general idea.

Cheers,


Guilherme