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 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 To: "Van Haaren, Harry" , Phil Yang , "thomas@monjalon.net" , "Ananyev, Konstantin" , "stephen@networkplumber.org" , "maxime.coquelin@redhat.com" , "dev@dpdk.org" CC: "david.marchand@redhat.com" , "jerinj@marvell.com" , "hemant.agrawal@nxp.com" , Gavin Hu , Ruifeng Wang , Joyce Kong , nd , "stable@dpdk.org" , nd , Honnappa Nagarahalli , nd 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: 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> In-Reply-To: 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: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > > > > > > > > > > Subject: [PATCH v3 09/12] service: avoid race condition for MT > > > > > > unsafe service > > > > > > > > > > > > From: Honnappa Nagarahalli > > > > > > > > > > > > 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 > > > > > > > > > > > > Reviewed-by: Phil Yang > > > > > > Reviewed-by: Gavin Hu > > > > > > > > > > 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? > > > > > > > >