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

[Jason Baron]

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. 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. It woulbe be good to better understand the use-case more here
and to try and spell out the nested semantics more clearly?

Thanks,

-Jason


> 
> PS. You call list_empty(&ep->rdllist) without ep->lock taken, that is
> fine,
> ÂÂÂÂ but you should be _careful_, so list_empty_careful(&ep->rdllist) call
> ÂÂÂÂ instead.
> 
> -- 
> Roman
> 
> 
> 
> On 2019-09-02 07:20, hev wrote:
>> From: Heiher <r@xxxxxx>
>>
>> The structure of event pools:
>> Âefd[1]: { efd[2] (EPOLLIN) }ÂÂÂÂÂÂÂ efd[0]: { efd[2] (EPOLLIN |
>> EPOLLET) }
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ +-----------------+-----------------+
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ v
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ efd[2]: { sfd[0] (EPOLLIN) }
>>
>> When sfd[0] to be readable:
>> Â* the epoll_wait(efd[0], ..., 0) should return efd[2]'s events on
>> first call,
>> ÂÂ and returns 0 on next calls, because efd[2] is added in
>> edge-triggered mode.
>> Â* the epoll_wait(efd[1], ..., 0) should returns efd[2]'s events on
>> every calls
>> ÂÂ until efd[2] is not readable (epoll_wait(efd[2], ...) => 0),
>> because efd[1]
>> ÂÂ is added in level-triggered mode.
>> Â* the epoll_wait(efd[2], ..., 0) should returns sfd[0]'s events on
>> every calls
>> ÂÂ until sfd[0] is not readable (read(sfd[0], ...) => EAGAIN), because
>> sfd[0]
>> ÂÂ is added in level-triggered mode.
>>
>> Test code:
>> Â#include <stdio.h>
>> Â#include <unistd.h>
>> Â#include <sys/epoll.h>
>> Â#include <sys/socket.h>
>>
>> Âint main(int argc, char *argv[])
>> Â{
>> ÂÂÂÂ int sfd[2];
>> ÂÂÂÂ int efd[3];
>> ÂÂÂÂ int nfds;
>> ÂÂÂÂ struct epoll_event e;
>>
>> ÂÂÂÂ if (socketpair(AF_UNIX, SOCK_STREAM, 0, sfd) < 0)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ efd[0] = epoll_create(1);
>> ÂÂÂÂ if (efd[0] < 0)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ efd[1] = epoll_create(1);
>> ÂÂÂÂ if (efd[1] < 0)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ efd[2] = epoll_create(1);
>> ÂÂÂÂ if (efd[2] < 0)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ e.events = EPOLLIN;
>> ÂÂÂÂ if (epoll_ctl(efd[2], EPOLL_CTL_ADD, sfd[0], &e) < 0)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ e.events = EPOLLIN;
>> ÂÂÂÂ if (epoll_ctl(efd[1], EPOLL_CTL_ADD, efd[2], &e) < 0)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ e.events = EPOLLIN | EPOLLET;
>> ÂÂÂÂ if (epoll_ctl(efd[0], EPOLL_CTL_ADD, efd[2], &e) < 0)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ if (write(sfd[1], "w", 1) != 1)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ nfds = epoll_wait(efd[0], &e, 1, 0);
>> ÂÂÂÂ if (nfds != 1)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ nfds = epoll_wait(efd[0], &e, 1, 0);
>> ÂÂÂÂ if (nfds != 0)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ nfds = epoll_wait(efd[1], &e, 1, 0);
>> ÂÂÂÂ if (nfds != 1)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ nfds = epoll_wait(efd[1], &e, 1, 0);
>> ÂÂÂÂ if (nfds != 1)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ nfds = epoll_wait(efd[2], &e, 1, 0);
>> ÂÂÂÂ if (nfds != 1)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ nfds = epoll_wait(efd[2], &e, 1, 0);
>> ÂÂÂÂ if (nfds != 1)
>> ÂÂÂÂÂÂÂÂ goto out;
>>
>> ÂÂÂÂ close(efd[2]);
>> ÂÂÂÂ close(efd[1]);
>> ÂÂÂÂ close(efd[0]);
>> ÂÂÂÂ close(sfd[0]);
>> ÂÂÂÂ close(sfd[1]);
>>
>> ÂÂÂÂ printf("PASS\n");
>> ÂÂÂÂ return 0;
>>
>> Âout:
>> ÂÂÂÂ printf("FAIL\n");
>> ÂÂÂÂ return -1;
>> Â}
>>
>> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Davide Libenzi <davidel@xxxxxxxxxxxxxxx>
>> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
>> Cc: Dominik Brodowski <linux@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Eric Wong <e@xxxxxxxxx>
>> Cc: Jason Baron <jbaron@xxxxxxxxxx>
>> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Roman Penyaev <rpenyaev@xxxxxxx>
>> Cc: Sridhar Samudrala <sridhar.samudrala@xxxxxxxxx>
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
>> Signed-off-by: hev <r@xxxxxx>
>> ---
>> Âfs/eventpoll.c | 6 +++++-
>> Â1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
>> index d7f1f5011fac..a44cb27c636c 100644
>> --- a/fs/eventpoll.c
>> +++ b/fs/eventpoll.c
>> @@ -672,6 +672,7 @@ static __poll_t ep_scan_ready_list(struct
>> eventpoll *ep,
>> Â{
>> ÂÂÂÂ __poll_t res;
>> ÂÂÂÂ int pwake = 0;
>> +ÂÂÂ int nwake = 0;
>> ÂÂÂÂ struct epitem *epi, *nepi;
>> ÂÂÂÂ LIST_HEAD(txlist);
>>
>> @@ -685,6 +686,9 @@ static __poll_t ep_scan_ready_list(struct
>> eventpoll *ep,
>> ÂÂÂÂ if (!ep_locked)
>> ÂÂÂÂÂÂÂÂ mutex_lock_nested(&ep->mtx, depth);
>>
>> +ÂÂÂ if (!depth || list_empty(&ep->rdllist))
>> +ÂÂÂÂÂÂÂ nwake = 1;
>> +
>> ÂÂÂÂ /*
>> ÂÂÂÂÂ * Steal the ready list, and re-init the original one to the
>> ÂÂÂÂÂ * empty list. Also, set ep->ovflist to NULL so that events
>> @@ -739,7 +743,7 @@ static __poll_t ep_scan_ready_list(struct
>> eventpoll *ep,
>> ÂÂÂÂ list_splice(&txlist, &ep->rdllist);
>> ÂÂÂÂ __pm_relax(ep->ws);
>>
>> -ÂÂÂ if (!list_empty(&ep->rdllist)) {
>> +ÂÂÂ if (nwake && !list_empty(&ep->rdllist)) {
>> ÂÂÂÂÂÂÂÂ /*
>> ÂÂÂÂÂÂÂÂÂ * Wake up (if active) both the eventpoll wait list and
>> ÂÂÂÂÂÂÂÂÂ * the ->poll() wait list (delayed after we release the lock).
>