diff mbox

[dpdk-dev] rte_alarm: modify it to make it not to be affected by discontinuous jumps in the system time

Message ID 1433472396-18852-1-git-send-email-wolkayang@gmail.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Wen-Chi Yang June 5, 2015, 2:46 a.m. UTC 
  Due to eal_alarm_callback() and rte_eal_alarm_set() use gettimeofday()
to get the current time, and gettimeofday() is affected by jumps.

For example, set up a rte_alarm which will be triggerd next second (
current time + 1 second) by rte_eal_alarm_set(). And the callback
function of this rte_alarm sets up another rte_alarm which will be
triggered next second (current time + 2 second).
Once we change the system time when the callback function is triggered,
it is possiblb that rte alarm functionalities work out of expectation.

Replace gettimeofday() with clock_gettime(CLOCK_MONOTONIC_RAW, &now)
could avoid this phenomenon.

Signed-off-by: Wen-Chi Yang <wolkayang@gmail.com>
---
 lib/librte_eal/linuxapp/eal/eal_alarm.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Stephen Hemminger Oct. 14, 2015, 12:33 a.m. UTC | #1 
On Fri,  5 Jun 2015 10:46:36 +0800
Wen-Chi Yang <wolkayang@gmail.com> wrote:

> Due to eal_alarm_callback() and rte_eal_alarm_set() use gettimeofday()
> to get the current time, and gettimeofday() is affected by jumps.
> 
> For example, set up a rte_alarm which will be triggerd next second (
> current time + 1 second) by rte_eal_alarm_set(). And the callback
> function of this rte_alarm sets up another rte_alarm which will be
> triggered next second (current time + 2 second).
> Once we change the system time when the callback function is triggered,
> it is possiblb that rte alarm functionalities work out of expectation.
> 
> Replace gettimeofday() with clock_gettime(CLOCK_MONOTONIC_RAW, &now)
> could avoid this phenomenon.
> 
> Signed-off-by: Wen-Chi Yang <wolkayang@gmail.com>

Agreed, this should be applied.
Does BSD version have same problem?

Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Jay Rolette Oct. 14, 2015, 12:09 p.m. UTC | #2 
Back when this was first submitted in June, I mentioned that
CLOCK_MONOTONIC_RAW was ~10x slower than CLOCK_MONOTONIC:

http://dpdk.org/ml/archives/dev/2015-June/018687.html

It's not completely free from NTP frequency adjustments, but it won't have
any discontinuities.

That's what we've been using in our tree since then...

Jay


On Tue, Oct 13, 2015 at 7:33 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Fri,  5 Jun 2015 10:46:36 +0800
> Wen-Chi Yang <wolkayang@gmail.com> wrote:
>
> > Due to eal_alarm_callback() and rte_eal_alarm_set() use gettimeofday()
> > to get the current time, and gettimeofday() is affected by jumps.
> >
> > For example, set up a rte_alarm which will be triggerd next second (
> > current time + 1 second) by rte_eal_alarm_set(). And the callback
> > function of this rte_alarm sets up another rte_alarm which will be
> > triggered next second (current time + 2 second).
> > Once we change the system time when the callback function is triggered,
> > it is possiblb that rte alarm functionalities work out of expectation.
> >
> > Replace gettimeofday() with clock_gettime(CLOCK_MONOTONIC_RAW, &now)
> > could avoid this phenomenon.
> >
> > Signed-off-by: Wen-Chi Yang <wolkayang@gmail.com>
>
> Agreed, this should be applied.
> Does BSD version have same problem?
>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
>
>
Thomas Monjalon Oct. 21, 2015, 2:57 p.m. UTC | #3 
2015-10-13 17:33, Stephen Hemminger:
> Agreed, this should be applied.
> Does BSD version have same problem?

Implementation of rte_alarm for BSD is empty.
So I would say there is no such problem ;)
Thomas Monjalon Oct. 21, 2015, 3 p.m. UTC | #4 
2015-10-14 07:09, Jay Rolette:
> Back when this was first submitted in June, I mentioned that
> CLOCK_MONOTONIC_RAW was ~10x slower than CLOCK_MONOTONIC:
> 
> http://dpdk.org/ml/archives/dev/2015-June/018687.html
> 
> It's not completely free from NTP frequency adjustments, but it won't have
> any discontinuities.
> 
> That's what we've been using in our tree since then...

