DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH 1/2] bus/dpaa: fix lock condition during error handling
@ 2024-11-13 16:14 Hemant Agrawal
  2024-11-13 16:14 ` [PATCH 2/2] examples/l2fwd-event: fix spinlock handling Hemant Agrawal
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Hemant Agrawal @ 2024-11-13 16:14 UTC (permalink / raw)
  To: thomas, dev; +Cc: stephen, stable

The error handling is missing FQ unlock code.
Detected by pvr-studio
Bug 89-93: very suspicious synchronization
The analyzer issued a pack of V1020 warnings that a resource
might remain blocked.

Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/dpaa/base/qbman/qman.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c
index 9c90ee25a6..c48fa3e073 100644
--- a/drivers/bus/dpaa/base/qbman/qman.c
+++ b/drivers/bus/dpaa/base/qbman/qman.c
@@ -2138,8 +2138,10 @@ int qman_set_vdq(struct qman_fq *fq, u16 num, uint32_t vdqcr_flags)
 
 	if (!p->vdqcr_owned) {
 		FQLOCK(fq);
-		if (fq_isset(fq, QMAN_FQ_STATE_VDQCR))
+		if (fq_isset(fq, QMAN_FQ_STATE_VDQCR)) {
+			FQUNLOCK(fq);
 			goto escape;
+		}
 		fq_set(fq, QMAN_FQ_STATE_VDQCR);
 		FQUNLOCK(fq);
 		p->vdqcr_owned = fq;
@@ -2172,8 +2174,10 @@ int qman_volatile_dequeue(struct qman_fq *fq, u32 flags __maybe_unused,
 
 	if (!p->vdqcr_owned) {
 		FQLOCK(fq);
-		if (fq_isset(fq, QMAN_FQ_STATE_VDQCR))
+		if (fq_isset(fq, QMAN_FQ_STATE_VDQCR)) {
+			FQUNLOCK(fq);
 			goto escape;
+		}
 		fq_set(fq, QMAN_FQ_STATE_VDQCR);
 		FQUNLOCK(fq);
 		p->vdqcr_owned = fq;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] examples/l2fwd-event: fix spinlock handling
  2024-11-13 16:14 [PATCH 1/2] bus/dpaa: fix lock condition during error handling Hemant Agrawal
@ 2024-11-13 16:14 ` Hemant Agrawal
  2024-11-13 16:48 ` [PATCH 1/2] bus/dpaa: fix lock condition during error handling David Marchand
  2024-11-14  7:44 ` [PATCH v2 1/3] " Hemant Agrawal
  2 siblings, 0 replies; 8+ messages in thread
From: Hemant Agrawal @ 2024-11-13 16:14 UTC (permalink / raw)
  To: thomas, dev; +Cc: stephen, pbhagavatula, stable

Detected by pvr-studio
Bug 89-93: very suspicious synchronization
The analyzer issued a pack of V1020 warnings that a resource
might remain blocked.

Fixes: 080f57bceca4 ("examples/l2fwd-event: add eventdev main loop")
Cc: pbhagavatula@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 examples/l2fwd-event/l2fwd_event.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/l2fwd-event/l2fwd_event.c b/examples/l2fwd-event/l2fwd_event.c
index 22472027b9..416957384b 100644
--- a/examples/l2fwd-event/l2fwd_event.c
+++ b/examples/l2fwd-event/l2fwd_event.c
@@ -141,6 +141,7 @@ l2fwd_get_free_event_port(struct l2fwd_event_resources *evt_rsrc)
 	rte_spinlock_lock(&evt_rsrc->evp.lock);
 	if (index >= evt_rsrc->evp.nb_ports) {
 		printf("No free event port is available\n");
+		rte_spinlock_unlock(&evt_rsrc->evp.lock);
 		return -1;
 	}
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] bus/dpaa: fix lock condition during error handling
  2024-11-13 16:14 [PATCH 1/2] bus/dpaa: fix lock condition during error handling Hemant Agrawal
  2024-11-13 16:14 ` [PATCH 2/2] examples/l2fwd-event: fix spinlock handling Hemant Agrawal
@ 2024-11-13 16:48 ` David Marchand
  2024-11-14  7:45   ` Hemant Agrawal
  2024-11-14  7:44 ` [PATCH v2 1/3] " Hemant Agrawal
  2 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2024-11-13 16:48 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: thomas, dev, stephen, stable

