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

2022-01-21 Thread Baoquan He
On 01/21/22 at 03:00pm, Michael Kelley (LINUX) wrote:
> From: Baoquan He  Sent: Thursday, January 20, 2022 6:31 PM
> > 
> > On 01/20/22 at 06:36pm, Guilherme G. Piccoli wrote:
> > > Hi Baoquan, some comments inline below:
> > >
> > >
> > > On 20/01/2022 05:51, Baoquan He wrote:
> > > > [...]
> > > >> From my POV, the function of panic notifiers is not well defined. They
> > > >> do various things, for example:
> > > >> [...]
> > > >> The do more that just providing information. Some are risky. It is not
> > > >> easy to disable a particular one.
> > > >
> > > > Yes, agree. Not all of them just provide information.
> > > >
> > > > Now panic_notifier_filter Guilherme added can help to disable some of
> > > > them.
> > >
> > > So, just for completeness, worth to mention Petr had some interesting
> > > suggestions in the other thread (about the filter) and we may end-up not
> > > having this implemented - in other words, maybe a refactor of that
> > > mechanism is going to be proposed.
> > 
> > OK, saw that. We can continue discuss that there.
> > 
> > >
> > >
> > > > [...]
> > > >>
> > > >>   + Guilherme uses crash dump only to dump the kernel log. It might
> > > >> be more reliable than kmsg_dump. In this case, 
> > > >> panic_print_sys_info()
> > > >> is the only way to get the extra information.
> > > >
> > > > Hmm, I haven't made clear what Guilherme really wants in his recent
> > > > post. In this patch he wants to get panic print info into pstore. He
> > > > also want to dump the kernel log poked by panic_print in kdump kernel.
> > > > And it's very weird people try to collect kernel log via crash dump
> > > > mechnism, that is obviously using a sledgehammer to crack a nut.
> > > > Sometime, we should not add or change code to a too specific corner
> > > > case.
> > >
> > > OK, I'll try to be really clear, hopefully I can explain the use case in
> > > better and simpler words. First of all, I wouldn't call it a corner case
> > > - it's just a valid use case that, in my opinion, should be allowed. Why
> > > not, right? Kernel shouldn't push policy on users, we should instead let
> > > the users decide how to use the tools/options.
> > 
> > Agree, sorry about my wrong expression.
> > 
> > >
> > > So imagine you cannot collect a vmcore, due to the lack of storage
> > > space. Yet, you want the most information as possible to investigate the
> > > cause of a panic. The kernel flag "panic_print" is the perfect fit, we
> > > can dump backtraces, task list, memory info...right on a panic event.
> > >
> > > But then, how to save this panic log with lots of information after a
> > > reboot? There are 2 ways in my understanding:
> > >
> > > (a) pstore/kmsg_dump()
> > > (b) kdump
> > >
> > > The option (a) is easily the best - we don't need to reserve lots of
> > > memory, then boot another kernel, etc. This patch (being hereby
> > > discussed) aims to enable the "panic_print" output for this case!
> > > But...there are cases in which option (a) cannot work. We need a backend
> > > of persistent storage, either a block device or, more common, RAM memory
> > > that is persistent across a reboot. What if it's not available?
> > >
> > > Then, we fallback to option (b) - kind of a sledgehammer, in your words 
> > > heh
> > > It's not ideal, but might be a last resort for users wanting to collect
> > > the most information they can without saving a full vmcore. And for
> > > that, we need to be able to invoke "panic_print" function before the
> > > __crash_kexec() call. Continue below...
> > 
> > OK, pstore via kmsg_dump is first option, then fallback to kdump.
> > This is what I suggested at below. This is what panic notifier has done
> > at below. I think both of them are similar, thus should take the same
> > way to handle.
> > 
> >  void panic()
> >  {
> >  if (!_crash_kexec_post_notifiers && !panic_print) {
> >  __crash_kexec(NULL);
> >  smp_send_stop();
> >  } else {
> >  crash_smp_send_stop();
> >  }
> > 
> > atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
> > panic_print_sys_info(false);
> > kmsg_dump(KMSG_DUMP_PANIC);
> > if (_crash_kexec_post_notifiers || panic_print)
> >  __crash_kexec(NULL);
> > ...
> > debug_locks_off();
> >  console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> > 
> >  panic_print_sys_info(true);
> > ..
> >  }
> > > >
> > >
> > > So, your idea is good and it mostly works, except it *requires* users to
> > > make use of "crash_kexec_post_notifiers" in order to use "panic_print"
> > > in the case (b) above discussed.
> > 
> > I don't get. Why it has to *require* users to make use of
> > "crash_kexec_post_notifiers" in order to use "panic_print"?
> > To enable panic notifiers and panic_print, we need add below parameter
> > to kernel cmdline separately.
> > 
> > crash_kexec_post_notifiers=1
> > panic_print=0x7f
> > 
> > With above co

