Re: [PATCH RESEND] fs/epoll: fix the edge-triggered mode for nested epoll

From: Heiher
Date: Wed Sep 11 2019 - 04:20:18 EST

Next message: Jean-Jacques Hiblot: "Re: [RESEND,v2 2/2] leds: tlc591xx: Use the OF version of the LED registration function"
Previous message: Jason Gunthorpe: "Re: [RFC PATCH v2 16/19] RDMA/uverbs: Add back pointer to system file object"
Next in thread: Jason Baron: "Re: [PATCH RESEND] fs/epoll: fix the edge-triggered mode for nested epoll"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On Fri, Sep 6, 2019 at 1:48 AM Jason Baron <jbaron@xxxxxxxxxx> wrote:
>
>
>
> On 9/5/19 1:27 PM, Roman Penyaev wrote:
> > On 2019-09-05 11:56, Heiher wrote:
> >> Hi,
> >>
> >> On Thu, Sep 5, 2019 at 10:53 AM Heiher <r@xxxxxx> wrote:
> >>>
> >>> Hi,
> >>>
> >>> I created an epoll wakeup test project, listed some possible cases,
> >>> and any other corner cases needs to be added?
> >>>
> >>> https://github.com/heiher/epoll-wakeup/blob/master/README.md
> >>>
> >>> On Wed, Sep 4, 2019 at 10:02 PM Heiher <r@xxxxxx> wrote:
> >>> >
> >>> > Hi,
> >>> >
> >>> > On Wed, Sep 4, 2019 at 8:02 PM Jason Baron <jbaron@xxxxxxxxxx> wrote:
> >>> > >
> >>> > >
> >>> > >
> >>> > > On 9/4/19 5:57 AM, Roman Penyaev wrote:
> >>> > > > On 2019-09-03 23:08, Jason Baron wrote:
> >>> > > >> On 9/2/19 11:36 AM, Roman Penyaev wrote:
> >>> > > >>> Hi,
> >>> > > >>>
> >>> > > >>> This is indeed a bug. (quick side note: could you please
> >>> remove efd[1]
> >>> > > >>> from your test, because it is not related to the reproduction
> >>> of a
> >>> > > >>> current bug).
> >>> > > >>>
> >>> > > >>> Your patch lacks a good description, what exactly you've
> >>> fixed. Let
> >>> > > >>> me speak out loud and please correct me if I'm wrong, my
> >>> understanding
> >>> > > >>> of epoll internals has become a bit rusty: when epoll fds are
> >>> nested
> >>> > > >>> an attempt to harvest events (ep_scan_ready_list() call)
> >>> produces a
> >>> > > >>> second (repeated) event from an internal fd up to an external
> >>> fd:
> >>> > > >>>
> >>> > > >>> epoll_wait(efd[0], ...):
> >>> > > >>> ep_send_events():
> >>> > > >>> ep_scan_ready_list(depth=0):
> >>> > > >>> ep_send_events_proc():
> >>> > > >>> ep_item_poll():
> >>> > > >>> ep_scan_ready_list(depth=1):
> >>> > > >>> ep_poll_safewake():
> >>> > > >>> ep_poll_callback()
> >>> > > >>> list_add_tail(&epi, &epi->rdllist);
> >>> > > >>> ^^^^^^
> >>> > > >>> repeated event
> >>> > > >>>
> >>> > > >>>
> >>> > > >>> In your patch you forbid wakeup for the cases, where depth !=
> >>> 0, i.e.
> >>> > > >>> for all nested cases. That seems clear. But what if we can
> >>> go further
> >>> > > >>> and remove the whole chunk, which seems excessive:
> >>> > > >>>
> >>> > > >>> @@ -885,26 +886,11 @@ static __poll_t ep_scan_ready_list(struct
> >>> > > >>> eventpoll *ep,
> >>> > > >>>
> >>> > > >>> -
> >>> > > >>> - if (!list_empty(&ep->rdllist)) {
> >>> > > >>> - /*
> >>> > > >>> - * Wake up (if active) both the eventpoll
> >>> wait list and
> >>> > > >>> - * the ->poll() wait list (delayed after we
> >>> release the
> >>> > > >>> lock).
> >>> > > >>> - */
> >>> > > >>> - if (waitqueue_active(&ep->wq))
> >>> > > >>> - wake_up(&ep->wq);
> >>> > > >>> - if (waitqueue_active(&ep->poll_wait))
> >>> > > >>> - pwake++;
> >>> > > >>> - }
> >>> > > >>> write_unlock_irq(&ep->lock);
> >>> > > >>>
> >>> > > >>> if (!ep_locked)
> >>> > > >>> mutex_unlock(&ep->mtx);
> >>> > > >>>
> >>> > > >>> - /* We have to call this outside the lock */
> >>> > > >>> - if (pwake)
> >>> > > >>> - ep_poll_safewake(&ep->poll_wait);
> >>> > > >>>
> >>> > > >>>
> >>> > > >>> I reason like that: by the time we've reached the point of
> >>> scanning events
> >>> > > >>> for readiness all wakeups from ep_poll_callback have been
> >>> already fired and
> >>> > > >>> new events have been already accounted in ready list
> >>> (ep_poll_callback()
> >>> > > >>> calls
> >>> > > >>> the same ep_poll_safewake()). Here, frankly, I'm not 100%
> >>> sure and probably
> >>> > > >>> missing some corner cases.
> >>> > > >>>
> >>> > > >>> Thoughts?
> >>> > > >>
> >>> > > >> So the: 'wake_up(&ep->wq);' part, I think is about waking up
> >>> other
> >>> > > >> threads that may be in waiting in epoll_wait(). For example,
> >>> there may
> >>> > > >> be multiple threads doing epoll_wait() on the same epoll fd,
> >>> and the
> >>> > > >> logic above seems to say thread 1 may have processed say N
> >>> events and
> >>> > > >> now its going to to go off to work those, so let's wake up
> >>> thread 2 now
> >>> > > >> to handle the next chunk.
> >>> > > >
> >>> > > > Not quite. Thread which calls ep_scan_ready_list() processes
> >>> all the
> >>> > > > events, and while processing those, removes them one by one
> >>> from the
> >>> > > > ready list. But if event mask is !0 and event belongs to
> >>> > > > Level Triggered Mode descriptor (let's say default mode) it
> >>> tails event
> >>> > > > again back to the list (because we are in level mode, so event
> >>> should
> >>> > > > be there). So at the end of this traversing loop ready list is
> >>> likely
> >>> > > > not empty, and if so, wake up again is called for nested epoll
> >>> fds.
> >>> > > > But, those nested epoll fds should get already all the
> >>> notifications
> >>> > > > from the main event callback ep_poll_callback(), regardless any
> >>> thread
> >>> > > > which traverses events.
> >>> > > >
> >>> > > > I suppose this logic exists for decades, when Davide (the
> >>> author) was
> >>> > > > reshuffling the code here and there.
> >>> > > >
> >>> > > > But I do not feel confidence to state that this extra wakeup is
> >>> bogus,
> >>> > > > I just have a gut feeling that it looks excessive.
> >>> > >
> >>> > > Note that I was talking about the wakeup done on ep->wq not
> >>> ep->poll_wait.
> >>> > > The path that I'm concerned about is let's say that there are N
> >>> events
> >>> > > queued on the ready list. A thread that was woken up in
> >>> epoll_wait may
> >>> > > decide to only process say N/2 of then. Then it will call wakeup
> >>> on ep->wq
> >>> > > and this will wakeup another thread to process the remaining N/2.
> >>> Without
> >>> > > the wakeup, the original thread isn't going to process the events
> >>> until
> >>> > > it finishes with the original N/2 and gets back to epoll_wait().
> >>> So I'm not
> >>> > > sure how important that path is but I wanted to at least note the
> >>> change
> >>> > > here would impact that behavior.
> >>> > >
> >>> > > Thanks,
> >>> > >
> >>> > > -Jason
> >>> > >
> >>> > >
> >>> > > >
> >>> > > >> So I think removing all that even for the
> >>> > > >> depth 0 case is going to change some behavior here. So
> >>> perhaps, it
> >>> > > >> should be removed for all depths except for 0? And if so, it
> >>> may be
> >>> > > >> better to make 2 patches here to separate these changes.
> >>> > > >>
> >>> > > >> For the nested wakeups, I agree that the extra wakeups seem
> >>> unnecessary
> >>> > > >> and it may make sense to remove them for all depths. I don't
> >>> think the
> >>> > > >> nested epoll semantics are particularly well spelled out, and
> >>> afaict,
> >>> > > >> nested epoll() has behaved this way for quite some time. And
> >>> the current
> >>> > > >> behavior is not bad in the way that a missing wakeup or false
> >>> negative
> >>> > > >> would be.
> >>> > > >
> >>> > > > That's 100% true! For edge mode extra wake up is not a bug, not
> >>> optimal
> >>> > > > for userspace - yes, but that can't lead to any lost wakeups.
> >>> > > >
> >>> > > > --
> >>> > > > Roman
> >>> > > >
> >>> >
> >>> > I tried to remove the whole chunk of code that Roman said, and it
> >>> > seems that there
> >>> > are no obvious problems with the two test programs below:
> >>
> >> I recall this message, the test case 9/25/26 of epoll-wakeup (on
> >> github) are failed while
> >> the whole chunk are removed.
> >>
> >> Apply the original patch, all tests passed.
> >
> >
> > These are failing on my bare 5.2.0-rc2
> >
> > TEST bin/epoll31 FAIL
> > TEST bin/epoll46 FAIL
> > TEST bin/epoll50 FAIL
> > TEST bin/epoll32 FAIL
> > TEST bin/epoll19 FAIL
> > TEST bin/epoll27 FAIL
> > TEST bin/epoll42 FAIL
> > TEST bin/epoll34 FAIL
> > TEST bin/epoll48 FAIL
> > TEST bin/epoll40 FAIL
> > TEST bin/epoll20 FAIL
> > TEST bin/epoll28 FAIL
> > TEST bin/epoll38 FAIL
> > TEST bin/epoll52 FAIL
> > TEST bin/epoll24 FAIL
> > TEST bin/epoll23 FAIL
> >
> >
> > These are failing if your patch is applied:
> > (my 5.2.0-rc2 is old? broken?)
> >
> > TEST bin/epoll46 FAIL
> > TEST bin/epoll42 FAIL
> > TEST bin/epoll34 FAIL
> > TEST bin/epoll48 FAIL
> > TEST bin/epoll40 FAIL
> > TEST bin/epoll44 FAIL
> > TEST bin/epoll38 FAIL
> >
> > These are failing if "ep_poll_safewake(&ep->poll_wait)" is not called,
> > but wakeup(&ep->wq); is still invoked:
> >
> > TEST bin/epoll46 FAIL
> > TEST bin/epoll42 FAIL
> > TEST bin/epoll34 FAIL
> > TEST bin/epoll40 FAIL
> > TEST bin/epoll44 FAIL
> > TEST bin/epoll38 FAIL
> >
> > So at least 48 has been "fixed".
> >
> > These are failing if the whole chunk is removed, like your
> > said 9,25,26 are among which do not pass:
> >
> > TEST bin/epoll26 FAIL
> > TEST bin/epoll42 FAIL
> > TEST bin/epoll34 FAIL
> > TEST bin/epoll9 FAIL
> > TEST bin/epoll48 FAIL
> > TEST bin/epoll40 FAIL
> > TEST bin/epoll25 FAIL
> > TEST bin/epoll44 FAIL
> > TEST bin/epoll38 FAIL
> >
> > This can be a good test suite, probably can be added to kselftests?

Thank you, I have updated epoll-tests to fix these issues. I think this is good
news if we can added to kselftests. ;)

> >
> > --
> > Roman
> >
>
>
> Indeed, I just tried the same test suite and I am seeing similar
> failures - it looks like its a bit timing dependent. It looks like all
> the failures are caused by a similar issue. For example, take epoll34:
>
> t0 t1
> (ew) | | (ew)
> e0 |
> (lt) \ /
> |
> e1
> | (et)
> s0
>
>
> The test is trying to assert that an epoll_wait() on e1 and and
> epoll_wait() on e0 both return 1 event for EPOLLIN. However, the
> epoll_wait on e1 is done in a thread and it can happen before or after
> the epoll_wait() is called against e0. If the epoll_wait() on e1 happens
> first then because its attached as 'et', it consumes the event. So that
> there is no longer an event reported at e0. I think that is reasonable
> semantics here. However if the wait on e0 happens after the wait on e1
> then the test will pass as both waits will see the event. Thus, a patch
> like this will 'fix' this testcase:
>
> --- a/src/epoll34.c
> +++ b/src/epoll34.c
> @@ -59,15 +59,15 @@ int main(int argc, char *argv[])
> if (epoll_ctl(efd[0], EPOLL_CTL_ADD, efd[1], &e) < 0)
> goto out;
>
> - if (pthread_create(&tw, NULL, thread_handler, NULL) < 0)
> - goto out;
> -
> if (pthread_create(&te, NULL, emit_handler, NULL) < 0)
> goto out;
>
> if (epoll_wait(efd[0], &e, 1, 500) == 1)
> count++;
>
> + if (pthread_create(&tw, NULL, thread_handler, NULL) < 0)
> + goto out;
> +
> if (pthread_join(tw, NULL) < 0)
> goto out;
>
>
> I found all the other failures to be of similar origin. I suspect Heiher
> didn't see failures due to the thread timings here.

Thank you. I also found a multi-threaded concurrent accumulation problem,
and that has been changed to atomic operations. I think we should allow two
different behaviors to be passed because they are all correctly.

thread 2:
if (epoll_wait(efd[1], &e, 1, 500) == 1)
__sync_fetch_and_or(&count, 1);

thread1:
if (epoll_wait(efd[0], &e, 1, 500) == 1)
__sync_fetch_and_or(&count, 2);

check:
if ((count != 1) && (count != 3))
goto out;

>
> I also found that all the testcases pass if we leave the wakeup(&ep->wq)
> call for the depth 0 case (and remove the pwake part).

So, We need to keep the wakeup(&ep-wq) for all depth, and only
wakeup(&ep->poll_wait)
for depth 0 and/or ep->rdlist from empty to be not empty?

>
> Thanks,
>
> -Jason
>
>
>
>

--
Best regards!
Hev
https://hev.cc

Next message: Jean-Jacques Hiblot: "Re: [RESEND,v2 2/2] leds: tlc591xx: Use the OF version of the LED registration function"
Previous message: Jason Gunthorpe: "Re: [RFC PATCH v2 16/19] RDMA/uverbs: Add back pointer to system file object"
Next in thread: Jason Baron: "Re: [PATCH RESEND] fs/epoll: fix the edge-triggered mode for nested epoll"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]