Hello Hemant,

On Wed, Nov 13, 2024 at 5:15 PM Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>
> The error handling is missing FQ unlock code.
> Detected by pvr-studio
> Bug 89-93: very suspicious synchronization
> The analyzer issued a pack of V1020 warnings that a resource
> might remain blocked.
>
> Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
> Cc: stable@dpdk.org
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

The fix looks good to me.

To avoid regressions on this topic, could you look into enabling clang
thread safety check?

I suppose that FQLOCK and FQUNLOCK always come in pairs (seems a safe
assumption), so enabling should be something like:

$ git diff --cached
diff --git a/drivers/bus/dpaa/base/qbman/qman.c
b/drivers/bus/dpaa/base/qbman/qman.c
index 9c90ee25a6..48f181c004 100644
--- a/drivers/bus/dpaa/base/qbman/qman.c
+++ b/drivers/bus/dpaa/base/qbman/qman.c
@@ -27,18 +27,8 @@
  * protection (and indeed, attempting to nest irq-protection doesn't work, as
  * the "irq en/disable" machinery isn't recursive...).
  */
-#define FQLOCK(fq) \
-       do { \
-               struct qman_fq *__fq478 = (fq); \
-               if (fq_isset(__fq478, QMAN_FQ_FLAG_LOCKED)) \
-                       spin_lock(&__fq478->fqlock); \
-       } while (0)
-#define FQUNLOCK(fq) \
-       do { \
-               struct qman_fq *__fq478 = (fq); \
-               if (fq_isset(__fq478, QMAN_FQ_FLAG_LOCKED)) \
-                       spin_unlock(&__fq478->fqlock); \
-       } while (0)
+#define FQLOCK(fq) fq_lock(fq)
+#define FQUNLOCK(fq) fq_unlock(fq)

 static qman_cb_free_mbuf qman_free_mbuf_cb;

@@ -57,6 +47,22 @@ static inline int fq_isset(struct qman_fq *fq, u32 mask)
        return fq->flags & mask;
 }

+static inline void fq_lock(struct qman_fq *fq)
+       __rte_exclusive_lock_function(&fq->fqlock)
+       __rte_no_thread_safety_analysis
+{
+       if (fq_isset(fq, QMAN_FQ_FLAG_LOCKED))
+               spin_lock(&fq->fqlock);
+}
+
+static inline void fq_unlock(struct qman_fq *fq)
+       __rte_unlock_function(&fq->fqlock)
+       __rte_no_thread_safety_analysis
+{
+       if (fq_isset(fq, QMAN_FQ_FLAG_LOCKED))
+               spin_unlock(&fq->fqlock);
+}
+
 static inline int fq_isclear(struct qman_fq *fq, u32 mask)
 {
        return !(fq->flags & mask);
diff --git a/drivers/bus/dpaa/meson.build b/drivers/bus/dpaa/meson.build
index 114e0a2265..5506f2bffc 100644
--- a/drivers/bus/dpaa/meson.build
+++ b/drivers/bus/dpaa/meson.build
@@ -29,5 +29,3 @@ if cc.has_argument('-Wno-pointer-arith')
 endif

 includes += include_directories('include', 'base/qbman')
-
-annotate_locks = false


-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] bus/dpaa: fix lock condition during error handling
  2024-11-13 16:14 [PATCH 1/2] bus/dpaa: fix lock condition during error handling Hemant Agrawal
  2024-11-13 16:14 ` [PATCH 2/2] examples/l2fwd-event: fix spinlock handling Hemant Agrawal
  2024-11-13 16:48 ` [PATCH 1/2] bus/dpaa: fix lock condition during error handling David Marchand