This patch will be applied with CLOCK_MONOTONIC_RAW.
Please send a patch to change it if relevant.
Thomas Monjalon Oct. 21, 2015, 3:10 p.m. UTC | #5 
2015-10-13 17:33, Stephen Hemminger:
> On Fri,  5 Jun 2015 10:46:36 +0800
> Wen-Chi Yang <wolkayang@gmail.com> wrote:
> 
> > Due to eal_alarm_callback() and rte_eal_alarm_set() use gettimeofday()
> > to get the current time, and gettimeofday() is affected by jumps.
> > 
> > For example, set up a rte_alarm which will be triggerd next second (
> > current time + 1 second) by rte_eal_alarm_set(). And the callback
> > function of this rte_alarm sets up another rte_alarm which will be
> > triggered next second (current time + 2 second).
> > Once we change the system time when the callback function is triggered,
> > it is possiblb that rte alarm functionalities work out of expectation.
> > 
> > Replace gettimeofday() with clock_gettime(CLOCK_MONOTONIC_RAW, &now)
> > could avoid this phenomenon.
> > 
> > Signed-off-by: Wen-Chi Yang <wolkayang@gmail.com>
> 
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Applied, thanks
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_alarm.c b/lib/librte_eal/linuxapp/eal/eal_alarm.c
index a0eae1e..ff57323 100644
--- a/lib/librte_eal/linuxapp/eal/eal_alarm.c
+++ b/lib/librte_eal/linuxapp/eal/eal_alarm.c
@@ -99,14 +99,14 @@  static void
 eal_alarm_callback(struct rte_intr_handle *hdl __rte_unused,
 		void *arg __rte_unused)
 {
-	struct timeval now;
+	struct timespec now;
 	struct alarm_entry *ap;
 
 	rte_spinlock_lock(&alarm_list_lk);
 	while ((ap = LIST_FIRST(&alarm_list)) !=NULL &&
-			gettimeofday(&now, NULL) == 0 &&
+			clock_gettime(CLOCK_MONOTONIC_RAW, &now) == 0 &&
 			(ap->time.tv_sec < now.tv_sec || (ap->time.tv_sec == now.tv_sec &&
-						ap->time.tv_usec <= now.tv_usec))){
+						(ap->time.tv_usec * NS_PER_US) <= now.tv_nsec))) {
 		ap->executing = 1;
 		ap->executing_id = pthread_self();
 		rte_spinlock_unlock(&alarm_list_lk);
@@ -126,11 +126,11 @@  eal_alarm_callback(struct rte_intr_handle *hdl __rte_unused,
 		atime.it_value.tv_sec = ap->time.tv_sec;
 		atime.it_value.tv_nsec = ap->time.tv_usec * NS_PER_US;
 		/* perform borrow for subtraction if necessary */
-		if (now.tv_usec > ap->time.tv_usec)
+		if (now.tv_nsec > (ap->time.tv_usec * NS_PER_US))
 			atime.it_value.tv_sec--, atime.it_value.tv_nsec += US_PER_S * NS_PER_US;
 
 		atime.it_value.tv_sec -= now.tv_sec;
-		atime.it_value.tv_nsec -= now.tv_usec * NS_PER_US;
+		atime.it_value.tv_nsec -= now.tv_nsec;
 		timerfd_settime(intr_handle.fd, 0, &atime, NULL);
 	}
 	rte_spinlock_unlock(&alarm_list_lk);
@@ -139,7 +139,7 @@  eal_alarm_callback(struct rte_intr_handle *hdl __rte_unused,
 int
 rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 {
-	struct timeval now;
+	struct timespec now;
 	int ret = 0;
 	struct alarm_entry *ap, *new_alarm;
 
@@ -152,12 +152,12 @@  rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 		return -ENOMEM;
 
 	/* use current time to calculate absolute time of alarm */
-	gettimeofday(&now, NULL);
+	clock_gettime(CLOCK_MONOTONIC_RAW, &now);
 
 	new_alarm->cb_fn = cb_fn;
 	new_alarm->cb_arg = cb_arg;
-	new_alarm->time.tv_usec = (now.tv_usec + us) % US_PER_S;
-	new_alarm->time.tv_sec = now.tv_sec + ((now.tv_usec + us) / US_PER_S);
+	new_alarm->time.tv_usec = ((now.tv_nsec / NS_PER_US) + us) % US_PER_S;
+	new_alarm->time.tv_sec = now.tv_sec + (((now.tv_nsec / NS_PER_US) + us) / US_PER_S);
 
 	rte_spinlock_lock(&alarm_list_lk);
 	if (!handler_registered) {