You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by yu...@apache.org on 2013/09/11 09:19:02 UTC
git commit: TS-2187: use nonblock eventfd in EventNotify
Updated Branches:
refs/heads/master 40212d2d1 -> 9a6fc2ab6
TS-2187: use nonblock eventfd in EventNotify
1) Use nonblock eventfd, so that we can tolerate write() failed with
errno EAGANIN – which is acceptable as the signal receiver will be
notified eventually in this case.
2) After using nonblock eventfd, read() will not block in wait(). So
I use epoll_wait() to implement block behavior, just like timedwait().
3) nonblock eventfd can fix a potential problem: if receiver didn't
read() data immediately, senders might block in write().
Signed-off-by: Yunkai Zhang <qi...@taobao.com>
Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/9a6fc2ab
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/9a6fc2ab
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/9a6fc2ab
Branch: refs/heads/master
Commit: 9a6fc2ab6b3147e82dfc229a7158083371d5545e
Parents: 40212d2
Author: Yunkai Zhang <qi...@taobao.com>
Authored: Sun Sep 8 23:35:39 2013 +0800
Committer: Yunkai Zhang <qi...@taobao.com>
Committed: Wed Sep 11 15:18:18 2013 +0800
----------------------------------------------------------------------
CHANGES | 2 ++
lib/ts/EventNotify.cc | 42 +++++++++++++++++++++++++++++++-----------
lib/ts/EventNotify.h | 2 +-
3 files changed, 34 insertions(+), 12 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/CHANGES
----------------------------------------------------------------------
diff --git a/CHANGES b/CHANGES
index d042183..adeb534 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,8 @@
-*- coding: utf-8 -*-
Changes with Apache Traffic Server 4.1.0
+ *) [TS-2187] failed assert `nr == sizeof(uint64_t)` in EventNotify::signal()
+
*) [TS-2206] The trafficserver RC script does not use absolute path to
traffic_line binary.
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/lib/ts/EventNotify.cc
----------------------------------------------------------------------
diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
index 3a499b6..feb9ad8 100644
--- a/lib/ts/EventNotify.cc
+++ b/lib/ts/EventNotify.cc
@@ -32,6 +32,7 @@
#ifdef HAVE_EVENTFD
#include <sys/eventfd.h>
+#include <sys/fcntl.h>
#include <sys/epoll.h>
#endif
@@ -41,11 +42,13 @@ EventNotify::EventNotify()
int ret;
struct epoll_event ev;
- // Don't use noblock here!
- m_event_fd = eventfd(0, EFD_CLOEXEC);
+ m_event_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
if (m_event_fd < 0) {
- // EFD_CLOEXEC invalid in <= Linux 2.6.27
+ // EFD_NONBLOCK/EFD_CLOEXEC invalid in <= Linux 2.6.27
m_event_fd = eventfd(0, 0);
+
+ fcntl(m_event_fd, F_SETFD, FD_CLOEXEC);
+ fcntl(m_event_fd, F_SETFL, O_NONBLOCK);
}
ink_release_assert(m_event_fd != -1);
@@ -69,23 +72,39 @@ EventNotify::signal(void)
#ifdef HAVE_EVENTFD
ssize_t nr;
uint64_t value = 1;
+ //
+ // If the addition would cause the counter’s value of eventfd
+ // to exceed the maximum, write() will fail with the errno EAGAIN,
+ // which is acceptable as the receiver will be notified eventually.
+ //
nr = write(m_event_fd, &value, sizeof(uint64_t));
- ink_release_assert(nr == sizeof(uint64_t));
#else
ink_cond_signal(&m_cond);
#endif
}
-void
+int
EventNotify::wait(void)
{
#ifdef HAVE_EVENTFD
- ssize_t nr;
+ ssize_t nr, nr_fd;
uint64_t value = 0;
+ struct epoll_event ev;
+
+ do {
+ nr_fd = epoll_wait(m_epoll_fd, &ev, 1, -1);
+ } while (nr_fd == -1 && errno == EINTR);
+
+ if (nr_fd == -1)
+ return errno;
+
nr = read(m_event_fd, &value, sizeof(uint64_t));
- ink_release_assert(nr == sizeof(uint64_t));
+ if (nr == sizeof(uint64_t))
+ return 0;
+ else
+ return errno;
#else
- ink_cond_wait(&m_cond, &m_mutex);
+ return ink_cond_wait(&m_cond, &m_mutex);
#endif
}
@@ -115,9 +134,10 @@ EventNotify::timedwait(int timeout) // milliseconds
return errno;
nr = read(m_event_fd, &value, sizeof(uint64_t));
- ink_release_assert(nr == sizeof(uint64_t));
-
- return 0;
+ if (nr == sizeof(uint64_t))
+ return 0;
+ else
+ return errno;
#else
ink_timestruc abstime;
ink_hrtime curtime;
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/lib/ts/EventNotify.h
----------------------------------------------------------------------
diff --git a/lib/ts/EventNotify.h b/lib/ts/EventNotify.h
index b74e3af..7223e6d 100644
--- a/lib/ts/EventNotify.h
+++ b/lib/ts/EventNotify.h
@@ -38,7 +38,7 @@ class EventNotify
public:
EventNotify();
void signal(void);
- void wait(void);
+ int wait(void);
int timedwait(int timeout); // milliseconds
void lock(void);
bool trylock(void);
Re: git commit: TS-2187: use nonblock eventfd in EventNotify
Posted by Yunkai Zhang <yu...@gmail.com>.
On Thu, Sep 12, 2013 at 12:19 AM, James Peach <jp...@apache.org> wrote:
> On Sep 11, 2013, at 12:19 AM, yunkai@apache.org wrote:
>
> > Updated Branches:
> > refs/heads/master 40212d2d1 -> 9a6fc2ab6
> >
> >
> > TS-2187: use nonblock eventfd in EventNotify
> >
> > 1) Use nonblock eventfd, so that we can tolerate write() failed with
> > errno EAGANIN – which is acceptable as the signal receiver will be
> > notified eventually in this case.
> >
> > 2) After using nonblock eventfd, read() will not block in wait(). So
> > I use epoll_wait() to implement block behavior, just like timedwait().
> >
> > 3) nonblock eventfd can fix a potential problem: if receiver didn't
> > read() data immediately, senders might block in write().
> >
> > Signed-off-by: Yunkai Zhang <qi...@taobao.com>
> >
> >
> > Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> > Commit:
> http://git-wip-us.apache.org/repos/asf/trafficserver/commit/9a6fc2ab
> > Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/9a6fc2ab
> > Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/9a6fc2ab
> >
> > Branch: refs/heads/master
> > Commit: 9a6fc2ab6b3147e82dfc229a7158083371d5545e
> > Parents: 40212d2
> > Author: Yunkai Zhang <qi...@taobao.com>
> > Authored: Sun Sep 8 23:35:39 2013 +0800
> > Committer: Yunkai Zhang <qi...@taobao.com>
> > Committed: Wed Sep 11 15:18:18 2013 +0800
> >
> > ----------------------------------------------------------------------
> > CHANGES | 2 ++
> > lib/ts/EventNotify.cc | 42 +++++++++++++++++++++++++++++++-----------
> > lib/ts/EventNotify.h | 2 +-
> > 3 files changed, 34 insertions(+), 12 deletions(-)
> > ----------------------------------------------------------------------
> >
> >
> >
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/CHANGES
> > ----------------------------------------------------------------------
> > diff --git a/CHANGES b/CHANGES
> > index d042183..adeb534 100644
> > --- a/CHANGES
> > +++ b/CHANGES
> > @@ -1,6 +1,8 @@
> > -*- coding:
> utf-8 -*-
> > Changes with Apache Traffic Server 4.1.0
> >
> > + *) [TS-2187] failed assert `nr == sizeof(uint64_t)` in
> EventNotify::signal()
> > +
> > *) [TS-2206] The trafficserver RC script does not use absolute path to
> > traffic_line binary.
> >
> >
> >
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/lib/ts/EventNotify.cc
> > ----------------------------------------------------------------------
> > diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
> > index 3a499b6..feb9ad8 100644
> > --- a/lib/ts/EventNotify.cc
> > +++ b/lib/ts/EventNotify.cc
> > @@ -32,6 +32,7 @@
> >
> > #ifdef HAVE_EVENTFD
> > #include <sys/eventfd.h>
> > +#include <sys/fcntl.h>
> > #include <sys/epoll.h>
> > #endif
> >
> > @@ -41,11 +42,13 @@ EventNotify::EventNotify()
> > int ret;
> > struct epoll_event ev;
> >
> > - // Don't use noblock here!
> > - m_event_fd = eventfd(0, EFD_CLOEXEC);
> > + m_event_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> > if (m_event_fd < 0) {
>
> Should be if (m_event_fd == -1 && errno == EINVAL).
>
> > - // EFD_CLOEXEC invalid in <= Linux 2.6.27
> > + // EFD_NONBLOCK/EFD_CLOEXEC invalid in <= Linux 2.6.27
> > m_event_fd = eventfd(0, 0);
> > +
> > + fcntl(m_event_fd, F_SETFD, FD_CLOEXEC);
> > + fcntl(m_event_fd, F_SETFL, O_NONBLOCK);
> > }
> > ink_release_assert(m_event_fd != -1);
> >
> > @@ -69,23 +72,39 @@ EventNotify::signal(void)
> > #ifdef HAVE_EVENTFD
> > ssize_t nr;
> > uint64_t value = 1;
> > + //
> > + // If the addition would cause the counter’s value of eventfd
> > + // to exceed the maximum, write() will fail with the errno EAGAIN,
> > + // which is acceptable as the receiver will be notified eventually.
> > + //
>
> This would have to be a bug, right?
>
It wouldn't, there are two notification paradigms:
a) strict notification:
The receiver should care about how many signals is received, that is say it
need to calculate counter's value.
b) weak notification:
The receiver don't care about how many signals it received, all signals
sent before it read are serve as a signal.
ATS use b) notification paradigm in all the code (event system, logging,
stats, AIO, ...), and that it's what I like.
So it's safe to ignore EAGAIN in this code. I didn't retry to write() when
it failed as I want to force all developers to use weak notification
paradigm in the future, if not it generally means *bad* design:D
> > nr = write(m_event_fd, &value, sizeof(uint64_t));
> > - ink_release_assert(nr == sizeof(uint64_t));
> > #else
> > ink_cond_signal(&m_cond);
> > #endif
> > }
> >
> > -void
> > +int
> > EventNotify::wait(void)
> > {
> > #ifdef HAVE_EVENTFD
> > - ssize_t nr;
> > + ssize_t nr, nr_fd;
> > uint64_t value = 0;
> > + struct epoll_event ev;
> > +
> > + do {
> > + nr_fd = epoll_wait(m_epoll_fd, &ev, 1, -1);
> > + } while (nr_fd == -1 && errno == EINTR);
> > +
> > + if (nr_fd == -1)
> > + return errno;
> > +
> > nr = read(m_event_fd, &value, sizeof(uint64_t));
> > - ink_release_assert(nr == sizeof(uint64_t));
> > + if (nr == sizeof(uint64_t))
> > + return 0;
> > + else
> > + return errno;
> > #else
> > - ink_cond_wait(&m_cond, &m_mutex);
> > + return ink_cond_wait(&m_cond, &m_mutex);
> > #endif
> > }
>
> So with eventfd() support, EventNotify::wait can fail, but otherwise it
> can't fail. It would be better if the API was consistent.
>
Here is the ink_cond_wait():
static inline void
ink_cond_wait(ink_cond * cp, ink_mutex * mp)
{
ink_assert(pthread_cond_wait(cp, mp) == 0);
}
Actually, ink_cond_wait() can fail, but we wouldn't notice it as the
"ink_assert" make no sense in release mode.
I want to make NotifyEvent::wait() keep consistent with pthread_cond_wait().
Maybe we could change ink_cond_wait() to:
static inline void
ink_cond_wait(ink_cond * cp, ink_mutex * mp)
{
return pthread_cond_wait(cp, mp);
}
to keep consistent?
>
> >
> > @@ -115,9 +134,10 @@ EventNotify::timedwait(int timeout) // milliseconds
> > return errno;
> >
> > nr = read(m_event_fd, &value, sizeof(uint64_t));
> > - ink_release_assert(nr == sizeof(uint64_t));
> > -
> > - return 0;
> > + if (nr == sizeof(uint64_t))
> > + return 0;
> > + else
> > + return errno;
>
> It might be cleaner to use eventfd_read() and eventfd_write().
>
I had considered eventfd_read()/eventfd_write(), but it depend on glibc. I
want to reduce dependency, so gave up.
>
> > #else
> > ink_timestruc abstime;
> > ink_hrtime curtime;
> >
> >
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/lib/ts/EventNotify.h
> > ----------------------------------------------------------------------
> > diff --git a/lib/ts/EventNotify.h b/lib/ts/EventNotify.h
> > index b74e3af..7223e6d 100644
> > --- a/lib/ts/EventNotify.h
> > +++ b/lib/ts/EventNotify.h
> > @@ -38,7 +38,7 @@ class EventNotify
> > public:
> > EventNotify();
> > void signal(void);
> > - void wait(void);
> > + int wait(void);
> > int timedwait(int timeout); // milliseconds
> > void lock(void);
> > bool trylock(void);
> >
>
>
--
Yunkai Zhang
Work at Taobao
Re: git commit: TS-2187: use nonblock eventfd in EventNotify
Posted by James Peach <jp...@apache.org>.
On Sep 11, 2013, at 12:19 AM, yunkai@apache.org wrote:
> Updated Branches:
> refs/heads/master 40212d2d1 -> 9a6fc2ab6
>
>
> TS-2187: use nonblock eventfd in EventNotify
>
> 1) Use nonblock eventfd, so that we can tolerate write() failed with
> errno EAGANIN – which is acceptable as the signal receiver will be
> notified eventually in this case.
>
> 2) After using nonblock eventfd, read() will not block in wait(). So
> I use epoll_wait() to implement block behavior, just like timedwait().
>
> 3) nonblock eventfd can fix a potential problem: if receiver didn't
> read() data immediately, senders might block in write().
>
> Signed-off-by: Yunkai Zhang <qi...@taobao.com>
>
>
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/9a6fc2ab
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/9a6fc2ab
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/9a6fc2ab
>
> Branch: refs/heads/master
> Commit: 9a6fc2ab6b3147e82dfc229a7158083371d5545e
> Parents: 40212d2
> Author: Yunkai Zhang <qi...@taobao.com>
> Authored: Sun Sep 8 23:35:39 2013 +0800
> Committer: Yunkai Zhang <qi...@taobao.com>
> Committed: Wed Sep 11 15:18:18 2013 +0800
>
> ----------------------------------------------------------------------
> CHANGES | 2 ++
> lib/ts/EventNotify.cc | 42 +++++++++++++++++++++++++++++++-----------
> lib/ts/EventNotify.h | 2 +-
> 3 files changed, 34 insertions(+), 12 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/CHANGES
> ----------------------------------------------------------------------
> diff --git a/CHANGES b/CHANGES
> index d042183..adeb534 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -1,6 +1,8 @@
> -*- coding: utf-8 -*-
> Changes with Apache Traffic Server 4.1.0
>
> + *) [TS-2187] failed assert `nr == sizeof(uint64_t)` in EventNotify::signal()
> +
> *) [TS-2206] The trafficserver RC script does not use absolute path to
> traffic_line binary.
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/lib/ts/EventNotify.cc
> ----------------------------------------------------------------------
> diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
> index 3a499b6..feb9ad8 100644
> --- a/lib/ts/EventNotify.cc
> +++ b/lib/ts/EventNotify.cc
> @@ -32,6 +32,7 @@
>
> #ifdef HAVE_EVENTFD
> #include <sys/eventfd.h>
> +#include <sys/fcntl.h>
> #include <sys/epoll.h>
> #endif
>
> @@ -41,11 +42,13 @@ EventNotify::EventNotify()
> int ret;
> struct epoll_event ev;
>
> - // Don't use noblock here!
> - m_event_fd = eventfd(0, EFD_CLOEXEC);
> + m_event_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> if (m_event_fd < 0) {
Should be if (m_event_fd == -1 && errno == EINVAL).
> - // EFD_CLOEXEC invalid in <= Linux 2.6.27
> + // EFD_NONBLOCK/EFD_CLOEXEC invalid in <= Linux 2.6.27
> m_event_fd = eventfd(0, 0);
> +
> + fcntl(m_event_fd, F_SETFD, FD_CLOEXEC);
> + fcntl(m_event_fd, F_SETFL, O_NONBLOCK);
> }
> ink_release_assert(m_event_fd != -1);
>
> @@ -69,23 +72,39 @@ EventNotify::signal(void)
> #ifdef HAVE_EVENTFD
> ssize_t nr;
> uint64_t value = 1;
> + //
> + // If the addition would cause the counter’s value of eventfd
> + // to exceed the maximum, write() will fail with the errno EAGAIN,
> + // which is acceptable as the receiver will be notified eventually.
> + //
This would have to be a bug, right?
> nr = write(m_event_fd, &value, sizeof(uint64_t));
> - ink_release_assert(nr == sizeof(uint64_t));
> #else
> ink_cond_signal(&m_cond);
> #endif
> }
>
> -void
> +int
> EventNotify::wait(void)
> {
> #ifdef HAVE_EVENTFD
> - ssize_t nr;
> + ssize_t nr, nr_fd;
> uint64_t value = 0;
> + struct epoll_event ev;
> +
> + do {
> + nr_fd = epoll_wait(m_epoll_fd, &ev, 1, -1);
> + } while (nr_fd == -1 && errno == EINTR);
> +
> + if (nr_fd == -1)
> + return errno;
> +
> nr = read(m_event_fd, &value, sizeof(uint64_t));
> - ink_release_assert(nr == sizeof(uint64_t));
> + if (nr == sizeof(uint64_t))
> + return 0;
> + else
> + return errno;
> #else
> - ink_cond_wait(&m_cond, &m_mutex);
> + return ink_cond_wait(&m_cond, &m_mutex);
> #endif
> }
So with eventfd() support, EventNotify::wait can fail, but otherwise it can't fail. It would be better if the API was consistent.
>
> @@ -115,9 +134,10 @@ EventNotify::timedwait(int timeout) // milliseconds
> return errno;
>
> nr = read(m_event_fd, &value, sizeof(uint64_t));
> - ink_release_assert(nr == sizeof(uint64_t));
> -
> - return 0;
> + if (nr == sizeof(uint64_t))
> + return 0;
> + else
> + return errno;
It might be cleaner to use eventfd_read() and eventfd_write().
> #else
> ink_timestruc abstime;
> ink_hrtime curtime;
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/lib/ts/EventNotify.h
> ----------------------------------------------------------------------
> diff --git a/lib/ts/EventNotify.h b/lib/ts/EventNotify.h
> index b74e3af..7223e6d 100644
> --- a/lib/ts/EventNotify.h
> +++ b/lib/ts/EventNotify.h
> @@ -38,7 +38,7 @@ class EventNotify
> public:
> EventNotify();
> void signal(void);
> - void wait(void);
> + int wait(void);
> int timedwait(int timeout); // milliseconds
> void lock(void);
> bool trylock(void);
>
Re: git commit: TS-2187: use nonblock eventfd in EventNotify
Posted by James Peach <jp...@apache.org>.
On Sep 11, 2013, at 12:19 AM, yunkai@apache.org wrote:
> Updated Branches:
> refs/heads/master 40212d2d1 -> 9a6fc2ab6
>
>
> TS-2187: use nonblock eventfd in EventNotify
>
> 1) Use nonblock eventfd, so that we can tolerate write() failed with
> errno EAGANIN – which is acceptable as the signal receiver will be
> notified eventually in this case.
>
> 2) After using nonblock eventfd, read() will not block in wait(). So
> I use epoll_wait() to implement block behavior, just like timedwait().
>
> 3) nonblock eventfd can fix a potential problem: if receiver didn't
> read() data immediately, senders might block in write().
>
> Signed-off-by: Yunkai Zhang <qi...@taobao.com>
>
>
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/9a6fc2ab
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/9a6fc2ab
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/9a6fc2ab
>
> Branch: refs/heads/master
> Commit: 9a6fc2ab6b3147e82dfc229a7158083371d5545e
> Parents: 40212d2
> Author: Yunkai Zhang <qi...@taobao.com>
> Authored: Sun Sep 8 23:35:39 2013 +0800
> Committer: Yunkai Zhang <qi...@taobao.com>
> Committed: Wed Sep 11 15:18:18 2013 +0800
>
> ----------------------------------------------------------------------
> CHANGES | 2 ++
> lib/ts/EventNotify.cc | 42 +++++++++++++++++++++++++++++++-----------
> lib/ts/EventNotify.h | 2 +-
> 3 files changed, 34 insertions(+), 12 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/CHANGES
> ----------------------------------------------------------------------
> diff --git a/CHANGES b/CHANGES
> index d042183..adeb534 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -1,6 +1,8 @@
> -*- coding: utf-8 -*-
> Changes with Apache Traffic Server 4.1.0
>
> + *) [TS-2187] failed assert `nr == sizeof(uint64_t)` in EventNotify::signal()
> +
> *) [TS-2206] The trafficserver RC script does not use absolute path to
> traffic_line binary.
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/lib/ts/EventNotify.cc
> ----------------------------------------------------------------------
> diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
> index 3a499b6..feb9ad8 100644
> --- a/lib/ts/EventNotify.cc
> +++ b/lib/ts/EventNotify.cc
> @@ -32,6 +32,7 @@
>
> #ifdef HAVE_EVENTFD
> #include <sys/eventfd.h>
> +#include <sys/fcntl.h>
> #include <sys/epoll.h>
> #endif
>
> @@ -41,11 +42,13 @@ EventNotify::EventNotify()
> int ret;
> struct epoll_event ev;
>
> - // Don't use noblock here!
> - m_event_fd = eventfd(0, EFD_CLOEXEC);
> + m_event_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> if (m_event_fd < 0) {
Should be if (m_event_fd == -1 && errno == EINVAL).
> - // EFD_CLOEXEC invalid in <= Linux 2.6.27
> + // EFD_NONBLOCK/EFD_CLOEXEC invalid in <= Linux 2.6.27
> m_event_fd = eventfd(0, 0);
> +
> + fcntl(m_event_fd, F_SETFD, FD_CLOEXEC);
> + fcntl(m_event_fd, F_SETFL, O_NONBLOCK);
> }
> ink_release_assert(m_event_fd != -1);
>
> @@ -69,23 +72,39 @@ EventNotify::signal(void)
> #ifdef HAVE_EVENTFD
> ssize_t nr;
> uint64_t value = 1;
> + //
> + // If the addition would cause the counter’s value of eventfd
> + // to exceed the maximum, write() will fail with the errno EAGAIN,
> + // which is acceptable as the receiver will be notified eventually.
> + //
This would have to be a bug, right?
> nr = write(m_event_fd, &value, sizeof(uint64_t));
> - ink_release_assert(nr == sizeof(uint64_t));
> #else
> ink_cond_signal(&m_cond);
> #endif
> }
>
> -void
> +int
> EventNotify::wait(void)
> {
> #ifdef HAVE_EVENTFD
> - ssize_t nr;
> + ssize_t nr, nr_fd;
> uint64_t value = 0;
> + struct epoll_event ev;
> +
> + do {
> + nr_fd = epoll_wait(m_epoll_fd, &ev, 1, -1);
> + } while (nr_fd == -1 && errno == EINTR);
> +
> + if (nr_fd == -1)
> + return errno;
> +
> nr = read(m_event_fd, &value, sizeof(uint64_t));
> - ink_release_assert(nr == sizeof(uint64_t));
> + if (nr == sizeof(uint64_t))
> + return 0;
> + else
> + return errno;
> #else
> - ink_cond_wait(&m_cond, &m_mutex);
> + return ink_cond_wait(&m_cond, &m_mutex);
> #endif
> }
So with eventfd() support, EventNotify::wait can fail, but otherwise it can't fail. It would be better if the API was consistent.
>
> @@ -115,9 +134,10 @@ EventNotify::timedwait(int timeout) // milliseconds
> return errno;
>
> nr = read(m_event_fd, &value, sizeof(uint64_t));
> - ink_release_assert(nr == sizeof(uint64_t));
> -
> - return 0;
> + if (nr == sizeof(uint64_t))
> + return 0;
> + else
> + return errno;
It might be cleaner to use eventfd_read() and eventfd_write().
> #else
> ink_timestruc abstime;
> ink_hrtime curtime;
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/9a6fc2ab/lib/ts/EventNotify.h
> ----------------------------------------------------------------------
> diff --git a/lib/ts/EventNotify.h b/lib/ts/EventNotify.h
> index b74e3af..7223e6d 100644
> --- a/lib/ts/EventNotify.h
> +++ b/lib/ts/EventNotify.h
> @@ -38,7 +38,7 @@ class EventNotify
> public:
> EventNotify();
> void signal(void);
> - void wait(void);
> + int wait(void);
> int timedwait(int timeout); // milliseconds
> void lock(void);
> bool trylock(void);
>