Re: epoll oddities following setup/teardown optimizations

[汪博文]

Hi,

I understood your meaning. You are right.
You mean the taskB should also get the POLLIN event when taskA send some
data event if taskB do not check the socket connected event.
I will take a look about this.

Regards
Wang Bowen

Mårten Svanfeldt  于2023年9月19日周二 21:41写道：

> Hi,
>
>
> I know a POLLOUT would be the right thing if I am interested in knowing
> when the connection is complete, but I don't care :) I _could_ add a
> POLLOUT just to "work around" this error however I prefer not.
>
>
> We only care about when there is data available, so POLLIN is correct, and
> it works on Linux and used to work on NuttX. Now I will never get any
> POLLIN event no matter how much data is sent from the other side.
>
>
> Regards
>
> Marten Svanfeldt
>
> 
> From: 汪博文 
> Sent: Tuesday, September 19, 2023 3:21:34 PM
> To: dev@nuttx.apache.org
> Subject: Re: epoll oddities following setup/teardown optimizations
>
> Hi, Marten
>
> I found some information about the epoll problem with the nonblocking local
> socket.
> From https://man7.org/linux/man-pages/man2/connect.2.html, we can see:
>
> EINPROGRESS
>   The socket is nonblocking and the connection cannot be
>   completed immediately.  (UNIX domain sockets failed with
>   EAGAIN instead.)  *It is possible to
> [select(2)](https://man7.org/linux/man-pages/man2/select.2.html
> <https://man7.org/linux/man-pages/man2/select.2.html>) or
> [poll(2)](https://man7.org/linux/man-pages/man2/poll.2.html
> <https://man7.org/linux/man-pages/man2/poll.2.html>)*
> *  for completion by selecting the socket for writing.*  After
>   [select(2)](
> https://man7.org/linux/man-pages/man2/select.2.html) indicates
> writability,
> use [getsockopt(2)](
> https://man7.org/linux/man-pages/man2/getsockopt.2.html)
> to read
>   the SO_ERROR option at level SOL_SOCKET to determine
>   whether connect() completed successfully (SO_ERROR is
>   zero) or unsuccessfully (SO_ERROR is one of the usual
>   error codes listed here, explaining the reason for the
>   failure).
>
> Use POLLOUT event to poll the non-connected nonblocking socket seems the
> right way instead of using POLLIN event.
> Could you try this ?
>
> Regrads
> Wang Bowen
>
> Mårten Svanfeldt  于2023年9月19日周二 19:17写道：
>
> > Hello,
> >
> >
> > I did some quick checking and it does indeed solve my problem. Do you
> want
> > me to give a review/ack on the github PR? I do have an account there with
> > id "thebolt" (but it is not yet linked to my current employers email from
> > which I am writing this)
> >
> >
> > I do however still have some epoll problems, also related to this change,
> > and now it is together with unix local (stream) socket.
> >
> >
> > The setup is as follows, I have to tasks, taskA and taskB.
> >
> >
> > taskA does setup a new local socket, does listen (blocking) followed by
> > accept (blocking) followed by "simple" poll+write/read
> >
> > taskB does socket(), make nonblock, connect() and then adds the socket fd
> > to an epoll fd with EPOLLIN only.
> >
> >
> > epoll_ctl(..EPOLL_CTL_ADD..) does poll setup, which in this case is
> > local_pollsetup (in local_netpoll.c). The connection is in state
> > LOCAL_STATE_CONNECTING (taskA didn't run accept yet) so the poll is setup
> > as an "event poll".
> >
> >
> > When taskA calls "accept" it eventually will perform a poll_notify(...
> > EPOLLOUT), but ask taskB only specified EPOLLIN it will be ignored, the
> > epoll not waken up, revents still set to 0 and the fd stays on the
> > setup-list inside the epoll, meaning it will never do another poll setup
> on
> > it, even though the socket itself changed state internally so that the
> next
> > call to local_pollsetup would setup the polling differently.
> >
> >
> >
> > In principle I wonder if you don't have to move from "setup" to
> "teardown"
> > list (so that the fd is re-setup the next epoll_wait) not only on
> > successful poll_notify but on _any_ poll_notify independent of if
> > (filtered) revents is changed or not. Otherwise internal state changes in
> > the underlying driver/module will never be detected and pollsetup not
> > re-ran when needed.
> >
> >
> > Regards
> >
> > Marten Svanfeldt
> >
> > 
> > From: 汪博文 
> > Sent: Tuesday, September 19, 2023 12:11:27

Re: epoll oddities following setup/teardown optimizations

[Mårten Svanfeldt]

Hi,


I know a POLLOUT would be the right thing if I am interested in knowing when 
the connection is complete, but I don't care :) I _could_ add a POLLOUT just to 
"work around" this error however I prefer not.


We only care about when there is data available, so POLLIN is correct, and it 
works on Linux and used to work on NuttX. Now I will never get any POLLIN event 
no matter how much data is sent from the other side.


Regards

Marten Svanfeldt


From: 汪博文 
Sent: Tuesday, September 19, 2023 3:21:34 PM
To: dev@nuttx.apache.org
Subject: Re: epoll oddities following setup/teardown optimizations

Hi, Marten

I found some information about the epoll problem with the nonblocking local
socket.
From https://man7.org/linux/man-pages/man2/connect.2.html, we can see:

EINPROGRESS
  The socket is nonblocking and the connection cannot be
  completed immediately.  (UNIX domain sockets failed with
  EAGAIN instead.)  *It is possible to
[select(2)](https://man7.org/linux/man-pages/man2/select.2.html
<https://man7.org/linux/man-pages/man2/select.2.html>) or
[poll(2)](https://man7.org/linux/man-pages/man2/poll.2.html
<https://man7.org/linux/man-pages/man2/poll.2.html>)*
*  for completion by selecting the socket for writing.*  After
  [select(2)](
https://man7.org/linux/man-pages/man2/select.2.html) indicates writability,
use [getsockopt(2)](https://man7.org/linux/man-pages/man2/getsockopt.2.html)
to read
  the SO_ERROR option at level SOL_SOCKET to determine
  whether connect() completed successfully (SO_ERROR is
  zero) or unsuccessfully (SO_ERROR is one of the usual
  error codes listed here, explaining the reason for the
  failure).

Use POLLOUT event to poll the non-connected nonblocking socket seems the
right way instead of using POLLIN event.
Could you try this ?

Regrads
Wang Bowen

Mårten Svanfeldt  于2023年9月19日周二 19:17写道：

> Hello,
>
>
> I did some quick checking and it does indeed solve my problem. Do you want
> me to give a review/ack on the github PR? I do have an account there with
> id "thebolt" (but it is not yet linked to my current employers email from
> which I am writing this)
>
>
> I do however still have some epoll problems, also related to this change,
> and now it is together with unix local (stream) socket.
>
>
> The setup is as follows, I have to tasks, taskA and taskB.
>
>
> taskA does setup a new local socket, does listen (blocking) followed by
> accept (blocking) followed by "simple" poll+write/read
>
> taskB does socket(), make nonblock, connect() and then adds the socket fd
> to an epoll fd with EPOLLIN only.
>
>
> epoll_ctl(..EPOLL_CTL_ADD..) does poll setup, which in this case is
> local_pollsetup (in local_netpoll.c). The connection is in state
> LOCAL_STATE_CONNECTING (taskA didn't run accept yet) so the poll is setup
> as an "event poll".
>
>
> When taskA calls "accept" it eventually will perform a poll_notify(...
> EPOLLOUT), but ask taskB only specified EPOLLIN it will be ignored, the
> epoll not waken up, revents still set to 0 and the fd stays on the
> setup-list inside the epoll, meaning it will never do another poll setup on
> it, even though the socket itself changed state internally so that the next
> call to local_pollsetup would setup the polling differently.
>
>
>
> In principle I wonder if you don't have to move from "setup" to "teardown"
> list (so that the fd is re-setup the next epoll_wait) not only on
> successful poll_notify but on _any_ poll_notify independent of if
> (filtered) revents is changed or not. Otherwise internal state changes in
> the underlying driver/module will never be detected and pollsetup not
> re-ran when needed.
>
>
> Regards
>
> Marten Svanfeldt
>
> 
> From: 汪博文 
> Sent: Tuesday, September 19, 2023 12:11:27 PM
> To: Mårten Svanfeldt
> Cc: dev@nuttx.apache.org
> Subject: RE: epoll oddities following setup/teardown optimizations
>
> Hi,
>
> I has a fix fs_epoll: add critical section for teardown process and reset
> the eph… by CV-Bowen · Pull Request #10706 · apache/nuttx (github.com)<
> https://github.com/apache/nuttx/pull/10706> about this problem.
> But I couldn’t find your GitHub ID, so I’m notifying you through email.
> Could you try this PR and check if this PR resolves your problem ?
>
> Regards
> Wang Bowen
> #/**本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
> This e-mail and its attachments contain confidential information from
> XIAOMI, which is intended only for the person or entity wh

Re: epoll oddities following setup/teardown optimizations

[汪博文]

Hi, Marten

I found some information about the epoll problem with the nonblocking local
socket.
>From https://man7.org/linux/man-pages/man2/connect.2.html, we can see:

EINPROGRESS
  The socket is nonblocking and the connection cannot be
  completed immediately.  (UNIX domain sockets failed with
  EAGAIN instead.)  *It is possible to
[select(2)](https://man7.org/linux/man-pages/man2/select.2.html
<https://man7.org/linux/man-pages/man2/select.2.html>) or
[poll(2)](https://man7.org/linux/man-pages/man2/poll.2.html
<https://man7.org/linux/man-pages/man2/poll.2.html>)*
*  for completion by selecting the socket for writing.*  After
  [select(2)](
https://man7.org/linux/man-pages/man2/select.2.html) indicates writability,
use [getsockopt(2)](https://man7.org/linux/man-pages/man2/getsockopt.2.html)
to read
  the SO_ERROR option at level SOL_SOCKET to determine
  whether connect() completed successfully (SO_ERROR is
  zero) or unsuccessfully (SO_ERROR is one of the usual
  error codes listed here, explaining the reason for the
  failure).

Use POLLOUT event to poll the non-connected nonblocking socket seems the
right way instead of using POLLIN event.
Could you try this ?

Regrads
Wang Bowen

Mårten Svanfeldt  于2023年9月19日周二 19:17写道：

> Hello,
>
>
> I did some quick checking and it does indeed solve my problem. Do you want
> me to give a review/ack on the github PR? I do have an account there with
> id "thebolt" (but it is not yet linked to my current employers email from
> which I am writing this)
>
>
> I do however still have some epoll problems, also related to this change,
> and now it is together with unix local (stream) socket.
>
>
> The setup is as follows, I have to tasks, taskA and taskB.
>
>
> taskA does setup a new local socket, does listen (blocking) followed by
> accept (blocking) followed by "simple" poll+write/read
>
> taskB does socket(), make nonblock, connect() and then adds the socket fd
> to an epoll fd with EPOLLIN only.
>
>
> epoll_ctl(..EPOLL_CTL_ADD..) does poll setup, which in this case is
> local_pollsetup (in local_netpoll.c). The connection is in state
> LOCAL_STATE_CONNECTING (taskA didn't run accept yet) so the poll is setup
> as an "event poll".
>
>
> When taskA calls "accept" it eventually will perform a poll_notify(...
> EPOLLOUT), but ask taskB only specified EPOLLIN it will be ignored, the
> epoll not waken up, revents still set to 0 and the fd stays on the
> setup-list inside the epoll, meaning it will never do another poll setup on
> it, even though the socket itself changed state internally so that the next
> call to local_pollsetup would setup the polling differently.
>
>
>
> In principle I wonder if you don't have to move from "setup" to "teardown"
> list (so that the fd is re-setup the next epoll_wait) not only on
> successful poll_notify but on _any_ poll_notify independent of if
> (filtered) revents is changed or not. Otherwise internal state changes in
> the underlying driver/module will never be detected and pollsetup not
> re-ran when needed.
>
>
> Regards
>
> Marten Svanfeldt
>
> 
> From: 汪博文 
> Sent: Tuesday, September 19, 2023 12:11:27 PM
> To: Mårten Svanfeldt
> Cc: dev@nuttx.apache.org
> Subject: RE: epoll oddities following setup/teardown optimizations
>
> Hi,
>
> I has a fix fs_epoll: add critical section for teardown process and reset
> the eph… by CV-Bowen · Pull Request #10706 · apache/nuttx (github.com)<
> https://github.com/apache/nuttx/pull/10706> about this problem.
> But I couldn’t find your GitHub ID, so I’m notifying you through email.
> Could you try this PR and check if this PR resolves your problem ?
>
> Regards
> Wang Bowen
> #/**本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
> This e-mail and its attachments contain confidential information from
> XIAOMI, which is intended only for the person or entity whose address is
> listed above. Any use of the information contained herein in any way
> (including, but not limited to, total or partial disclosure, reproduction,
> or dissemination) by persons other than the intended recipient(s) is
> prohibited. If you receive this e-mail in error, please notify the sender
> by phone or email immediately and delete it!**/#
>

Re: epoll oddities following setup/teardown optimizations

[Mårten Svanfeldt]

Hello,


I did some quick checking and it does indeed solve my problem. Do you want me 
to give a review/ack on the github PR? I do have an account there with id 
"thebolt" (but it is not yet linked to my current employers email from which I 
am writing this)


I do however still have some epoll problems, also related to this change, and 
now it is together with unix local (stream) socket.


The setup is as follows, I have to tasks, taskA and taskB.


taskA does setup a new local socket, does listen (blocking) followed by accept 
(blocking) followed by "simple" poll+write/read

taskB does socket(), make nonblock, connect() and then adds the socket fd to an 
epoll fd with EPOLLIN only.


epoll_ctl(..EPOLL_CTL_ADD..) does poll setup, which in this case is 
local_pollsetup (in local_netpoll.c). The connection is in state 
LOCAL_STATE_CONNECTING (taskA didn't run accept yet) so the poll is setup as an 
"event poll".


When taskA calls "accept" it eventually will perform a poll_notify(... 
EPOLLOUT), but ask taskB only specified EPOLLIN it will be ignored, the epoll 
not waken up, revents still set to 0 and the fd stays on the setup-list inside 
the epoll, meaning it will never do another poll setup on it, even though the 
socket itself changed state internally so that the next call to local_pollsetup 
would setup the polling differently.



In principle I wonder if you don't have to move from "setup" to "teardown" list 
(so that the fd is re-setup the next epoll_wait) not only on successful 
poll_notify but on _any_ poll_notify independent of if (filtered) revents is 
changed or not. Otherwise internal state changes in the underlying 
driver/module will never be detected and pollsetup not re-ran when needed.


Regards

Marten Svanfeldt


From: 汪博文 
Sent: Tuesday, September 19, 2023 12:11:27 PM
To: Mårten Svanfeldt
Cc: dev@nuttx.apache.org
Subject: RE: epoll oddities following setup/teardown optimizations

Hi,

I has a fix fs_epoll: add critical section for teardown process and reset the 
eph… by CV-Bowen · Pull Request #10706 · apache/nuttx 
(github.com)<https://github.com/apache/nuttx/pull/10706> about this problem.
But I couldn’t find your GitHub ID, so I’m notifying you through email. Could 
you try this PR and check if this PR resolves your problem ?

Regards
Wang Bowen
#/**本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
 This e-mail and its attachments contain confidential information from XIAOMI, 
which is intended only for the person or entity whose address is listed above. 
Any use of the information contained herein in any way (including, but not 
limited to, total or partial disclosure, reproduction, or dissemination) by 
persons other than the intended recipient(s) is prohibited. If you receive this 
e-mail in error, please notify the sender by phone or email immediately and 
delete it!**/#

RE: epoll oddities following setup/teardown optimizations

[汪博文]

Hi,

I has a fix fs_epoll: add critical section for teardown process and reset the 
eph… by CV-Bowen ・ Pull Request #10706 ・ apache/nuttx 
(github.com) about this problem.
But I couldn’t find your GitHub ID, so I’m notifying you through email. Could 
you try this PR and check if this PR resolves your problem ?

Regards
Wang Bowen
#/**本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
 This e-mail and its attachments contain confidential information from XIAOMI, 
which is intended only for the person or entity whose address is listed above. 
Any use of the information contained herein in any way (including, but not 
limited to, total or partial disclosure, reproduction, or dissemination) by 
persons other than the intended recipient(s) is prohibited. If you receive this 
e-mail in error, please notify the sender by phone or email immediately and 
delete it!**/#

Re: epoll oddities following setup/teardown optimizations

[Alan C. Assis]

Hi Wang Bowen and Marten,

Is it possible to include an ostest with this code to avoid similar
regression in the future?

BR,

Alan

On 9/18/23, 汪博文  wrote:
> Hi,
>
> I did the epoll optimization last year. And your analysis is right. The new
> epoll implementation has BUG and I has reproduced this issue locally based
> on your description.
> I’m very sorry that this issue has affected your nuttx upgrade. I will fix
> this issue as soon as possible. Thanks.
>
> Regards
> Wang Bowen
>
> #/**本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
> This e-mail and its attachments contain confidential information from
> XIAOMI, which is intended only for the person or entity whose address is
> listed above. Any use of the information contained herein in any way
> (including, but not limited to, total or partial disclosure, reproduction,
> or dissemination) by persons other than the intended recipient(s) is
> prohibited. If you receive this e-mail in error, please notify the sender by
> phone or email immediately and delete it!**/#
>

RE: epoll oddities following setup/teardown optimizations

[汪博文]

Hi,

I did the epoll optimization last year. And your analysis is right. The new 
epoll implementation has BUG and I has reproduced this issue locally based on 
your description.
I’m very sorry that this issue has affected your nuttx upgrade. I will fix this 
issue as soon as possible. Thanks.

Regards
Wang Bowen

#/**本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
 This e-mail and its attachments contain confidential information from XIAOMI, 
which is intended only for the person or entity whose address is listed above. 
Any use of the information contained herein in any way (including, but not 
limited to, total or partial disclosure, reproduction, or dissemination) by 
persons other than the intended recipient(s) is prohibited. If you receive this 
e-mail in error, please notify the sender by phone or email immediately and 
delete it!**/#

epoll oddities following setup/teardown optimizations

[Mårten Svanfeldt]

Hi,


Our software heavily relies on epoll based wait/event loops, using mixes of 
"real" FDs (mostly sockets), eventfd and timerfds, and after upgrading to Nuttx 
12.2.1 I have some oddities in behaviour that I think can be traced to the 
optimizations in 
https://github.com/apache/nuttx/commit/25bfd437fe5b30d0221c68288e27fb7efc57fe0c


The behaviour can easily be triggered by creating an epoll fd, adding two 
timerfd with same timeout to it and then doing a loop with epoll_wait. First 
iteration (on my board) epoll_wait will return 2, with 2 events having EPOLLIN 
set, as expected, however second time epoll_wait is called (with infinite 
timeout) it returns immediately, returning 0 (zero) meaning no ready fds which 
as far as I can read the spec of epoll_wait (in the linux kernel) is an invalid 
return for an infinite wait.


Reading the code and trying to debug using prints (without affecting the 
timing) I think the following is what happens:


  1.  epoll_wait blocks on nxsem_wait, waiting for the semaphore to be 
signaled. Semcount is decreased so it becomes -1.
  2.  timerfd1 expires, timerfd_timeout calls poll_notify which in turn signals 
the semaphore. Semcount increased by 1 so it becomes 0.
  3.  timerfd2 expires, timerfd_timeout calls poll_notify which in turn signals 
the semaphore again. Semcount increased by 1 so it becomes 1.
  4.  epoll_wait is woken up, processed the fds, copies revent etc to the 
output indicating (correctly) that both fds have been affected. epoll_wait 
returns 2.
  5.  epoll_wait is called again. nxsem_wait is called, but as semcount is 
already 1 it returns immediately so epoll_wait does not go to sleep. When 
processing fds all revents is now still zero, so epoll_wait returns 0 (as in 
fact there is nothing ready to process).

It is possible that the epoll_wait wakeup happens before timerfd2 expiration, 
however timerfd2 expiration definitley happens before epoll_teardown processes 
all the fds.

I am not sure about the correct way to handle this, and the setup/teardown 
optimizations in themselves seems good and desirable. Could someone who was 
involved in writing or reviewing the original patch maybe comment if my 
analysis seems wrong of if you have any idea how to solve this?

Regards
Marten Svanfeldt