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.nagaraha...@arm.com>
> Sent: Sunday, June 6, 2021 19:50
> To: dev@dpdk.org; honnappa.nagaraha...@arm.com; Matan Azrad
> <ma...@nvidia.com>; Shahaf Shuler <shah...@nvidia.com>; Slava Ovsiienko
> <viachesl...@nvidia.com>
> Cc: ruifeng.w...@arm.com; Matan Azrad <ma...@nvidia.com>;
> sta...@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: ma...@mellanox.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.w...@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
