On 2019-01-08 11:01, Roman Penyaev wrote:
That was wrong assumption that all drivers disable irqs before waking
up
a wait queue. Even assert line is removed the whole logic stays
correct:
epoll always locks rwlock with irqs disabled and by itself does not
call
from interrupts, thus it is up to driver how to call wake_up_locked(),
because if driver does not handle any interrupts (like fuse in the the
report) of course it is safe on its side to take a simple spin_lock.
This is wrong and can lead to dead lock: we always call read_lock(),
caller
can call us with irqs enabled. Another driver, which also calls
ep_poll_callback(), can be called from interrupt context (irqs disabled)
thus it can interrupt the one who does not disable irqs. Even we take
a read_lock() (which should be fine to interrupt), write_lock(), which
comes in the middle, can cause a dead lock:
#CPU0 #CPU1
task: task: irq:
spin_lock(&wq1->lock);
ep_poll_callback():
read_lock(&ep->lock)
....
write_lock_irq(&ep->lock) ....
#waits reads .... >>>>>>>>>>>>>> IRQ CPU1
spin_lock_irqsave(&wq2->lock)
ep_poll_callback():
read_lock(&ep->lock);
# to avoid write
starve should
# wait writer to
finish, thus
# dead lock
What we can do:
a) disable irqs if we are not in interrupt.
b) revert the patch completely.
David, is it really crucial in terms of performance to avoid double
local_irq_save() on Xen on this ep_poll_callback() hot path?
For example why not to do the following:
if (!in_interrupt())
local_irq_save(flags);
read_lock(ep->lock);
with huge comment explaining performance number.
Or just give up and simply revert the original patch completely
and always call read_lock_irqsave().
--
Roman