Re: [PATCH V4] notifier/panic: Introduce panic_notifier_filter

2022-01-21 Thread Guilherme G. Piccoli
Hi Petr, thanks for the great response, and for CCing more (potentially)
interested parties! Some comments inline below; also, I'm CCing Michael
Kelley as well.


On 20/01/2022 12:14, Petr Mladek wrote:
> Adding some more people into Cc. Some modified the logic in the past.
> Some are familiar with some interesting areas where the panic
> notfiers are used.
> 
> On Sat 2022-01-08 12:34:51, Guilherme G. Piccoli wrote:
>> [...]
>> There are some cases though in which kdump users might want to
>> allow panic notifier callbacks to execute _before_ the kexec to
>> the crash kernel, for a variety of reasons - for example, users
>> may think kexec is very prone to fail and want to give a chance
>> to kmsg dumpers to run (and save logs using pstore),
> 
> Yes, this seems to be original intention for the
> "crash_kexec_post_notifiers" option, see the commit
> f06e5153f4ae2e2f3b0300f ("kernel/panic.c: add
> "crash_kexec_post_notifiers" option for kdump after panic_notifiers")
> 
>> some panic notifier is required to properly quiesce some hardware
>> that must be used to the crash kernel.
> 
> Do you have any example, please? The above mentioned commit
> says "crash_kexec_post_notifiers" actually increases risk
> of kdump failure.
> 
> Note that kmsg_dump() is called after the notifiers only because
> some are printing more information, see the commit
> 6723734cdff15211bb78a ("panic: call panic handlers before kmsg_dump").
> They might still increase the change that kmsg_dump() will never
> be called.
> 

Sure! I guess Michael Kelley's response here is the perfect example:
https://lore.kernel.org/lkml/mwhpr21mb1593a32a3433f5f262796fcfd7...@mwhpr21mb1593.namprd21.prod.outlook.com/

In my understanding, he is referring the function hyperv_panic_event().
But I also found another 2 examples in a quick look: bcm_vk_on_panic()
and brcmstb_pm_panic_notify().


>> [...]
>> So, this patch aims to ease this decision: we hereby introduce a filter
>> for the panic notifier list, in which users may select specifically
>> which callbacks they wish to run, allowing a safer kdump. The allowlist
>> should be provided using the parameter "panic_notifier_filter=a,b,..."
>> where a, b are valid callback names. Invalid symbols are discarded.
> 
> I am afraid that this is almost unusable solution:
> 
>+ requires deep knowledge of what each notifier does
>+ might need debugging what notifier causes problems
>+ the list might need to be updated when new notifiers are added
>+ function names are implementation detail and might change
>+ requires kallsyms
> 
> 
> It is only workaround for a real problem. The problem is that
> "panic_notifier_list" is used for many purposes that break
> each other.
> 
> I checked some notifiers and found few groups:
> 
>+ disable watchdogs:
>   + hung_task_panic()
>   + rcu_panic()
> 
>+ dump information:
>   + kernel_offset_notifier()
>   + trace_panic_handler() (duplicate of panic_print=0x10)
> 
>+ inform hypervisor
>   + xen_panic_event()
>   + pvpanic_panic_notify()
>   + hyperv_panic_event()
> 
>+ misc cleanup / flush / blinking
>   + panic_event()   in ipmi_msghandler.c
>   + panic_happened()   in heartbeat.c
>   + led_trigger_panic_notifier()
> 
> 
> IMHO, the right solution is to split the callbacks into 2 or more
> notifier list. Then we might rework panic() to do:
> 
> void panic(void)
> {
>   [...]
> 
>   /* stop watchdogs + extra info */
>   atomic_notifier_call_chain(&panic_disable_watchdogs_notifier_list, 0, 
> buf);
>   atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
>   panic_print_sys_info();
> 
>   /* crash_kexec + kmsg_dump in configurable order */
>   if (!_crash_kexec_post_kmsg_dump) {
>   __crash_kexec(NULL);
>   smp_send_stop();
>   } else {
>   crash_smp_send_stop();
>   }
> 
>   kmsg_dump();
>   if (_crash_kexec_post_kmsg_dump)
>   __crash_kexec(NULL);
> 
>   /* infinite loop or reboot */
>   atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
>   atomic_notifier_call_chain(&panic_rest_notifier_list, 0, buf);
> 
>   console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> [...] 
> Two notifier lists might be enough in the above scenario. I would call
> them:
> 
>   panic_pre_dump_notifier_list
>   panic_post_dump_notifier_list
> 
> 
> It is a real solution that will help everyone. It is more complicated now
> but it will makes things much easier in the long term. And it might be done
> step by step:
> 
>  1. introduce the two notifier lists
>  2. convert all users: one by one
>  3. remove the original notifier list when there is no user

That's a great idea! I'm into it, if we have a consensus. The thing that
scares me most here is that this is a big change and consumes time to
implement - I'd not risk such time if somebody is really against that.
So, let's 

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

2022-01-21 Thread Michael Kelley (LINUX)
From: Baoquan He  Sent: Thursday, January 20, 2022 6:31 PM
> 
> On 01/20/22 at 06:36pm, Guilherme G. Piccoli wrote:
> > Hi Baoquan, some comments inline below:
> >
> >
> > On 20/01/2022 05:51, Baoquan He wrote:
> > > [...]
> > >> From my POV, the function of panic notifiers is not well defined. They
> > >> do various things, for example:
> > >> [...]
> > >> The do more that just providing information. Some are risky. It is not
> > >> easy to disable a particular one.
> > >
> > > Yes, agree. Not all of them just provide information.
> > >
> > > Now panic_notifier_filter Guilherme added can help to disable some of
> > > them.
> >
> > So, just for completeness, worth to mention Petr had some interesting
> > suggestions in the other thread (about the filter) and we may end-up not
> > having this implemented - in other words, maybe a refactor of that
> > mechanism is going to be proposed.
> 
> OK, saw that. We can continue discuss that there.
> 
> >
> >
> > > [...]
> > >>
> > >>   + Guilherme uses crash dump only to dump the kernel log. It might
> > >> be more reliable than kmsg_dump. In this case, panic_print_sys_info()
> > >> is the only way to get the extra information.
> > >
> > > Hmm, I haven't made clear what Guilherme really wants in his recent
> > > post. In this patch he wants to get panic print info into pstore. He
> > > also want to dump the kernel log poked by panic_print in kdump kernel.
> > > And it's very weird people try to collect kernel log via crash dump
> > > mechnism, that is obviously using a sledgehammer to crack a nut.
> > > Sometime, we should not add or change code to a too specific corner
> > > case.
> >
> > OK, I'll try to be really clear, hopefully I can explain the use case in
> > better and simpler words. First of all, I wouldn't call it a corner case
> > - it's just a valid use case that, in my opinion, should be allowed. Why
> > not, right? Kernel shouldn't push policy on users, we should instead let
> > the users decide how to use the tools/options.
> 
> Agree, sorry about my wrong expression.
> 
> >
> > So imagine you cannot collect a vmcore, due to the lack of storage
> > space. Yet, you want the most information as possible to investigate the
> > cause of a panic. The kernel flag "panic_print" is the perfect fit, we
> > can dump backtraces, task list, memory info...right on a panic event.
> >
> > But then, how to save this panic log with lots of information after a
> > reboot? There are 2 ways in my understanding:
> >
> > (a) pstore/kmsg_dump()
> > (b) kdump
> >
> > The option (a) is easily the best - we don't need to reserve lots of
> > memory, then boot another kernel, etc. This patch (being hereby
> > discussed) aims to enable the "panic_print" output for this case!
> > But...there are cases in which option (a) cannot work. We need a backend
> > of persistent storage, either a block device or, more common, RAM memory
> > that is persistent across a reboot. What if it's not available?
> >
> > Then, we fallback to option (b) - kind of a sledgehammer, in your words heh
> > It's not ideal, but might be a last resort for users wanting to collect
> > the most information they can without saving a full vmcore. And for
> > that, we need to be able to invoke "panic_print" function before the
> > __crash_kexec() call. Continue below...
> 
> OK, pstore via kmsg_dump is first option, then fallback to kdump.
> This is what I suggested at below. This is what panic notifier has done
> at below. I think both of them are similar, thus should take the same
> way to handle.
> 
>  void panic()
>  {
>  if (!_crash_kexec_post_notifiers && !panic_print) {
>  __crash_kexec(NULL);
>  smp_send_stop();
>  } else {
>  crash_smp_send_stop();
>  }
> 
>   atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
>   panic_print_sys_info(false);
>   kmsg_dump(KMSG_DUMP_PANIC);
>   if (_crash_kexec_post_notifiers || panic_print)
>  __crash_kexec(NULL);
>   ...
>   debug_locks_off();
>  console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> 
>  panic_print_sys_info(true);
>   ..
>  }
> > >
> >
> > So, your idea is good and it mostly works, except it *requires* users to
> > make use of "crash_kexec_post_notifiers" in order to use "panic_print"
> > in the case (b) above discussed.
> 
> I don't get. Why it has to *require* users to make use of
> "crash_kexec_post_notifiers" in order to use "panic_print"?
> To enable panic notifiers and panic_print, we need add below parameter
> to kernel cmdline separately.
> 
>   crash_kexec_post_notifiers=1
> panic_print=0x7f
> 
> With above code, we have:
> 1) None specified in cmdline, only kdump enabled.
>Crash dump will work to get vmcore.
> 2) crash_kexec_post_notifiers=1 , kdump enabled
>panic_notifers are executed, then crash dump
> 3) panic_print=0x7f, kdump enabled,
>Panic_print get system info printed, t

