From: Thomas Monjalon <thomas@monjalon.net>
To: Matan Azrad <matan@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
Raslan Darawsheh <rasland@mellanox.com>,
Shiri Kuzin <shirik@mellanox.com>,
"stable@dpdk.org" <stable@dpdk.org>,
Shahaf Shuler <shahafs@mellanox.com>,
Slava Ovsiienko <viacheslavo@mellanox.com>,
Ray Kinsella <mdr@ashroe.eu>, Neil Horman <nhorman@tuxdriver.com>
Subject: Re: [dpdk-dev] [PATCH] common/mlx5: fix CPU detection for PCI relaxed ordering
Date: Sun, 19 Jul 2020 13:13:09 +0200 [thread overview]
Message-ID: <4865593.5jmsT3qBYc@thomas> (raw)
In-Reply-To: <DB3PR0502MB40284BD5A1EAF26762F4678DD27A0@DB3PR0502MB4028.eurprd05.prod.outlook.com>
19/07/2020 12:56, Matan Azrad:
>
> From: Thomas Monjalon
> > The detection of the CPU was done in a constructor and shared in a global
> > variable.
> >
> > This variable may not be visible in the net PMD because it was not exported
> > as part of the .map file.
>
> Can you explain exactly when it is not visible?
I depends on linker options.
> > It is fixed by exporting a function, which is cleaner than a variable.
>
> Can you explain why?
> We have classic example - rte_eth_devices.
There is more control and more abstraction in functions,
it can provide futre-proof abstraction.
We should not export variables at all,
it is a basic rule of writing API.
Having a bad example in ethdev doesn't mean we should follow it.
> > By checking the CPU only at the first call of the function, doing the check in a
> > constructor becomes useless.
>
> Yes, but why not to do it in constructor? this variable is initialized only once and doesn't depend in any parameter.
Constructor must remain minimal.
If constructor can be avoided, it must be.
This is a golden rule.
> > Note: the priority of the constructor was probably irrelevant.
No comment about the constructor priority which was set as LOG
for no good reason, proving that this code was not well reviewed?
> > At the same time, the comments are reworded or dropped if useless.
> >
> > Fixes: 4c204fe5e5d2 ("common/mlx5: disable relaxed ordering in unsuitable
> > CPUs")
> > Cc: shirik@mellanox.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
next prev parent reply other threads:[~2020-07-19 11:13 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-19 10:07 Thomas Monjalon
2020-07-19 10:11 ` Slava Ovsiienko
2020-07-19 10:56 ` Matan Azrad
2020-07-19 11:13 ` Thomas Monjalon [this message]
2020-07-19 11:41 ` Matan Azrad
2020-07-19 13:33 ` Thomas Monjalon
2020-07-24 15:43 ` Matan Azrad
2020-07-28 10:21 ` Thomas Monjalon
2020-07-28 10:38 ` Matan Azrad
2020-07-24 14:53 ` David Marchand
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=4865593.5jmsT3qBYc@thomas \
--to=thomas@monjalon.net \
--cc=dev@dpdk.org \
--cc=matan@mellanox.com \
--cc=mdr@ashroe.eu \
--cc=nhorman@tuxdriver.com \
--cc=rasland@mellanox.com \
--cc=shahafs@mellanox.com \
--cc=shirik@mellanox.com \
--cc=stable@dpdk.org \
--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).