@ 2024-11-14  7:44 ` Hemant Agrawal
  2024-11-14  7:44   ` [PATCH v2 2/3] examples/l2fwd-event: fix spinlock handling Hemant Agrawal
  2024-11-14  7:44   ` [PATCH v2 3/3] bus/dpaa: enabling clang thread safety for locks Hemant Agrawal
  2 siblings, 2 replies; 8+ messages in thread
From: Hemant Agrawal @ 2024-11-14  7:44 UTC (permalink / raw)
  To: thomas, dev, david.marchand; +Cc: stephen, stable

The error handling is missing FQ unlock code.
Detected by pvs-studio
Bug 89-93: very suspicious synchronization
The analyzer issued a pack of V1020 warnings that a resource
might remain blocked.

Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/dpaa/base/qbman/qman.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c
index 9c90ee25a6..c48fa3e073 100644
--- a/drivers/bus/dpaa/base/qbman/qman.c
+++ b/drivers/bus/dpaa/base/qbman/qman.c
@@ -2138,8 +2138,10 @@ int qman_set_vdq(struct qman_fq *fq, u16 num, uint32_t vdqcr_flags)
 
 	if (!p->vdqcr_owned) {
 		FQLOCK(fq);
-		if (fq_isset(fq, QMAN_FQ_STATE_VDQCR))
+		if (fq_isset(fq, QMAN_FQ_STATE_VDQCR)) {
+			FQUNLOCK(fq);
 			goto escape;
+		}
 		fq_set(fq, QMAN_FQ_STATE_VDQCR);
 		FQUNLOCK(fq);
 		p->vdqcr_owned = fq;
@@ -2172,8 +2174,10 @@ int qman_volatile_dequeue(struct qman_fq *fq, u32 flags __maybe_unused,
 
 	if (!p->vdqcr_owned) {
 		FQLOCK(fq);
-		if (fq_isset(fq, QMAN_FQ_STATE_VDQCR))
+		if (fq_isset(fq, QMAN_FQ_STATE_VDQCR)) {
+			FQUNLOCK(fq);
 			goto escape;
+		}
 		fq_set(fq, QMAN_FQ_STATE_VDQCR);
 		FQUNLOCK(fq);
 		p->vdqcr_owned = fq;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] examples/l2fwd-event: fix spinlock handling
  2024-11-14  7:44 ` [PATCH v2 1/3] " Hemant Agrawal
@ 2024-11-14  7:44   ` Hemant Agrawal
  2024-11-18  9:32     ` [EXTERNAL] " Jerin Jacob
  2024-11-14  7:44   ` [PATCH v2 3/3] bus/dpaa: enabling clang thread safety for locks Hemant Agrawal
  1 sibling, 1 reply; 8+ messages in thread
From: Hemant Agrawal @ 2024-11-14  7:44 UTC (permalink / raw)
  To: thomas, dev, david.marchand; +Cc: stephen, pbhagavatula, stable

Detected by pvs-studio
Bug 89-93: very suspicious synchronization
The analyzer issued a pack of V1020 warnings that a resource
might remain blocked.

Fixes: 080f57bceca4 ("examples/l2fwd-event: add eventdev main loop")
Cc: pbhagavatula@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 examples/l2fwd-event/l2fwd_event.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/l2fwd-event/l2fwd_event.c b/examples/l2fwd-event/l2fwd_event.c
index 22472027b9..416957384b 100644
--- a/examples/l2fwd-event/l2fwd_event.c
+++ b/examples/l2fwd-event/l2fwd_event.c
@@ -141,6 +141,7 @@ l2fwd_get_free_event_port(struct l2fwd_event_resources *evt_rsrc)
 	rte_spinlock_lock(&evt_rsrc->evp.lock);
 	if (index >= evt_rsrc->evp.nb_ports) {
 		printf("No free event port is available\n");
+		rte_spinlock_unlock(&evt_rsrc->evp.lock);
 		return -1;
 	}
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] bus/dpaa: enabling clang thread safety for locks
  2024-11-14  7:44 ` [PATCH v2 1/3] " Hemant Agrawal
  2024-11-14  7:44   ` [PATCH v2 2/3] examples/l2fwd-event: fix spinlock handling Hemant Agrawal
@ 2024-11-14  7:44   ` Hemant Agrawal
  1 sibling, 0 replies; 8+ messages in thread
