[dpdk-dev] [PATCH] net/mlx5: remove unwanted barrier

[Slava Ovsiienko]

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 at arm.com>
> Sent: Sunday, June 6, 2021 19:50
> To: dev at dpdk.org; honnappa.nagarahalli at arm.com; Matan Azrad
> <matan at nvidia.com>; Shahaf Shuler <shahafs at nvidia.com>; Slava Ovsiienko
> <viacheslavo at nvidia.com>
> Cc: ruifeng.wang at arm.com; Matan Azrad <matan at nvidia.com>;
> stable at 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 at mellanox.com
> Cc: stable at dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang at 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