Re: [PATCH v3 6/6] crash hp: Add x86 crash hotplug support

2022-01-21 Thread Eric DeVolder

Baoquan,
Thanks for the feedback, inline responses below!
eric


On 1/19/22 04:23, Baoquan He wrote:

On 01/10/22 at 02:57pm, Eric DeVolder wrote:

For x86_64, when CPU or memory is hot un/plugged, the crash
elfcorehdr, which describes the CPUs and memory in the system,
must also be updated.

To update the elfcorehdr for x86_64, a new elfcorehdr must be
generated from the available CPUs and memory. The new elfcorehdr
is prepared into a buffer, and if no errors occur, it is
installed over the top of the existing elfcorehdr.

In the patch 'crash hp: kexec_file changes for crash hotplug support'
the need to update purgatory due to the change in elfcorehdr was
eliminated.  As a result, no changes to purgatory or boot_params
(as the elfcorehdr= kernel command line parameter pointer
remains unchanged and correct) are needed, just elfcorehdr.

To accommodate a growing number of resources via hotplug, the
elfcorehdr segment must be sufficiently large enough to accommodate
changes, see the CRASH_HOTPLUG_ELFCOREHDR_SZ configure item.

NOTE that this supports both kexec_load and kexec_file_load. Support
for kexec_load is made possible by identifying the elfcorehdr segment
at load time and updating it as previously described. However, it is
the responsibility of the userspace kexec utility to ensure that:
  - the elfcorehdr segment is sufficiently large enough to accommodate
