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 37674A052A; Fri, 24 Jul 2020 17:43:48 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 665821C042; Fri, 24 Jul 2020 17:43:46 +0200 (CEST) Received: from EUR04-DB3-obe.outbound.protection.outlook.com (mail-eopbgr60058.outbound.protection.outlook.com [40.107.6.58]) by dpdk.org (Postfix) with ESMTP id F195DA3; Fri, 24 Jul 2020 17:43:44 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=DdQRfgX8Hvpn2yzEAxujLExDuiA6Ce8y1aHnpoZEMhw8nipfv8Vs8cq3q+h7O8Ckpo0Mq1V+L/OBPFbqdnIrpmniS+Vn7dsTs08GelLv65meLMIdnX0m3hSuaVDyrRYi9YKylIv3zJ7ENRYygfJfDSzlk0AdFXX+8x05yBaarxyWKtFnp3Tfp+KCwZkPJtTxH2jo9FoTL5r9IiF5g3cGM/Z3L2hLOUcQeYf20ghksB8gmz3ybeSnOBqmGPIjxlq4G0N1tE5b74XA2iK0SNmQVfgSGGiDXbXIlu26pTtDKNojv/Sat/VB2M5o9P+rMT+4emDcvg9sGcJWBgs6RPUSZA== 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=lYDGWMfG9mAedudoBKCOk+Nvb65gdhExj1W2zSq9Y7w=; b=gvrdOsahhYp1a/EtyL7NX+sLW6kyzxhjyfRmBMvYItdnmUoJz9zo0T6mYa2MnciE8nYv5G+VumODkTInadaH2C+PJ0li+h9LkV2AOQIOfcoQf3fUBYZas14uwHWVVR3ViXdbvTy73mbaPrGnTPUo2V2wgWLcLPLQA67ofRBwSN36HWgG2SLHaCz8UkJvflkWHo0AvtBVAWNiBP9ltN0kMMXGXzGgyd4nTM5xB/PvmuIr4x6/iVhXWa67kM2hmnISn/XrxOrjD2c0pmZkZvz7L/wYngytDHQPYCXutViF34MGlPQW4Q2xfqHVjENhz1JGFEwqsaiOtgRWLb7A9Xk+Ig== 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=lYDGWMfG9mAedudoBKCOk+Nvb65gdhExj1W2zSq9Y7w=; b=Vd710uDeoZayN32+KP1ceNLC8EOPB7cqIJRQ2NdwHDSQIM0lfgas2OysI5bop5Nmw5DdNHhZiM7I2tqqxsSK+6VKvU/wp+gnxxcxwSlEPm7oymsIEhCQLw9QVSqP1yag5FQwCsCg0Kigb/zD36WXxHJIzPCav9NeYeU66BJPqHI= Received: from AM0PR0502MB4019.eurprd05.prod.outlook.com (2603:10a6:208:f::11) by AM0PR05MB4595.eurprd05.prod.outlook.com (2603:10a6:208:ae::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3195.23; Fri, 24 Jul 2020 15:43:43 +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.3195.028; Fri, 24 Jul 2020 15:43:43 +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/MvbRPakOuSNggAAGnoCAAAJrAIAAJK8AgAf5cyA= Date: Fri, 24 Jul 2020 15:43:43 +0000 Message-ID: References: <20200719100735.2473014-1-thomas@monjalon.net> <4865593.5jmsT3qBYc@thomas> <8766955.pzj3U7fHRR@thomas> In-Reply-To: <8766955.pzj3U7fHRR@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: [2a02:ed0:43f3:a800:2986:65e7:9ad3:197c] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: be0baf84-83dd-4e37-83d9-08d82fe8584c x-ms-traffictypediagnostic: AM0PR05MB4595: 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: 1i8tylmm4kt39Qc9lGs0O0MeD6D5hZP9cbwkhLg4DYKDpfr9ihUjYLMJbNEmbXsIUqYZ5j4B4SqIBlw3ez57IVD0HZ7s+8uPvEDOE81w3RKQ+AOMtSLi9dhJI0PH4wJgbZp3f+WoMuo2N6BSkhgs/BGJuTzqZ+eahS2MX+H8DvwbQJchPLw0aJBkBPWb3LImkTSPbXcwrD1mrvx6cJ0r+ajwkrbkebpZCeY1y8TUu4ruxhl4pXEeEl9kQfk5BVJO/ShT7vcgV8TLltEl/B+JexwAX6NaYNfTlOztgOJWiSSwgHknV14I15jYThhe+VUkIoK2Bs6urRD9iTDJ2B5fbg== 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)(376002)(346002)(396003)(366004)(39860400002)(136003)(8936002)(66946007)(76116006)(6506007)(86362001)(71200400001)(66476007)(66556008)(55016002)(66446008)(6916009)(5660300002)(64756008)(52536014)(316002)(9686003)(7696005)(54906003)(83380400001)(478600001)(186003)(33656002)(2906002)(8676002)(4326008); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: ssM70WI91SfWgzFzrCTr2g/j2W6bOQFL3psocWx9wwgQ9EppIc3SxcZqdJAxEq4W2thwQ9ZH3ik34Nbr3JG7uiHfg65sQkj8+ppJ8MhKilHE4yKUXPGeNG2+IFS26x9ntAcSXT06zNxLcXtDAkl+WSTH/MwTtGwb4epsd2ItXnne45WcWMnJ0rcieGjP4WBuVuw5GW+9ImgEBMmn7jtnDgGKWFXhB2p4WIa74tax2KPgJhaPuUZm+aZV7NwAholgpdjbhZ9BHkU3XCuL+P7toAWgS9MMu3DQjRtVBe4Go3jZphFtyjIqr3qJ1anmvIGGwfEU58pBZMpQFTsX+S5jD3sz+1RB3s94mhRH0eEQHZ6eT6kyhpGcrX0i4/1jKVRzerh8w25k3Xqqz9LYqvGulyCYjqeyZoWyRH7HgdmzIShQr0D670aNK3BIWOyUoYSemKqbpr0xtD7zCB2PyEE1h6D8K1rUMgO2ljM1uJ/TnjvYR3jUWUdntTLdXliYcwQ32rPjV9vG44KSmMVTjn91cURtFiguk1fl+sh4WnS6IyyXovKIx7z/A6Fmh0b+cfbb 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: be0baf84-83dd-4e37-83d9-08d82fe8584c X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jul 2020 15:43:43.2013 (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: mcX2sSbcdlXkYjDWhKpsAyigFYxUoXpw/WfoFCD6UY81ayu5jqoKsvevT277tSmvQI4hxzDz8QjCBoaCcJLUQA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR05MB4595 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" 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 vari= able. > > > > > > > > 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? >=20 > No, we avoid changing API. >=20 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?). >=20 > 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 time. The ask for the relaxed ordering cpu can be happened at the same time. =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. >=20 > No I mean this constructor is declared with LOG priority, but it is not d= oing > any log registration. > I hope you understand the motivation for higher priority, The LOG is just the one above. =20 > > We want to be sure that the variable is ready before any usage of it in= the > drivers (even in driver contractors). >=20 > 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 will = be done at initialization time. Now, when I understand the community relevant guys don't like priorities(al= so 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 > > > > > 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 =20 We need to fix the race issue introduced by this patch. My favor is to call it from constructor. In any case we should validate the patch on ARM and Intel cores... The relaxes ordering original patch had a lot of testing before applying, Need to be sure we don't break nothing. Matan