diff mbox series

[v2] net/mlx5: use just sufficient barrier for Arm platforms

Message ID 20220830200038.1694160-1-honnappa.nagarahalli@arm.com (mailing list archive)
State New
Delegated to: Raslan Darawsheh
Headers show
Series [v2] net/mlx5: use just sufficient barrier for Arm platforms | expand

Checks

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

Commit Message

Honnappa Nagarahalli Aug. 30, 2022, 8 p.m. UTC
cqe->op_own indicates if the CQE is owned by the NIC. The rest of
the fields in CQE should be read only after op_own is read. On Arm
platforms using "dmb ishld" is sufficient to enforce this.

Fixes: 88c0733535d6 ("net/mlx5: extend Rx completion with error handling")
Cc: matan@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 drivers/common/mlx5/mlx5_common.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Slava Ovsiienko Sept. 27, 2022, 6:34 a.m. UTC | #1
Hi, Honnappa

We discussed the barrier here:
http://patches.dpdk.org/project/dpdk/patch/20210606164948.35997-1-honnappa.nagarahalli@arm.com/

(BTW, it is good practice to keep the reference to previous patch versions below Commit Message of the next ones).

This barrier is not about compiler ordering, it is about external HW agent memory action completions.
So, I'm not sure the rte_atomic_thread_fence() is safe for x86 - patch impacts x86 as well.

