From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 53F3FA052B;
	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 D2A111C00E;
	Tue, 28 Jul 2020 12:38:06 +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 <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>
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: <AM0PR0502MB401951FDE09F7BE2487D290AD2730@AM0PR0502MB4019.eurprd05.prod.outlook.com>
References: <20200719100735.2473014-1-thomas@monjalon.net>
 <8766955.pzj3U7fHRR@thomas>
 <AM0PR0502MB401986C8C45B584010830E2DD2770@AM0PR0502MB4019.eurprd05.prod.outlook.com>
 <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: <AM4PR05MB3155D92B5A3D5084CD69DC05D2730@AM4PR05MB3155.eurprd05.prod.outlook.com>
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-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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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....