DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Slava Ovsiienko <viacheslavo@mellanox.com>
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 14:11:29 +0100	[thread overview]
Message-ID: <13971515.JCcGWNJJiE@xps> (raw)
In-Reply-To: <AM4PR05MB32658C75D84405B520B6EC66D2380@AM4PR05MB3265.eurprd05.prod.outlook.com>

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.

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.



  reply	other threads:[~2020-01-10 13:11 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 [this message]
2020-01-10 13:42                       ` Slava Ovsiienko
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=13971515.JCcGWNJJiE@xps \
    --to=thomas@monjalon.net \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=matan@mellanox.com \
    --cc=orika@mellanox.com \
    --cc=rasland@mellanox.com \
    --cc=viacheslavo@mellanox.com \
    /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).