diff mbox series

[3/4] timer: fix rte_timer_stop_all

Message ID 20220803162651.3145945-1-s.v.naga.harish.k@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers
Series [1/4] eventdev/timer: add periodic event timer support |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing fail Testing issues
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed

Commit Message

Naga Harish K, S V Aug. 3, 2022, 4:26 p.m. UTC 
  there is a possibility of deadlock in this api,
as same spinlock is tried to be acquired in nested manner.

This patch removes the acquisition of nested locking.

Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>
---
 lib/timer/rte_timer.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Stephen Hemminger Aug. 3, 2022, 5:49 p.m. UTC | #1 
On Wed,  3 Aug 2022 11:26:51 -0500
Naga Harish K S V <s.v.naga.harish.k@intel.com> wrote:

> there is a possibility of deadlock in this api,
> as same spinlock is tried to be acquired in nested manner.
> 
> This patch removes the acquisition of nested locking.
> 
> Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>

The wording in this commit message is a little confusing, what is
the exact lock conflict? 

After your patch, there are no longer any callers for __rte_timer_stop()
with the local_is_locked flag. Please resubmit and remove all the
the local_is_locked from __rte_timer_stop().

It looks like the test suite for rte_timer is not exercising all
the exposed API's. That's a problem
Naga Harish K, S V Aug. 10, 2022, 7:20 a.m. UTC | #2 
Hi,
  V2 of the patch is submitted with suggested changes in __rte_timer_stop() function.

-Harish

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, August 3, 2022 11:19 PM
> To: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> Cc: Carrillo, Erik G <erik.g.carrillo@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH 3/4] timer: fix rte_timer_stop_all
> 
> On Wed,  3 Aug 2022 11:26:51 -0500
> Naga Harish K S V <s.v.naga.harish.k@intel.com> wrote:
> 
> > there is a possibility of deadlock in this api, as same spinlock is
> > tried to be acquired in nested manner.
> >
> > This patch removes the acquisition of nested locking.
> >
> > Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>
> 
> The wording in this commit message is a little confusing, what is the exact
> lock conflict?
> 
> After your patch, there are no longer any callers for c with
> the local_is_locked flag. Please resubmit and remove all the the
> local_is_locked from __rte_timer_stop().
> 
> It looks like the test suite for rte_timer is not exercising all the exposed API's.
> That's a problem
diff mbox series

Patch

diff --git a/lib/timer/rte_timer.c b/lib/timer/rte_timer.c
index 9994813d0d..cfbc8cb028 100644
--- a/lib/timer/rte_timer.c
+++ b/lib/timer/rte_timer.c
@@ -987,21 +987,16 @@  rte_timer_stop_all(uint32_t timer_data_id, unsigned int *walk_lcores,
 		walk_lcore = walk_lcores[i];
 		priv_timer = &timer_data->priv_timer[walk_lcore];
 
-		rte_spinlock_lock(&priv_timer->list_lock);
-
 		for (tim = priv_timer->pending_head.sl_next[0];
 		     tim != NULL;
 		     tim = next_tim) {
 			next_tim = tim->sl_next[0];
 
-			/* Call timer_stop with lock held */
-			__rte_timer_stop(tim, 1, timer_data);
+			__rte_timer_stop(tim, 0, timer_data);
 
 			if (f)
 				f(tim, f_arg);
 		}
-
-		rte_spinlock_unlock(&priv_timer->list_lock);
 	}
 
 	return 0;