net/mlx5: remove unwanted barrier

  The IO barrier is not required as cqe->op_own is read once. The
checks done on the local variable and the memory is not read again.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Hi, Honnappa

The rte_io_rmb() was inserted not to prevent the extra access to cqe->op_own
(the volatile qualifier is quite enough, if we had some doubts, we would insert rte_compiler_barrier),
but the real intention of io_rmw was to isolate cqe->op_own loads on hardware level.

cqe points to the Completion Queue Entry (CQE), that is mapped to the memory that is continuously
being updated by the device (NIC). CQE is 64B size structure and op_own is located at the structure end,
and is updated by HW in last order, after the entire CQE is completely written to the host memory.

After detecting by cqe_check() the CQE is owned by software (hardware completed operation)
the PMD starts touching other CQE fields, i.e. the next load transactions from CQE are triggered.
And we must make sure these loads happen in correct order, only if cqe->op_own load was completed and
valid ownership flags were seen, i.e. - do not allow speculative reads with possible incorrect values fetched).

Just hypothetical case (I agree in advance - it is very unlikely, but is not impossible :)):

1. owner = cqe->op_own - load A triggered
2. some code is being speculatively executed, no barrier
3. length = cqe->length - load B triggered
4. Let's suppose CPU reordered A and B, ie order of loads: B, A
5. In memory/CPU cache we have old CQE, owned by HW
6. B load gets the old length value (invalid)
7. Hardware writes the new CQE and CPU cache is invalidated
8. A load gets the CQE is owned by SW and the invalid results of load B will be used by PMD

Hence, I would consider the patch as risky, and as one that is extremely hard to be covered completely with tests -
we should test for race conditions on multiple architectures, on many CPU models, PCIe root complexes, etc.

With best regards,
Slava

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Sunday, June 6, 2021 19:50
> To: dev@dpdk.org; honnappa.nagarahalli@arm.com; Matan Azrad
> <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>
> Cc: ruifeng.wang@arm.com; Matan Azrad <matan@nvidia.com>;
> stable@dpdk.org
> Subject: [PATCH] net/mlx5: remove unwanted barrier
> 
> The IO barrier is not required as cqe->op_own is read once. The checks done on
> the local variable and the memory is not read again.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_common.h
> b/drivers/common/mlx5/mlx5_common.h
> index 5028a05b49..a4c29f51f1 100644
> --- a/drivers/common/mlx5/mlx5_common.h
> +++ b/drivers/common/mlx5/mlx5_common.h
> @@ -195,7 +195,7 @@ 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();
> +
>  	if (unlikely(op_code == MLX5_CQE_RESP_ERR ||
>  		     op_code == MLX5_CQE_REQ_ERR))
>  		return MLX5_CQE_STATUS_ERR;
> --
> 2.17.1
  

Hi Slava,
	Thanks for taking the pain to explain this (this is similar to DD bits in Intel i40e PMD). Agree, that for ensuring the ordering of the loads between op_own and the rest of the fields we need a barrier. For Arm architecture, we can use the barrier for inner sharable domain (rte_smp_rmb()) which is less costly than the outer sharable domain (rte_io_rmb()). I will re-spin this patch.

Thanks,
Honnappa

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Thursday, July 1, 2021 7:53 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> dev@dpdk.org; Matan Azrad <matan@nvidia.com>; Shahaf Shuler
> <shahafs@nvidia.com>
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; Matan Azrad
> <matan@nvidia.com>; stable@dpdk.org
> Subject: RE: [PATCH] net/mlx5: remove unwanted barrier
> 
> Hi, Honnappa
> 
> The rte_io_rmb() was inserted not to prevent the extra access to cqe-
> >op_own (the volatile qualifier is quite enough, if we had some doubts, we
> would insert rte_compiler_barrier), but the real intention of io_rmw was to
> isolate cqe->op_own loads on hardware level.
> 
> cqe points to the Completion Queue Entry (CQE), that is mapped to the
> memory that is continuously being updated by the device (NIC). CQE is 64B
> size structure and op_own is located at the structure end, and is updated by
> HW in last order, after the entire CQE is completely written to the host
> memory.
> 
> After detecting by cqe_check() the CQE is owned by software (hardware
> completed operation) the PMD starts touching other CQE fields, i.e. the next
> load transactions from CQE are triggered.
> And we must make sure these loads happen in correct order, only if cqe-
> >op_own load was completed and valid ownership flags were seen, i.e. - do
> not allow speculative reads with possible incorrect values fetched).
> 
> Just hypothetical case (I agree in advance - it is very unlikely, but is not
> impossible :)):
> 
> 1. owner = cqe->op_own - load A triggered 2. some code is being speculatively
> executed, no barrier 3. length = cqe->length - load B triggered 4. Let's suppose
> CPU reordered A and B, ie order of loads: B, A 5. In memory/CPU cache we
> have old CQE, owned by HW 6. B load gets the old length value (invalid) 7.
> Hardware writes the new CQE and CPU cache is invalidated 8. A load gets the
> CQE is owned by SW and the invalid results of load B will be used by PMD
> 
> Hence, I would consider the patch as risky, and as one that is extremely hard
> to be covered completely with tests - we should test for race conditions on
> multiple architectures, on many CPU models, PCIe root complexes, etc.
> 
> With best regards,
> Slava
> 
> > -----Original Message-----
> > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Sent: Sunday, June 6, 2021 19:50
> > To: dev@dpdk.org; honnappa.nagarahalli@arm.com; Matan Azrad
> > <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava
> > Ovsiienko <viacheslavo@nvidia.com>
> > Cc: ruifeng.wang@arm.com; Matan Azrad <matan@nvidia.com>;
> > stable@dpdk.org
> > Subject: [PATCH] net/mlx5: remove unwanted barrier
> >
> > The IO barrier is not required as cqe->op_own is read once. The checks
> > done on the local variable and the memory is not read again.
> >
> > 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/common/mlx5/mlx5_common.h
> > b/drivers/common/mlx5/mlx5_common.h
> > index 5028a05b49..a4c29f51f1 100644
> > --- a/drivers/common/mlx5/mlx5_common.h
> > +++ b/drivers/common/mlx5/mlx5_common.h
> > @@ -195,7 +195,7 @@ 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();
> > +
> >  	if (unlikely(op_code == MLX5_CQE_RESP_ERR ||
> >  		     op_code == MLX5_CQE_REQ_ERR))
> >  		return MLX5_CQE_STATUS_ERR;
> > --
> > 2.17.1