[PATCH 0/2]

[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

[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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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

[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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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: [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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
 >
> [...]
 >
> --- 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

[PATCH 0/2]

[Vadim Suraev]

rte_timer: 2 bug fixes

Vadim Suraev (2):
  rte_timer bug fix: pending timers count is not  decremented when
    going running. Fix: decrement pending when going running, increment
    if reloaded, do nothing if not reloaded     Signed-off-by: Vadim
    Suraev <vadim.suraev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  rte_timer bug fix: when a perdiodic timer's callback manipulates
    another timer, updated flag prevents reloading the periodic timer.
    Fix: move  updated flag to rte_timer structure (one per core)    
    Signed-off-by: Vadim Suraev <vadim.suraev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

 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

Re: [PATCH 0/2]

[Thomas Monjalon]

Hi Vadim,

Your new patchset trial is better but not correct:
- all is in the title.
- the title must be short and begin with "timer:" as it's done for previous 
timer patches
- you must describe the bug you are fixing

I think you'll understand the problem by looking at the archives:
http://dpdk.org/ml/archives/dev/2014-May/002466.html

Please try again
-- 
Thomas