diff mbox

[dpdk-dev,1/3] timer: inform periodic timers of multiple expiries

Message ID 20170531091621.203189-2-bruce.richardson@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Bruce Richardson May 31, 2017, 9:16 a.m. UTC 
  if timer_manage is called much less frequently than the period of a
periodic timer, then timer expiries will be missed. For example, if a timer
has a period of 300us, but timer_manage is called every 1ms, then there
will only be one timer callback called every 1ms instead of 3 within that
time.

While we can fix this by having each function called multiple times within
timer-manage, this will lead to out-of-order timeouts, and will be slower
with all the function call overheads - especially in the case of a timeout
doing something trivial like incrementing a counter. Therefore, we instead
modify the callback functions to take a counter value of the number of
expiries that have passed since the last time it was called.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> 

--- 
RFC->V1: added in an update of the expiry time after multiple calls, so that
the next expiry is set correctly, rather than being set in the past 

---
examples/l2fwd-jobstats/main.c                     |  7 +++++--
examples/l2fwd-keepalive/main.c                    |  8 ++++----
examples/l3fwd-power/main.c                        |  5 +++--
examples/performance-thread/common/lthread_sched.c |  4 +++-
examples/performance-thread/common/lthread_sched.h |  2 +-
examples/timer/main.c                              | 10 ++++++----
lib/librte_timer/rte_timer.c                       | 13 +++++++++++--
lib/librte_timer/rte_timer.h                       |  2 +-
test/test/test_timer.c                             | 14 +++++++++-----
test/test/test_timer_perf.c                        |  4 +++-
test/test/test_timer_racecond.c                    |  3 ++- 11 files
changed, 48 insertions(+), 24 deletions(-)

Comments

Olivier Matz June 30, 2017, 10:14 a.m. UTC | #1 
Hi Bruce,

On Wed, 31 May 2017 10:16:19 +0100, Bruce Richardson <bruce.richardson@intel.com> wrote:
> if timer_manage is called much less frequently than the period of a
> periodic timer, then timer expiries will be missed. For example, if a timer
> has a period of 300us, but timer_manage is called every 1ms, then there
> will only be one timer callback called every 1ms instead of 3 within that
> time.
> 
> While we can fix this by having each function called multiple times within
> timer-manage, this will lead to out-of-order timeouts, and will be slower
> with all the function call overheads - especially in the case of a timeout
> doing something trivial like incrementing a counter. Therefore, we instead
> modify the callback functions to take a counter value of the number of
> expiries that have passed since the last time it was called.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> 

Sorry, it's probably a bit late to react. If it's too late, nevermind.
I'm not really convinced that adding another argument to the callback
function is the best solution.

Invoking the callbacks several times would result in a much smaller patch
that does not need a heavy ABI compat.

I'm not sure the function call overhead is really significant in that
case. I'm not sure I understand your point related to out-of-order timeouts,
nor I see why this patchset would behave better.

About the problem itself, my understanding was that the timer manage
function has to be called frequently enough to process the timers.


Olivier
Bruce Richardson June 30, 2017, 12:06 p.m. UTC | #2 
On Fri, Jun 30, 2017 at 12:14:31PM +0200, Olivier Matz wrote:
> Hi Bruce,
> 
> On Wed, 31 May 2017 10:16:19 +0100, Bruce Richardson <bruce.richardson@intel.com> wrote:
> > if timer_manage is called much less frequently than the period of a
> > periodic timer, then timer expiries will be missed. For example, if a timer
> > has a period of 300us, but timer_manage is called every 1ms, then there
> > will only be one timer callback called every 1ms instead of 3 within that
> > time.
> > 
> > While we can fix this by having each function called multiple times within
> > timer-manage, this will lead to out-of-order timeouts, and will be slower
> > with all the function call overheads - especially in the case of a timeout
> > doing something trivial like incrementing a counter. Therefore, we instead
> > modify the callback functions to take a counter value of the number of
> > expiries that have passed since the last time it was called.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> 
> 
> Sorry, it's probably a bit late to react. If it's too late, nevermind.
> I'm not really convinced that adding another argument to the callback
> function is the best solution.
> 
> Invoking the callbacks several times would result in a much smaller patch
> that does not need a heavy ABI compat.
> 
Yes, that is true. My first implementation did just that, and I'm not
adverse to that as a possible solution. However, my opinion is that this
is a better solution - primarily as it can let the worker know that it
is very late (from the multiple expiries info).

