[dpdk-dev] [RFC PATCH] timer: inform periodic timers of multiple expiries

[dpdk-dev] [RFC PATCH] timer: inform periodic timers of multiple expiries

[Bruce Richardson]

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>
---
 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                       |  9 ++++++++-
 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, 45 insertions(+), 23 deletions(-)

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 4623d2a..26eba12 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -144,8 +144,9 @@ struct rte_keepalive *rte_global_keepalive_info;
 
 /* 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;
@@ -748,8 +749,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 ec40a17..318aefd 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..d1e2c12 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -590,7 +590,14 @@ 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);
+		}
 
 		__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);
-- 
2.9.3

Re: [dpdk-dev] [RFC PATCH] timer: inform periodic timers of multiple expiries

[Bruce Richardson]

I'd like some agreement soon on the approach to be taken to fix this issue,
in case we need an ABI change notice in 17.05 - i.e. if we take the
approach given in the patch below.

Also, while the alternative solution of calling a function multiple
times is not an ABI/API change, I view it as more problematic from a
compatibility point of view, as it would be a "silent" functionality
change for existing apps.

Regards,
/Bruce

On Fri, Apr 28, 2017 at 02:25:38PM +0100, Bruce Richardson 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>
> ---
>  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                       |  9 ++++++++-
>  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, 45 insertions(+), 23 deletions(-)
> 
> 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 4623d2a..26eba12 100644
> --- a/examples/l2fwd-keepalive/main.c
> +++ b/examples/l2fwd-keepalive/main.c
> @@ -144,8 +144,9 @@ struct rte_keepalive *rte_global_keepalive_info;
>  
>  /* 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;
> @@ -748,8 +749,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 ec40a17..318aefd 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..d1e2c12 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -590,7 +590,14 @@ 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);
> +		}
>  
>  		__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);
> -- 
> 2.9.3
> 

Re: [dpdk-dev] [RFC PATCH] timer: inform periodic timers of multiple expiries

[Bruce Richardson]

+maintainer

On Fri, Apr 28, 2017 at 02:25:38PM +0100, Bruce Richardson 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>
> ---
>  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                       |  9 ++++++++-
>  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, 45 insertions(+), 23 deletions(-)
> 
> 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 4623d2a..26eba12 100644
> --- a/examples/l2fwd-keepalive/main.c
> +++ b/examples/l2fwd-keepalive/main.c
> @@ -144,8 +144,9 @@ struct rte_keepalive *rte_global_keepalive_info;
>  
>  /* 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;
> @@ -748,8 +749,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 ec40a17..318aefd 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..d1e2c12 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -590,7 +590,14 @@ 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);
> +		}
>  
>  		__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);
> -- 
> 2.9.3
> 

[dpdk-dev] [PATCH 0/3] timer: inform periodic timers of multiple expiries

[Bruce Richardson]

For periodic timers, with the current there is no way to know if timer
expiries have been missed between calls to rte_timer_manage(). This
patchset adds in a new parameter to timer callbacks, to give the number of
expiries since the last one. ABI compatibility with previous releases is
kept, and a new unit test for that functionality is added

Bruce Richardson (3):
  timer: inform periodic timers of multiple expiries
  timer: add symbol versions for ABI compatibility
  test/test: add test for multiple timer expiries

 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                       | 88 ++++++++++++++++++++--
 lib/librte_timer/rte_timer.h                       | 29 ++++++-
 lib/librte_timer/rte_timer_version.map             |  9 +++
 test/test/test_timer.c                             | 68 +++++++++++++++--
 test/test/test_timer_perf.c                        |  4 +-
 test/test/test_timer_racecond.c                    |  3 +-
 12 files changed, 203 insertions(+), 34 deletions(-)

-- 
2.9.4

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

[Bruce Richardson]

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(-)

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);
-- 
2.9.4

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

[Olivier Matz]

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

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

[Bruce Richardson]

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

[dpdk-dev] [PATCH 2/3] timer: add symbol versions for ABI compatibility

[Bruce Richardson]

With the change in parameters to the callback function for timers, ABI
compatibility needs to be managed. Do this by adding symbol version
information and adding back in older copies of some functions.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_timer/rte_timer.c           | 79 ++++++++++++++++++++++++++++++----
 lib/librte_timer/rte_timer.h           | 27 ++++++++++--
 lib/librte_timer/rte_timer_version.map |  9 ++++
 3 files changed, 104 insertions(+), 11 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 2c5d5f3..c272643 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -365,7 +365,7 @@ timer_del(struct rte_timer *tim, union rte_timer_status prev_status,
 static int
 __rte_timer_reset(struct rte_timer *tim, uint64_t expire,
 		  uint64_t period, unsigned tim_lcore,
-		  rte_timer_cb_t fct, void *arg,
+		  void *fct, void *arg,
 		  int local_is_locked)
 {
 	union rte_timer_status prev_status, status;
@@ -424,9 +424,9 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t expire,
 
 /* Reset and start the timer associated with the timer handle tim */
 int
