[dpdk-dev] [PATCH 0/2] timer bugs fixes

[dpdk-dev] [PATCH 0/2] timer bugs fixes

[Vadim Suraev]

Vadim Suraev (2):
Bug: when a periodic timer's callback is running, if another      
    timer is manipulated, the periodic timer is not reloaded.    
Solution: set the update flag only if the modified timer is      
    in RUNNING state
Bug: When a timer is running       - if rte_timer_stop is called, the
    pending decrement is       skipped (decremented only if the timer
    is pending) and due       to the update flag the future processing
    is skipped so the       timer is counted as pending while it is
    stopped. - the same       applies when rte_timer_reset is called
    but then the pending       statistics is additionally incremented
    so the timer is       counted pending twice.     
Solution:
    decrement the pending       statistics after returning from the
    callback. If       rte_timer_stop was called, it skipped
    decrementing the       pending statistics. If rte_time_reset was
    called, the       pending statistics was incremented. If neither
    was called       and the timer is periodic, the pending statistics
    is       incremented when it is reloaded

 lib/librte_timer/rte_timer.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[dpdk-dev] [PATCH 1/2] timer bug fix

[Vadim Suraev]

Bug: when a periodic timer's callback is running, if
 another   timer is manipulated, the periodic timer is
 not reloaded. 
Solution: set the update flag only if the
 modified timer is   in RUNNING state

Signed-off-by: Vadim Suraev <vadim.suraev@gmail.com>
---
 lib/librte_timer/rte_timer.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 884ee0e..d07232b 100755
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -378,7 +378,9 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t expire,
 		return -1;
 
 	__TIMER_STAT_ADD(reset, 1);
