From: Matan Azrad <matan@mellanox.com> To: Thomas Monjalon <thomas@monjalon.net> 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 11:41:27 +0000 Message-ID: <AM0PR0502MB4019444F343F2053DE71C524D27A0@AM0PR0502MB4019.eurprd05.prod.outlook.com> (raw) In-Reply-To: <4865593.5jmsT3qBYc@thomas> From: Thomas Monjalon: > 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. Also variable have more abstraction - struct. In future, if it will be needed, we can change it. > We should not export variables at all, > it is a basic rule of writing API. It is variable which is depended only in the running CPU - almost like compile time condition, so it is not regular case. I think it makes sense also to use a singleton variable as internal API. > Having a bad example in ethdev doesn't mean we should follow it. If ethdev rte_eth_devices is bad API, Are you going to change 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. The cpu detection is a fast code. Using constructor here makes sense: 1. we need only one initialization for all the program. 2. no need to take care of multithreading on the single initialization (are your code thread safe?). 3. no parameters are required. > > > > 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? I guess you mean that comment is missing - you right. We want to be sure that the variable is ready before any usage of it in the drivers (even in driver contractors). > > > > 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:41 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 2020-07-19 11:41 ` Matan Azrad [this message] 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=AM0PR0502MB4019444F343F2053DE71C524D27A0@AM0PR0502MB4019.eurprd05.prod.outlook.com \ --to=matan@mellanox.com \ --cc=dev@dpdk.org \ --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=thomas@monjalon.net \ --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
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git