10/01/2020 10:55, Slava Ovsiienko: > From: Thomas Monjalon <tho...@monjalon.net> > > 10/01/2020 10:28, Slava Ovsiienko: > > > From: Thomas Monjalon <tho...@monjalon.net> > > > > 09/01/2020 17:22, Slava Ovsiienko: > > > > > From: Ferruh Yigit <ferruh.yi...@intel.com> > > > > > > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote: > > > > > > > From: Ferruh Yigit <ferruh.yi...@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") ? > > > > I mean we should use RTE_ASSERT in mlx5, as it is already done in some > > files. > > > This would make not possible to engage asserts in mlx5 module only. > It is a question of structuring/layering, not "different behavior". > As for me - it is very nice to have fine grained debug control option, > and I use this feature actively, it just saves my time. Also, it seems > these options are implemented in many other PMDs > (with its own xxx_ASSERTs).
I disagree, it is not nice. It makes it more complicate to use. Can you imagine every file having its own tools and configs in a project? As a maintainer, my role is to make things simpler for everyone in general so we can know easily how things work. About time saving, I also disagree. If you enable assert for the whole project during all your development, it is a good practice which does not cost any time. About other PMDs, they must be fixed.
