lkml.org 
[lkml]   [2008]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: kerneloops.org: 2.6.28-rc regression in epoll (list corruption)
On Sun, 26 Oct 2008, Linus Torvalds wrote:

>
>
> On Sun, 26 Oct 2008, Arjan van de Ven wrote:
> >
> > This one is upcoming fast (and I just hit it as well)
> >
> > http://www.kerneloops.org/searchweek.php?search=ep_poll_callback
> >
> > seems epoll grew some list corruption....
>
> It sounds very much like f337b9c58332bdecde965b436e47ea4c94d30da0 ("epoll:
> drop unnecessary test") deleted a test that wasn't so unnecessary after
> all..
>
> That ep_poll_callback() code is:
>
> /* If this file is already in the ready list we exit soon */
> if (ep_is_linked(&epi->rdllink))
> goto is_linked;
>
> list_add_tail(&epi->rdllink, &ep->rdllist);
>
> and the unnecessary test that was removed looks _very_ much like that kind
> of code.

No, the test was in the re-insertion loop. This is the patch that fixes it:

---
fs/eventpoll.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

Index: linux-2.6.mod/fs/eventpoll.c
===================================================================
--- linux-2.6.mod.orig/fs/eventpoll.c 2008-10-17 15:51:09.000000000 -0700
+++ linux-2.6.mod/fs/eventpoll.c 2008-10-17 15:54:14.000000000 -0700
@@ -930,8 +930,15 @@
* inside the main ready-list here.
*/
for (nepi = ep->ovflist; (epi = nepi) != NULL;
- nepi = epi->next, epi->next = EP_UNACTIVE_PTR)
- list_add_tail(&epi->rdllink, &ep->rdllist);
+ nepi = epi->next, epi->next = EP_UNACTIVE_PTR) {
+ /*
+ * If the above loop quit with errors, the epoll item might still
+ * be linked to "txlist", and the list_splice() done below will
+ * take care of those cases.
+ */
+ if (!ep_is_linked(&epi->rdllink))
+ list_add_tail(&epi->rdllink, &ep->rdllist);
+ }
/*
* We need to set back ep->ovflist to EP_UNACTIVE_PTR, so that after
* releasing the lock, events will be queued in the normal way inside


> And if somebody knows how to reproduce this reliably, it would be really
> good to hear if doing a revert on that thing just fixed it. It should
> revert cleanly - it's the only change to fs/eventpoll.c since 2.6.27.

That's 100% due to the removed test that went in when you sucked up Andrew
bits after .27.
Patch has been confirmed by me and bug submitters.




> I'm somewhat inclined to revert it without even getting confirmation,
> since I wanted to do an early -rc2 today with all the brown-paper-bag
> fixes that have accumulated. But it would be good to get some
> confirmation.

No need to revert, since one of the removed tests is really un-needed.




> Btw, that whole logic in ep_send_events() sounds a bit scary. It says:
>
> We can loop without lock because this is a task private list.
>
> and it's true that "txlist" is a private list, but it still seems to
> depend on the fact that none of the "struct epitem"s on that list can be
> reached by any other means. And that whole thing depends on the magic
> behaviour of 'ep->ovflist', but there are a lot of code sequences that do
> *not* seem to test that at all.
>
> Maybe the rqace/bug has always been there (ie some sequence that works
> with an "epi->rdllink" without checking ovflist), but the "unnecessary"
> test protected us from seeing it in practice.

No. Bug was introduced by the removed test. Thomas contacted me telling
that such test was un-needed, and at first sight it looked OK to drop it
(since during the loop time the callback inserts into 'ep->ovflist').
But we didn't notice that a premature loop exit might leave items linked
to 'txlist', that the splice below takes care of re-insert them.
You can speed up things by sucking the patch directly, w/out waiting
Andrew's pull.




- Davide




\
 
 \ /
  Last update: 2008-10-26 20:39    [W:0.058 / U:0.400 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site