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

[Thomas Monjalon <thomas@monjalon.net>]

09/01/2020 17:22, Slava Ovsiienko:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> > >>> +		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.

No, it controls the whole DPDK except mlx PMDs.

> 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.

I don't like having mlx PMDs behave differently.
It make things difficult for newcomers.
And with meson, such options are cleaned up.