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/08/22 14:01:15 UTC
git commit: TS-2137: Use relative timeout in EventNotify::timedwait()
Updated Branches:
refs/heads/master e5d27294b -> fa176442f
TS-2137: Use relative timeout in EventNotify::timedwait()
1) Use relative timeout in EventNotify::timedwait() function.
2) Remove unused m_name variable.
3) Use the correct marco HAVE_EVENTFD instead of TS_HAS_EVENTFD which
is outdated.
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/fa176442
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/fa176442
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/fa176442
Branch: refs/heads/master
Commit: fa176442f45a3d5e8a9dce332775172192f53f34
Parents: e5d2729
Author: Yunkai Zhang <qi...@taobao.com>
Authored: Thu Aug 22 16:13:36 2013 +0800
Committer: Yunkai Zhang <qi...@taobao.com>
Committed: Thu Aug 22 19:56:02 2013 +0800
----------------------------------------------------------------------
lib/ts/EventNotify.cc | 39 +++++++++++++++++++--------------------
lib/ts/EventNotify.h | 7 +++----
2 files changed, 22 insertions(+), 24 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.cc
----------------------------------------------------------------------
diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
index 5d0cc53..3a499b6 100644
--- a/lib/ts/EventNotify.cc
+++ b/lib/ts/EventNotify.cc
@@ -30,14 +30,14 @@
#include "EventNotify.h"
#include "ink_hrtime.h"
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
#include <sys/eventfd.h>
#include <sys/epoll.h>
#endif
-EventNotify::EventNotify(const char *name): m_name(name)
+EventNotify::EventNotify()
{
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
int ret;
struct epoll_event ev;
@@ -59,14 +59,14 @@ EventNotify::EventNotify(const char *name): m_name(name)
ink_release_assert(ret != -1);
#else
ink_cond_init(&m_cond);
- ink_mutex_init(&m_mutex, m_name);
+ ink_mutex_init(&m_mutex, NULL);
#endif
}
void
EventNotify::signal(void)
{
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
ssize_t nr;
uint64_t value = 1;
nr = write(m_event_fd, &value, sizeof(uint64_t));
@@ -79,7 +79,7 @@ EventNotify::signal(void)
void
EventNotify::wait(void)
{
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
ssize_t nr;
uint64_t value = 0;
nr = read(m_event_fd, &value, sizeof(uint64_t));
@@ -90,20 +90,13 @@ EventNotify::wait(void)
}
int
-EventNotify::timedwait(ink_timestruc *abstime)
+EventNotify::timedwait(int timeout) // milliseconds
{
-#ifdef TS_HAS_EVENTFD
- int timeout;
+#ifdef HAVE_EVENTFD
ssize_t nr, nr_fd = 0;
uint64_t value = 0;
- struct timeval curtime;
struct epoll_event ev;
- // Convert absolute time to relative time
- gettimeofday(&curtime, NULL);
- timeout = (abstime->tv_sec - curtime.tv_sec) * 1000
- + (abstime->tv_nsec / 1000 - curtime.tv_usec) / 1000;
-
//
// When timeout < 0, epoll_wait() will wait indefinitely, but
// pthread_cond_timedwait() will return ETIMEDOUT immediately.
@@ -126,14 +119,20 @@ EventNotify::timedwait(ink_timestruc *abstime)
return 0;
#else
- return ink_cond_timedwait(&m_cond, &m_mutex, abstime);
+ ink_timestruc abstime;
+ ink_hrtime curtime;
+
+ curtime = ink_get_hrtime_internal() + timeout * HRTIME_MSECOND;
+ abstime = ink_based_hrtime_to_timespec(curtime);
+
+ return ink_cond_timedwait(&m_cond, &m_mutex, &abstime);
#endif
}
void
EventNotify::lock(void)
{
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
// do nothing
#else
ink_mutex_acquire(&m_mutex);
@@ -143,7 +142,7 @@ EventNotify::lock(void)
bool
EventNotify::trylock(void)
{
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
return true;
#else
return ink_mutex_try_acquire(&m_mutex);
@@ -153,7 +152,7 @@ EventNotify::trylock(void)
void
EventNotify::unlock(void)
{
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
// do nothing
#else
ink_mutex_release(&m_mutex);
@@ -162,7 +161,7 @@ EventNotify::unlock(void)
EventNotify::~EventNotify()
{
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
close(m_event_fd);
close(m_epoll_fd);
#else
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.h
----------------------------------------------------------------------
diff --git a/lib/ts/EventNotify.h b/lib/ts/EventNotify.h
index 16e4809..135f037 100644
--- a/lib/ts/EventNotify.h
+++ b/lib/ts/EventNotify.h
@@ -33,18 +33,17 @@
class EventNotify
{
public:
- EventNotify(const char *name = NULL);
+ EventNotify();
void signal(void);
void wait(void);
- int timedwait(ink_timestruc *abstime);
+ int timedwait(int timeout); // milliseconds
void lock(void);
bool trylock(void);
void unlock(void);
~EventNotify();
private:
- const char *m_name;
-#ifdef TS_HAS_EVENTFD
+#ifdef HAVE_EVENTFD
int m_event_fd;
int m_epoll_fd;
#else
Re: git commit: TS-2137: Use relative timeout in
EventNotify::timedwait()
Posted by James Peach <jp...@apache.org>.
On Aug 23, 2013, at 3:58 AM, Igor Galić <i....@brainsware.org> wrote:
>
>
> ----- Original Message -----
>> On Fri, Aug 23, 2013 at 12:17 AM, James Peach <jp...@apache.org> wrote:
>>
>>> On Aug 22, 2013, at 5:01 AM, yunkai@apache.org wrote:
>>>
[snip]
>>
>> In some scenario, the upper layer store absolute time before calling
>> timedwait, such as ProtectedQueue.
>>
>> By using int type, the upper layer can calculate timeout more simply:
>>
>> timeout = now - abstime;
>>
>> The upper layer needn't to care about whether the timeout is < 0. We will
>> check it automatically inside the timedwait() function. I think use int
>> type could be more user friendly.
>
> maybe int64 is a better choice?
int64 would be a bit overkill for a timeout I think. I can live with int, though I do think that unsigned is a better choice. The reason is that when you do a timed what, you almost never do an absolute timeout; you do something like lock.timedwait(10), so taking a signed typed allows ambiguity where none is needed.
The meaning of lock.timedwait(-1) in this case will be the same as lock.timedwait(0), however I agree with Yunkai that using a signed type could be more robust against signed arithmetic errors.
J
Re: git commit: TS-2137: Use relative timeout in
EventNotify::timedwait()
Posted by Igor Galić <i....@brainsware.org>.
----- Original Message -----
> On Fri, Aug 23, 2013 at 12:17 AM, James Peach <jp...@apache.org> wrote:
>
> > On Aug 22, 2013, at 5:01 AM, yunkai@apache.org wrote:
> >
> > > Updated Branches:
> > > refs/heads/master e5d27294b -> fa176442f
> > >
> > >
> > > TS-2137: Use relative timeout in EventNotify::timedwait()
> > >
> > > 1) Use relative timeout in EventNotify::timedwait() function.
> > > 2) Remove unused m_name variable.
> > > 3) Use the correct marco HAVE_EVENTFD instead of TS_HAS_EVENTFD which
> > > is outdated.
> >
> > Good catch!
> >
> > >
> > > 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/fa176442
> > > Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/fa176442
> > > Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/fa176442
> > >
> > > Branch: refs/heads/master
> > > Commit: fa176442f45a3d5e8a9dce332775172192f53f34
> > > Parents: e5d2729
> > > Author: Yunkai Zhang <qi...@taobao.com>
> > > Authored: Thu Aug 22 16:13:36 2013 +0800
> > > Committer: Yunkai Zhang <qi...@taobao.com>
> > > Committed: Thu Aug 22 19:56:02 2013 +0800
> > >
> > > ----------------------------------------------------------------------
> > > lib/ts/EventNotify.cc | 39 +++++++++++++++++++--------------------
> > > lib/ts/EventNotify.h | 7 +++----
> > > 2 files changed, 22 insertions(+), 24 deletions(-)
> > > ----------------------------------------------------------------------
> > >
> > >
> > >
> > http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.cc
> > > ----------------------------------------------------------------------
> > > diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
> > > index 5d0cc53..3a499b6 100644
> > > --- a/lib/ts/EventNotify.cc
> > > +++ b/lib/ts/EventNotify.cc
> > > @@ -30,14 +30,14 @@
> > > #include "EventNotify.h"
> > > #include "ink_hrtime.h"
> > >
> > > -#ifdef TS_HAS_EVENTFD
> > > +#ifdef HAVE_EVENTFD
> > > #include <sys/eventfd.h>
> > > #include <sys/epoll.h>
> > > #endif
> > >
> > > -EventNotify::EventNotify(const char *name): m_name(name)
> > > +EventNotify::EventNotify()
> > > {
> > > -#ifdef TS_HAS_EVENTFD
> > > +#ifdef HAVE_EVENTFD
> > > int ret;
> > > struct epoll_event ev;
> > >
> > > @@ -59,14 +59,14 @@ EventNotify::EventNotify(const char *name):
> > m_name(name)
> > > ink_release_assert(ret != -1);
> > > #else
> > > ink_cond_init(&m_cond);
> > > - ink_mutex_init(&m_mutex, m_name);
> > > + ink_mutex_init(&m_mutex, NULL);
> > > #endif
> > > }
> > >
> > > void
> > > EventNotify::signal(void)
> > > {
> > > -#ifdef TS_HAS_EVENTFD
> > > +#ifdef HAVE_EVENTFD
> > > ssize_t nr;
> > > uint64_t value = 1;
> > > nr = write(m_event_fd, &value, sizeof(uint64_t));
> > > @@ -79,7 +79,7 @@ EventNotify::signal(void)
> > > void
> > > EventNotify::wait(void)
> > > {
> > > -#ifdef TS_HAS_EVENTFD
> > > +#ifdef HAVE_EVENTFD
> > > ssize_t nr;
> > > uint64_t value = 0;
> > > nr = read(m_event_fd, &value, sizeof(uint64_t));
> > > @@ -90,20 +90,13 @@ EventNotify::wait(void)
> > > }
> > >
> > > int
> > > -EventNotify::timedwait(ink_timestruc *abstime)
> > > +EventNotify::timedwait(int timeout) // milliseconds
> >
> > Unless -1 is a meaningful value, I'd prefer this was unsigned.
> >
>
> In some scenario, the upper layer store absolute time before calling
> timedwait, such as ProtectedQueue.
>
> By using int type, the upper layer can calculate timeout more simply:
>
> timeout = now - abstime;
>
> The upper layer needn't to care about whether the timeout is < 0. We will
> check it automatically inside the timedwait() function. I think use int
> type could be more user friendly.
maybe int64 is a better choice?
--
Igor Galić
Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515 2EA5 4B1D 9E08 A097 C9AE
Re: git commit: TS-2137: Use relative timeout in EventNotify::timedwait()
Posted by Yunkai Zhang <yu...@gmail.com>.
On Fri, Aug 23, 2013 at 12:17 AM, James Peach <jp...@apache.org> wrote:
> On Aug 22, 2013, at 5:01 AM, yunkai@apache.org wrote:
>
> > Updated Branches:
> > refs/heads/master e5d27294b -> fa176442f
> >
> >
> > TS-2137: Use relative timeout in EventNotify::timedwait()
> >
> > 1) Use relative timeout in EventNotify::timedwait() function.
> > 2) Remove unused m_name variable.
> > 3) Use the correct marco HAVE_EVENTFD instead of TS_HAS_EVENTFD which
> > is outdated.
>
> Good catch!
>
> >
> > 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/fa176442
> > Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/fa176442
> > Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/fa176442
> >
> > Branch: refs/heads/master
> > Commit: fa176442f45a3d5e8a9dce332775172192f53f34
> > Parents: e5d2729
> > Author: Yunkai Zhang <qi...@taobao.com>
> > Authored: Thu Aug 22 16:13:36 2013 +0800
> > Committer: Yunkai Zhang <qi...@taobao.com>
> > Committed: Thu Aug 22 19:56:02 2013 +0800
> >
> > ----------------------------------------------------------------------
> > lib/ts/EventNotify.cc | 39 +++++++++++++++++++--------------------
> > lib/ts/EventNotify.h | 7 +++----
> > 2 files changed, 22 insertions(+), 24 deletions(-)
> > ----------------------------------------------------------------------
> >
> >
> >
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.cc
> > ----------------------------------------------------------------------
> > diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
> > index 5d0cc53..3a499b6 100644
> > --- a/lib/ts/EventNotify.cc
> > +++ b/lib/ts/EventNotify.cc
> > @@ -30,14 +30,14 @@
> > #include "EventNotify.h"
> > #include "ink_hrtime.h"
> >
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > #include <sys/eventfd.h>
> > #include <sys/epoll.h>
> > #endif
> >
> > -EventNotify::EventNotify(const char *name): m_name(name)
> > +EventNotify::EventNotify()
> > {
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > int ret;
> > struct epoll_event ev;
> >
> > @@ -59,14 +59,14 @@ EventNotify::EventNotify(const char *name):
> m_name(name)
> > ink_release_assert(ret != -1);
> > #else
> > ink_cond_init(&m_cond);
> > - ink_mutex_init(&m_mutex, m_name);
> > + ink_mutex_init(&m_mutex, NULL);
> > #endif
> > }
> >
> > void
> > EventNotify::signal(void)
> > {
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > ssize_t nr;
> > uint64_t value = 1;
> > nr = write(m_event_fd, &value, sizeof(uint64_t));
> > @@ -79,7 +79,7 @@ EventNotify::signal(void)
> > void
> > EventNotify::wait(void)
> > {
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > ssize_t nr;
> > uint64_t value = 0;
> > nr = read(m_event_fd, &value, sizeof(uint64_t));
> > @@ -90,20 +90,13 @@ EventNotify::wait(void)
> > }
> >
> > int
> > -EventNotify::timedwait(ink_timestruc *abstime)
> > +EventNotify::timedwait(int timeout) // milliseconds
>
> Unless -1 is a meaningful value, I'd prefer this was unsigned.
>
In some scenario, the upper layer store absolute time before calling
timedwait, such as ProtectedQueue.
By using int type, the upper layer can calculate timeout more simply:
timeout = now - abstime;
The upper layer needn't to care about whether the timeout is < 0. We will
check it automatically inside the timedwait() function. I think use int
type could be more user friendly.
>
> > {
> > -#ifdef TS_HAS_EVENTFD
> > - int timeout;
> > +#ifdef HAVE_EVENTFD
> > ssize_t nr, nr_fd = 0;
> > uint64_t value = 0;
> > - struct timeval curtime;
> > struct epoll_event ev;
> >
> > - // Convert absolute time to relative time
> > - gettimeofday(&curtime, NULL);
> > - timeout = (abstime->tv_sec - curtime.tv_sec) * 1000
> > - + (abstime->tv_nsec / 1000 - curtime.tv_usec) / 1000;
> > -
> > //
> > // When timeout < 0, epoll_wait() will wait indefinitely, but
> > // pthread_cond_timedwait() will return ETIMEDOUT immediately.
> > @@ -126,14 +119,20 @@ EventNotify::timedwait(ink_timestruc *abstime)
> >
> > return 0;
> > #else
> > - return ink_cond_timedwait(&m_cond, &m_mutex, abstime);
> > + ink_timestruc abstime;
> > + ink_hrtime curtime;
> > +
> > + curtime = ink_get_hrtime_internal() + timeout * HRTIME_MSECOND;
> > + abstime = ink_based_hrtime_to_timespec(curtime);
> > +
> > + return ink_cond_timedwait(&m_cond, &m_mutex, &abstime);
> > #endif
> > }
> >
> > void
> > EventNotify::lock(void)
> > {
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > // do nothing
> > #else
> > ink_mutex_acquire(&m_mutex);
> > @@ -143,7 +142,7 @@ EventNotify::lock(void)
> > bool
> > EventNotify::trylock(void)
> > {
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > return true;
> > #else
> > return ink_mutex_try_acquire(&m_mutex);
> > @@ -153,7 +152,7 @@ EventNotify::trylock(void)
> > void
> > EventNotify::unlock(void)
> > {
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > // do nothing
> > #else
> > ink_mutex_release(&m_mutex);
> > @@ -162,7 +161,7 @@ EventNotify::unlock(void)
> >
> > EventNotify::~EventNotify()
> > {
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > close(m_event_fd);
> > close(m_epoll_fd);
> > #else
> >
> >
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.h
> > ----------------------------------------------------------------------
> > diff --git a/lib/ts/EventNotify.h b/lib/ts/EventNotify.h
> > index 16e4809..135f037 100644
> > --- a/lib/ts/EventNotify.h
> > +++ b/lib/ts/EventNotify.h
>
> Please add header guards to this file.
>
I don't known what is header guards, can you give me an example? :D
>
> > @@ -33,18 +33,17 @@
> > class EventNotify
> > {
> > public:
> > - EventNotify(const char *name = NULL);
> > + EventNotify();
> > void signal(void);
> > void wait(void);
> > - int timedwait(ink_timestruc *abstime);
> > + int timedwait(int timeout); // milliseconds
> > void lock(void);
> > bool trylock(void);
> > void unlock(void);
> > ~EventNotify();
> >
> > private:
> > - const char *m_name;
> > -#ifdef TS_HAS_EVENTFD
> > +#ifdef HAVE_EVENTFD
> > int m_event_fd;
> > int m_epoll_fd;
> > #else
> >
>
>
--
Yunkai Zhang
Work at Taobao
Re: git commit: TS-2137: Use relative timeout in
EventNotify::timedwait()
Posted by James Peach <jp...@apache.org>.
On Aug 22, 2013, at 5:01 AM, yunkai@apache.org wrote:
> Updated Branches:
> refs/heads/master e5d27294b -> fa176442f
>
>
> TS-2137: Use relative timeout in EventNotify::timedwait()
>
> 1) Use relative timeout in EventNotify::timedwait() function.
> 2) Remove unused m_name variable.
> 3) Use the correct marco HAVE_EVENTFD instead of TS_HAS_EVENTFD which
> is outdated.
Good catch!
>
> 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/fa176442
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/fa176442
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/fa176442
>
> Branch: refs/heads/master
> Commit: fa176442f45a3d5e8a9dce332775172192f53f34
> Parents: e5d2729
> Author: Yunkai Zhang <qi...@taobao.com>
> Authored: Thu Aug 22 16:13:36 2013 +0800
> Committer: Yunkai Zhang <qi...@taobao.com>
> Committed: Thu Aug 22 19:56:02 2013 +0800
>
> ----------------------------------------------------------------------
> lib/ts/EventNotify.cc | 39 +++++++++++++++++++--------------------
> lib/ts/EventNotify.h | 7 +++----
> 2 files changed, 22 insertions(+), 24 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.cc
> ----------------------------------------------------------------------
> diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
> index 5d0cc53..3a499b6 100644
> --- a/lib/ts/EventNotify.cc
> +++ b/lib/ts/EventNotify.cc
> @@ -30,14 +30,14 @@
> #include "EventNotify.h"
> #include "ink_hrtime.h"
>
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> #include <sys/eventfd.h>
> #include <sys/epoll.h>
> #endif
>
> -EventNotify::EventNotify(const char *name): m_name(name)
> +EventNotify::EventNotify()
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> int ret;
> struct epoll_event ev;
>
> @@ -59,14 +59,14 @@ EventNotify::EventNotify(const char *name): m_name(name)
> ink_release_assert(ret != -1);
> #else
> ink_cond_init(&m_cond);
> - ink_mutex_init(&m_mutex, m_name);
> + ink_mutex_init(&m_mutex, NULL);
> #endif
> }
>
> void
> EventNotify::signal(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> ssize_t nr;
> uint64_t value = 1;
> nr = write(m_event_fd, &value, sizeof(uint64_t));
> @@ -79,7 +79,7 @@ EventNotify::signal(void)
> void
> EventNotify::wait(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> ssize_t nr;
> uint64_t value = 0;
> nr = read(m_event_fd, &value, sizeof(uint64_t));
> @@ -90,20 +90,13 @@ EventNotify::wait(void)
> }
>
> int
> -EventNotify::timedwait(ink_timestruc *abstime)
> +EventNotify::timedwait(int timeout) // milliseconds
Unless -1 is a meaningful value, I'd prefer this was unsigned.
> {
> -#ifdef TS_HAS_EVENTFD
> - int timeout;
> +#ifdef HAVE_EVENTFD
> ssize_t nr, nr_fd = 0;
> uint64_t value = 0;
> - struct timeval curtime;
> struct epoll_event ev;
>
> - // Convert absolute time to relative time
> - gettimeofday(&curtime, NULL);
> - timeout = (abstime->tv_sec - curtime.tv_sec) * 1000
> - + (abstime->tv_nsec / 1000 - curtime.tv_usec) / 1000;
> -
> //
> // When timeout < 0, epoll_wait() will wait indefinitely, but
> // pthread_cond_timedwait() will return ETIMEDOUT immediately.
> @@ -126,14 +119,20 @@ EventNotify::timedwait(ink_timestruc *abstime)
>
> return 0;
> #else
> - return ink_cond_timedwait(&m_cond, &m_mutex, abstime);
> + ink_timestruc abstime;
> + ink_hrtime curtime;
> +
> + curtime = ink_get_hrtime_internal() + timeout * HRTIME_MSECOND;
> + abstime = ink_based_hrtime_to_timespec(curtime);
> +
> + return ink_cond_timedwait(&m_cond, &m_mutex, &abstime);
> #endif
> }
>
> void
> EventNotify::lock(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> // do nothing
> #else
> ink_mutex_acquire(&m_mutex);
> @@ -143,7 +142,7 @@ EventNotify::lock(void)
> bool
> EventNotify::trylock(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> return true;
> #else
> return ink_mutex_try_acquire(&m_mutex);
> @@ -153,7 +152,7 @@ EventNotify::trylock(void)
> void
> EventNotify::unlock(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> // do nothing
> #else
> ink_mutex_release(&m_mutex);
> @@ -162,7 +161,7 @@ EventNotify::unlock(void)
>
> EventNotify::~EventNotify()
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> close(m_event_fd);
> close(m_epoll_fd);
> #else
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.h
> ----------------------------------------------------------------------
> diff --git a/lib/ts/EventNotify.h b/lib/ts/EventNotify.h
> index 16e4809..135f037 100644
> --- a/lib/ts/EventNotify.h
> +++ b/lib/ts/EventNotify.h
Please add header guards to this file.
> @@ -33,18 +33,17 @@
> class EventNotify
> {
> public:
> - EventNotify(const char *name = NULL);
> + EventNotify();
> void signal(void);
> void wait(void);
> - int timedwait(ink_timestruc *abstime);
> + int timedwait(int timeout); // milliseconds
> void lock(void);
> bool trylock(void);
> void unlock(void);
> ~EventNotify();
>
> private:
> - const char *m_name;
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> int m_event_fd;
> int m_epoll_fd;
> #else
>
Re: git commit: TS-2137: Use relative timeout in
EventNotify::timedwait()
Posted by James Peach <jp...@apache.org>.
On Aug 22, 2013, at 5:01 AM, yunkai@apache.org wrote:
> Updated Branches:
> refs/heads/master e5d27294b -> fa176442f
>
>
> TS-2137: Use relative timeout in EventNotify::timedwait()
>
> 1) Use relative timeout in EventNotify::timedwait() function.
> 2) Remove unused m_name variable.
> 3) Use the correct marco HAVE_EVENTFD instead of TS_HAS_EVENTFD which
> is outdated.
Good catch!
>
> 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/fa176442
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/fa176442
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/fa176442
>
> Branch: refs/heads/master
> Commit: fa176442f45a3d5e8a9dce332775172192f53f34
> Parents: e5d2729
> Author: Yunkai Zhang <qi...@taobao.com>
> Authored: Thu Aug 22 16:13:36 2013 +0800
> Committer: Yunkai Zhang <qi...@taobao.com>
> Committed: Thu Aug 22 19:56:02 2013 +0800
>
> ----------------------------------------------------------------------
> lib/ts/EventNotify.cc | 39 +++++++++++++++++++--------------------
> lib/ts/EventNotify.h | 7 +++----
> 2 files changed, 22 insertions(+), 24 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.cc
> ----------------------------------------------------------------------
> diff --git a/lib/ts/EventNotify.cc b/lib/ts/EventNotify.cc
> index 5d0cc53..3a499b6 100644
> --- a/lib/ts/EventNotify.cc
> +++ b/lib/ts/EventNotify.cc
> @@ -30,14 +30,14 @@
> #include "EventNotify.h"
> #include "ink_hrtime.h"
>
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> #include <sys/eventfd.h>
> #include <sys/epoll.h>
> #endif
>
> -EventNotify::EventNotify(const char *name): m_name(name)
> +EventNotify::EventNotify()
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> int ret;
> struct epoll_event ev;
>
> @@ -59,14 +59,14 @@ EventNotify::EventNotify(const char *name): m_name(name)
> ink_release_assert(ret != -1);
> #else
> ink_cond_init(&m_cond);
> - ink_mutex_init(&m_mutex, m_name);
> + ink_mutex_init(&m_mutex, NULL);
> #endif
> }
>
> void
> EventNotify::signal(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> ssize_t nr;
> uint64_t value = 1;
> nr = write(m_event_fd, &value, sizeof(uint64_t));
> @@ -79,7 +79,7 @@ EventNotify::signal(void)
> void
> EventNotify::wait(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> ssize_t nr;
> uint64_t value = 0;
> nr = read(m_event_fd, &value, sizeof(uint64_t));
> @@ -90,20 +90,13 @@ EventNotify::wait(void)
> }
>
> int
> -EventNotify::timedwait(ink_timestruc *abstime)
> +EventNotify::timedwait(int timeout) // milliseconds
Unless -1 is a meaningful value, I'd prefer this was unsigned.
> {
> -#ifdef TS_HAS_EVENTFD
> - int timeout;
> +#ifdef HAVE_EVENTFD
> ssize_t nr, nr_fd = 0;
> uint64_t value = 0;
> - struct timeval curtime;
> struct epoll_event ev;
>
> - // Convert absolute time to relative time
> - gettimeofday(&curtime, NULL);
> - timeout = (abstime->tv_sec - curtime.tv_sec) * 1000
> - + (abstime->tv_nsec / 1000 - curtime.tv_usec) / 1000;
> -
> //
> // When timeout < 0, epoll_wait() will wait indefinitely, but
> // pthread_cond_timedwait() will return ETIMEDOUT immediately.
> @@ -126,14 +119,20 @@ EventNotify::timedwait(ink_timestruc *abstime)
>
> return 0;
> #else
> - return ink_cond_timedwait(&m_cond, &m_mutex, abstime);
> + ink_timestruc abstime;
> + ink_hrtime curtime;
> +
> + curtime = ink_get_hrtime_internal() + timeout * HRTIME_MSECOND;
> + abstime = ink_based_hrtime_to_timespec(curtime);
> +
> + return ink_cond_timedwait(&m_cond, &m_mutex, &abstime);
> #endif
> }
>
> void
> EventNotify::lock(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> // do nothing
> #else
> ink_mutex_acquire(&m_mutex);
> @@ -143,7 +142,7 @@ EventNotify::lock(void)
> bool
> EventNotify::trylock(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> return true;
> #else
> return ink_mutex_try_acquire(&m_mutex);
> @@ -153,7 +152,7 @@ EventNotify::trylock(void)
> void
> EventNotify::unlock(void)
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> // do nothing
> #else
> ink_mutex_release(&m_mutex);
> @@ -162,7 +161,7 @@ EventNotify::unlock(void)
>
> EventNotify::~EventNotify()
> {
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> close(m_event_fd);
> close(m_epoll_fd);
> #else
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/fa176442/lib/ts/EventNotify.h
> ----------------------------------------------------------------------
> diff --git a/lib/ts/EventNotify.h b/lib/ts/EventNotify.h
> index 16e4809..135f037 100644
> --- a/lib/ts/EventNotify.h
> +++ b/lib/ts/EventNotify.h
Please add header guards to this file.
> @@ -33,18 +33,17 @@
> class EventNotify
> {
> public:
> - EventNotify(const char *name = NULL);
> + EventNotify();
> void signal(void);
> void wait(void);
> - int timedwait(ink_timestruc *abstime);
> + int timedwait(int timeout); // milliseconds
> void lock(void);
> bool trylock(void);
> void unlock(void);
> ~EventNotify();
>
> private:
> - const char *m_name;
> -#ifdef TS_HAS_EVENTFD
> +#ifdef HAVE_EVENTFD
> int m_event_fd;
> int m_epoll_fd;
> #else
>