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 6D31AA0598;
Sat, 18 Apr 2020 08:21:53 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 2D48D1DEB1;
Sat, 18 Apr 2020 08:21:52 +0200 (CEST)
Received: from EUR02-HE1-obe.outbound.protection.outlook.com
(mail-eopbgr10085.outbound.protection.outlook.com [40.107.1.85])
by dpdk.org (Postfix) with ESMTP id 76F121DEAF;
Sat, 18 Apr 2020 08:21:50 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com;
s=selector2-armh-onmicrosoft-com;
h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;
bh=y2y1wv9Z4+l5bOfxBkg/SfJGa+MnGMArrlelIg7NvIg=;
b=DPZISsOv9vXY9FEScUyblVUIXhS2hpCckWuZKueKRJL1b4MCxknECqOKhZKO4uUDT1+YQT2zC4UnkdjB+lxBOtmyw++cH380AcaWaS/3QmsHs4Uj1oALZRtHg+3pBMUXV/XZNWZaJdjlgy3tB5B2irRW5xTGqbeyB7+KZeb487w=
Received: from DB6PR07CA0057.eurprd07.prod.outlook.com (2603:10a6:6:2a::19) by
AM0PR08MB4019.eurprd08.prod.outlook.com (2603:10a6:208:128::20) with
Microsoft SMTP Server (version=TLS1_2,
cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2921.25; Sat, 18 Apr
2020 06:21:48 +0000
Received: from DB5EUR03FT059.eop-EUR03.prod.protection.outlook.com
(2603:10a6:6:2a:cafe::c4) by DB6PR07CA0057.outlook.office365.com
(2603:10a6:6:2a::19) with Microsoft SMTP Server (version=TLS1_2,
cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2937.6 via Frontend
Transport; Sat, 18 Apr 2020 06:21:48 +0000
Authentication-Results: spf=pass (sender IP is 63.35.35.123)
smtp.mailfrom=arm.com; dpdk.org; dkim=pass (signature was verified)
header.d=armh.onmicrosoft.com;dpdk.org; dmarc=bestguesspass action=none
header.from=arm.com;
Received-SPF: Pass (protection.outlook.com: domain of arm.com designates
63.35.35.123 as permitted sender) receiver=protection.outlook.com;
client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com;
Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by
DB5EUR03FT059.mail.protection.outlook.com (10.152.21.175) with
Microsoft SMTP
Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
15.20.2900.15 via Frontend Transport; Sat, 18 Apr 2020 06:21:48 +0000
Received: ("Tessian outbound 43fc5cd677c4:v53");
Sat, 18 Apr 2020 06:21:48 +0000
X-CR-MTA-TID: 64aa7808
Received: from 2573bd32ed40.2
by 64aa7808-outbound-1.mta.getcheckrecipient.com id
2C0B1DED-3F0A-4D9A-AAD6-4FFDA184C8FD.1;
Sat, 18 Apr 2020 06:21:43 +0000
Received: from EUR03-AM5-obe.outbound.protection.outlook.com
by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 2573bd32ed40.2
(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384);
Sat, 18 Apr 2020 06:21:43 +0000
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;
b=a0sQwmR0vTRzBImuV581ZJYkFZVRZWFskxxumucURaz0KaJ0xGrHf7C4JeWX/Sa4JV6loZ1VUKwyBx365DmcdVwsTXHd33HB080I+T5fjXMLmvF8egUfV7lTxKBWkwuK2zso6EffEagG81crGmGdIiwihKU3hG9YVf1AyumVkfmJ2fvZMlKWua+iNgFA/p9GOnhG79rBAZhwtO7QQBT4iBtmUtBxQNtN7r34mIq58dERXyG/bpcSiqQEuz7ysilmspcaG8pLEAq45J8EdAdCAaLcIfggpG0oy+RVfk0Se/vT8xAqtQ+6PJCVPaGTHzSqBeEw3iEDMgOU629PavQBWQ==
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=y2y1wv9Z4+l5bOfxBkg/SfJGa+MnGMArrlelIg7NvIg=;
b=Ndq7vSfirg3Mv15bke0dGZTmxmGl42/6CAikvEafZmN6wookYb21wP0ovdf/LelCKOMKpwnvQhNk0TL6HDCwFnmiYH/+zyTuMrTySBCLH7bLLf0WWsPV/9ZGVW4c82TLvxa7KoPcENgMAIQoQLTBZo6NLkulWXH3OvRYpzeb6b8DQ8YdDCKGcpnvoA1UPjCqKGJXTTupXupdmJfT4tNJut6M4rTL0DD055/M2jBUG2NQxR/FYt4BOswag2gJo70yU/8BmgGCW0NiUdizhdGAjEcKdugpjBncKUDn3FFVtNbPXK40k7XCrsw7ha/YaTKoBf+d4io8jJLQ3aGpkRFTaw==
ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass
smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass
header.d=arm.com; arc=none
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com;
s=selector2-armh-onmicrosoft-com;
h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;
bh=y2y1wv9Z4+l5bOfxBkg/SfJGa+MnGMArrlelIg7NvIg=;
b=DPZISsOv9vXY9FEScUyblVUIXhS2hpCckWuZKueKRJL1b4MCxknECqOKhZKO4uUDT1+YQT2zC4UnkdjB+lxBOtmyw++cH380AcaWaS/3QmsHs4Uj1oALZRtHg+3pBMUXV/XZNWZaJdjlgy3tB5B2irRW5xTGqbeyB7+KZeb487w=
Received: from DBBPR08MB4646.eurprd08.prod.outlook.com (2603:10a6:10:f5::16)
by DBBPR08MB4361.eurprd08.prod.outlook.com (2603:10a6:10:c8::21) with
Microsoft SMTP Server (version=TLS1_2,
cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2900.26; Sat, 18 Apr
2020 06:21:40 +0000
Received: from DBBPR08MB4646.eurprd08.prod.outlook.com
([fe80::1870:afc4:b90f:609d]) by DBBPR08MB4646.eurprd08.prod.outlook.com
([fe80::1870:afc4:b90f:609d%5]) with mapi id 15.20.2921.027; Sat, 18 Apr 2020
06:21:40 +0000
From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
To: "Van Haaren, Harry" <harry.van.haaren@intel.com>, Phil Yang
<Phil.Yang@arm.com>, "thomas@monjalon.net" <thomas@monjalon.net>, "Ananyev,
Konstantin" <konstantin.ananyev@intel.com>, "stephen@networkplumber.org"
<stephen@networkplumber.org>, "maxime.coquelin@redhat.com"
<maxime.coquelin@redhat.com>, "dev@dpdk.org" <dev@dpdk.org>
CC: "david.marchand@redhat.com" <david.marchand@redhat.com>,
"jerinj@marvell.com" <jerinj@marvell.com>, "hemant.agrawal@nxp.com"
<hemant.agrawal@nxp.com>, Gavin Hu <Gavin.Hu@arm.com>, Ruifeng Wang
<Ruifeng.Wang@arm.com>, Joyce Kong <Joyce.Kong@arm.com>, nd <nd@arm.com>,
"stable@dpdk.org" <stable@dpdk.org>, nd <nd@arm.com>, Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com>, nd <nd@arm.com>
Thread-Topic: [PATCH v3 09/12] service: avoid race condition for MT unsafe
service
Thread-Index: AQHWCa8oxGah6HGPZEGhfjgtjJWkFahn43MggAep2gCAABczAIABZOQAgA1trPA=
Date: Sat, 18 Apr 2020 06:21:40 +0000
Message-ID: <DBBPR08MB464683996C2AFF93FB7D149298D60@DBBPR08MB4646.eurprd08.prod.outlook.com>
References: <1583999071-22872-1-git-send-email-phil.yang@arm.com>
<1584407863-774-1-git-send-email-phil.yang@arm.com>
<1584407863-774-10-git-send-email-phil.yang@arm.com>
<BYAPR11MB3143B499A98B511E39EC6F5AD7C70@BYAPR11MB3143.namprd11.prod.outlook.com>
<DBBPR08MB4646018C7047165A829D11DD98C40@DBBPR08MB4646.eurprd08.prod.outlook.com>
<BYAPR11MB31431BFA916927D037D05122D7C00@BYAPR11MB3143.namprd11.prod.outlook.com>
<AM6PR08MB4644AD62E177324970AD749F98C10@AM6PR08MB4644.eurprd08.prod.outlook.com>
<BYAPR11MB31433CA5C231E89AB60DF4BED7C10@BYAPR11MB3143.namprd11.prod.outlook.com>
In-Reply-To: <BYAPR11MB31433CA5C231E89AB60DF4BED7C10@BYAPR11MB3143.namprd11.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-ts-tracking-id: dd096f3b-46df-4cea-8cda-be33ac761694.0
x-checkrecipientchecked: true
Authentication-Results-Original: spf=none (sender IP is )
smtp.mailfrom=Honnappa.Nagarahalli@arm.com;
x-originating-ip: [70.113.25.165]
x-ms-publictraffictype: Email
X-MS-Office365-Filtering-HT: Tenant
X-MS-Office365-Filtering-Correlation-Id: e4b06e21-c090-4822-96bb-08d7e360c6d7
x-ms-traffictypediagnostic: DBBPR08MB4361:|DBBPR08MB4361:|AM0PR08MB4019:
x-ld-processed: f34e5979-57d9-4aaa-ad4d-b122a662184d,ExtAddr
x-ms-exchange-transport-forked: True
X-Microsoft-Antispam-PRVS: <AM0PR08MB40190E4D50A2DADF6DCBB65598D60@AM0PR08MB4019.eurprd08.prod.outlook.com>
x-checkrecipientrouted: true
nodisclaimer: true
x-ms-oob-tlc-oobclassifiers: OLM:8273;OLM:8273;
x-forefront-prvs: 0377802854
X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255; CTRY:; LANG:en;
SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DBBPR08MB4646.eurprd08.prod.outlook.com;
PTR:; CAT:NONE; SFTY:;
SFS:(10009020)(4636009)(346002)(136003)(396003)(376002)(366004)(39860400002)(6506007)(2906002)(9686003)(4326008)(71200400001)(54906003)(8936002)(316002)(55016002)(30864003)(81156014)(478600001)(5660300002)(110136005)(7696005)(26005)(66446008)(76116006)(66946007)(64756008)(66476007)(33656002)(86362001)(8676002)(66556008)(186003)(52536014)(7416002);
DIR:OUT; SFP:1101;
received-spf: None (protection.outlook.com: arm.com does not designate
permitted sender hosts)
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam-Untrusted: BCL:0;
X-Microsoft-Antispam-Message-Info-Original: KlVOW4RbX8TJESBE/w2VvZMuEBa3nWjSl55hj9eMngqYxmjc1phuyyOfAcmoxIRyAy34S2whMRiIBwyRnlfqifZ5hPyY9RIZJ/kCb+Gh9aHQSbvaWxqm8SV7C++HtbmFNDykrWVmDR6VqLKjqn+Vzd1w7U6ihDXDWBJv/bzyKBvJG12wCymIzIDX8v1AM1ZmEkz/Z/POGrj67a4Ntf8+k8teyClORS+YsTjfr+WKoilp7rlSj+/KUxmjqbo+4GRgJ4G/YD108nm8UY9Xsb2/iecJs3tr/5xR1K6+gWd6Pf5itqm4dc+T2QB+Qkd0bRfKt8T8FPCuYVumUHeopi6hpeY6Dfng19WQFeg7fLvsL4Aq/u3YxyQ5aiHLdBQZ6wJJPXGsfQnYzWMwaWrSppyuH6RtQsZDSfu9blIxM2hQ/3CkcOtup8OjVPNMVtfyXBJt
x-ms-exchange-antispam-messagedata: APZKCMvZC8184D/VE6zo8nEGJ5bd2uWG/1Mik+V+H4A/J2xeRoczG2jTsDIAkkCysQuK9dVBNmmBaJfPTHOD+KHGsE+S7Eet0EzGACgJ+T73+rW45Omkm2rS9aMeFXPPk3q1t3vMdxKA/CTy8w7UdQ==
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR08MB4361
Original-Authentication-Results: spf=none (sender IP is )
smtp.mailfrom=Honnappa.Nagarahalli@arm.com;
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped: DB5EUR03FT059.eop-EUR03.prod.protection.outlook.com
X-Forefront-Antispam-Report: CIP:63.35.35.123; CTRY:IE; LANG:en; SCL:1; SRV:;
IPV:CAL; SFV:NSPM; H:64aa7808-outbound-1.mta.getcheckrecipient.com;
PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com; CAT:NONE; SFTY:;
SFS:(10009020)(4636009)(396003)(346002)(376002)(136003)(39860400002)(46966005)(86362001)(7696005)(54906003)(110136005)(52536014)(33656002)(5660300002)(30864003)(82740400003)(81166007)(6506007)(478600001)(8676002)(26005)(8936002)(186003)(2906002)(4326008)(81156014)(47076004)(316002)(70586007)(450100002)(356005)(55016002)(70206006)(336012)(9686003);
DIR:OUT; SFP:1101;
X-MS-Office365-Filtering-Correlation-Id-Prvs: 1b5c0b4c-978d-4973-ef99-08d7e360c1ca
X-Forefront-PRVS: 0377802854
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: e051SYJSsWuNZ43uqRsSNyXWZRlEUvse1Ym/Ug16y+bN85m7pYQcy1JVHNbxUHYCrR34SWi+6SM7XgVVYrDNOjmvtaYkAFzBZk16fwuWd4Qr/bHr+PkyodYyu38trUpGNVwE6Lsh8PkkSc8Nv0t2gKn3wQib05MPrHuCQ0uju2FButTzFlibQ8bxIJnPd3FX8O9a4ByeB5Njzb3v7Bh3HRblcXVzkM0SniKFJXANA13XKcehgzCtNs/nSrHGjb4ha8JNfGw/A4zzNEB5z/uShZXMN7Rx8sb/SF2adxymtcXHUzaEb3WVwM564W4LlrsHXHYpMf2wNcqambnF6y5nbGPg6oOseaFX/IZAmi5Y7muZp84N04aZEgF9QcQdWvlDWqRwK+h51pvBG6MwJot/SYN1VpfV+C9SylSKmdf7TodxB4xE7OOZuyLXPrJNGZdhcJAyFu96WOEg5zaJmc2vQq5RQnph00RHqrL3WOxqSdmvpjRDgfdlqrvW1N6fq7TA3iH/qpV87tfq+IFG0WBsxQ==
X-OriginatorOrg: arm.com
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2020 06:21:48.8477 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: e4b06e21-c090-4822-96bb-08d7e360c6d7
X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d; Ip=[63.35.35.123];
Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR08MB4019
Subject: Re: [dpdk-dev] [PATCH v3 09/12] service: avoid race condition for
MT unsafe service
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>
<snip>
> > > >
> > > > > > Subject: [PATCH v3 09/12] service: avoid race condition for MT
> > > > > > unsafe service
> > > > > >
> > > > > > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > > >
> > > > > > There has possible that a MT unsafe service might get
> > > > > > configured to run on another core while the service is running
> currently.
> > > > > > This might result in the MT unsafe service running on multiple
> > > > > > cores simultaneously. Use 'execute_lock' always when the
> > > > > > service is MT unsafe.
> > > > > >
> > > > > > Fixes: e9139a32f6e8 ("service: add function to run on app
> > > > > > lcore")
> > > > > > Cc: stable@dpdk.org
> > > > > >
> > > > > > Signed-off-by: Honnappa Nagarahalli
> > > > > > <honnappa.nagarahalli@arm.com>
> > > > > > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > > > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > >
> > > > > We should put "fix" in the title, once converged on an implementa=
tion.
> > > > Ok, will replace 'avoid' with 'fix' (once we agree on the
> > > > solution)
> > > >
> > > > >
> > > > > Regarding Fixes and stable backport, we should consider if
> > > > > fixing this in
> > > > stable
> > > > > with a performance degradation, fixing with more complex
> > > > > solution, or documenting a known issue a better solution.
> > > > >
> > > > >
> > > > > This fix (always taking the atomic lock) will have a negative
> > > > > performance impact on existing code using services. We should
> > > > > investigate a way to fix
> > > > it
> > > > > without causing datapath performance degradation.
> > > > Trying to gauge the impact on the existing applications...
> > > > The documentation does not explicitly disallow run time mapping of
> > > > cores to service.
> > > > 1) If the applications are mapping the cores to services at run
> > > > time, they are running with a bug. IMO, bug fix resulting in a
> > > > performance drop should be acceptable.
> > > > 2) If the service is configured to run on single core
> > > > (num_mapped_cores =3D=3D 1), but service is set to MT unsafe - this
> > > > will have a (possible) performance impact.
> > > > a) This can be solved by setting the service to MT safe and can
> > > > be documented. This might be a reasonable solution for
> > > > applications which are compiling with
> > > > future DPDK releases.
> > > > b) We can also solve this using symbol versioning - the old
> > > > version of this function will use the old code, the new version of
> > > > this function will use the code in
> > > > this patch. So, if the application is run with
> > > > future DPDK releases without recompiling, it will continue to use
> > > > the old version. If the application is compiled
> > > > with future releases, they can use solution in 2=
a.
> > > > We also should think if this is an appropriate solution as this
> > > > would force 1) to recompile to get the fix.
> > > > 3) If the service is configured to run on multiple cores
> > > > (num_mapped_cores > 1), then for those applications, the lock is
> > > > being taken already. These applications might see some
> > > > improvements as this patch removes few instructions.
> > > >
> > > > >
> > > > > I think there is a way to achieve this by moving more
> > > > > checks/time to the control path (lcore updating the map), and
> > > > > not forcing the datapath lcore to always take an atomic.
> > > > I think 2a above is the solution.
> > >
> > > 2a above is e.g. the Eventdev SW routines like Rx/Tx scheduler servic=
es.
> > I scanned through the code briefly
> > I see that Eth RX/TX, Crypto adapters are setting the MT_SAFE
> > capabilities, can be ignored.
> > Timer adaptor and some others do not set MT_SAFE. Seems like the cores
> > to run on are mapped during run time. But it is not clear to me if it
> > can get mapped to run on multiple cores. If they are, they are running =
with
> the bug.
>=20
> EAL will map each service to a single lcore. It will "round-robin" if the=
re are
> more services than service-lcores to run them on. So agree that DPDK's
> default mappings will not suffer this issue.
>=20
>=20
> > But, these are all internal to DPDK and can be fixed.
> > Are there no performance tests in these components that we can run?
> >
> > > We should strive to not reduce datapath performance at all here.
> > >
> > >
> > > > > In this particular case, we have a counter for number of
> > > > > iterations that a
> > > > Which counter are you thinking about?
> > > > All the counters I checked are not atomic operations currently. If
> > > > we are going to use counters they have to be atomic, which means
> > > > additional cycles in the data path.
> > >
> > > I'll try to explain the concept better, take this example:
I tried to implement this algorithm, but there are few issues, please see b=
elow.
> > > - One service core is mapped to a MT_UNSAFE service, like event/sw
> > > pmd
> > > - Application wants to map a 2nd lcore to the same service
> > > - You point out that today there is a race over the lock
> > > -- the lock is not taken if (num_mapped_lcores =3D=3D 1)
> > > -- this avoids an atomic lock/unlock on the datapath
> > >
> > > To achieve our desired goal;
> > > - control thread doing mapping performs the following operations
> > > -- write service->num_mapped_lcores++ (atomic not required, only
> > > single- writer allowed by APIs)
> > This has to be atomic because of rte_service_run_iter_on_app_lcore API.
> > Performance should be fine as this API is not called frequently. But
> > need to consider the implications of more than one thread updating
> num_mapped_cores.
> >
> > > -- MFENCE (full st-ld barrier) to flush write, and force later
> > > loads to
> > issue
> > > after
> > I am not exactly sure what MFENCE on x86 does. On Arm platforms, the
> > full barrier (DMB ISH) just makes sure that memory operations are not
> > re-ordered around it. It does not say anything about when that store
> > is visible to other cores. It will be visible at some point in time to =
cores.
> > But, I do not think we need to be worried about flushing to memory.
> >
> > > -- read the "calls_per_service" counter for each lcores, add them=
up.
> > This can be trimmed down to the single core the service is mapped to
> > currently, no need to add all the counters.
>=20
> Correct - however that requires figuring out which lcore is running the s=
ervice.
> Anyway, agree - it's an implementation detail as to exactly how we detect=
it.
>=20
> >
> > > ---- Wait :)
> > > -- re-read the "calls_per_service", and ensure the count has chan=
ged.
Here, there is an assumption that the service core function is running on t=
he service core. If the service core is not running, the code will be stuck=
in this polling loop.
I could not come up with a good way to check if the service core is running=
. Checking the app_runstate and comp_runstate is not enough as they just in=
dicate that the service is ready to run. Using the counter 'calls_per_servi=
ce' introduces race conditions.
Only way I can think of is asking the user to follow a specific sequence of=
APIs to ensure the service core is running before calling rte_service_map_=
lcore_set.
> > Basically, polling. This causes more traffic on the interconnect
> > between the cores. But might be ok since this API might not be called
> frequently.
>=20
> Agree this will not be called frequently, and that some polling here will=
not be
> a problem.
>=20
>=20
> > > ---- The fact that the calls_per_service has changed ensures the
> > service-
> > > lcore
> > > has seen the new "num_mapped_cores" value, and has now
> > > taken the lock!
> > > -- *now* it is safe to map the 2nd lcore to the service
> > >
> > > There is a caveat here that the increments to the "calls_per_service"
> > variable
> > > must become globally-observable. To force this immediately would
> > > require a write memory barrier, which could impact datapath
> > > performance. Given the service is now taking a lock, the unlock()
> > > thereof would ensure the "calls_per_service"
> > > is flushed to memory.
> > If we increment this variable only when the lock is held, we should be =
fine.
> > We could have a separate variable.
>=20
> Sure, if a separate variable is preferred that's fine with me.
>=20
>=20
> > > Note: we could use calls_per_service, or add a new variable to the
> > > service struct.
> > > Writes to this do not need to be atomic, as it is either mapped to a
> > > single
> > core,
> > > or else there's a lock around it.
> > I think it is better to have a separate variable that is updated only
> > when the lock is held.
> > I do not see any change in API sequence. We do this hand-shake only if
> > the service is running (which is all controlled in the writer thread), =
correct?
>=20
> Yes this increment can be localized to just the branch when the unlock()
> occurs, as that is the only time it could make a difference.
>=20
> > This does not solve the problem with rte_service_run_iter_on_app_lcore
> > getting called on multiple cores concurrently for the same service.
>=20
> Agreed. This "on_app_lcore" API was an addition required to enable unit-
> testing in a sane way, to run iterations of eg Eventdev PMD.
>=20
> I am in favor of documenting that the application is responsible to ensur=
e the
> service being run on a specific application lcore is not concurrently run=
ning on
> another application lcore.
>=20
>=20
> > > > > service has done. If this increments we know that the lcore
> > > > > running the service has re-entered the critical section, so
> > > > > would see an updated "needs atomic" flag.
> > > > >
> > > > > This approach may introduce a predictable branch on the
> > > > > datapath, however the cost of a predictable branch vs always
> > > > > taking an atomic is order(s?) of magnitude, so a branch is much
> preferred.
> > > > >
> > > > > It must be possible to avoid the datapath overhead using a
> > > > > scheme like
> > > this.
> > > > It
> > > > > will likely be more complex than your proposed change below,
> > > > > however if it avoids datapath performance drops I feel that a
> > > > > more complex solution is worth investigating at least.
> > >
> > > > I do not completely understand the approach you are proposing, may
> > > > be you can elaborate more.
> > >
> > > Expanded above, showing a possible solution that does not require
> > > additional atomics on the datapath.
> > >
> > >
> > > > But, it seems to be based on a counter approach. Following is my
> > > > assessment on what happens if we use a counter. Let us say we kept
> > > > track of how many cores are running the service currently. We need
> > > > an atomic counter other than 'num_mapped_cores'. Let us call that
> > > > counter
> > > 'num_current_cores'.
> > > > The code to call the service would look like below.
> > > >
> > > > 1) rte_atomic32_inc(&num_current_cores); /* this results in a full
> > > > memory barrier */
> > > > 2) if (__atomic_load_n(&num_current_cores, __ATOMIC_ACQUIRE) =3D=3D=
1)
> > > > {
> > > > /* rte_atomic_read is not enough here as it does not provide the
> > > > required memory barrier for any architecture */
> > > > 3) run_service(); /* Call the service */
> > > > 4) }
> > > > 5) rte_atomic32_sub(&num_current_cores); /* Calling
> > > > rte_atomic32_clear is not enough as it is not an atomic operation
> > > > and does not provide the required memory barrier */
> > > >
> > > > But the above code has race conditions in lines 1 and 2. It is
> > > > possible that none of the cores will ever get to run the service
> > > > as they all could simultaneously increment the counter. Hence
> > > > lines 1 and
> > > > 2 together need to be atomic, which is nothing but 'compare-exchang=
e'
> > > operation.
> > > >
> > > > BTW, the current code has a bug where it calls
> > > > 'rte_atomic_clear(&s-
> > > > >execute_lock)', it is missing memory barriers which results in
> > > > >clearing the
> > > > execute_lock before the service has completed running. I suggest
> > > > changing the 'execute_lock' to rte_spinlock_t and using
> > > > rte_spinlock_try_lock and rte_spinlock_unlock APIs.
> > >
> > > I don't think a spinlock is what we want here:
> > >
> > > The idea is that a service-lcore can be mapped to multiple services.
> > > If one service is already being run (by another thread), we do not
> > > want to
> > spin
> > > here waiting for it to become "free" to run by this thread, it
> > > should
> > continue
> > > to the next service that it is mapped to.
> > Agree. I am suggesting to use 'rte_spinlock_try_lock' (does not spin)
> > which is nothing but 'compare-exchange'. Since the API is available,
> > we should make use of it instead of repeating the code.
>=20
> Ah apologies, I misread the spinlock usage. Sure if the spinlock_t code i=
s
> preferred I'm ok with a change. It would be clean to have a separate patc=
h in
> the patchset to make this change, and have it later in the set than the c=
hanges
> for backporting to ease integration with stable branch.
>=20
>=20
> > > > > A unit test is required to validate a fix like this - although
> > > > > perhaps found
> > > > by
> > > > > inspection/review, a real-world test to validate would give confi=
dence.
> > > > Agree, need to have a test case.
> > > >
> > > > >
> > > > >
> > > > > Thoughts on such an approach?
> > > > >
> > > <snip patch contents>