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 B61D8A053C for ; Tue, 28 Jul 2020 12:38:08 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5158D1C030; Tue, 28 Jul 2020 12:38:08 +0200 (CEST) Received: from EUR04-DB3-obe.outbound.protection.outlook.com (mail-eopbgr60047.outbound.protection.outlook.com [40.107.6.47]) by dpdk.org (Postfix) with ESMTP id 9BDCB2B9C; Tue, 28 Jul 2020 12:38:05 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=odMgOM0tSdTv96w2qhrm/NTrSQazvXU3NJvYUEq62a9ljs+eSwwEEDkETj6Hb/kkii8zUuZaamtVndxXjbOsp7xsaZgLI0b3+QloKHpiicZeC0qZ54eEtIMxdlFUMOKCzSHnhpNIN0O+xuJPK+iATapi+GcG9U1ID3EISKz05H2KmBu6nIBkguEJJ60AXxmM71zvApOMLGMo26ESwn0flZnw96d8ymU2rbk7SnqX1eUFKOt0UczZKP3fnuvozcSCtT93X59CtywfKVcIx/DU82OwXk5++At8C6eI4R1Os/+PZ3/qVl/TLMIyxbzUG1MUkStGeck7WBBoAthVM0/sQg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=KOvHkJrF5Ry9FoJtEFn6WSzESSD0aiLbf7UbtB2Hkgk=; b=m+izJ8s97eHiqT8/SjgfEL3zZYpqIbcDf1adz+jh0gC7xyVxuefiFi9FCSditw1Pe/ELQ8HcGifw/jaonZCUIupq0LwhWRn8w/vPfQoWwPghPYfd7F7XQNfEhWWVJCqpG+KdHgqVhYjYy9eTOQPYZQeMhzXN7INGRctStAp/667jd13eWEh6n/E+U2C0odV1JSHAStbIeKOmobffm3TBC2v5VD4A+koXEr0A5GXPYYi4DeWoBRIhTEg7SxJ1h4KdGOp3Mie9O5MQivD622g+l+ezTFUQ1eqMv+T/rV7zZ89V8XHqQgO0VvegNKVTYvVg4VVIUi0uadE1nGbtXEIJYA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=mellanox.com; dmarc=pass action=none header.from=mellanox.com; dkim=pass header.d=mellanox.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=KOvHkJrF5Ry9FoJtEFn6WSzESSD0aiLbf7UbtB2Hkgk=; b=RvkHn4OWMyLImH2vhBF47y4R2VA2J2rOiKmVv8EH+P1iwI0dPoLFe+5Am3i0PBg8+7o5hl1XarlqOh6Z9krTu3/L15wUbdBbHKp4CimklivZ7Y2RHS1o1ohbbPd6sot4NvuwK2sB84WY3QCagqkf8k2FgzEMwmnIArBlcEX5c+I= Received: from AM0PR0502MB4019.eurprd05.prod.outlook.com (2603:10a6:208:f::11) by AM4PR05MB3155.eurprd05.prod.outlook.com (2603:10a6:205:10::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3216.24; Tue, 28 Jul 2020 10:38:01 +0000 Received: from AM0PR0502MB4019.eurprd05.prod.outlook.com ([fe80::c010:7069:920a:3663]) by AM0PR0502MB4019.eurprd05.prod.outlook.com ([fe80::c010:7069:920a:3663%6]) with mapi id 15.20.3216.034; Tue, 28 Jul 2020 10:38:01 +0000 From: Matan Azrad To: Thomas Monjalon CC: "dev@dpdk.org" , Raslan Darawsheh , Shiri Kuzin , "stable@dpdk.org" , Shahaf Shuler , Slava Ovsiienko , Ray Kinsella , Neil Horman Thread-Topic: [PATCH] common/mlx5: fix CPU detection for PCI relaxed ordering Thread-Index: AQHWXbS7Xt/9LATOx0a0pYP/MvbRPakOuSNggAAGnoCAAAJrAIAAJK8AgAf5cyCABfXcAIAAAOEw Date: Tue, 28 Jul 2020 10:38:01 +0000 Message-ID: References: <20200719100735.2473014-1-thomas@monjalon.net> <8766955.pzj3U7fHRR@thomas> <8862837.XqZ19bOpPU@thomas> In-Reply-To: <8862837.XqZ19bOpPU@thomas> Accept-Language: en-US, he-IL Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: monjalon.net; dkim=none (message not signed) header.d=none;monjalon.net; dmarc=none action=none header.from=mellanox.com; x-originating-ip: [77.126.22.121] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: fc7fa54b-f0f5-440c-8097-08d832e24d3f x-ms-traffictypediagnostic: AM4PR05MB3155: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtFwd,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: GNL0XCDxAmK6lPOCThxetPfbHiSKVLvdMTJ2YMiO603Vi2ELmP/Al4rcSsn3JXOzzmgqmGWeo29F0xrfa6wk0+M6lyWR1eRHzq1qiTOvFfgWS0XGbbAMBIeC0qdu5l6sJoFWNzDn4oTSX0kq0/vjLQlOzNNeKjLdU+A3WrQK7Nj8nSN9IdIHEke8DQSLVXteoLIArSaihfGimobNMMxmkCdKxI12b/1uF6Tk1q8KIvJy7j37wT1kOApjzwpEHCGojP33kuUE5geN2myZ0uZkT7NlpmDZaqSHw/7HC1MWZUrOU76/96AY0/lxeAr9AqZ8XUStJvfqde8nY1JVQlOJdQ== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:AM0PR0502MB4019.eurprd05.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(4636009)(39860400002)(396003)(366004)(346002)(376002)(136003)(76116006)(5660300002)(52536014)(26005)(83380400001)(2906002)(478600001)(66946007)(4326008)(33656002)(66556008)(66476007)(8936002)(64756008)(66446008)(54906003)(316002)(55016002)(9686003)(86362001)(7696005)(6916009)(6506007)(8676002)(71200400001)(186003); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: IQfQOhxoFJoq/4GbRyuUd0Xo1IoIecvl66js4PYXtepvBpEjnj0McWFZzdwQr6Qh2TOuoUhntDl3TydCJ8M0nzqJ/0zJUsHPgg8mVxqISS2qk7Vk8gS1IeK6QYJdIYjgbeViJdzfcilLHaLQzqJHFnwa96qeRhBMNgauqT6E/bqCez3bqg6mgIpp0fPHd06GqT+WKzQ9tppICVUj5CG2jBog7a7nBWyCuaNl0ZDBtTnrZBa40OTb4uAgHaMLHvUaowMlxA7iLDC+B2h+ITvb0guxFh4tllx8m1Z8ERNn7zNdx6gX5HOxHyWOP8s7XsOM20rki0yWPwDcoaVE1vDpt5ddb5QrUh/Q4SjitpiFBa5op0h9sEruR2vf4TnlFA+BMHH7NQhG/73EVp0VZxx0gFhs4t5su+wmJo1ySdkDY/yPzPnZWm10z5UkQFl6wx7OWeQyFv8TvXYNGPtktc2ASePGConsXKW7Ys9J3KpAEEs= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: AM0PR0502MB4019.eurprd05.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: fc7fa54b-f0f5-440c-8097-08d832e24d3f X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Jul 2020 10:38:01.1041 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: cZFrI872AuH2fZx0pqnvl01mLSUit/QIxtXF1hg8TRYVqXddelj1r1ECCRtAGI475l1MugUWq2LSNECNIzsTaw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR05MB3155 Subject: Re: [dpdk-stable] [PATCH] common/mlx5: fix CPU detection for PCI relaxed ordering X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" Hi Thomas From: Thomas Monjalon: > 24/07/2020 17:43, Matan Azrad: > > Hi Thomas > > > > From: Thomas Monjalon: > > > 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 will be good to add here the compiling command which failed for you.= .. ? > > > > > > > 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. > > > > > > > It is internal API, I don't understand your concern... > > > > > > > > > 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. > > > > 2 mlx5 devices running configuration from 2 different threads. > > The first MR from each one of the devices can be created at the same ti= me. > > The ask for the relaxed ordering cpu can be happened at the same time. >=20 > DPDK configuration is not thread safe. > Do you know any DPDK application configuring devices in 2 different threa= ds? A lot of effort were done in our driver and in other areas in dpdk in order= to protect this scenario, we need to follow. One case is failsafe, configuration can be done from either host thread or = the caller thread. Also, we may configure MR from data-path cores. =20 > > > > 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. > > > > > I hope you understand the motivation for higher priority, The LOG is > > just the one above. > > > > > > 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. > > > > Yes, It is for future, because it makes sense the cpu detection query w= ill be > done at initialization time. > > > > Now, when I understand the community relevant guys don't like > priorities(also I didn't convinced on the reasons), I think you can call = it from > common init function because it is the first call of mlx5 constructors. >=20 > We want to avoid priorities, and more importantly, we want to avoid havin= g > too much code in constructors. The code here is not too much and as I said it makes sense to do this prior= code in constructor. =20 > > We need to fix the race issue introduced by this patch. > > My favor is to call it from constructor. >=20 > Initialization and configuration is supposed to be done by a single threa= d. > There should not have any race condition. So, please explain all the dpdk effort to protect it....