-	priv_timer[lcore_id].updated = 1;
+	if (prev_status.state == RTE_TIMER_RUNNING) {
+		priv_timer[lcore_id].updated = 1;
+	}
 
 	/* remove it from list */
 	if (prev_status.state == RTE_TIMER_PENDING) {
@@ -453,7 +455,9 @@ rte_timer_stop(struct rte_timer *tim)
 		return -1;
 
 	__TIMER_STAT_ADD(stop, 1);
-	priv_timer[lcore_id].updated = 1;
+	if (prev_status.state == RTE_TIMER_RUNNING) {
+		priv_timer[lcore_id].updated = 1;
+	}
 
 	/* remove it from list */
 	if (prev_status.state == RTE_TIMER_PENDING) {
-- 
1.7.9.5

[dpdk-dev] [PATCH 2/2] timer bug fix

[Vadim Suraev]

Bug: When a timer is running   - if rte_timer_stop is
 called, the pending decrement is   skipped (decremented
 only if the timer is pending) and due   to the update
 flag the future processing is skipped so the   timer is
 counted as pending while it is stopped. - the same  
 applies when rte_timer_reset is called but then the
 pending   statistics is additionally incremented so the
 timer is   counted pending twice. 
Solution: decrement
 the pending   statistics after returning from the
 callback. If   rte_timer_stop was called, it skipped
 decrementing the   pending statistics. If
 rte_time_reset was called, the   pending statistics was
 incremented. If neither was called   and the timer is
 periodic, the pending statistics is   incremented when
 it is reloaded

Signed-off-by: Vadim Suraev <vadim.suraev@gmail.com>
---
 lib/librte_timer/rte_timer.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index d07232b..f0f0c2f 100755
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -551,7 +551,7 @@ void rte_timer_manage(void)
 		tim->f(tim, tim->arg);
 
 		rte_spinlock_lock(&priv_timer[lcore_id].list_lock);
-
+		__TIMER_STAT_ADD(pending, -1);
 		/* the timer was stopped or reloaded by the callback
 		 * function, we have nothing to do here */
 		if (priv_timer[lcore_id].updated == 1)
@@ -559,7 +559,6 @@ void rte_timer_manage(void)
 
 		if (tim->period == 0) {
 			/* remove from done list and mark timer as stopped */
-			__TIMER_STAT_ADD(pending, -1);
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
 			rte_wmb();
@@ -568,6 +567,7 @@ void rte_timer_manage(void)
 		else {
 			/* keep it in list and mark timer as pending */
 			status.state = RTE_TIMER_PENDING;
+			__TIMER_STAT_ADD(pending, 1);
 			status.owner = (int16_t)lcore_id;
 			rte_wmb();
 			tim->status.u32 = status.u32;
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH 2/2] timer bug fix

[Olivier MATZ]

Hi Vadim,

On 05/16/2014 12:15 PM, Vadim Suraev wrote:
> Description: while running a periodic timer's callback, if another
>   timer is manipulated, the updated flag is raised
>   preventing the periodic timer to reload.
> Fix: move
>   updated flag from priv_timer to rte_timer stucture (one
>   per core)
>
> Signed-off-by: Vadim Suraev <vadim.suraev@gmail.com>
 >
> [...]
 >
> --- a/lib/librte_timer/rte_timer.h
> +++ b/lib/librte_timer/rte_timer.h
> @@ -129,6 +129,10 @@ struct rte_timer
>   	uint64_t period;       /**< Period of timer (0 if not periodic). */
>   	rte_timer_cb_t *f;     /**< Callback function. */
>   	void *arg;             /**< Argument to callback function. */
> +        /** per-core variable that true if a timer was updated on this
> +	 *  core since last reset of the variable */
> +	int updated[RTE_MAX_LCORE];
> +
>   };

I don't think that adding a quite large table in the rte_timer
structure is a good idea.

Instead, I suggest to add a new field in the per-core structure
priv_timer:

	struct rte_timer *cur_timer;

This timer pointer is set before invoking the callback of the
timer. Then, you could do this in rte_timer_reset() and
rte_timer_stop():

	if (tim == priv_timer[lcore_id].cur_timer)
		priv_timer[lcore_id].updated = 1;

I think it will also fix the problem you are describing (which
is a real problem), in a more simple way.

Regards,
Olivier

[dpdk-dev] [PATCH 2/2] timer bug fix

[Vadim Suraev]

Description: while running a periodic timer's callback, if another
 timer is manipulated, the updated flag is raised
 preventing the periodic timer to reload. 
Fix: move
 updated flag from priv_timer to rte_timer stucture (one
 per core)

Signed-off-by: Vadim Suraev <vadim.suraev@gmail.com>
---
 lib/librte_timer/rte_timer.c |   17 ++++++++---------
 lib/librte_timer/rte_timer.h |    7 ++++++-
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index f98e904..0cc5fa9 100755
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -58,11 +58,6 @@ LIST_HEAD(rte_timer_list, rte_timer);
 struct priv_timer {
 	struct rte_timer pending_head;  /**< dummy timer instance to head up list */
 	rte_spinlock_t list_lock;       /**< lock to protect list access */
-
-	/** per-core variable that true if a timer was updated on this
-	 *  core since last reset of the variable */
-	int updated;
-
 	/** track the current depth of the skiplist */
 	unsigned curr_skiplist_depth;
 
@@ -107,10 +102,14 @@ void
 rte_timer_init(struct rte_timer *tim)
 {
 	union rte_timer_status status;
+	unsigned lcore_id;
 
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
 	tim->status.u32 = status.u32;
+	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id ++) {
+		tim->updated[lcore_id] = 0;
+	}
 }
 
 /*
@@ -378,7 +377,7 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t expire,
 		return -1;
 
 	__TIMER_STAT_ADD(reset, 1);
-	priv_timer[lcore_id].updated = 1;
+	tim->updated[lcore_id] = 1;
 
 	/* remove it from list */
 	if (prev_status.state == RTE_TIMER_PENDING) {
@@ -453,7 +452,7 @@ rte_timer_stop(struct rte_timer *tim)
 		return -1;
 
 	__TIMER_STAT_ADD(stop, 1);
-	priv_timer[lcore_id].updated = 1;
+	tim->updated[lcore_id] = 1;
 
 	/* remove it from list */
 	if (prev_status.state == RTE_TIMER_PENDING) {
@@ -541,7 +540,7 @@ void rte_timer_manage(void)
 
 		rte_spinlock_unlock(&priv_timer[lcore_id].list_lock);
 
-		priv_timer[lcore_id].updated = 0;
+		tim->updated[lcore_id] = 0;
 
 		/* execute callback function with list unlocked */
 		tim->f(tim, tim->arg);
@@ -550,7 +549,7 @@ void rte_timer_manage(void)
 
 		/* the timer was stopped or reloaded by the callback
 		 * function, we have nothing to do here */
-		if (priv_timer[lcore_id].updated == 1)
+		if (tim->updated[lcore_id] == 1)
 			continue;
 
 		if (tim->period == 0) {
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index c5f936b..235dfd6 100755
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -129,6 +129,10 @@ struct rte_timer
 	uint64_t period;       /**< Period of timer (0 if not periodic). */
 	rte_timer_cb_t *f;     /**< Callback function. */
 	void *arg;             /**< Argument to callback function. */
+        /** per-core variable that true if a timer was updated on this
+	 *  core since last reset of the variable */
+	int updated[RTE_MAX_LCORE];
+
 };
 
 
@@ -142,7 +146,8 @@ struct rte_timer
 	{{RTE_TIMER_STOP, RTE_TIMER_NO_OWNER}}, \
 	0,                                      \
 	NULL,                                   \
-	NULL,                                   \
+	NULL,                                  \
+	{0},                                   \
 	}
 #else
 /**
-- 
1.7.9.5