[dpdk-dev] [PATCH] timer bug fix.

[dpdk-dev] [PATCH] 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 1ebd223..7035bed 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] timer bug fix.

[Olivier MATZ]

Acked-by: Olivier Matz <olivier.matz@6wind.com>

On 05/21/2014 10:35 PM, Vadim Suraev wrote:
> 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 1ebd223..7035bed 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;
>

Re: [dpdk-dev] [PATCH] timer bug fix.

[Thomas Monjalon]

> > 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>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied for version 1.7.0 with title:
	timer: fix pending counter

Thanks
-- 
Thomas

[dpdk-dev] [PATCH] 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 is 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..1ebd223 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

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

[Olivier MATZ]

Hi Vadim,

It's even more simple that what I've suggested. It should
work since the only case where state is RTE_TIMER_RUNNING
is that we are modifying the timer currently running on the
same lcore. Indeed, timer_set_config_state() prevents us to
modify a running timer belonging to another lcore.

Just 3 small typos:

On 05/21/2014 09:53 PM, Vadim Suraev wrote:
> 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 is the modified timer is
>   in RUNNING state

s/is the modified/if the modified

> -	priv_timer[lcore_id].updated = 1;
> +	if(prev_status.state == RTE_TIMER_RUNNING) {
> +		priv_timer[lcore_id].updated = 1;
> +	}

missing a space after the if.

> -	priv_timer[lcore_id].updated = 1;
> +	if(prev_status.state == RTE_TIMER_RUNNING) {
> +		priv_timer[lcore_id].updated = 1;
> +	}

same here.

Acked-by: Olivier Matz <olivier.matz@6wind.com>

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

[Thomas Monjalon]

2014-05-23 16:52, Olivier MATZ:
> > 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 is the modified timer is
> >  in RUNNING state
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied in version 1.7.0 with title:
	timer: fix reloading after changes

Thanks
-- 
Thomas