[dpdk-dev] [PATCH 0/2]

[dpdk-dev] [PATCH 0/2]

[Vadim Suraev]

two timer bugs fixed

 lib/librte_timer/rte_timer.c |   21 ++++++++++-----------
 lib/librte_timer/rte_timer.h |    7 ++++++-
 2 files changed, 16 insertions(+), 12 deletions(-)

-- 
1.7.9.5

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

[Vadim Suraev]

Description: when going running, the
 pending count is not incremented. 
Fix: decremend
 pending when going running. Then, if periodic,
 increment, if one shot - do nothing

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 a3d5cca..f98e904 100755
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -173,7 +173,7 @@ timer_set_running_state(struct rte_timer *tim)
 		/* timer is not pending anymore */
 		if (prev_status.state != RTE_TIMER_PENDING)
 			return -1;
-
+		__TIMER_STAT_ADD(pending, -1);
 		/* here, we know that timer is stopped or pending,
 		 * mark it atomically as beeing configured */
 		status.state = RTE_TIMER_RUNNING;
@@ -555,7 +555,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();
@@ -564,6 +563,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

[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

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]

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