[dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion queue

[Slava Ovsiienko]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit at intel.com>
> Sent: Thursday, January 9, 2020 17:44
> To: Slava Ovsiienko <viacheslavo at mellanox.com>; dev at dpdk.org
> Cc: Matan Azrad <matan at mellanox.com>; Raslan Darawsheh
> <rasland at mellanox.com>; Ori Kam <orika at mellanox.com>; Thomas
> Monjalon <thomas at monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
> 
> On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit at intel.com>
> >> Sent: Thursday, January 9, 2020 17:19
> >> To: Slava Ovsiienko <viacheslavo at mellanox.com>; dev at dpdk.org
> >> Cc: Matan Azrad <matan at mellanox.com>; Raslan Darawsheh
> >> <rasland at mellanox.com>; Ori Kam <orika at mellanox.com>; Thomas
> Monjalon
> >> <thomas at monjalon.net>
> >> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on
> >> completion queue
> >>
> >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> >>> The free on completion queue keeps the indices of elts array, all
> >>> mbuf stored below this index should be freed on arrival of normal
> >>> send completion. In debug version it also contains an index of
> >>> completed transmitting descriptor (WQE) to check queues
> synchronization.
> >>>
> >>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo at mellanox.com>
> >>> Acked-by: Matan Azrad <matan at mellanox.com>
> >>
> >> <...>
> >>
> >>> @@ -2108,17 +2108,18 @@ enum mlx5_txcmp_code {
> >>>  			/*
> >>>  			 * We are going to fetch all entries with
> >>>  			 * MLX5_CQE_SYNDROME_WR_FLUSH_ERR status.
> >>> +			 * The send queue is supposed to be empty.
> >>>  			 */
> >>>  			++ci;
> >>> +			txq->cq_pi = ci;
> >>> +			last_cqe = NULL;
> >>>  			continue;
> >>>  		}
> >>>  		/* Normal transmit completion. */
> >>> +		assert(ci != txq->cq_pi);
> >>> +		assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> >>> wqe_counter);
> >>
> >> And same comments on these as previous patches, we spend some effort
> >> to remove the 'rte_panic' from drivers, this is almost same thing.
> >>
> >> I think a driver shouldn't decide to exit whole application, it's
> >> effect should be limited to the driver.
> >>
> >> Assert is useful for debug and during development, but not sure
> >> having them in the production code.
> >
> > IIRC, "assert" is standard C function. Compiled only if there is no NDEBUG
> defined.
> > So, assert does exactly what you are saying - provide the debug break
> > not allowing the bug to evolve. And no this break in production code.
> >
> 
> Since mlx driver is using NDEBUG defined, what you said is right indeed. But
> why not using RTE_ASSERT to be consistent with rest. There is a specific config
> option to control assert (RTE_ENABLE_ASSERT) and anyone using it will get
> different behavior with mlx5.

We have the dedicated option to control mlx5 debug:
CONFIG_RTE_ENABLE_ASSERT controls the whole DPDK.
CONFIG_RTE_LIBRTE_MLX5_DEBUG controls NDEBUG for mlx5

From my practice - I switch the mlx5 debug option (in the process of the debugging/testing
datapath and checking the resulting performance, by directly defining NDEBUG in mlx5.h and
not reconfiguring/rebuilding the entire DPDK), this fine grained option seems to be useful.

With best regards, Slava