On 03/05/2022 15:03, Evan Green wrote: > [...] > gsmi_shutdown_reason() is a common function called in other scenarios > as well, like reboot and thermal trip, where it may still make sense > to wait to acquire a spinlock. Maybe we should add a parameter to > gsmi_shutdown_reason() so that you can get your change on panic, but > we don't convert other callbacks into try-fail scenarios causing us to > miss logs. >
Hi Evan, thanks for your feedback, much appreciated! What I've done in other cases like this was to have a helper checking the spinlock in the panic notifier - if we can acquire that, go ahead but if not, bail out. For a proper example of an implementation, check patch 13 of the series: https://lore.kernel.org/lkml/20220427224924.592546-14-gpicc...@igalia.com/ . Do you agree with that, or prefer really a parameter in gsmi_shutdown_reason() ? I'll follow your choice =) > Though thinking more about it, is this really a Good Change (TM)? The > spinlock itself already disables interrupts, meaning the only case > where this change makes a difference is if the panic happens from > within the function that grabbed the spinlock (in which case the > callback is also likely to panic), or in an NMI that panics within > that window. The downside of this change is that if one core was > politely working through an event with the lock held, and another core > panics, we now might lose the panic log, even though it probably would > have gone through fine assuming the other core has a chance to > continue. My feeling is that this is a good change, indeed - a lot of places are getting changed like this, in this series. Reasoning: the problem with your example is that, by default, secondary CPUs are disabled in the panic path, through an IPI mechanism. IPIs take precedence and interrupt the work in these CPUs, effectively interrupting the "polite work" with the lock held heh Then, such CPU is put to sleep and we finally reach the panic notifier hereby discussed, in the main CPU. If the other CPU was shut-off *with the lock held*, it's never finishing such work, so the lock is never to be released. Conclusion: the spinlock can't be acquired, hence we broke the machine (which is already broken, given it's panic) in the path of this notifier. This should be really rare, but..possible. So I think we should protect against this scenario. We can grab others' feedback if you prefer, and of course you have the rights to refuse this change in the gsmi code, but from my point-of-view, I don't see any advantage in just assume the risk, specially since the change is very very simple. Cheers, Guilherme