From: Hemant Agrawal @ 2024-11-14  7:44 UTC (permalink / raw)
  To: thomas, dev, david.marchand; +Cc: stephen

To avoid regressions on this topic, could you look into enabling clang
thread safety check?

Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/dpaa/base/qbman/qman.c | 35 ++++++++++++++++--------------
 drivers/bus/dpaa/meson.build       |  2 --
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c
index c48fa3e073..f92b25343a 100644
--- a/drivers/bus/dpaa/base/qbman/qman.c
+++ b/drivers/bus/dpaa/base/qbman/qman.c
@@ -22,23 +22,10 @@
 #define FSL_QMAN_POLL_LIMIT 8
 
 /* Lock/unlock frame queues, subject to the "LOCKED" flag. This is about
- * inter-processor locking only. Note, FQLOCK() is always called either under a
- * local_irq_save() or from interrupt context - hence there's no need for irq
- * protection (and indeed, attempting to nest irq-protection doesn't work, as
- * the "irq en/disable" machinery isn't recursive...).
+ * inter-processor locking only.
  */
-#define FQLOCK(fq) \
-	do { \
-		struct qman_fq *__fq478 = (fq); \
-		if (fq_isset(__fq478, QMAN_FQ_FLAG_LOCKED)) \
-			spin_lock(&__fq478->fqlock); \
-	} while (0)
-#define FQUNLOCK(fq) \
-	do { \
-		struct qman_fq *__fq478 = (fq); \
-		if (fq_isset(__fq478, QMAN_FQ_FLAG_LOCKED)) \
-			spin_unlock(&__fq478->fqlock); \
-	} while (0)
+#define FQLOCK(fq) fq_lock(fq)
+#define FQUNLOCK(fq) fq_unlock(fq)
 
 static qman_cb_free_mbuf qman_free_mbuf_cb;
 
@@ -57,6 +44,22 @@ static inline int fq_isset(struct qman_fq *fq, u32 mask)
 	return fq->flags & mask;
 }
 
+static inline void fq_lock(struct qman_fq *fq)
+	__rte_exclusive_lock_function(&fq->fqlock)
+	__rte_no_thread_safety_analysis
+{
+	if (fq_isset(fq, QMAN_FQ_FLAG_LOCKED))
+		spin_lock(&fq->fqlock);
+}
+
+static inline void fq_unlock(struct qman_fq *fq)
+	 __rte_unlock_function(&fq->fqlock)
+	__rte_no_thread_safety_analysis
+{
+	if (fq_isset(fq, QMAN_FQ_FLAG_LOCKED))
+		spin_unlock(&fq->fqlock);
+}
+
 static inline int fq_isclear(struct qman_fq *fq, u32 mask)
 {
 	return !(fq->flags & mask);
diff --git a/drivers/bus/dpaa/meson.build b/drivers/bus/dpaa/meson.build
index 114e0a2265..5506f2bffc 100644
--- a/drivers/bus/dpaa/meson.build
+++ b/drivers/bus/dpaa/meson.build
@@ -29,5 +29,3 @@ if cc.has_argument('-Wno-pointer-arith')
 endif
 
 includes += include_directories('include', 'base/qbman')
-
-annotate_locks = false
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] bus/dpaa: fix lock condition during error handling
  2024-11-13 16:48 ` [PATCH 1/2] bus/dpaa: fix lock condition during error handling David Marchand
@ 2024-11-14  7:45   ` Hemant Agrawal
  0 siblings, 0 replies; 8+ messages in thread
From: Hemant Agrawal @ 2024-11-14  7:45 UTC (permalink / raw)
  To: David Marchand, Hemant Agrawal; +Cc: thomas, dev, stephen, stable


