Re: [PATCH 16/30] drivers/hv/vmbus, video/hyperv_fb: Untangle and refactor Hyper-V panic notifiers
Hi Michael, first of all thanks for the great review, much appreciated. Some comments inline below: On 29/04/2022 14:16, Michael Kelley (LINUX) wrote: > [...] >> hypervisor I/O completion), so we postpone that to run late. But more >> relevant: this *same* vmbus unloading happens in the crash_shutdown() >> handler, so if kdump is set, we can safely skip this panic notifier and >> defer such clean-up to the kexec crash handler. > > While the last sentence is true for Hyper-V on x86/x64, it's not true for > Hyper-V on ARM64. x86/x64 has the 'machine_ops' data structure > with the ability to provide a custom crash_shutdown() function, which > Hyper-V does in the form of hv_machine_crash_shutdown(). But ARM64 > has no mechanism to provide such a custom function that will eventually > do the needed vmbus_initiate_unload() before running kdump. > > I'm not immediately sure what the best solution is for ARM64. At this > point, I'm just pointing out the problem and will think about the tradeoffs > for various possible solutions. Please do the same yourself. :-) > Oh, you're totally right! I just assumed ARM64 would the the same, my bad. Just to propose some alternatives, so you/others can also discuss here and we can reach a consensus about the trade-offs: (a) We could forget about this change, and always do the clean-up here, not relying in machine_crash_shutdown(). Pro: really simple, behaves the same as it is doing currently. Con: less elegant/concise, doesn't allow arm64 customization. (b) Add a way to allow ARM64 customization of shutdown crash handler. Pro: matches x86, more customizable, improves arm64 arch code. Con: A tad more complex. Also, a question that came-up: if ARM64 has no way of calling special crash shutdown handler, how can you execute hv_stimer_cleanup() and hv_synic_disable_regs() there? Or are they not required in ARM64? >> >> (c) There is also a Hyper-V framebuffer panic notifier, which relies in >> doing a vmbus operation that demands a valid connection. So, we must >> order this notifier with the panic notifier from vmbus_drv.c, in order to >> guarantee that the framebuffer code executes before the vmbus connection >> is unloaded. > > Patch 21 of this set puts the Hyper-V FB panic notifier on the pre_reboot > notifier list, which means it won't execute before the VMbus connection > unload in the case of kdump. This notifier is making sure that Hyper-V > is notified about the last updates made to the frame buffer before the > panic, so maybe it needs to be put on the hypervisor notifier list. It > sends a message to Hyper-V over its existing VMbus channel, but it > does not wait for a reply. It does, however, obtain a spin lock on the > ring buffer used to communicate with Hyper-V. Unless someone has > a better suggestion, I'm inclined to take the risk of blocking on that > spin lock. The logic behind that was: when kdump is set, we'd skip the vmbus disconnect on notifiers, deferring that to crash_shutdown(), logic this one refuted in the above discussion on ARM64 (one more Pro argument to the idea of refactoring aarch64 code to allow a custom crash shutdown handler heh). But you're right, for the default level 2, we skip the pre_reboot notifiers on kdump, effectively skipping this notifier. Some ideas of what we can do here: I) we could change the framebuffer notifier to rely on trylocks, instead of risking a lockup scenario, and with that, we can execute it before the vmbus disconnect in the hypervisor list; II) we ignore the hypervisor notifier in case of kdump _by default_, and if the users don't want that, they can always set the panic notifier level to 4 and run all notifiers prior to kdump; would that be terrible you think? Kdump users might don't care about the framebuffer... III) we go with approach (b) above and refactor arm64 code to allow the custom crash handler on kdump time, then [with point (I) above] the logic proposed in this series is still valid - seems more and more the most correct/complete solution. In any case, I guess we should avoid workarounds if possible and do the things the best way we can, to encompass all (or almost all) the possible scenarios and don't force things on users (like enforcing panic notifier level 4 for Hyper-V or something like this...) More feedback from you / Hyper-V folks is pretty welcome about this. > >> [...] > The "Fixes:" tags imply that these changes should be backported to older > longterm kernel versions, which I don't think is the case. There is a > dependency on Patch 14 of your series where PANIC_NOTIFIER is > introduced. > Oh, this was more related with archeology of the kernel. When I'm investigating stuff, I really want to understand why code was added and that usually require some time git blaming stuff, so having that pronto in the commit message is a bonus. But of course we don't need to use the Fixes tag for that, easy to only mention it in the text. A secondary benefit by using this
Re: [PATCH 02/30] ARM: kexec: Disable IRQs/FIQs also on crash CPUs shutdown path
On Fri, 29 Apr 2022 22:45:14 +0100, "Russell King (Oracle)" wrote: > > On Fri, Apr 29, 2022 at 06:38:19PM -0300, Guilherme G. Piccoli wrote: > > Thanks Marc and Michael for the review/discussion. > > > > On 29/04/2022 15:20, Marc Zyngier wrote: > > > [...] > > > > > My expectations would be that, since we're getting here using an IPI, > > > interrupts are already masked. So what reenabled them the first place? > > > > > > Thanks, > > > > > > M. > > > > > > > Marc, I did some investigation in the code (and tried/failed in the ARM > > documentation as well heh), but this is still not 100% clear for me. > > > > You're saying IPI calls disable IRQs/FIQs by default in the the target > > CPUs? Where does it happen? I'm a bit confused if this a processor > > mechanism, or it's in code. > > When we taken an IRQ, IRQs will be masked, FIQs will not. IPIs are > themselves interrupts, so IRQs will be masked while the IPI is being > processed. Therefore, there should be no need to re-disable the > already disabled interrupts. > > > But crash_smp_send_stop() is different, it seems to IPI the other CPUs > > with the flag IPI_CALL_FUNC, which leads to calling > > generic_smp_call_function_interrupt() - does it disable interrupts/FIQs > > as well? I couldn't find it. > > It's buried in the architecture behaviour. When the CPU takes an > interrupt and jumps to the interrupt vector in the vectors page, it is > architecturally defined that interrupts will be disabled. If they > weren't architecturally disabled at this point, then as soon as the > first instruction is processed (at the interrupt vector, likely a > branch) the CPU would immediately take another jump to the interrupt > vector, and this process would continue indefinitely, making interrupt > handling utterly useless. > > So, you won't find an explicit instruction in the code path from the > vectors to the IPI handler that disables interrupts - because it's > written into the architecture that this is what must happen. > > IRQs are a lower priority than FIQs, so FIQs remain unmasked. Ah, you're of course right. That's one of the huge differences between AArch32 and AArch64, where the former has per target mode masking rules, and the later masks everything on entry... M. -- Without deviation from the norm, progress is not possible. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 02/30] ARM: kexec: Disable IRQs/FIQs also on crash CPUs shutdown path
On 29/04/2022 18:45, Russell King (Oracle) wrote: > [...] >> Marc, I did some investigation in the code (and tried/failed in the ARM >> documentation as well heh), but this is still not 100% clear for me. >> >> You're saying IPI calls disable IRQs/FIQs by default in the the target >> CPUs? Where does it happen? I'm a bit confused if this a processor >> mechanism, or it's in code. > > When we taken an IRQ, IRQs will be masked, FIQs will not. IPIs are > themselves interrupts, so IRQs will be masked while the IPI is being > processed. Therefore, there should be no need to re-disable the > already disabled interrupts. > >> But crash_smp_send_stop() is different, it seems to IPI the other CPUs >> with the flag IPI_CALL_FUNC, which leads to calling >> generic_smp_call_function_interrupt() - does it disable interrupts/FIQs >> as well? I couldn't find it. > > It's buried in the architecture behaviour. When the CPU takes an > interrupt and jumps to the interrupt vector in the vectors page, it is > architecturally defined that interrupts will be disabled. If they > weren't architecturally disabled at this point, then as soon as the > first instruction is processed (at the interrupt vector, likely a > branch) the CPU would immediately take another jump to the interrupt > vector, and this process would continue indefinitely, making interrupt > handling utterly useless. > > So, you won't find an explicit instruction in the code path from the > vectors to the IPI handler that disables interrupts - because it's > written into the architecture that this is what must happen. > > IRQs are a lower priority than FIQs, so FIQs remain unmasked. > Thanks a lot for the *great* explanation Russell, much appreciated. So, this leads to the both following questions: a) Shall we then change the patch to only disable FIQs, since it's panic path and we don't want secondary CPUs getting interrupted, but only spinning quietly "forever"? b) How about cleaning ipi_cpu_stop() then, by dropping the call to local_irq_disable() there, to avoid the double IRQ disabling? Thanks, Guilherme ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 02/30] ARM: kexec: Disable IRQs/FIQs also on crash CPUs shutdown path
On Fri, Apr 29, 2022 at 06:38:19PM -0300, Guilherme G. Piccoli wrote: > Thanks Marc and Michael for the review/discussion. > > On 29/04/2022 15:20, Marc Zyngier wrote: > > [...] > > > My expectations would be that, since we're getting here using an IPI, > > interrupts are already masked. So what reenabled them the first place? > > > > Thanks, > > > > M. > > > > Marc, I did some investigation in the code (and tried/failed in the ARM > documentation as well heh), but this is still not 100% clear for me. > > You're saying IPI calls disable IRQs/FIQs by default in the the target > CPUs? Where does it happen? I'm a bit confused if this a processor > mechanism, or it's in code. When we taken an IRQ, IRQs will be masked, FIQs will not. IPIs are themselves interrupts, so IRQs will be masked while the IPI is being processed. Therefore, there should be no need to re-disable the already disabled interrupts. > But crash_smp_send_stop() is different, it seems to IPI the other CPUs > with the flag IPI_CALL_FUNC, which leads to calling > generic_smp_call_function_interrupt() - does it disable interrupts/FIQs > as well? I couldn't find it. It's buried in the architecture behaviour. When the CPU takes an interrupt and jumps to the interrupt vector in the vectors page, it is architecturally defined that interrupts will be disabled. If they weren't architecturally disabled at this point, then as soon as the first instruction is processed (at the interrupt vector, likely a branch) the CPU would immediately take another jump to the interrupt vector, and this process would continue indefinitely, making interrupt handling utterly useless. So, you won't find an explicit instruction in the code path from the vectors to the IPI handler that disables interrupts - because it's written into the architecture that this is what must happen. IRQs are a lower priority than FIQs, so FIQs remain unmasked. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 02/30] ARM: kexec: Disable IRQs/FIQs also on crash CPUs shutdown path
Thanks Marc and Michael for the review/discussion. On 29/04/2022 15:20, Marc Zyngier wrote: > [...] > My expectations would be that, since we're getting here using an IPI, > interrupts are already masked. So what reenabled them the first place? > > Thanks, > > M. > Marc, I did some investigation in the code (and tried/failed in the ARM documentation as well heh), but this is still not 100% clear for me. You're saying IPI calls disable IRQs/FIQs by default in the the target CPUs? Where does it happen? I'm a bit confused if this a processor mechanism, or it's in code. Looking the smp_send_stop() in arch/arm/, it does IPI the CPUs, with the flag IPI_CPU_STOP, eventually calling ipi_cpu_stop(), and the latter does disable IRQ/FIQ in code - that's where I stole my code from. But crash_smp_send_stop() is different, it seems to IPI the other CPUs with the flag IPI_CALL_FUNC, which leads to calling generic_smp_call_function_interrupt() - does it disable interrupts/FIQs as well? I couldn't find it. Appreciate your clarifications about that, thanks again. Cheers, Guilherme ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 18/30] notifier: Show function names on notifier routines if DEBUG_NOTIFIERS is set
On 27/04/2022 22:01, Xiaoming Ni wrote:
> [...]
> Duplicate Code.
>
> Is it better to use __func__ and %pS?
>
> pr_info("%s: %pS\n", __func__, n->notifier_call);
>
>
This is a great suggestion Xiaoming, much appreciated!
I feel like reinventing the wheel here - with your idea, code was super
clear and concise, very nice suggestion!!
The only 2 things that diverge from your idea: I'm using '%ps' (not
showing offsets) and also, kept the wording "(un)registered/calling",
not using __func__ - I feel it's a bit odd in the output.
OK for you?
I'm definitely using your idea in V2 heh
Cheers,
Guilherme
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 21/30] panic: Introduce the panic pre-reboot notifier list
On 29/04/2022 13:04, Max Filippov wrote: > [...] >> arch/xtensa/platforms/iss/setup.c | 4 ++--For xtensa: > > For xtensa: > Acked-by: Max Filippov > Perfect, thanks Max! Cheers, Guilherme ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 02/30] ARM: kexec: Disable IRQs/FIQs also on crash CPUs shutdown path
On Wed, 27 Apr 2022 23:48:56 +0100, "Guilherme G. Piccoli" wrote: > > Currently the regular CPU shutdown path for ARM disables IRQs/FIQs > in the secondary CPUs - smp_send_stop() calls ipi_cpu_stop(), which > is responsible for that. This makes sense, since we're turning off > such CPUs, putting them in an endless busy-wait loop. > > Problem is that there is an alternative path for disabling CPUs, > in the form of function crash_smp_send_stop(), used for kexec/panic > paths. This functions relies in a SMP call that also triggers a > busy-wait loop [at machine_crash_nonpanic_core()], but *without* > disabling interrupts. This might lead to odd scenarios, like early > interrupts in the boot of kexec'd kernel or even interrupts in > other CPUs while the main one still works in the panic path and > assumes all secondary CPUs are (really!) off. > > This patch mimics the ipi_cpu_stop() interrupt disable mechanism > in the crash CPU shutdown path, hence disabling IRQs/FIQs in all > secondary CPUs in the kexec/panic path as well. > > Cc: Marc Zyngier > Cc: Russell King > Signed-off-by: Guilherme G. Piccoli > --- > arch/arm/kernel/machine_kexec.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c > index f567032a09c0..ef788ee00519 100644 > --- a/arch/arm/kernel/machine_kexec.c > +++ b/arch/arm/kernel/machine_kexec.c > @@ -86,6 +86,9 @@ void machine_crash_nonpanic_core(void *unused) > set_cpu_online(smp_processor_id(), false); > atomic_dec(_for_crash_ipi); > > + local_fiq_disable(); > + local_irq_disable(); > + My expectations would be that, since we're getting here using an IPI, interrupts are already masked. So what reenabled them the first place? Thanks, M. -- Without deviation from the norm, progress is not possible. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 13/30] s390/consoles: Improve panic notifiers reliability
On Wed, Apr 27, 2022 at 07:49:07PM -0300, Guilherme G. Piccoli wrote: > Currently many console drivers for s390 rely on panic/reboot notifiers > to invoke callbacks on these events. The panic() function disables local > IRQs, secondary CPUs and preemption, so callbacks invoked on panic are > effectively running in atomic context. > > Happens that most of these console callbacks from s390 doesn't take the > proper care with regards to atomic context, like taking spinlocks that > might be taken in other function/CPU and hence will cause a lockup > situation. > > The goal for this patch is to improve the notifiers reliability, acting > on 4 console drivers, as detailed below: > > (1) con3215: changed a regular spinlock to the trylock alternative. > > (2) con3270: also changed a regular spinlock to its trylock counterpart, > but here we also have another problem: raw3270_activate_view() takes a > different spinlock. So, we worked a helper to validate if this other lock > is safe to acquire, and if so, raw3270_activate_view() should be safe. > > Notice though that there is a functional change here: it's now possible > to continue the notifier code [reaching con3270_wait_write() and > con3270_rebuild_update()] without executing raw3270_activate_view(). > > (3) sclp: a global lock is used heavily in the functions called from > the notifier, so we added a check here - if the lock is taken already, > we just bail-out, preventing the lockup. > > (4) sclp_vt220: same as (3), a lock validation was added to prevent the > potential lockup problem. > > Besides (1)-(4), we also removed useless void functions, adding the > code called from the notifier inside its own body, and changed the > priority of such notifiers to execute late, since they are "heavyweight" > for the panic environment, so we aim to reduce risks here. > Changed return values to NOTIFY_DONE as well, the standard one. > > Cc: Alexander Gordeev > Cc: Christian Borntraeger > Cc: Heiko Carstens > Cc: Sven Schnelle > Cc: Vasily Gorbik > Signed-off-by: Guilherme G. Piccoli > --- > > As a design choice, the option used here to verify a given spinlock is taken > was the function "spin_is_locked()" - but we noticed that it is not often > used. > An alternative would to take the lock with a spin_trylock() and if it > succeeds, > just release the spinlock and continue the code. But that seemed weird... > > Also, we'd like to ask a good validation of case (2) potential functionality > change from the s390 console experts - far from expert here, and in our naive > code observation, that seems fine, but that analysis might be missing some > corner case. > > Thanks in advance! > > drivers/s390/char/con3215.c| 36 +++-- > drivers/s390/char/con3270.c| 34 +++ > drivers/s390/char/raw3270.c| 18 +++ > drivers/s390/char/raw3270.h| 1 + > drivers/s390/char/sclp_con.c | 28 +-- > drivers/s390/char/sclp_vt220.c | 42 +++--- > 6 files changed, 96 insertions(+), 63 deletions(-) Code looks good, and everything still seems to work. I applied this internally for the time being, and if it passes testing, I'll schedule it for the next merge window. Thanks! ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 13/30] s390/consoles: Improve panic notifiers reliability
On 29/04/2022 15:46, Heiko Carstens wrote: > [...] > > Code looks good, and everything still seems to work. I applied this > internally for the time being, and if it passes testing, I'll schedule > it for the next merge window. > > Thanks! Perfect Heiko, thanks a bunch for your review and tests! Let me know if anything breaks heh Cheers, Guilherme ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 21/30] panic: Introduce the panic pre-reboot notifier list
On 28/04/2022 13:26, Corey Minyard wrote: > [...] > > For the IPMI portion: > > Acked-by: Corey Minyard Thanks Alex and Corey for the ACKs! > > Note that the IPMI panic_event() should always return, but it may take > some time, especially if the IPMI controller is no longer functional. > So the risk of a long delay is there and it makes sense to move it very > late. > Thanks, I agree - the patch moves it to the (latest - 1) position, since some arch code might run as the latest and effectively stops the machine. Cheers, Guilherme ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 24/30] panic: Refactor the panic path
On 27/04/2022 21:28, Randy Dunlap wrote: > > > On 4/27/22 15:49, Guilherme G. Piccoli wrote: >> +crash_kexec_post_notifiers >> +This was DEPRECATED - users should always prefer the > > This is DEPRECATED - users should always prefer the > >> +parameter "panic_notifiers_level" - check its entry >> +in this documentation for details on how it works. >> +Setting this parameter is exactly the same as setting >> +"panic_notifiers_level=4". > Thanks Randy, for your suggestion - but I confess I couldn't understand it properly. It's related to spaces/tabs, right? What you suggest me to change in this formatting? Just by looking the email I can't parse. Cheers, Guilherme ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 12/30] parisc: Replace regular spinlock with spin_trylock on panic path
On 28/04/2022 13:55, Helge Deller wrote:
> [...]
> You may add:
> Acked-by: Helge Deller # parisc
>
> Helge
Thanks Helge, added!
Cheers,
Guilherme
>
>
>> ---
>> arch/parisc/include/asm/pdc.h | 1 +
>> arch/parisc/kernel/firmware.c | 27 +++
>> drivers/parisc/power.c| 17 ++---
>> 3 files changed, 34 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h
>> index b643092d4b98..7a106008e258 100644
>> --- a/arch/parisc/include/asm/pdc.h
>> +++ b/arch/parisc/include/asm/pdc.h
>> @@ -83,6 +83,7 @@ int pdc_do_firm_test_reset(unsigned long ftc_bitmap);
>> int pdc_do_reset(void);
>> int pdc_soft_power_info(unsigned long *power_reg);
>> int pdc_soft_power_button(int sw_control);
>> +int pdc_soft_power_button_panic(int sw_control);
>> void pdc_io_reset(void);
>> void pdc_io_reset_devices(void);
>> int pdc_iodc_getc(void);
>> diff --git a/arch/parisc/kernel/firmware.c b/arch/parisc/kernel/firmware.c
>> index 6a7e315bcc2e..0e2f70b592f4 100644
>> --- a/arch/parisc/kernel/firmware.c
>> +++ b/arch/parisc/kernel/firmware.c
>> @@ -1232,15 +1232,18 @@ int __init pdc_soft_power_info(unsigned long
>> *power_reg)
>> }
>>
>> /*
>> - * pdc_soft_power_button - Control the soft power button behaviour
>> - * @sw_control: 0 for hardware control, 1 for software control
>> + * pdc_soft_power_button{_panic} - Control the soft power button behaviour
>> + * @sw_control: 0 for hardware control, 1 for software control
>> *
>> *
>> * This PDC function places the soft power button under software or
>> * hardware control.
>> - * Under software control the OS may control to when to allow to shut
>> - * down the system. Under hardware control pressing the power button
>> + * Under software control the OS may control to when to allow to shut
>> + * down the system. Under hardware control pressing the power button
>> * powers off the system immediately.
>> + *
>> + * The _panic version relies in spin_trylock to prevent deadlock
>> + * on panic path.
>> */
>> int pdc_soft_power_button(int sw_control)
>> {
>> @@ -1254,6 +1257,22 @@ int pdc_soft_power_button(int sw_control)
>> return retval;
>> }
>>
>> +int pdc_soft_power_button_panic(int sw_control)
>> +{
>> +int retval;
>> +unsigned long flags;
>> +
>> +if (!spin_trylock_irqsave(_lock, flags)) {
>> +pr_emerg("Couldn't enable soft power button\n");
>> +return -EBUSY; /* ignored by the panic notifier */
>> +}
>> +
>> +retval = mem_pdc_call(PDC_SOFT_POWER, PDC_SOFT_POWER_ENABLE,
>> __pa(pdc_result), sw_control);
>> +spin_unlock_irqrestore(_lock, flags);
>> +
>> +return retval;
>> +}
>> +
>> /*
>> * pdc_io_reset - Hack to avoid overlapping range registers of Bridges
>> devices.
>> * Primarily a problem on T600 (which parisc-linux doesn't support) but
>> diff --git a/drivers/parisc/power.c b/drivers/parisc/power.c
>> index 456776bd8ee6..8512884de2cf 100644
>> --- a/drivers/parisc/power.c
>> +++ b/drivers/parisc/power.c
>> @@ -37,7 +37,6 @@
>> #include
>> #include
>> #include
>> -#include
>> #include
>> #include
>> #include
>> @@ -175,16 +174,21 @@ static void powerfail_interrupt(int code, void *x)
>>
>>
>>
>> -/* parisc_panic_event() is called by the panic handler.
>> - * As soon as a panic occurs, our tasklets above will not be
>> - * executed any longer. This function then re-enables the
>> - * soft-power switch and allows the user to switch off the system
>> +/*
>> + * parisc_panic_event() is called by the panic handler.
>> + *
>> + * As soon as a panic occurs, our tasklets above will not
>> + * be executed any longer. This function then re-enables
>> + * the soft-power switch and allows the user to switch off
>> + * the system. We rely in pdc_soft_power_button_panic()
>> + * since this version spin_trylocks (instead of regular
>> + * spinlock), preventing deadlocks on panic path.
>> */
>> static int parisc_panic_event(struct notifier_block *this,
>> unsigned long event, void *ptr)
>> {
>> /* re-enable the soft-power switch */
>> -pdc_soft_power_button(0);
>> +pdc_soft_power_button_panic(0);
>> return NOTIFY_DONE;
>> }
>>
>> @@ -193,7 +197,6 @@ static struct notifier_block parisc_panic_block = {
>> .priority = INT_MAX,
>> };
>>
>> -
>> static int __init power_init(void)
>> {
>> unsigned long ret;
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 09/30] coresight: cpu-debug: Replace mutex with mutex_trylock on panic notifier
On 28/04/2022 05:11, Suzuki K Poulose wrote: > Hi Guilherme, > [...] > How would you like to proceed with queuing this ? I am happy > either way. In case you plan to push this as part of this > series (I don't see any potential conflicts) : > > Reviewed-by: Suzuki K Poulose Thanks for your review Suzuki, much appreciated! About your question, I'm not sure yet - in case the core changes would take a while (like if community find them polemic, require many changes, etc) I might split this series in 2 parts, the fixes part vs the improvements per se. Either way, a V2 is going to happen for sure, and in that moment, I'll let you know what I think it's best. But either way, any choice you prefer is fine by me as well (like if you want to merge it now or postpone to get merged in the future), this is not an urgent fix I think =) Cheers, Guilherme ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 20/30] panic: Add the panic informational notifier list
Thanks Paul and Suzuki for the ACKs. Cheers, Guilherme ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
On 29/04/2022 10:23, Steven Rostedt wrote:
> On Fri, 29 Apr 2022 12:22:44 +0300
> Sergei Shtylyov wrote:
>
>>> + switch (ev) {
>>> + case DIE_OOPS:
>>> + do_dump = 1;
>>> + break;
>>> + case PANIC_NOTIFIER:
>>> + do_dump = 1;
>>> + break;
>>
>>Why not:
>>
>> case DIE_OOPS:
>> case PANIC_NOTIFIER:
>> do_dump = 1;
>> break;
>
> Agreed.
>
> Other than that.
>
> Acked-by: Steven Rostedt (Google)
>
> -- Steve
Thanks Sergei and Steven, good idea! I thought about the switch change
you propose, but I confess I got a bit confused by the "fallthrough"
keyword - do I need to use it?
About the s/int/bool, for sure! Not sure why I didn't use bool at
first...heheh
Cheers,
Guilherme
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
On Fri, 29 Apr 2022 10:46:35 -0300 "Guilherme G. Piccoli" wrote: > Thanks Sergei and Steven, good idea! I thought about the switch change > you propose, but I confess I got a bit confused by the "fallthrough" > keyword - do I need to use it? No. The fallthrough keyword is only needed when there's code between case labels. As it is very common to list multiple cases for the same code path. That is: case DIE_OOPS: case PANIC_NOTIFIER: do_dump = 1; break; Does not need a fall through label, as there's no code between the DIE_OOPS and the PANIC_NOTIFIER. But if you had: case DIE_OOPS: x = true; case PANIC_NOTIFIER: do_dump = 1; break; Then you do. -- Steve ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
On Fri, 29 Apr 2022 12:22:44 +0300
Sergei Shtylyov wrote:
> > + switch (ev) {
> > + case DIE_OOPS:
> > + do_dump = 1;
> > + break;
> > + case PANIC_NOTIFIER:
> > + do_dump = 1;
> > + break;
>
>Why not:
>
> case DIE_OOPS:
> case PANIC_NOTIFIER:
> do_dump = 1;
> break;
Agreed.
Other than that.
Acked-by: Steven Rostedt (Google)
-- Steve
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 24/30] panic: Refactor the panic path
On 29/04/2022 14:53, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli Sent: Wednesday, April 27,
> 2022 3:49 PM
>> [...]
>> +panic_notifiers_level=
>> +[KNL] Set the panic notifiers execution order.
>> +Format:
>> +We currently have 4 lists of panic notifiers; based
>> +on the functionality and risk (for panic success) the
>> +callbacks are added in a given list. The lists are:
>> +- hypervisor/FW notification list (low risk);
>> +- informational list (low/medium risk);
>> +- pre_reboot list (higher risk);
>> +- post_reboot list (only run late in panic and after
>> +kdump, not configurable for now).
>> +This parameter defines the ordering of the first 3
>> +lists with regards to kdump; the levels determine
>> +which set of notifiers execute before kdump. The
>> +accepted levels are:
>> +0: kdump is the first thing to run, NO list is
>> +executed before kdump.
>> +1: only the hypervisor list is executed before kdump.
>> +2 (default level): the hypervisor list and (*if*
>> +there's any kmsg_dumper defined) the informational
>> +list are executed before kdump.
>> +3: both the hypervisor and the informational lists
>> +(always) execute before kdump.
>
> I'm not clear on why level 2 exists. What is the scenario where
> execution of the info list before kdump should be conditional on the
> existence of a kmsg_dumper? Maybe the scenario is described
> somewhere in the patch set and I just missed it.
>
Hi Michael, thanks for your review/consideration. So, this idea started
kind of some time ago. It all started with a need of exposing more
information on kernel log *before* kdump and *before* pstore -
specifically, we're talking about panic_print. But this cause some
reactions, Baoquan was very concerned with that [0]. Soon after, I've
proposed a panic notifiers filter (orthogonal) approach, to which Petr
suggested instead doing a major refactor [1] - it finally is alive in
the form of this series.
The theory behind the level 2 is to allow a scenario of kdump with the
minimum amount of notifiers - what is the point in printing more
information if the user doesn't care, since it's going to kdump? Now, if
there is a kmsg dumper, it means that there is likely some interest in
collecting information, and that might as well be required before the
potential kdump (which is my case, hence the proposal on [0]).
Instead of forcing one of the two behaviors (level 1 or level 3), we
have a middle-term/compromise: if there's interest in collecting such
data (in the form of a kmsg dumper), we then execute the informational
notifiers before kdump. If not, why to increase (even slightly) the risk
for kdump?
I'm OK in removing the level 2 if people prefer, but I don't feel it's a
burden, quite opposite - seems a good way to accommodate the somewhat
antagonistic ideas (jump to kdump ASAP vs collecting more info in the
panicked kernel log).
[0] https://lore.kernel.org/lkml/20220126052246.GC2086@MiWiFi-R3L-srv/
[1] https://lore.kernel.org/lkml/YfPxvzSzDLjO5ldp@alley/
>[...]
>> + * Based on the level configured (smaller than 4), we clear the
>> + * proper bits in "panic_notifiers_bits". Notice that this bitfield
>> + * is initialized with all notifiers set.
>> + */
>> +switch (panic_notifiers_level) {
>> +case 3:
>> +clear_bit(PN_PRE_REBOOT_BIT, _notifiers_bits);
>> +break;
>> +case 2:
>> +clear_bit(PN_PRE_REBOOT_BIT, _notifiers_bits);
>> +
>> +if (!kmsg_has_dumpers())
>> +clear_bit(PN_INFO_BIT, _notifiers_bits);
>> +break;
>> +case 1:
>> +clear_bit(PN_PRE_REBOOT_BIT, _notifiers_bits);
>> +clear_bit(PN_INFO_BIT, _notifiers_bits);
>> +break;
>> +case 0:
>> +clear_bit(PN_PRE_REBOOT_BIT, _notifiers_bits);
>> +clear_bit(PN_INFO_BIT, _notifiers_bits);
>> +clear_bit(PN_HYPERVISOR_BIT, _notifiers_bits);
>> +break;
>> +}
>
> I think the above switch statement could be done as follows:
>
> if (panic_notifiers_level <= 3)
> clear_bit(PN_PRE_REBOOT_BIT, _notifiers_bits);
> if (panic_notifiers_level <= 2)
> if (!kmsg_has_dumpers())
> clear_bit(PN_INFO_BIT, _notifiers_bits);
> if (panic_notifiers_level <=1)
> clear_bit(PN_INFO_BIT, _notifiers_bits);
> if (panic_notifiers_level == 0)
> clear_bit(PN_HYPERVISOR_BIT, _notifiers_bits);
>
> That's about half the lines of code. It's somewhat a matter of style,
> so treat this as just a suggestion to
Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list
On 29/04/2022 14:30, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli Sent: Wednesday, April 27,
> 2022 3:49 PM
>> [...]
>>
>> @@ -2843,7 +2843,7 @@ static void __exit vmbus_exit(void)
>> if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
>> kmsg_dump_unregister(_kmsg_dumper);
>> unregister_die_notifier(_die_report_block);
>> -atomic_notifier_chain_unregister(_notifier_list,
>> +atomic_notifier_chain_unregister(_hypervisor_list,
>> _panic_report_block);
>> }
>>
>
> Using the hypervisor_list here produces a bit of a mismatch. In many cases
> this notifier will do nothing, and will defer to the kmsg_dump() mechanism
> to notify the hypervisor about the panic. Running the kmsg_dump()
> mechanism is linked to the info_list, so I'm thinking the Hyper-V panic report
> notifier should be on the info_list as well. That way the reporting behavior
> is triggered at the same point in the panic path regardless of which
> reporting mechanism is used.
>
Hi Michael, thanks for your feedback! I agree that your idea could work,
but...there is one downside: imagine the kmsg_dump() approach is not set
in some Hyper-V guest, then we would rely in the regular notification
mechanism [hv_die_panic_notify_crash()], right?
But...you want then to run this notifier in the informational list,
which...won't execute *by default* before kdump if no kmsg_dump() is
set. So, this logic is convoluted when you mix it with the default level
concept + kdump.
May I suggest something? If possible, take a run with this patch set +
DEBUG_NOTIFIER=y, in *both* cases (with and without the kmsg_dump()
set). I did that and they run almost at the same time...I've checked the
notifiers called, it's like almost nothing runs in-between.
I feel the panic notification mechanism does really fit with a
hypervisor list, it's a good match with the nature of the list, which
aims at informing the panic notification to the hypervisor/FW.
Of course we can modify it if you prefer...but please take into account
the kdump case and how it complicates the logic.
Let me know your considerations, in case you can experiment with the
patch set as-is.
Cheers,
Guilherme
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 21/30] panic: Introduce the panic pre-reboot notifier list
On Wed, Apr 27, 2022 at 3:55 PM Guilherme G. Piccoli wrote: > > This patch renames the panic_notifier_list to panic_pre_reboot_list; > the idea is that a subsequent patch will refactor the panic path > in order to better split the notifiers, running some of them very > early, some of them not so early [but still before kmsg_dump()] and > finally, the rest should execute late, after kdump. The latter ones > are now in the panic pre-reboot list - the name comes from the idea > that these notifiers execute before panic() attempts rebooting the > machine (if that option is set). > > We also took the opportunity to clean-up useless header inclusions, > improve some notifier block declarations (e.g. in ibmasm/heartbeat.c) > and more important, change some priorities - we hereby set 2 notifiers > to run late in the list [iss_panic_event() and the IPMI panic_event()] > due to the risks they offer (may not return, for example). > Proper documentation is going to be provided in a subsequent patch, > that effectively refactors the panic path. [...] > arch/xtensa/platforms/iss/setup.c | 4 ++--For xtensa: For xtensa: Acked-by: Max Filippov -- Thanks. -- Max ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 17/30] tracing: Improve panic/die notifiers
On 29/04/2022 10:56, Steven Rostedt wrote: > [...] > No. The fallthrough keyword is only needed when there's code between case > labels. As it is very common to list multiple cases for the same code path. > That is: > > case DIE_OOPS: > case PANIC_NOTIFIER: > do_dump = 1; > break; > > Does not need a fall through label, as there's no code between the DIE_OOPS > and the PANIC_NOTIFIER. But if you had: > > case DIE_OOPS: > x = true; > case PANIC_NOTIFIER: > do_dump = 1; > break; > > Then you do. > > -- Steve Thanks a bunch for the clarification, changed that for V2 =) ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
