lkml.org 
[lkml]   [2019]   [Jan]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/1] epoll: remove wrong assert that ep_poll_callback is always called with irqs off
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

\
 
 \ /
  Last update: 2019-01-08 13:42    [W:0.116 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site