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 <viachesl...@nvidia.com>
> Sent: Thursday, July 1, 2021 7:53 AM
> To: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>;
> dev@dpdk.org; Matan Azrad <ma...@nvidia.com>; Shahaf Shuler
> <shah...@nvidia.com>
> Cc: Ruifeng Wang <ruifeng.w...@arm.com>; Matan Azrad
> <ma...@nvidia.com>; sta...@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.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