-rte_timer_reset(struct rte_timer *tim, uint64_t ticks,
+rte_timer_reset_v20(struct rte_timer *tim, uint64_t ticks,
 		enum rte_timer_type type, unsigned tim_lcore,
-		rte_timer_cb_t fct, void *arg)
+		rte_timer_cb_t_v20 fct, void *arg)
 {
 	uint64_t cur_time = rte_get_timer_cycles();
 	uint64_t period;
@@ -443,17 +443,58 @@ rte_timer_reset(struct rte_timer *tim, uint64_t ticks,
 	return __rte_timer_reset(tim,  cur_time + ticks, period, tim_lcore,
 			  fct, arg, 0);
 }
+VERSION_SYMBOL(rte_timer_reset, _v20, 2.0);
 
 /* loop until rte_timer_reset() succeed */
 void
-rte_timer_reset_sync(struct rte_timer *tim, uint64_t ticks,
+rte_timer_reset_sync_v20(struct rte_timer *tim, uint64_t ticks,
 		     enum rte_timer_type type, unsigned tim_lcore,
-		     rte_timer_cb_t fct, void *arg)
+		     rte_timer_cb_t_v20 fct, void *arg)
+{
+	while (rte_timer_reset_v20(tim, ticks, type, tim_lcore,
+			       fct, arg) != 0)
+		rte_pause();
+}
+VERSION_SYMBOL(rte_timer_reset_sync, _v20, 2.0);
+
+/* Reset and start the timer associated with the timer handle tim */
+int
+rte_timer_reset_v1708(struct rte_timer *tim, uint64_t ticks,
+		enum rte_timer_type type, unsigned int tim_lcore,
+		rte_timer_cb_t fct, void *arg)
+{
+	uint64_t cur_time = rte_get_timer_cycles();
+	uint64_t period;
+
+	if (unlikely((tim_lcore != (unsigned int)LCORE_ID_ANY) &&
+			!rte_lcore_is_enabled(tim_lcore)))
+		return -1;
+
+	if (type == PERIODICAL)
+		period = ticks;
+	else
+		period = 0;
+
+	return __rte_timer_reset(tim,  cur_time + ticks, period, tim_lcore,
+			  fct, arg, 0);
+}
+MAP_STATIC_SYMBOL(int rte_timer_reset(struct rte_timer *tim, uint64_t ticks,
+		    enum rte_timer_type type, unsigned int tim_lcore,
+		    rte_timer_cb_t fct, void *arg), rte_timer_reset_v1708);
+
+/* loop until rte_timer_reset() succeed */
+void
+rte_timer_reset_sync_v1708(struct rte_timer *tim, uint64_t ticks,
+		enum rte_timer_type type, unsigned int tim_lcore,
+		rte_timer_cb_t fct, void *arg)
 {
 	while (rte_timer_reset(tim, ticks, type, tim_lcore,
 			       fct, arg) != 0)
 		rte_pause();
 }
+MAP_STATIC_SYMBOL(void rte_timer_reset_sync(struct rte_timer *tim,
+		uint64_t ticks, enum rte_timer_type type, unsigned int tim_lcore,
+		rte_timer_cb_t fct, void *arg), rte_timer_reset_sync_v1708);
 
 /* Stop the timer associated with the timer handle tim */
 int
@@ -505,8 +546,13 @@ rte_timer_pending(struct rte_timer *tim)
 	return tim->status.state == RTE_TIMER_PENDING;
 }
 