hotplug changes, ala CRASH_HOTPLUG_ELFCOREHDR_SZ.
  - provides a purgatory that excludes the elfcorehdr from its list of
run-time segments to check.
These changes to the userspace kexec utility are not yet available.

Signed-off-by: Eric DeVolder 
---
  arch/x86/kernel/crash.c | 138 +++-
  1 file changed, 137 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9730c88530fc..d185137b33d4 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -25,6 +25,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include 

  #include 
@@ -265,7 +266,8 @@ static int prepare_elf_headers(struct kimage *image, void 
**addr,
goto out;
  
  	/* By default prepare 64bit headers */

-   ret =  crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), 
addr, sz);
+   ret =  crash_prepare_elf64_headers(image, cmem,
+   IS_ENABLED(CONFIG_X86_64), addr, sz);
  
  out:

vfree(cmem);
@@ -397,7 +399,17 @@ int crash_load_segments(struct kimage *image)
image->elf_headers = kbuf.buffer;
image->elf_headers_sz = kbuf.bufsz;
  
+#ifdef CONFIG_CRASH_HOTPLUG

+   /* Ensure elfcorehdr segment large enough for hotplug changes */
+   kbuf.memsz = CONFIG_CRASH_HOTPLUG_ELFCOREHDR_SZ;


