From: Slava Ovsiienko <viacheslavo@mellanox.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: Ferruh Yigit <ferruh.yigit@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>, Matan Azrad <matan@mellanox.com>,
Raslan Darawsheh <rasland@mellanox.com>,
Ori Kam <orika@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion queue
Date: Fri, 10 Jan 2020 13:42:29 +0000 [thread overview]
Message-ID: <AM4PR05MB32656B1720B5D6C4D760CC0CD2380@AM4PR05MB3265.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <13971515.JCcGWNJJiE@xps>
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 10, 2020 15:11
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Matan Azrad
> <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>; Ori
> Kam <orika@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
>
> 10/01/2020 10:55, Slava Ovsiienko:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 10/01/2020 10:28, Slava Ovsiienko:
> > > > From: 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.
> > > >
> > > > 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.
Not every file, but every module. It is quite common practice to have local
configuration options for module in the large projects. So, it is native for
PMDs to have its dedicated configuration options. And what we have
currently follows this approach.
>
> 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.
During the day I might switch multiple times between debug (with assert enabled)
and performance check (debug/assert disabled) modes. Now I can switch it easily and quickly,
just commenting [out] NDEBUG in mlx5.h. In the large projects the global configuration changing price
is getting higher. You just propose to pay this price ☹ - I would have to reconfig/recompile
the entire DPDK. The same might concern the other PMDs - many of them have the private
debug option(s).
>
> About other PMDs, they must be fixed.
I suppose the developers of other PMDs use the module debug options either :)
>
I agree the NDEBUG is "different behavior", we should think how to eliminate it.
But I do not understand why we should drop the partial configuration, the feature that is actively used.
Now code is split into several debug domains (at least for assert), we can control each domain in independent
fashion, and the practice shows it is useful and saves developer's time. DPDK is the large project definitely,
it contains a lot of modules, we can't address all development needs with one simple common configuration
option.
Having the module/private debug options does not eliminate the introducing the global control -
say "RTE_CONFIG_ENABLE_MODULE_DEBUG_OPTIONS" to provide production code generation
"in one click", but just dropping the module debug options is not nice as for me.
With best regards, Slava
next prev parent reply other threads:[~2020-01-10 13:42 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-08 16:15 [dpdk-dev] [PATCH 0/4] net/mlx5: remove Tx descriptor reserved field usage Viacheslav Ovsiienko
2020-01-08 16:15 ` [dpdk-dev] [PATCH 1/4] net/mlx5: move Tx complete request routine Viacheslav Ovsiienko
2020-01-08 16:15 ` [dpdk-dev] [PATCH 2/4] net/mlx5: update Tx error handling routine Viacheslav Ovsiienko
2020-01-08 16:16 ` [dpdk-dev] [PATCH 3/4] net/mlx5: add free on completion queue Viacheslav Ovsiienko
2020-01-08 16:16 ` [dpdk-dev] [PATCH 4/4] net/mlx5: engage " Viacheslav Ovsiienko
2020-01-09 10:56 ` [dpdk-dev] [PATCH v2 0/4] net/mlx5: remove Tx descriptor reserved field usage Viacheslav Ovsiienko
2020-01-09 10:56 ` [dpdk-dev] [PATCH v2 1/4] net/mlx5: move Tx complete request routine Viacheslav Ovsiienko
2020-01-09 10:56 ` [dpdk-dev] [PATCH v2 2/4] net/mlx5: update Tx error handling routine Viacheslav Ovsiienko
2020-01-09 10:56 ` [dpdk-dev] [PATCH v2 3/4] net/mlx5: add free on completion queue Viacheslav Ovsiienko
2020-01-09 15:12 ` Ferruh Yigit
2020-01-09 15:22 ` Slava Ovsiienko
2020-01-09 10:56 ` [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage " Viacheslav Ovsiienko
2020-01-09 15:18 ` Ferruh Yigit
2020-01-09 15:27 ` Slava Ovsiienko
2020-01-09 15:43 ` Ferruh Yigit
2020-01-09 16:22 ` Slava Ovsiienko
2020-01-10 9:06 ` Thomas Monjalon
2020-01-10 9:28 ` Slava Ovsiienko
2020-01-10 9:34 ` Thomas Monjalon
2020-01-10 9:55 ` Slava Ovsiienko
2020-01-10 13:11 ` Thomas Monjalon
2020-01-10 13:42 ` Slava Ovsiienko [this message]
2020-01-17 10:44 ` Slava Ovsiienko
2020-01-09 14:22 ` [dpdk-dev] [PATCH v2 0/4] net/mlx5: remove Tx descriptor reserved field usage Raslan Darawsheh
2020-01-13 9:35 ` Raslan Darawsheh
2020-01-09 17:16 ` [dpdk-dev] [PATCH v3 " Viacheslav Ovsiienko
2020-01-09 17:16 ` [dpdk-dev] [PATCH v3 1/4] net/mlx5: move Tx complete request routine Viacheslav Ovsiienko
2020-01-09 17:16 ` [dpdk-dev] [PATCH v3 2/4] net/mlx5: update Tx error handling routine Viacheslav Ovsiienko
2020-01-09 17:16 ` [dpdk-dev] [PATCH v3 3/4] net/mlx5: add free on completion queue Viacheslav Ovsiienko
2020-01-09 17:16 ` [dpdk-dev] [PATCH v3 4/4] net/mlx5: engage " Viacheslav Ovsiienko
2020-01-13 9:36 ` [dpdk-dev] [PATCH v3 0/4] net/mlx5: remove Tx descriptor reserved field usage Raslan Darawsheh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM4PR05MB32656B1720B5D6C4D760CC0CD2380@AM4PR05MB3265.eurprd05.prod.outlook.com \
--to=viacheslavo@mellanox.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=matan@mellanox.com \
--cc=orika@mellanox.com \
--cc=rasland@mellanox.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).