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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

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