trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: git commit: TS-2187: use nonblock eventfd in EventNotify
Date Wed, 11 Sep 2013 16:19:54 GMT
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 <qiushu.zyk@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.zyk@taobao.com>
> Authored: Sun Sep 8 23:35:39 2013 +0800
> Committer: Yunkai Zhang <qiushu.zyk@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);
> 


Mime
View raw message