On Sep 11, 2013, at 12:19 AM, yun...@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 <qiushu....@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 <qiushu....@taobao.com>
> Authored: Sun Sep 8 23:35:39 2013 +0800
> Committer: Yunkai Zhang <qiushu....@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);
>