+enum timer_version {
+	TIMER_VERSION_20,
+	TIMER_VERSION_1708,
+};
 /* must be called periodically, run all timer that expired */
-void rte_timer_manage(void)
+static void
+__timer_manage(enum timer_version ver)
 {
 	union rte_timer_status status;
 	struct rte_timer *tim, *next_tim;
@@ -590,9 +636,12 @@ void rte_timer_manage(void)
 		priv_timer[lcore_id].running_tim = tim;
 
 		/* execute callback function with list unlocked */
-		if (tim->period == 0)
+		if (ver == TIMER_VERSION_20) {
+			rte_timer_cb_t_v20 f = (void *)tim->f;
+			f(tim, tim->arg);
+		} else if (tim->period == 0) {
 			tim->f(tim, 1, tim->arg);
-		else {
+		} 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;
@@ -630,6 +679,20 @@ void rte_timer_manage(void)
 	priv_timer[lcore_id].running_tim = NULL;
 }
 
+void
+rte_timer_manage_v20(void)
+{
+	__timer_manage(TIMER_VERSION_20);
+}
+VERSION_SYMBOL(rte_timer_manage, _v20, 2.0);
+
+void
+rte_timer_manage_v1708(void)
+{
+	__timer_manage(TIMER_VERSION_1708);
+}
+MAP_STATIC_SYMBOL(void rte_timer_manage(void), rte_timer_manage_v1708);
+
 /* dump statistics about timers */
 void rte_timer_dump_stats(FILE *f)
 {
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index bc434ec..049af72 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -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
@@ -67,6 +67,7 @@
 #include <stdint.h>
 #include <stddef.h>
 #include <rte_common.h>
+#include <rte_compat.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -224,7 +225,7 @@ void rte_timer_init(struct rte_timer *tim);
 int rte_timer_reset(struct rte_timer *tim, uint64_t ticks,
 		    enum rte_timer_type type, unsigned tim_lcore,
 		    rte_timer_cb_t fct, void *arg);
-
+BIND_DEFAULT_SYMBOL(rte_timer_reset, _v1708, 17.08);
 
 /**
  * Loop until rte_timer_reset() succeeds.
@@ -256,6 +257,7 @@ void
 rte_timer_reset_sync(struct rte_timer *tim, uint64_t ticks,
 		     enum rte_timer_type type, unsigned tim_lcore,
 		     rte_timer_cb_t fct, void *arg);
+BIND_DEFAULT_SYMBOL(rte_timer_reset_sync, _v1708, 17.08);
 
 /**
  * Stop a timer.
@@ -321,7 +323,7 @@ int rte_timer_pending(struct rte_timer *tim);
  * CPU resources it will use.
  */
 void rte_timer_manage(void);
-
+BIND_DEFAULT_SYMBOL(rte_timer_manage, _v1708, 17.08);
 /**
  * Dump statistics about timers.
  *
@@ -330,6 +332,25 @@ void rte_timer_manage(void);
  */
 void rte_timer_dump_stats(FILE *f);
 
+/* legacy definition for ABI compatibility */
+typedef void (*rte_timer_cb_t_v20)(struct rte_timer *, void *);
+
+/* prototypes for versioned functions for ABI compatibility */
+int rte_timer_reset_v20(struct rte_timer *tim, uint64_t ticks,
+		enum rte_timer_type type, unsigned int tim_lcore,
+		rte_timer_cb_t_v20 fct, void *arg);
+void rte_timer_reset_sync_v20(struct rte_timer *tim, uint64_t ticks,
+		enum rte_timer_type type, unsigned int tim_lcore,
+		rte_timer_cb_t_v20 fct, void *arg);
+int rte_timer_reset_v1708(struct rte_timer *tim, uint64_t ticks,
+		enum rte_timer_type type, unsigned int tim_lcore,
+		rte_timer_cb_t fct, void *arg);
+void rte_timer_reset_sync_v1708(struct rte_timer *tim, uint64_t ticks,
+		enum rte_timer_type type, unsigned int tim_lcore,
+		rte_timer_cb_t fct, void *arg);
+void rte_timer_manage_v20(void);
+void rte_timer_manage_v1708(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_timer/rte_timer_version.map b/lib/librte_timer/rte_timer_version.map
index 9b2e4b8..06553d1 100644
--- a/lib/librte_timer/rte_timer_version.map
+++ b/lib/librte_timer/rte_timer_version.map
@@ -13,3 +13,12 @@ DPDK_2.0 {
 
 	local: *;
 };
+
+DPDK_17.08 {
+	global:
+
+	rte_timer_reset;
+	rte_timer_reset_sync;
+	rte_timer_manage;
+
+} DPDK_2.0;
-- 
2.9.4

[dpdk-dev] [PATCH 3/3] test/test: add test for multiple timer expiries

[Bruce Richardson]

The API for the rte_timer callbacks has been modified to allow us to return
the number of times a periodic callback would have been called. Test this
capability by adding a new unit test.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 test/test/test_timer.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/test/test/test_timer.c b/test/test/test_timer.c
index 0b86d3c..299171c 100644
--- a/test/test/test_timer.c
+++ b/test/test/test_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
@@ -565,6 +565,54 @@ timer_sanity_check(void)
 }
 
 static int
+test_multiple_timeouts(void)
+{
+	struct rte_timer pt; /* a periodic timer */
+	uint64_t hz = rte_get_timer_hz();
+
+	RTE_LOG(INFO, TESTTIMER, "Running multiple-expiry tests\n");
+
+	rte_timer_init(&pt);
+	/* set timer for 1/100th of a second */
+	rte_timer_reset(&pt, hz/100, PERIODICAL, rte_lcore_id(),
+			timer_stress2_cb, NULL);
+	cb_count = 0;
+
+	/* delay for 1/10th second */
+	rte_delay_us(100000);
+	rte_timer_manage();
+	if (cb_count != 10) {
+		RTE_LOG(ERR, TESTTIMER,
+				"Unexpected callback count. Expected 10, got %d\n",
+				cb_count);
+		return -1;
+	}
+
+	/* should be no further expiries just yet */
+	rte_timer_manage();
+	if (cb_count > 10) {
+		RTE_LOG(ERR, TESTTIMER,
+				"Premature callbacks got. Expected only 10, got %d\n",
+				cb_count);
+		return -1;
+	}
+
+	/* delay 2/100ths of a second and check for two more expiries */
+	rte_delay_us(20000);
+	rte_timer_manage();
+	if (cb_count != 12) {
+		RTE_LOG(ERR, TESTTIMER,
+				"Unexpected callback count. Expected 12, got %d\n",
+				cb_count);
+		return -1;
+	}
+
+	RTE_LOG(INFO, TESTTIMER, "Multiple-expiry tests passed\n");
+
+	return 0;
+}
+
+static int
 test_timer(void)
 {
 	unsigned i;
@@ -625,6 +673,10 @@ test_timer(void)
 		rte_timer_stop_sync(&mytiminfo[i].tim);
 	}
 
+	/* sanity test for multiple timeouts between timer_manage calls */
+	if (test_multiple_timeouts() < 0)
+		return -1;
+
 	rte_timer_dump_stats(stdout);
 
 	return TEST_SUCCESS;
-- 
2.9.4