On Thu, Sep 12, 2013 at 12:19 AM, James Peach <jpe...@apache.org> wrote:
> 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? > 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