> I'm not sure the function call overhead is really significant in that
> case.
Yes, you are probably right in many cases. However, for a timeout doing
a simple operations, e.g. updating a counter or two, or setting a flag,
the function call overhead will be significant compared to the work
being done.

> I'm not sure I understand your point related to out-of-order timeouts,
> nor I see why this patchset would behave better.

My problem with the multiple expiries and ordering is that if we call
timeout X multiple times, we should really order those calls with other
timeouts that also need to expire, rather than just calling them three
times in a row. Not doing so seems wrong. By instead passing in the
extra parameter, there is no expectation of ordering of the callbacks,
and the callback function can know itself that it is running very late
and can take appropriate action when needed.

> 
> About the problem itself, my understanding was that the timer manage
> function has to be called frequently enough to process the timers.
> 

Yes. However, if some operation ends up taking a longer than expected
period of time, we should not drop timer expiries.

Anyone else want to weigh in on this problem. Any other opinions as to
which solution people would prefer?

/Bruce
diff mbox

Patch

diff --git a/examples/l2fwd-jobstats/main.c b/examples/l2fwd-jobstats/main.c
index e6e6c22..b264344 100644
--- a/examples/l2fwd-jobstats/main.c
+++ b/examples/l2fwd-jobstats/main.c
@@ -410,7 +410,8 @@  l2fwd_job_update_cb(struct rte_jobstats *job, int64_t result)
 }
 
 static void
-l2fwd_fwd_job(__rte_unused struct rte_timer *timer, void *arg)
+l2fwd_fwd_job(__rte_unused struct rte_timer *timer,
+		__rte_unused unsigned int count, void *arg)
 {
 	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
 	struct rte_mbuf *m;
@@ -460,7 +461,9 @@  l2fwd_fwd_job(__rte_unused struct rte_timer *timer, void *arg)
 }
 
 static void
-l2fwd_flush_job(__rte_unused struct rte_timer *timer, __rte_unused void *arg)
+l2fwd_flush_job(__rte_unused struct rte_timer *timer,
+		__rte_unused unsigned int count,
+		__rte_unused void *arg)
 {
 	uint64_t now;
 	unsigned lcore_id;
diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-keepalive/main.c
index 3745348..e4456ff 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -154,8 +154,9 @@  static void handle_sigterm(__rte_unused int value)
 
 /* Print out statistics on packets dropped */
 static void
-print_stats(__attribute__((unused)) struct rte_timer *ptr_timer,
-	__attribute__((unused)) void *ptr_data)
+print_stats(__rte_unused struct rte_timer *ptr_timer,
+		__rte_unused unsigned int count,
+		__rte_unused void *ptr_data)
 {
 	uint64_t total_packets_dropped, total_packets_tx, total_packets_rx;
 	unsigned portid;
@@ -769,8 +770,7 @@  main(int argc, char **argv)
 				(check_period * rte_get_timer_hz()) / 1000,
 				PERIODICAL,
 				rte_lcore_id(),
-				(void(*)(struct rte_timer*, void*))
-				&rte_keepalive_dispatch_pings,
+				(void *)&rte_keepalive_dispatch_pings,
 				rte_global_keepalive_info
 				) != 0 )
 			rte_exit(EXIT_FAILURE, "Keepalive setup failure.\n");
diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 9d57fde..0c5e81d 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -410,8 +410,9 @@  signal_exit_now(int sigtype)
 
 /*  Freqency scale down timer callback */
 static void
-power_timer_cb(__attribute__((unused)) struct rte_timer *tim,
-			  __attribute__((unused)) void *arg)
+power_timer_cb(__rte_unused struct rte_timer *tim,
+		__rte_unused unsigned int count,
+		__rte_unused void *arg)
 {
 	uint64_t hz;
 	float sleep_time_ratio;
diff --git a/examples/performance-thread/common/lthread_sched.c b/examples/performance-thread/common/lthread_sched.c
index c64c21f..c4c7d3b 100644
--- a/examples/performance-thread/common/lthread_sched.c
+++ b/examples/performance-thread/common/lthread_sched.c
@@ -437,7 +437,9 @@  static inline void _lthread_resume(struct lthread *lt)
  * Handle sleep timer expiry
 */
 void
-_sched_timer_cb(struct rte_timer *tim, void *arg)
+_sched_timer_cb(struct rte_timer *tim,
+		unsigned int count __rte_unused,
+		void *arg)
 {
 	struct lthread *lt = (struct lthread *) arg;
 	uint64_t state = lt->state;
diff --git a/examples/performance-thread/common/lthread_sched.h b/examples/performance-thread/common/lthread_sched.h
index 7cddda9..f2af8b3 100644
--- a/examples/performance-thread/common/lthread_sched.h
+++ b/examples/performance-thread/common/lthread_sched.h
@@ -149,7 +149,7 @@  _reschedule(void)
 }
 
 extern struct lthread_sched *schedcore[];
-void _sched_timer_cb(struct rte_timer *tim, void *arg);
+void _sched_timer_cb(struct rte_timer *tim, unsigned int count, void *arg);
 void _sched_shutdown(__rte_unused void *arg);
 
 #ifdef __cplusplus
diff --git a/examples/timer/main.c b/examples/timer/main.c
index 37ad559..92a6a1f 100644
--- a/examples/timer/main.c
+++ b/examples/timer/main.c
@@ -55,8 +55,9 @@  static struct rte_timer timer1;
 
 /* timer0 callback */
 static void
-timer0_cb(__attribute__((unused)) struct rte_timer *tim,
-	  __attribute__((unused)) void *arg)
+timer0_cb(__rte_unused struct rte_timer *tim,
+	  __rte_unused unsigned int count,
+	  __rte_unused void *arg)
 {
 	static unsigned counter = 0;
 	unsigned lcore_id = rte_lcore_id();
@@ -71,8 +72,9 @@  timer0_cb(__attribute__((unused)) struct rte_timer *tim,
 
 /* timer1 callback */
 static void
-timer1_cb(__attribute__((unused)) struct rte_timer *tim,
-	  __attribute__((unused)) void *arg)
+timer1_cb(__rte_unused struct rte_timer *tim,
+	  __rte_unused unsigned int count,
+	  __rte_unused void *arg)
 {
 	unsigned lcore_id = rte_lcore_id();
 	uint64_t hz;
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 18782fa..2c5d5f3 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -1,7 +1,7 @@ 
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -590,7 +590,16 @@  void rte_timer_manage(void)
 		priv_timer[lcore_id].running_tim = tim;
 
 		/* execute callback function with list unlocked */
-		tim->f(tim, tim->arg);
+		if (tim->period == 0)
+			tim->f(tim, 1, tim->arg);
+		else {
+			/* for periodic check how many expiries we have */
+			uint64_t over_time = cur_time - tim->expire;
+			unsigned int extra_expiries = over_time / tim->period;
+			tim->f(tim, 1 + extra_expiries, tim->arg);
+			/* adjust expiry to last handled expiry time */
+			tim->expire += (extra_expiries * tim->period);
+		}
 
 		__TIMER_STAT_ADD(pending, -1);
 		/* the timer was stopped or reloaded by the callback
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index a276a73..bc434ec 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -117,7 +117,7 @@  struct rte_timer;
 /**
  * Callback function type for timer expiry.
  */
-typedef void (*rte_timer_cb_t)(struct rte_timer *, void *);
+typedef void (*rte_timer_cb_t)(struct rte_timer *, unsigned int count, void *);
 
 #define MAX_SKIPLIST_DEPTH 10
 
diff --git a/test/test/test_timer.c b/test/test/test_timer.c
index 2f6525a..0b86d3c 100644
--- a/test/test/test_timer.c
+++ b/test/test/test_timer.c
@@ -153,7 +153,8 @@  struct mytimerinfo {
 
 static struct mytimerinfo mytiminfo[NB_TIMER];
 
-static void timer_basic_cb(struct rte_timer *tim, void *arg);
+static void
+timer_basic_cb(struct rte_timer *tim, unsigned int count, void *arg);
 
 static void
 mytimer_reset(struct mytimerinfo *timinfo, uint64_t ticks,
@@ -167,6 +168,7 @@  mytimer_reset(struct mytimerinfo *timinfo, uint64_t ticks,
 /* timer callback for stress tests */
 static void
 timer_stress_cb(__attribute__((unused)) struct rte_timer *tim,
+		__attribute__((unused)) unsigned int count,
 		__attribute__((unused)) void *arg)
 {
 	long r;
@@ -293,9 +295,11 @@  static volatile int cb_count = 0;
 /* callback for second stress test. will only be called
  * on master lcore */
 static void
-timer_stress2_cb(struct rte_timer *tim __rte_unused, void *arg __rte_unused)
+timer_stress2_cb(struct rte_timer *tim __rte_unused,
+		unsigned int count,
+		void *arg __rte_unused)
 {
-	cb_count++;
+	cb_count += count;
 }
 
 #define NB_STRESS2_TIMERS 8192
@@ -430,7 +434,7 @@  timer_stress2_main_loop(__attribute__((unused)) void *arg)
 
 /* timer callback for basic tests */
 static void
-timer_basic_cb(struct rte_timer *tim, void *arg)
+timer_basic_cb(struct rte_timer *tim, unsigned int count, void *arg)
 {
 	struct mytimerinfo *timinfo = arg;
 	uint64_t hz = rte_get_timer_hz();
@@ -440,7 +444,7 @@  timer_basic_cb(struct rte_timer *tim, void *arg)
 	if (rte_timer_pending(tim))
 		return;
 
-	timinfo->count ++;
+	timinfo->count += count;
 
 	RTE_LOG(INFO, TESTTIMER,
 		"%"PRIu64": callback id=%u count=%u on core %u\n",
diff --git a/test/test/test_timer_perf.c b/test/test/test_timer_perf.c
index fa77efb..5b3867d 100644
--- a/test/test/test_timer_perf.c
+++ b/test/test/test_timer_perf.c
@@ -48,7 +48,9 @@ 
 int outstanding_count = 0;
 
 static void
-timer_cb(struct rte_timer *t __rte_unused, void *param __rte_unused)
+timer_cb(struct rte_timer *t __rte_unused,
+		unsigned int count __rte_unused,
+		void *param __rte_unused)
 {
 	outstanding_count--;
 }
diff --git a/test/test/test_timer_racecond.c b/test/test/test_timer_racecond.c
index 7824ec4..b1c5c86 100644
--- a/test/test/test_timer_racecond.c
+++ b/test/test/test_timer_racecond.c
@@ -65,7 +65,8 @@  static volatile unsigned stop_slaves;
 static int reload_timer(struct rte_timer *tim);
 
 static void
-timer_cb(struct rte_timer *tim, void *arg __rte_unused)
+timer_cb(struct rte_timer *tim, unsigned int count __rte_unused,
+		void *arg __rte_unused)
 {
 	/* Simulate slow callback function, 100 us. */
 	rte_delay_us(100);