Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH RESEND] fs/epoll: fix the edge-triggered mode for nested
 epoll
To:     Roman Penyaev <rpenyaev@suse.de>, hev <r@hev.cc>
Cc:     linux-fsdevel@vger.kernel.org, e@80x24.org,
        Al Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Davide Libenzi <davidel@xmailserver.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Dominik Brodowski <linux@dominikbrodowski.net>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Sridhar Samudrala <sridhar.samudrala@intel.com>,
        linux-kernel@vger.kernel.org
References: <20190902052034.16423-1-r@hev.cc>
 <0cdc9905efb9b77b159e09bee17d3ad4@suse.de>
From:   Jason Baron <jbaron@akamai.com>
Openpgp: preference=signencrypt
Autocrypt: addr=jbaron@akamai.com; prefer-encrypt=mutual; keydata=
 xsFNBFnyIJMBEADamFSO/WCelO/HZTSNbJ1YU9uoEUwmypV2TvyrTrXULcAlH1sXVHS3pNdR
 I/koZ1V7Ruew5HJC4K9Z5Fuw/RHYWcnQz2X+dSL6rX3BwRZEngjA4r/GDi0EqIdQeQQWCAgT
 VLWnIenNgmEDCoFQjFny5NMNL+i8SA6hPPRdNjxDowDhbFnkuVUBp1DBqPjHpXMzf3UYsZZx
 rxNY5YKFNLCpQb1cZNsR2KXZYDKUVALN3jvjPYReWkqRptOSQnvfErikwXRgCTasWtowZ4cu
 hJFSM5Asr/WN9Wy6oPYObI4yw+KiiWxiAQrfiQVe7fwznStaYxZ2gZmlSPG/Y2/PyoCWYbNZ
 mJ/7TyED5MTt22R7dqcmrvko0LIpctZqHBrWnLTBtFXZPSne49qGbjzzHywZ0OqZy9nqdUFA
 ZH+DALipwVFnErjEjFFRiwCWdBNpIgRrHd2bomlyB5ZPiavoHprgsV5ZJNal6fYvvgCik77u
 6QgE4MWfhf3i9A8Dtyf8EKQ62AXQt4DQ0BRwhcOW5qEXIcKj33YplyHX2rdOrD8J07graX2Q
 2VsRedNiRnOgcTx5Zl3KARHSHEozpHqh7SsthoP2yVo4A3G2DYOwirLcYSCwcrHe9pUEDhWF
 bxdyyESSm/ysAVjvENsdcreWJqafZTlfdOCE+S5fvC7BGgZu7QARAQABzR9KYXNvbiBCYXJv
 biA8amJhcm9uQGFrYW1haS5jb20+wsF+BBMBAgAoBQJZ8iCTAhsDBQkJZgGABgsJCAcDAgYV
 CAIJCgsEFgIDAQIeAQIXgAAKCRC4s7mct4u0M9E0EADBxyL30W9HnVs3x7umqUbl+uBqbBIS
 GIvRdMDIJXX+EEA6c82ElV2cCOS7dvE3ssG1jRR7g3omW7qEeLdy/iQiJ/qGNdcf0JWHYpmS
 ThZP3etrl5n7FwLm+51GPqD0046HUdoVshRs10qERDo+qnvMtTdXsfk8uoQ5lyTSvgX4s1H1
 ppN1BfkG10epsAtjOJJlBoV9e92vnVRIUTnDeTVXfK11+hT5hjBxxs7uS46wVbwPuPjMlbSa
 ifLnt7Jz590rtzkeGrUoM5SKRL4DVZYNoAVFp/ik1fe53Wr5GJZEgDC3SNGS/u+IEzEGCytj
 gejvv6KDs3KcTVSp9oJ4EIZRmX6amG3dksXa4W2GEQJfPfV5+/FR8IOg42pz9RpcET32AL1n
 GxWzY4FokZB0G6eJ4h53DNx39/zaGX1i0cH+EkyZpfgvFlBWkS58JRFrgY25qhPZiySRLe0R
 TkUcQdqdK77XDJN5zmUP5xJgF488dGKy58DcTmLoaBTwuCnX2OF+xFS4bCHJy93CluyudOKs
 e4CUCWaZ2SsrMRuAepypdnuYf3DjP4DpEwBeLznqih4hMv5/4E/jMy1ZMdT+Q8Qz/9pjEuVF
 Yz2AXF83Fqi45ILNlwRjCjdmG9oJRJ+Yusn3A8EbCtsi2g443dKBzhFcmdA28m6MN9RPNAVS
 ucz3Oc7BTQRZ8iCTARAA2uvxdOFjeuOIpayvoMDFJ0v94y4xYdYGdtiaqnrv01eOac8msBKy
 4WRNQ2vZeoilcrPxLf2eRAfsA4dx8Q8kOPvVqDc8UX6ttlHcnwxkH2X4XpJJliA6jx29kBOc
 oQOeL9R8c3CWL36dYbosZZwHwY5Jjs7R6TJHx1FlF9mOGIPxIx3B5SuJLsm+/WPZW1td7hS0
 Alt4Yp8XWW8a/X765g3OikdmvnJryTo1s7bojmwBCtu1TvT0NrX5AJId4fELlCTFSjr+J3Up
 MnmkTSyovPkj8KcvBU1JWVvMnkieqrhHOmf2qdNMm61LGNG8VZQBVDMRg2szB79p54DyD+qb
 gTi8yb0MFqNvXGRnU/TZmLlxblHA4YLMAuLlJ3Y8Qlw5fJ7F2U1Xh6Z6m6YCajtsIF1VkUhI
 G2dSAigYpe6wU71Faq1KHp9C9VsxlnSR1rc4JOdj9pMoppzkjCphyX3eV9eRcfm4TItTNTGJ
 7DAUQHYS3BVy1fwyuSDIJU/Jrg7WWCEzZkS4sNcBz0/GajYFM7Swybn/VTLtCiioThw4OQIw
 9Afb+3sB9WR86B7N7sSUTvUArknkNDFefTJJLMzEboRMJBWzpR5OAyLxCWwVSQtPp0IdiIC2
 KGF3QXccv/Q9UkI38mWvkilr3EWAOJnPgGCM/521axcyWqXsqNtIxpUAEQEAAcLBZQQYAQIA
 DwUCWfIgkwIbDAUJCWYBgAAKCRC4s7mct4u0M+AsD/47Q9Gi+HmLyqmaaLBzuI3mmU4vDn+f
 50A/U9GSVTU/sAN83i1knpv1lmfG2DgjLXslU+NUnzwFMLI3QsXD3Xx/hmdGQnZi9oNpTMVp
 tG5hE6EBPsT0BM6NGbghBsymc827LhfYICiahOR/iv2yv6nucKGBM51C3A15P8JgfJcngEnM
 fCKRuQKWbRDPC9dEK9EBglUYoNPVNL7AWJWKAbVQyCCsJzLBgh9jIfmZ9GClu8Sxi0vu/PpA
 DSDSJuc9wk+m5mczzzwd4Y6ly9+iyk/CLNtqjT4sRMMV0TCl8ichxlrdt9rqltk22HXRF7ng
 txomp7T/zRJAqhH/EXWI6CXJPp4wpMUjEUd1B2+s1xKypq//tChF+HfUU4zXUyEXY8nHl6lk
 hFjW/geTcf6+i6mKaxGY4oxuIjF1s2Ak4J3viSeYfTDBH/fgUzOGI5siBhHWvtVzhQKHfOxg
 i8t1q09MJY6je8l8DLEIWTHXXDGnk+ndPG3foBucukRqoTv6AOY49zjrt6r++sujjkE4ax8i
 ClKvS0n+XyZUpHFwvwjSKc+UV1Q22BxyH4jRd1paCrYYurjNG5guGcDDa51jIz69rj6Q/4S9
 Pizgg49wQXuci1kcC1YKjV2nqPC4ybeT6z/EuYTGPETKaegxN46vRVoE2RXwlVk+vmadVJlG
 JeQ7iQ==
Message-ID: <7075dd44-feea-a52f-ddaa-087d7bb2c4f6@akamai.com>
Date:   Tue, 3 Sep 2019 17:08:56 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <0cdc9905efb9b77b159e09bee17d3ad4@suse.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@hev.cc>
>>
>> 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@ZenIV.linux.org.uk>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Davide Libenzi <davidel@xmailserver.org>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
>> Cc: Eric Wong <e@80x24.org>
>> Cc: Jason Baron <jbaron@akamai.com>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Roman Penyaev <rpenyaev@suse.de>
>> Cc: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-fsdevel@vger.kernel.org
>> Signed-off-by: hev <r@hev.cc>
>> ---
>>  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).
>