On 13-11-2024 22:18, David Marchand wrote:
> Hello Hemant,
>
> On Wed, Nov 13, 2024 at 5:15 PM Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>> The error handling is missing FQ unlock code.
>> Detected by pvr-studio
>> Bug 89-93: very suspicious synchronization
>> The analyzer issued a pack of V1020 warnings that a resource
>> might remain blocked.
>>
>> Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> The fix looks good to me.
>
> To avoid regressions on this topic, could you look into enabling clang
> thread safety check?
>
> I suppose that FQLOCK and FQUNLOCK always come in pairs (seems a safe
> assumption), so enabling should be something like:

Thanks for the suggestion and code snippet. It looks good.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [EXTERNAL] [PATCH v2 2/3] examples/l2fwd-event: fix spinlock handling
  2024-11-14  7:44   ` [PATCH v2 2/3] examples/l2fwd-event: fix spinlock handling Hemant Agrawal
@ 2024-11-18  9:32     ` Jerin Jacob
  0 siblings, 0 replies; 8+ messages in thread
From: Jerin Jacob @ 2024-11-18  9:32 UTC (permalink / raw)
  To: Hemant Agrawal, thomas, dev, david.marchand
  Cc: stephen, Pavan Nikhilesh Bhagavatula, stable



> -----Original Message-----
> From: Hemant Agrawal <hemant.agrawal@nxp.com>
> Sent: Thursday, November 14, 2024 1:15 PM
> To: thomas@monjalon.net; dev@dpdk.org; david.marchand@redhat.com
> Cc: stephen@networkplumber.org; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; stable@dpdk.org
> Subject: [EXTERNAL] [PATCH v2 2/3] examples/l2fwd-event: fix spinlock
> handling
> 
> Detected by pvs-studio Bug 89-93: very suspicious synchronization The analyzer
> issued a pack of V1020 warnings that a resource might remain blocked. Fixes:
> 080f57bceca4 ("examples/l2fwd-event: add eventdev main loop") Cc:
> pbhagavatula@ marvell. com 
> Detected by pvs-studio
> Bug 89-93: very suspicious synchronization The analyzer issued a pack of V1020
> warnings that a resource might remain blocked.
> 
> Fixes: 080f57bceca4 ("examples/l2fwd-event: add eventdev main loop")
> Cc: pbhagavatula@marvell.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>


1/3 and 3/3 are  NOT eventdev patches. So I Applied 2/3 to dpdk-next-eventdev/for-main and delegated 1/3 and 3/3 to Thomas.

> ---
>  examples/l2fwd-event/l2fwd_event.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/examples/l2fwd-event/l2fwd_event.c b/examples/l2fwd-
> event/l2fwd_event.c
> index 22472027b9..416957384b 100644
> --- a/examples/l2fwd-event/l2fwd_event.c
> +++ b/examples/l2fwd-event/l2fwd_event.c
> @@ -141,6 +141,7 @@ l2fwd_get_free_event_port(struct
> l2fwd_event_resources *evt_rsrc)
>  	rte_spinlock_lock(&evt_rsrc->evp.lock);
>  	if (index >= evt_rsrc->evp.nb_ports) {
>  		printf("No free event port is available\n");
> +		rte_spinlock_unlock(&evt_rsrc->evp.lock);
>  		return -1;
>  	}
> 
> --
> 2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-11-18  9:32 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-13 16:14 [PATCH 1/2] bus/dpaa: fix lock condition during error handling Hemant Agrawal
2024-11-13 16:14 ` [PATCH 2/2] examples/l2fwd-event: fix spinlock handling Hemant Agrawal
2024-11-13 16:48 ` [PATCH 1/2] bus/dpaa: fix lock condition during error handling David Marchand
2024-11-14  7:45   ` Hemant Agrawal
2024-11-14  7:44 ` [PATCH v2 1/3] " Hemant Agrawal
2024-11-14  7:44   ` [PATCH v2 2/3] examples/l2fwd-event: fix spinlock handling Hemant Agrawal
2024-11-18  9:32     ` [EXTERNAL] " Jerin Jacob
2024-11-14  7:44   ` [PATCH v2 3/3] bus/dpaa: enabling clang thread safety for locks Hemant Agrawal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).