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 1DF2EA0562; Sat, 4 Apr 2020 20:03:16 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 18A5A4C8A; Sat, 4 Apr 2020 20:03:15 +0200 (CEST) Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05on2052.outbound.protection.outlook.com [40.107.21.52]) by dpdk.org (Postfix) with ESMTP id CA7EA3B5; Sat, 4 Apr 2020 20:03:12 +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=pK5xa5GNixPJ+FAyOsI/EQVTectWECu+ZnUPYr8+TUU=; b=4P8G9A1RDMJ2yC/JBmaCYz26moZkgZGV5eQHDSS8RJlwvDEhoNup41HTTBuW4srrLU3s8llauLWRt8NcQskdbN35/XKL86r2HSrqC7hxkqHKoXcjS78DAycOK041OkMXh2O7RQdd8hXfUq2yB7TLrN3QDwZmZ8IvSnvZCAaEoM0= Received: from DB8PR06CA0031.eurprd06.prod.outlook.com (2603:10a6:10:100::44) by AM0PR08MB4145.eurprd08.prod.outlook.com (2603:10a6:208:133::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2878.16; Sat, 4 Apr 2020 18:03:10 +0000 Received: from DB5EUR03FT057.eop-EUR03.prod.protection.outlook.com (2603:10a6:10:100:cafe::ca) by DB8PR06CA0031.outlook.office365.com (2603:10a6:10:100::44) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2878.15 via Frontend Transport; Sat, 4 Apr 2020 18:03:10 +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 DB5EUR03FT057.mail.protection.outlook.com (10.152.20.235) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2856.17 via Frontend Transport; Sat, 4 Apr 2020 18:03:10 +0000 Received: ("Tessian outbound 1425309d4c0b:v50"); Sat, 04 Apr 2020 18:03:10 +0000 X-CR-MTA-TID: 64aa7808 Received: from 6ac91f12749a.2 by 64aa7808-outbound-1.mta.getcheckrecipient.com id E7550612-0384-4FF2-88D6-4946D56C0EA2.1; Sat, 04 Apr 2020 18:03:05 +0000 Received: from EUR03-AM5-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 6ac91f12749a.2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Sat, 04 Apr 2020 18:03:05 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jGyP4aRhKmSFVr9nGgpIPG+XZl5sDqwaqk4txzE9i7eN7w7UxvGZQfDYmu5Cqj+F/MpeU/maPdDYB21T1+USnHiosO93Zwx0AJJ9lobbKwaQBtXOrKtNJNJOGvx+3Znv3My04mU3W+YzKEoqUbJwQInsEK+6aaqqaR1sLvdkvYP/uW0+5azvVHUfqvDoxAmIaib56sEast0GaBMTosyXwhVm2bcIstVNYoGCGnDP6LXuiyiMRbZO2aNK4sv6/u0Wq8zFbhBIPeBT1K0iqu5QpEXu88W7mGuFwjgwNdJLletkKkS/FoDwVaoGrgcbwFTJaCU4kVt1kaSfvIVbPau/Jw== 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=pK5xa5GNixPJ+FAyOsI/EQVTectWECu+ZnUPYr8+TUU=; b=lwZQjMwzgsQuH6dcugrgxenAV47kJc6/NVKxPfi35zHg8HuY1jFPa7TTrmYnwUJzrUDR4xynVP/558Bzr75Ury6nGTBtaCPPU3fMN3LR/wbCzcfCO7w/CXs0gvnKu9dc8j/8cSFeCeM8/XaRfuweq/PVsD7XOsLJ3ud6V9izRHTE69aGdYCWGitTRm7XA3KisBUzxTYj6NZOjwuCqS6FdudZi6Jlpdm0q732ILLBMRf6SGr6213MIxW6NlIKBugIflFMWvupYow/YUVs5u+PqK6A9HVMG5CP2IutvLrfb9lwGKmKCrHwGxWf0iRhe/2FKjR5o816MsbxR71bTMMUQw== 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=pK5xa5GNixPJ+FAyOsI/EQVTectWECu+ZnUPYr8+TUU=; b=4P8G9A1RDMJ2yC/JBmaCYz26moZkgZGV5eQHDSS8RJlwvDEhoNup41HTTBuW4srrLU3s8llauLWRt8NcQskdbN35/XKL86r2HSrqC7hxkqHKoXcjS78DAycOK041OkMXh2O7RQdd8hXfUq2yB7TLrN3QDwZmZ8IvSnvZCAaEoM0= Received: from DBBPR08MB4646.eurprd08.prod.outlook.com (10.255.79.144) by DBBPR08MB4233.eurprd08.prod.outlook.com (20.179.44.151) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2878.16; Sat, 4 Apr 2020 18:03:03 +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.2878.018; Sat, 4 Apr 2020 18:03:03 +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" , Honnappa Nagarahalli , nd Thread-Topic: [PATCH v3 09/12] service: avoid race condition for MT unsafe service Thread-Index: AQHWCa8oxGah6HGPZEGhfjgtjJWkFahn43Mg Date: Sat, 4 Apr 2020 18:03:03 +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: d4087728-4b0a-4921-b7b9-9f2cd5a0341f.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: 22659f61-2546-4f37-cc80-08d7d8c26fd0 x-ms-traffictypediagnostic: DBBPR08MB4233:|DBBPR08MB4233:|AM0PR08MB4145: 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:6108;OLM:6108; x-forefront-prvs: 03630A6A4A 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)(396003)(136003)(39860400002)(366004)(346002)(376002)(6506007)(66946007)(26005)(5660300002)(7696005)(64756008)(66476007)(81156014)(81166006)(186003)(33656002)(4326008)(66556008)(8676002)(66446008)(54906003)(110136005)(71200400001)(52536014)(316002)(7416002)(55016002)(76116006)(9686003)(86362001)(8936002)(2906002)(478600001); 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: RqptqIVpnQKwtXrZ36kOHEFa5IyBlgJWvZz1WB08G9vsv6Xo0UDhpB+hTiyo81GcDsz7EsVBNb0S9QEHSIr/OA8lUyOrhAK0aU4mdEnIg4f1EbKP/nby3V/6DlcyaEx902TILIs50cs61XQmM6pCc5Qsb1W13Onm+1Z9Q+MKH6Nvh/G6+pOiFp0mgPoVula2lbM1+1hcZxGBo9OK7svzkJoo508HIvBljUZ/KsbZY0L7GKR3zXLdF/929ez/u2d7kkFSn85RkJj8RtQVhCokFHZP2btKqJQ3Onj4qTLt7xJWBji36jUp61skpAzrqUBKXNWJ4ybF3HW5twwgkrLdmyBLT77VqEibmWpEdOVDtMBMWNfoRObris1BE8GFpombwStUtcmlUfaB+K+MaAjjXbNb3sLqeJ8gsQmUZhTuRptFC8qkWj10mWMLohXaClKU x-ms-exchange-antispam-messagedata: V8e0k6HRjasG93ts7wwmSXdklFZiNuqRLqoNkXQ77C3GOKc9A/gMPb5h651v9BGX2TXDhwNFs9o+nYteq+KF5FcT7MH9nlmfiLOTWsIyidQbO5SXPD16CTjREk4+IqUc3EThzSLPHjB9+2By+B7LXQ== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR08MB4233 Original-Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Honnappa.Nagarahalli@arm.com; X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: DB5EUR03FT057.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)(346002)(376002)(136003)(39850400004)(396003)(46966005)(9686003)(81156014)(316002)(5660300002)(81166006)(8676002)(52536014)(110136005)(55016002)(86362001)(8936002)(26005)(2906002)(54906003)(450100002)(186003)(6506007)(478600001)(70206006)(47076004)(4326008)(26826003)(336012)(33656002)(70586007)(356004)(7696005)(82740400003); DIR:OUT; SFP:1101; X-MS-Office365-Filtering-Correlation-Id-Prvs: 9f80c59f-a375-4527-ffb6-08d7d8c26bb1 X-Forefront-PRVS: 03630A6A4A X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: qFRIf33yC6ic/Fj1mXrCTdWn5fVwIprgHpOqSZzKWlIMHdBRwDPdvIowyv6VA0elV9O+FHo3+yrRzUI5hm7zRErQxpAGE01EzUIXCCIxzEiD3OAxEPmnvV6/b6mNxmjYHpb0ICKr6EqYkcyjsSGUKpOvzHzXW2VF5KAF6YlroZG8WaZOyB7l5bg4kDpHx0D+98nZJkI1U+SY8Fot8TiLWapKX/h4MDAzGNJiNUuP39vXQPp88QIPN/Nj42jnZZA1eSwLAGf6UGJz3UhjxbpY4Kx+/fc42UGIPsCY2Kby2u5RT2Y92M6+RtFYlNS4Zee47OK7C5D9bjjc6jsJvywNpVDP+VpaKT3YbYYPXPNs0Zwb1PkOrfuDFna/AZSvsc/glbkttKbBPvOPuacKP2G83AKpVTH08heGXv7vKhW3WYDCMzbl+rGp0HUDoUBWAWWpB7VqNFkshDYBF6rdNVNW5uNRFLLQ38Jb/QFVVi1PKJtsMOwiBk8jFDk/0XEpc3ecH8oiXpz+AdjtqKhQvqKUrQ== X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Apr 2020 18:03:10.7214 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 22659f61-2546-4f37-cc80-08d7d8c26fd0 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: AM0PR08MB4145 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 >=20 > We should put "fix" in the title, once converged on an implementation. Ok, will replace 'avoid' with 'fix' (once we agree on the solution) >=20 > 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. >=20 >=20 > This fix (always taking the atomic lock) will have a negative performance > impact on existing code using services. We should investigate a way to fi= x 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) perfo= rmance impact. a) This can be solved by setting the service to MT safe and can be documen= ted. This might be a reasonable solution for applications which are compili= ng with future DPDK releases. b) We can also solve this using symbol versioning - the old version of thi= s 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 DP= DK releases without recompiling, it will continue to use the old version. I= f the application is compiled=20 with future releases, they can use solution in 2a. We al= so should think if this is an appropriate solution as this would force 1) t= o 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 a= pplications might see some improvements as this patch removes few instructi= ons. >=20 > 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. >=20 > 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 g= oing to use counters they have to be atomic, which means additional cycles = in the data path. > 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 "nee= ds > atomic" flag. >=20 > 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. >=20 > It must be possible to avoid the datapath overhead using a scheme like th= is. It > will likely be more complex than your proposed change below, however if i= t > 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 c= an elaborate more. But, it seems to be based on a counter approach. Followi= ng 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 atom= ic counter other than 'num_mapped_cores'. Let us call that counter 'num_cur= rent_cores'. The code to call the service would look like below. 1) rte_atomic32_inc(&num_current_cores); /* this results in a full memory b= arrier */ 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 mem= ory barrier for any architecture */ 3) run_service(); /* Call the service */ 4) } 5) rte_atomic32_sub(&num_current_cores); /* Calling rte_atomic32_clear is n= ot enough as it is not an atomic operation and does not provide the require= d memory barrier */ But the above code has race conditions in lines 1 and 2. It is possible tha= t none of the cores will ever get to run the service as they all could simu= ltaneously increment the counter. Hence lines 1 and 2 together need to be a= tomic, which is nothing but 'compare-exchange' operation. BTW, the current code has a bug where it calls 'rte_atomic_clear(&s->execut= e_lock)', it is missing memory barriers which results in clearing the execu= te_lock before the service has completed running. I suggest changing the 'e= xecute_lock' to rte_spinlock_t and using rte_spinlock_try_lock and rte_spin= lock_unlock APIs. >=20 > A unit test is required to validate a fix like this - although perhaps fo= und by > inspection/review, a real-world test to validate would give confidence. Agree, need to have a test case. >=20 >=20 > Thoughts on such an approach? >=20 >=20 >=20 > > --- > > lib/librte_eal/common/rte_service.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/lib/librte_eal/common/rte_service.c > > b/lib/librte_eal/common/rte_service.c > > index 557b5a9..32a2f8a 100644 > > --- a/lib/librte_eal/common/rte_service.c > > +++ b/lib/librte_eal/common/rte_service.c > > @@ -50,6 +50,10 @@ struct rte_service_spec_impl { > > uint8_t internal_flags; > > > > /* per service statistics */ > > + /* Indicates how many cores the service is mapped to run on. > > + * It does not indicate the number of cores the service is running > > + * on currently. > > + */ > > rte_atomic32_t num_mapped_cores; > > uint64_t calls; > > uint64_t cycles_spent; > > @@ -370,12 +374,7 @@ service_run(uint32_t i, struct core_state *cs, > > uint64_t service_mask, > > > > cs->service_active_on_lcore[i] =3D 1; > > > > - /* check do we need cmpset, if MT safe or <=3D 1 core > > - * mapped, atomic ops are not required. > > - */ > > - const int use_atomics =3D (service_mt_safe(s) =3D=3D 0) && > > - (rte_atomic32_read(&s- > >num_mapped_cores) > 1); > > - if (use_atomics) { > > + if (service_mt_safe(s) =3D=3D 0) { > > if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1)) > > return -EBUSY; > > > > -- > > 2.7.4