* [PATCH 1/1] epoll: remove wrong assert that ep_poll_callback is always called with irqs off
@ 2019-01-08 10:01 Roman Penyaev
2019-01-08 12:42 ` Roman Penyaev
0 siblings, 1 reply; 4+ messages in thread
From: Roman Penyaev @ 2019-01-08 10:01 UTC (permalink / raw)
Cc: Roman Penyaev, Davidlohr Bueso, Jason Baron, Al Viro,
Andrew Morton, Linus Torvalds, linux-fsdevel, linux-kernel
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.
Signed-off-by: Roman Penyaev <rpenyaev@suse.de>
Reported-by: syzbot+aea82bf9ee6ffd9a79d9@syzkaller.appspotmail.com
Cc: Davidlohr Bueso <dbueso@suse.de>
Cc: Jason Baron <jbaron@akamai.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
fs/eventpoll.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index f307c8679027..f5f88250cdf2 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1217,12 +1217,6 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
__poll_t pollflags = key_to_poll(key);
int ewake = 0;
- /*
- * Called by irq context or interrupts are disabled by the wake_up_*poll
- * callers.
- */
- lockdep_assert_irqs_disabled();
-
read_lock(&ep->lock);
ep_set_busy_poll_napi_id(epi);
--
2.19.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] epoll: remove wrong assert that ep_poll_callback is always called with irqs off
2019-01-08 10:01 [PATCH 1/1] epoll: remove wrong assert that ep_poll_callback is always called with irqs off Roman Penyaev
@ 2019-01-08 12:42 ` Roman Penyaev
2019-01-08 15:16 ` Davidlohr Bueso
0 siblings, 1 reply; 4+ messages in thread
From: Roman Penyaev @ 2019-01-08 12:42 UTC (permalink / raw)
To: Davidlohr Bueso, Jason Baron, Al Viro, Andrew Morton,
Linus Torvalds, linux-fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] epoll: remove wrong assert that ep_poll_callback is always called with irqs off
2019-01-08 12:42 ` Roman Penyaev
@ 2019-01-08 15:16 ` Davidlohr Bueso
2019-01-08 16:07 ` Roman Penyaev
0 siblings, 1 reply; 4+ messages in thread
From: Davidlohr Bueso @ 2019-01-08 15:16 UTC (permalink / raw)
To: Roman Penyaev
Cc: Jason Baron, Al Viro, Andrew Morton, Linus Torvalds,
linux-fsdevel, linux-kernel
On 2019-01-08 04:42, Roman Penyaev wrote:
> 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?
Note that such optimizations are also relevant for baremetal, ie: x86
PUSHF + POPF can be pretty expensive because of insn dependencies.
>
> 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().
Yeah so the reason why I had done the other epoll lock irq optimizations
was because they were painfully obvious. ep_poll_callback(), however is
a different beast, as you've encountered. I vote for not shooting
ourselves in the foot and just dropping this patch -- most large
performance benefits will come from microbenches anyway.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] epoll: remove wrong assert that ep_poll_callback is always called with irqs off
2019-01-08 15:16 ` Davidlohr Bueso
@ 2019-01-08 16:07 ` Roman Penyaev
0 siblings, 0 replies; 4+ messages in thread
From: Roman Penyaev @ 2019-01-08 16:07 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Jason Baron, Al Viro, Andrew Morton, Linus Torvalds,
linux-fsdevel, linux-kernel
On 2019-01-08 16:16, Davidlohr Bueso wrote:
> On 2019-01-08 04:42, Roman Penyaev wrote:
>> 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?
>
> Note that such optimizations are also relevant for baremetal, ie: x86
> PUSHF + POPF can be pretty expensive because of insn dependencies.
>
>>
>> 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().
>
> Yeah so the reason why I had done the other epoll lock irq
> optimizations was because they were painfully obvious.
> ep_poll_callback(), however is a different beast, as you've
> encountered. I vote for not shooting ourselves in the foot and just
> dropping this patch -- most large performance benefits will come from
> microbenches anyway.
Then I will send another patch which changes read_lock() on
read_lock_irqsave(),
since simple revert of the original patch won't work.
Thanks.
--
Roman
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-08 16:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-08 10:01 [PATCH 1/1] epoll: remove wrong assert that ep_poll_callback is always called with irqs off Roman Penyaev
2019-01-08 12:42 ` Roman Penyaev
2019-01-08 15:16 ` Davidlohr Bueso
2019-01-08 16:07 ` Roman Penyaev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).