From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B86BDA0540; Sun, 19 Jul 2020 15:33:11 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9BF361C10E; Sun, 19 Jul 2020 15:33:11 +0200 (CEST) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 1A3D02BE5; Sun, 19 Jul 2020 15:33:10 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailout.nyi.internal (Postfix) with ESMTP id 58E1C5C0114; Sun, 19 Jul 2020 09:33:09 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute7.internal (MEProxy); Sun, 19 Jul 2020 09:33:09 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm1; bh= LheA/xZt90DY6F5Amr3zBwZsnLq8Vrih+rKUUXooWhs=; b=CbCqzXdrUKUCI++u TV2f1R3ZgMLI0lI5I/qm1Awk8BJzrW+lpy5Ad/x5ksYTRHncvP9WlV8cuOuENxT5 TKrxleAb1+tfXctU1ZhlxHhhx3N2+i5a/Pa6Ni6raaoT8quKgNpiuaAtHpC74Wv5 s1BBx3An9ex7LbD51wVqMMz0rMKMqFZbQQORhKg5QnJqNaRMQvwrZZDAShCV0sno lZO4PKw1nGUI6Rq5vMWK3o4r3t/0UXWIMQbZUyMVikBNlOTl3cxdUdmGWJqRrwqX R7PyG573VrOhmqrcbT+g1Ulo+Yi0Hr9L+OMnt0eeyn2MRQfLa2WfCigVBr6kpPmI e0Qr5w== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=LheA/xZt90DY6F5Amr3zBwZsnLq8Vrih+rKUUXooW hs=; b=chee/UtwifJTBZr63cVadC23HTmx7SUXVmktvwqMsBkKco//jpYw6T25l 6GB9mkRdi94v+Yos6ABFvP6PFzAaEKHmXWu/EETX8f3kCXalEpbQKV+OBuB7avte IgNACH9ZQc3T+YJ0dOu5jSS3aflro/+Ml6jUliBZwnYKrLuLY6FB9VmGB39cv5uG KJ+mEBFoNRUzuxXJR/7G74A6dmAfZ42lg8DcDVQMfOvO55PE7GLGaygW1Ixvf/HV mw5ykhD2GD8Zq51XfK7gLpGgsu6wgO4vK5HIAQphs5PtsAuxjpUnw+mE7T0EYJ2S peifw4GixwAjAoJUPuj4WNWPC7ztQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduiedrgedugdeiiecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdejueei iedvffegheenucfkphepleefrdeirddugeelrdduudegnecuvehluhhsthgvrhfuihiivg eptdenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdr nhgvth X-ME-Proxy: Received: from xps.localnet (114.149.6.93.rev.sfr.net [93.6.149.114]) by mail.messagingengine.com (Postfix) with ESMTPA id 9B9A730600A6; Sun, 19 Jul 2020 09:33:07 -0400 (EDT) From: Thomas Monjalon To: Matan Azrad Cc: "dev@dpdk.org" , Raslan Darawsheh , Shiri Kuzin , "stable@dpdk.org" , Shahaf Shuler , Slava Ovsiienko , Ray Kinsella , Neil Horman Date: Sun, 19 Jul 2020 15:33:06 +0200 Message-ID: <8766955.pzj3U7fHRR@thomas> In-Reply-To: References: <20200719100735.2473014-1-thomas@monjalon.net> <4865593.5jmsT3qBYc@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH] common/mlx5: fix CPU detection for PCI relaxed ordering X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 19/07/2020 13:41, Matan Azrad: > > 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? No, we avoid changing API. > > > > 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?). I don't see what could be the issue. > 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. No I mean this constructor is declared with LOG priority, but it is not doing any log registration. > We want to be sure that the variable is ready before any usage of it in the drivers (even in driver contractors). It is not used by other constructors. And avoiding constructor dependencies is exactly why we avoid using constructors at all. > > > > 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