I would define a default value for the size, meantime provide a Kconfig
option to allow user to customize.


In patch 2/6 of this series, "crash hp: Introduce CRASH_HOTPLUG configuration options", I provide 
the following:


+config CRASH_HOTPLUG_ELFCOREHDR_SZ
+   depends on CRASH_HOTPLUG
+   int
+   default 131072
+   help
+ Specify the maximum size of the elfcorehdr buffer/segment.

which defines a default value of 128KiB, and can be overriden at configure time.

Are you asking for a different technique?




+   /* For marking as usable to crash kernel */
+   image->elf_headers_sz = kbuf.memsz;
+   /* Record the index of the elfcorehdr segment */
+   image->elf_index = image->nr_segments;
+   image->elf_index_valid = true;
+#else
kbuf.memsz = kbuf.bufsz;
+#endif
kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer(&kbuf);
@@ -412,3 +424,127 @@ int crash_load_segments(struct kimage *image)
return ret;
  }
  #endif /* CONFIG_KEXEC_FILE */
+
+#ifdef CONFIG_CRASH_HOTPLUG


These two helper function should be carved out into a separate patch as
a preparatory one. I am considering how to rearrange and split the
patches, will reply to cover letter.


OK, I look forward to that insight!




+void *map_crash_pages(unsigned long paddr, unsigned long size)
+{
+   /*
+* NOTE: The addresses and sizes passed to this routine have
+* already been fully aligned on page boundaries. There is no
+* need for massaging the address or size.
+*/
+   void *ptr = NULL;
+
+   /* NOTE: requires arch_kexec_[un]protect_crashkres() for write access */
+   if (size > 0) {
+   struct page *page = pfn_to_page(paddr >> PAGE_SHIFT);
+
+   ptr = kmap(page);
+   }
+
+   return ptr;
+}
+
+void unmap_crash_pages(void **ptr)
+{
+   if (ptr) {
+   if (*ptr)
+   kunmap(*ptr);
+   *ptr = NULL;
+   }
+}
+
+void arch_crash_hotplug_handler(struct kimage *image,
+   unsigned int hp_action, unsigned long a, unsigned long b)
+{
+   /*
+* To accurately reflect hot un/plug changes, the elfcorehdr (which
+  

Re: [PATCH v3 3/6] crash hp: definitions and prototype changes

2022-01-21 Thread Eric DeVolder

Baoquan,
Thanks for looking at this. Inline responses below.
eric

On 1/19/22 02:26, Baoquan He wrote:

On 01/10/22 at 02:57pm, Eric DeVolder wrote:

This change adds members to struct kimage to facilitate crash
hotplug support.

This change also defines crash hotplug events and associated
prototypes.

Signed-off-by: Eric DeVolder 
---
  include/linux/kexec.h | 21 +++--
  1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 0c994ae37729..068f853f1c65 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -221,8 +221,9 @@ struct crash_mem {
  extern int crash_exclude_mem_range(struct crash_mem *mem,
   unsigned long long mstart,
   unsigned long long mend);
-extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
-  void **addr, unsigned long *sz);
+extern int crash_prepare_elf64_headers(struct kimage *image,
+   struct crash_mem *mem, int kernel_map,
+   void **addr, unsigned long *sz);
  #endif /* CONFIG_KEXEC_FILE */
  
  #ifdef CONFIG_KEXEC_ELF

@@ -299,6 +300,13 @@ struct kimage {
  
  	/* Information for loading purgatory */

struct purgatory_info purgatory_info;
+
+#ifdef CONFIG_CRASH_HOTPLUG
+   bool hotplug_event;
+   int offlinecpu;
+   bool elf_index_valid;
+   int elf_index;


Do we really need elf_index_valid? Can we initialize elf_index to , e.g '-1',
then check if the value is valid?


These members become part of struct kimage, and when the kimage is allocated, it is automatically 
zero'd. Wrt/ elf_index, 0 is a valid index, and so it needs to be qualified. I initially had used 
-1, but that required code and was fragile as I had to find the right place to do that. Using the 
boolean elf_index_valid, the problems with -1 vanish, and for free! I also found when examining the 
code that reading 'elf_index_valid' was better than 'elf_index != -1', more clear.


Let me know what you think.
eric




+#endif
  #endif
  
  #ifdef CONFIG_IMA_KEXEC

@@ -315,6 +323,15 @@ struct kimage {
unsigned long elf_load_addr;
  };
  
+#ifdef CONFIG_CRASH_HOTPLUG

+void arch_crash_hotplug_handler(struct kimage *image,
+   unsigned int hp_action, unsigned long a, unsigned long b);
+#define KEXEC_CRASH_HP_REMOVE_CPU   0
+#define KEXEC_CRASH_HP_ADD_CPU  1
+#define KEXEC_CRASH_HP_REMOVE_MEMORY 2
+#define KEXEC_CRASH_HP_ADD_MEMORY   3
+#endif /* CONFIG_CRASH_HOTPLUG */
+
  /* kexec interface functions */
  extern void machine_kexec(struct kimage *image);
  extern int machine_kexec_prepare(struct kimage *image);
--
2.27.0





___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


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

2022-01-21 Thread Guilherme G. Piccoli
Hi Baoquan , thanks again for your prompt reply!
Comments inline below:


On 20/01/2022 23:31, Baoquan He wrote:
> [...]
>> OK, I'll try to be really clear, hopefully I can explain the use case in
>> better and simpler words. First of all, I wouldn't call it a corner case
>> - it's just a valid use case that, in my opinion, should be allowed. Why
>> not, right? Kernel shouldn't push policy on users, we should instead let
>> the users decide how to use the tools/options.
> 
> Agree, sorry about my wrong expression.

No need to be sorry at all! And if you indeed consider that a corner
case, feel free to express that and we should take it into account =)
Your opinion is much appreciated!


> 
> OK, pstore via kmsg_dump is first option, then fallback to kdump.
> This is what I suggested at below. This is what panic notifier has done
> at below. I think both of them are similar, thus should take the same
> way to handle.
> 
>  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 }
>  
>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);
>   ..
>  }
>[...] 
> I don't get. Why it has to *require* users to make use of
> "crash_kexec_post_notifiers" in order to use "panic_print"? 
> To enable panic notifiers and panic_print, we need add below parameter
> to kernel cmdline separately.
> 
>   crash_kexec_post_notifiers=1
> panic_print=0x7f
> 
> With above code, we have:
> 1) None specified in cmdline, only kdump enabled.
>Crash dump will work to get vmcore.
> 2) crash_kexec_post_notifiers=1 , kdump enabled
>panic_notifers are executed, then crash dump
> 3) panic_print=0x7f, kdump enabled,
>Panic_print get system info printed, then crash dump
> 4) crash_kexec_post_notifiers=1 panic_print=0x7f, kdump enabled
>panic_notifers are executed firstly, then panic_print, at last crash dump
> 
> Here I don't list the no kdump enabled case. Please help point out if I
> misunderstood anything.

OK, this is a really great summary list of the possible cases, thanks
for that. I might be wrong here, this code is a bit confusing for
me...so I put line numbers in your code and we can discuss based on that.

Case (1) - Line L2 is reached, we jump to the kdump kernel, right?
Case (2) - Line L5 and lines L8->L12 executed, correct?

Case (3) - I don't understand this case! If kdump is enabled and
panic_print as well, we execute Line L2 right? If that's not the case,
then we jump to kdump kernel at line L12, but that means L8 was
executed, the notifiers list. Right?

So, how is it possible in your code to execute
"panic_print_sys_info(false);" and then jump to kdump *without* reaching L8?

I apologize in advance if I'm silly and it's obvious - I guess I won't
get the C-programmer-prize of the year anyway heheh


>> Sure, I'll rename "after_kmsg_dumpers" to "console_flush" in next
>> iteration, although my nerd side won't be so happy ;-)
> 
> No offence at all. My wife always call me nerd. Sorry about that.

No offense taken, and no need to be sorry - we're cool!
I got the joke =D

And the variable name suggestion was indeed good.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec