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

[Slava Ovsiienko]

> -----Original Message-----
> From: Thomas Monjalon <thomas at monjalon.net>
> Sent: Friday, January 10, 2020 11:07
> To: Slava Ovsiienko <viacheslavo at mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit at intel.com>; dev at dpdk.org; Matan Azrad
> <matan at mellanox.com>; Raslan Darawsheh <rasland at mellanox.com>; Ori
> Kam <orika at mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
> 
> 09/01/2020 17:22, Slava Ovsiienko:
> > From: Ferruh Yigit <ferruh.yigit at intel.com>
> > > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> > > > From: Ferruh Yigit <ferruh.yigit at 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.

Do you mean we should eliminate NDEBUG usage and convert it to some explicit "MLX5_NDEBUG"
(and convert "assert" to "MLX5_ASSERT") ? 

With best regards, Slava