| Message ID | 20220427224924.592546-6-gpiccoli@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | The panic notifiers refactor | expand |
On Wed 2022-04-27 19:48:59, Guilherme G. Piccoli wrote: > The pvpanic driver relies on panic notifiers to execute a callback > on panic event. Such function is executed in atomic context - the > panic function disables local IRQs, preemption and all other CPUs > that aren't running the panic code. > > With that said, it's dangerous to use regular spinlocks in such path, > as introduced by commit b3c0f8774668 ("misc/pvpanic: probe multiple instances"). > This patch fixes that by replacing regular spinlocks with the trylock > safer approach. It seems that the lock is used just to manipulating a list. A super safe solution would be to use the rcu API: rcu_add_rcu() and list_del_rcu() under rcu_read_lock(). The spin lock will not be needed and the list will always be valid. The advantage would be that it will always call members that were successfully added earlier. That said, I am not familiar with pvpanic and am not sure if it is worth it. > It also fixes an old comment (about a long gone framebuffer code) and > the notifier priority - we should execute hypervisor notifiers early, > deferring this way the panic action to the hypervisor, as expected by > the users that are setting up pvpanic. This should be done in a separate patch. It changes the behavior. Also there might be a discussion whether it really should be the maximal priority. Best Regards, Petr
On 10/05/2022 09:14, Petr Mladek wrote: > [...] >> With that said, it's dangerous to use regular spinlocks in such path, >> as introduced by commit b3c0f8774668 ("misc/pvpanic: probe multiple instances"). >> This patch fixes that by replacing regular spinlocks with the trylock >> safer approach. > > It seems that the lock is used just to manipulating a list. A super > safe solution would be to use the rcu API: rcu_add_rcu() and > list_del_rcu() under rcu_read_lock(). The spin lock will not be > needed and the list will always be valid. > > The advantage would be that it will always call members that > were successfully added earlier. That said, I am not familiar > with pvpanic and am not sure if it is worth it. > >> It also fixes an old comment (about a long gone framebuffer code) and >> the notifier priority - we should execute hypervisor notifiers early, >> deferring this way the panic action to the hypervisor, as expected by >> the users that are setting up pvpanic. > > This should be done in a separate patch. It changes the behavior. > Also there might be a discussion whether it really should be > the maximal priority. > > Best Regards, > Petr Thanks for the review Petr. Patch was already merged - my goal was to be concise, i.e., a patch per driver / module, so the patch kinda fixes whatever I think is wrong with the driver with regards panic handling. Do you think it worth to remove this patch from Greg's branch just to split it in 2? Personally I think it's not worth, but opinions are welcome. About the RCU part, this one really could be a new patch, a good improvement patch - it makes sense to me, we can think about that after the fixes I guess. Cheers, Guilherme
On Tue 2022-05-10 10:00:58, Guilherme G. Piccoli wrote: > On 10/05/2022 09:14, Petr Mladek wrote: > > [...] > >> With that said, it's dangerous to use regular spinlocks in such path, > >> as introduced by commit b3c0f8774668 ("misc/pvpanic: probe multiple instances"). > >> This patch fixes that by replacing regular spinlocks with the trylock > >> safer approach. > > > > It seems that the lock is used just to manipulating a list. A super > > safe solution would be to use the rcu API: rcu_add_rcu() and > > list_del_rcu() under rcu_read_lock(). The spin lock will not be > > needed and the list will always be valid. > > > > The advantage would be that it will always call members that > > were successfully added earlier. That said, I am not familiar > > with pvpanic and am not sure if it is worth it. > > > >> It also fixes an old comment (about a long gone framebuffer code) and > >> the notifier priority - we should execute hypervisor notifiers early, > >> deferring this way the panic action to the hypervisor, as expected by > >> the users that are setting up pvpanic. > > > > This should be done in a separate patch. It changes the behavior. > > Also there might be a discussion whether it really should be > > the maximal priority. > > > > Best Regards, > > Petr > > Thanks for the review Petr. Patch was already merged - my goal was to be > concise, i.e., a patch per driver / module, so the patch kinda fixes > whatever I think is wrong with the driver with regards panic handling. > > Do you think it worth to remove this patch from Greg's branch just to > split it in 2? Personally I think it's not worth, but opinions are welcome. No problem. It is not worth the effort. > About the RCU part, this one really could be a new patch, a good > improvement patch - it makes sense to me, we can think about that after > the fixes I guess. Yup. Best Regards, Petr
On 17/05/2022 07:58, Petr Mladek wrote: > [...] >> Thanks for the review Petr. Patch was already merged - my goal was to be >> concise, i.e., a patch per driver / module, so the patch kinda fixes >> whatever I think is wrong with the driver with regards panic handling. >> >> Do you think it worth to remove this patch from Greg's branch just to >> split it in 2? Personally I think it's not worth, but opinions are welcome. > > No problem. It is not worth the effort. > OK, perfect! Cheers, Guilherme
diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c index 4b8f1c7d726d..049a12006348 100644 --- a/drivers/misc/pvpanic/pvpanic.c +++ b/drivers/misc/pvpanic/pvpanic.c @@ -34,7 +34,9 @@ pvpanic_send_event(unsigned int event) { struct pvpanic_instance *pi_cur; - spin_lock(&pvpanic_lock); + if (!spin_trylock(&pvpanic_lock)) + return; + list_for_each_entry(pi_cur, &pvpanic_list, list) { if (event & pi_cur->capability & pi_cur->events) iowrite8(event, pi_cur->base); @@ -55,9 +57,13 @@ pvpanic_panic_notify(struct notifier_block *nb, unsigned long code, void *unused return NOTIFY_DONE; } +/* + * Call our notifier very early on panic, deferring the + * action taken to the hypervisor. + */ static struct notifier_block pvpanic_panic_nb = { .notifier_call = pvpanic_panic_notify, - .priority = 1, /* let this called before broken drm_fb_helper() */ + .priority = INT_MAX, }; static void pvpanic_remove(void *param)
The pvpanic driver relies on panic notifiers to execute a callback on panic event. Such function is executed in atomic context - the panic function disables local IRQs, preemption and all other CPUs that aren't running the panic code. With that said, it's dangerous to use regular spinlocks in such path, as introduced by commit b3c0f8774668 ("misc/pvpanic: probe multiple instances"). This patch fixes that by replacing regular spinlocks with the trylock safer approach. It also fixes an old comment (about a long gone framebuffer code) and the notifier priority - we should execute hypervisor notifiers early, deferring this way the panic action to the hypervisor, as expected by the users that are setting up pvpanic. Fixes: b3c0f8774668 ("misc/pvpanic: probe multiple instances") Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Cc: Mihai Carabas <mihai.carabas@oracle.com> Cc: Shile Zhang <shile.zhang@linux.alibaba.com> Cc: Wang ShaoBo <bobo.shaobowang@huawei.com> Cc: zhenwei pi <pizhenwei@bytedance.com> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- drivers/misc/pvpanic/pvpanic.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)