With best regards,
Slava

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Tuesday, August 30, 2022 23:01
> To: dev@dpdk.org; honnappa.nagarahalli@arm.com; ruifeng.wang@arm.com; Matan
> Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>
> Cc: nd@arm.com; Matan Azrad <matan@nvidia.com>; stable@dpdk.org
> Subject: [PATCH v2] net/mlx5: use just sufficient barrier for Arm platforms
> 
> cqe->op_own indicates if the CQE is owned by the NIC. The rest of
> the fields in CQE should be read only after op_own is read. On Arm platforms
> using "dmb ishld" is sufficient to enforce this.
> 
> Fixes: 88c0733535d6 ("net/mlx5: extend Rx completion with error handling")
> Cc: matan@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  drivers/common/mlx5/mlx5_common.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_common.h
> b/drivers/common/mlx5/mlx5_common.h
> index 5028a05b49..ac2e85b15f 100644
> --- a/drivers/common/mlx5/mlx5_common.h
> +++ b/drivers/common/mlx5/mlx5_common.h
> @@ -195,7 +195,11 @@ check_cqe(volatile struct mlx5_cqe *cqe, const uint16_t
> cqes_n,
> 
>  	if (unlikely((op_owner != (!!(idx))) || (op_code ==
> MLX5_CQE_INVALID)))
>  		return MLX5_CQE_STATUS_HW_OWN;
> -	rte_io_rmb();
> +	/* Prevent speculative reading of other fields in CQE until
> +	 * CQE is valid.
> +	 */
> +	rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
> +
>  	if (unlikely(op_code == MLX5_CQE_RESP_ERR ||
>  		     op_code == MLX5_CQE_REQ_ERR))
>  		return MLX5_CQE_STATUS_ERR;
> --
> 2.17.1
Honnappa Nagarahalli Sept. 27, 2022, 9:03 p.m. UTC | #2
<snip>

> 
> Hi, Honnappa
Hi Slava, thanks for the feedback.

> 
> We discussed the barrier here:
> http://patches.dpdk.org/project/dpdk/patch/20210606164948.35997-1-
> honnappa.nagarahalli@arm.com/
Yes, I have changed the patch according to the discussion. i.e. barrier is needed, but different (inner sharable domain) barrier is required.

> 
> (BTW, it is good practice to keep the reference to previous patch versions
> below Commit Message of the next ones).
> 
> This barrier is not about compiler ordering, it is about external HW agent
> memory action completions.
> So, I'm not sure the rte_atomic_thread_fence() is safe for x86 - patch impacts
> x86 as well.
The earlier barrier 'rte_io_rmb()', resolves to a compiler barrier on x86 [1]. The rte_atomic_thread_fence(__ATOMIC_ACQUIRE) on x86 also acts as a compiler barrier. So, there is no change for x86.


[1] https://github.com/DPDK/dpdk/blob/main/lib/eal/x86/include/rte_atomic.h#L80

> 
> With best regards,
> Slava
> 
> > -----Original Message-----
> > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Sent: Tuesday, August 30, 2022 23:01
> > To: dev@dpdk.org; honnappa.nagarahalli@arm.com;
> ruifeng.wang@arm.com;
> > Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
> > Slava Ovsiienko <viacheslavo@nvidia.com>
> > Cc: nd@arm.com; Matan Azrad <matan@nvidia.com>; stable@dpdk.org
> > Subject: [PATCH v2] net/mlx5: use just sufficient barrier for Arm
> > platforms
> >
> > cqe->op_own indicates if the CQE is owned by the NIC. The rest of
> > the fields in CQE should be read only after op_own is read. On Arm
> > platforms using "dmb ishld" is sufficient to enforce this.
> >
> > Fixes: 88c0733535d6 ("net/mlx5: extend Rx completion with error
> > handling")
> > Cc: matan@mellanox.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  drivers/common/mlx5/mlx5_common.h | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/common/mlx5/mlx5_common.h
> > b/drivers/common/mlx5/mlx5_common.h
> > index 5028a05b49..ac2e85b15f 100644
> > --- a/drivers/common/mlx5/mlx5_common.h
> > +++ b/drivers/common/mlx5/mlx5_common.h
> > @@ -195,7 +195,11 @@ check_cqe(volatile struct mlx5_cqe *cqe, const
> > uint16_t cqes_n,
> >
> >  	if (unlikely((op_owner != (!!(idx))) || (op_code ==
> > MLX5_CQE_INVALID)))
> >  		return MLX5_CQE_STATUS_HW_OWN;
> > -	rte_io_rmb();
> > +	/* Prevent speculative reading of other fields in CQE until
> > +	 * CQE is valid.
> > +	 */
> > +	rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
> > +
> >  	if (unlikely(op_code == MLX5_CQE_RESP_ERR ||
> >  		     op_code == MLX5_CQE_REQ_ERR))
> >  		return MLX5_CQE_STATUS_ERR;
> > --
> > 2.17.1
Honnappa Nagarahalli Sept. 27, 2022, 9:06 p.m. UTC | #3
<snip>

> 
> Hi, Honnappa
> 
> We discussed the barrier here:
> http://patches.dpdk.org/project/dpdk/patch/20210606164948.35997-1-
> honnappa.nagarahalli@arm.com/
> 
> (BTW, it is good practice to keep the reference to previous patch versions
> below Commit Message of the next ones).
Apologies, I did not understand this. I would like to fix this if I can understand it better.

> 
> This barrier is not about compiler ordering, it is about external HW agent
> memory action completions.
> So, I'm not sure the rte_atomic_thread_fence() is safe for x86 - patch impacts
> x86 as well.
> 
> With best regards,
> Slava
> 
> > -----Original Message-----
> > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Sent: Tuesday, August 30, 2022 23:01
> > To: dev@dpdk.org; honnappa.nagarahalli@arm.com;
> ruifeng.wang@arm.com;
> > Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
> > Slava Ovsiienko <viacheslavo@nvidia.com>
> > Cc: nd@arm.com; Matan Azrad <matan@nvidia.com>; stable@dpdk.org
> > Subject: [PATCH v2] net/mlx5: use just sufficient barrier for Arm
> > platforms
> >
> > cqe->op_own indicates if the CQE is owned by the NIC. The rest of
> > the fields in CQE should be read only after op_own is read. On Arm
> > platforms using "dmb ishld" is sufficient to enforce this.
> >
> > Fixes: 88c0733535d6 ("net/mlx5: extend Rx completion with error
> > handling")
> > Cc: matan@mellanox.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  drivers/common/mlx5/mlx5_common.h | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/common/mlx5/mlx5_common.h
> > b/drivers/common/mlx5/mlx5_common.h
> > index 5028a05b49..ac2e85b15f 100644
> > --- a/drivers/common/mlx5/mlx5_common.h
> > +++ b/drivers/common/mlx5/mlx5_common.h
> > @@ -195,7 +195,11 @@ check_cqe(volatile struct mlx5_cqe *cqe, const
> > uint16_t cqes_n,
> >
> >  	if (unlikely((op_owner != (!!(idx))) || (op_code ==
> > MLX5_CQE_INVALID)))
> >  		return MLX5_CQE_STATUS_HW_OWN;
> > -	rte_io_rmb();
> > +	/* Prevent speculative reading of other fields in CQE until
> > +	 * CQE is valid.
> > +	 */
> > +	rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
> > +
> >  	if (unlikely(op_code == MLX5_CQE_RESP_ERR ||
> >  		     op_code == MLX5_CQE_REQ_ERR))
> >  		return MLX5_CQE_STATUS_ERR;
> > --
> > 2.17.1
Honnappa Nagarahalli Nov. 15, 2022, 1:45 a.m. UTC | #4
<snip>

> 
> >
> > Hi, Honnappa
> Hi Slava, thanks for the feedback.
> 
> >
> > We discussed the barrier here:
> > http://patches.dpdk.org/project/dpdk/patch/20210606164948.35997-1-
> > honnappa.nagarahalli@arm.com/
> Yes, I have changed the patch according to the discussion. i.e. barrier is
> needed, but different (inner sharable domain) barrier is required.
> 
> >
> > (BTW, it is good practice to keep the reference to previous patch
> > versions below Commit Message of the next ones).
> >
> > This barrier is not about compiler ordering, it is about external HW
> > agent memory action completions.
> > So, I'm not sure the rte_atomic_thread_fence() is safe for x86 - patch
> > impacts
> > x86 as well.
> The earlier barrier 'rte_io_rmb()', resolves to a compiler barrier on x86 [1].
> The rte_atomic_thread_fence(__ATOMIC_ACQUIRE) on x86 also acts as a
> compiler barrier. So, there is no change for x86.
> 
> 
> [1]
> https://github.com/DPDK/dpdk/blob/main/lib/eal/x86/include/rte_atomic.h#
> L80
Hi Slava, any more comments on this?

> 
> >
> > With best regards,
> > Slava
> >
> > > -----Original Message-----
> > > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Sent: Tuesday, August 30, 2022 23:01
> > > To: dev@dpdk.org; honnappa.nagarahalli@arm.com;
> > ruifeng.wang@arm.com;
> > > Matan Azrad <matan@nvidia.com>; Shahaf Shuler
> <shahafs@nvidia.com>;
> > > Slava Ovsiienko <viacheslavo@nvidia.com>
> > > Cc: nd@arm.com; Matan Azrad <matan@nvidia.com>; stable@dpdk.org
> > > Subject: [PATCH v2] net/mlx5: use just sufficient barrier for Arm
> > > platforms
> > >
> > > cqe->op_own indicates if the CQE is owned by the NIC. The rest of
> > > the fields in CQE should be read only after op_own is read. On Arm
> > > platforms using "dmb ishld" is sufficient to enforce this.
> > >
> > > Fixes: 88c0733535d6 ("net/mlx5: extend Rx completion with error
> > > handling")
> > > Cc: matan@mellanox.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > >  drivers/common/mlx5/mlx5_common.h | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/common/mlx5/mlx5_common.h
> > > b/drivers/common/mlx5/mlx5_common.h
> > > index 5028a05b49..ac2e85b15f 100644
> > > --- a/drivers/common/mlx5/mlx5_common.h
> > > +++ b/drivers/common/mlx5/mlx5_common.h
> > > @@ -195,7 +195,11 @@ check_cqe(volatile struct mlx5_cqe *cqe, const
> > > uint16_t cqes_n,
> > >
> > >  	if (unlikely((op_owner != (!!(idx))) || (op_code ==
> > > MLX5_CQE_INVALID)))
> > >  		return MLX5_CQE_STATUS_HW_OWN;
> > > -	rte_io_rmb();
> > > +	/* Prevent speculative reading of other fields in CQE until
> > > +	 * CQE is valid.
> > > +	 */
> > > +	rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
> > > +
> > >  	if (unlikely(op_code == MLX5_CQE_RESP_ERR ||
> > >  		     op_code == MLX5_CQE_REQ_ERR))
> > >  		return MLX5_CQE_STATUS_ERR;
> > > --
> > > 2.17.1
diff mbox series

Patch

diff --git a/drivers/common/mlx5/mlx5_common.h b/drivers/common/mlx5/mlx5_common.h
index 5028a05b49..ac2e85b15f 100644
--- a/drivers/common/mlx5/mlx5_common.h
+++ b/drivers/common/mlx5/mlx5_common.h
@@ -195,7 +195,11 @@  check_cqe(volatile struct mlx5_cqe *cqe, const uint16_t cqes_n,
 
 	if (unlikely((op_owner != (!!(idx))) || (op_code == MLX5_CQE_INVALID)))
 		return MLX5_CQE_STATUS_HW_OWN;
-	rte_io_rmb();
+	/* Prevent speculative reading of other fields in CQE until
+	 * CQE is valid.
+	 */
+	rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
+
 	if (unlikely(op_code == MLX5_CQE_RESP_ERR ||
 		     op_code == MLX5_CQE_REQ_ERR))
 		return MLX5_CQE_STATUS_ERR;