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 7F42EA0562; Fri, 3 Apr 2020 13:58:21 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E100C1C12C; Fri, 3 Apr 2020 13:58:13 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id CD32A1C0D4; Fri, 3 Apr 2020 13:58:11 +0200 (CEST) IronPort-SDR: N4+JaEiWbm0PfMfxISkltGStFmo5L5t1eRJ10iKrhTGdKnV4HLmvJzF3UU+vN3TZ1HApbRb7NI SRi8YuOh+Q7g== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Apr 2020 04:58:10 -0700 IronPort-SDR: zMOYl5CE90dDXCh2j7Ym8SzSnce4cD7uwe0HkNUXnrQsm2FpPWZY1yYUEiabnoWGXMHt0pzyWQ 5zWniCsqwv8w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,339,1580803200"; d="scan'208";a="285103497" Received: from orsmsx103.amr.corp.intel.com ([10.22.225.130]) by fmsmga002.fm.intel.com with ESMTP; 03 Apr 2020 04:58:10 -0700 Received: from orsmsx159.amr.corp.intel.com (10.22.240.24) by ORSMSX103.amr.corp.intel.com (10.22.225.130) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 3 Apr 2020 04:58:09 -0700 Received: from ORSEDG001.ED.cps.intel.com (10.7.248.4) by ORSMSX159.amr.corp.intel.com (10.22.240.24) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 3 Apr 2020 04:58:09 -0700 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.173) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 3 Apr 2020 04:58:09 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=heOy/X2gVwRAU8lW1cyZM1otyT/0PZOSWVlBYl292Iq6CE+IZg0rrKUlJu4vU8oR81435EB68m/X94e2vXDTgOsKGs9qbxX1d+vAVgnTD0wSCa6nKzqx+D71pQBIOW6ZA0nw8fLQQoDyUbkM5lAgkoif8b26migp4stNfIkQAQpCPxmWXOR2JFXiXhKwhzSRnYxYrDjKQL0iOP5a9hJgpi4QOoYz0IzTCXROrJm1QGukcB5fKBTzpJ052gzqMke8pNhdmF9Gc6x+zWU6aXpzMGrJmMGcavYuyQIZUW950jlNjtiblMP9rzQW7se9+KXN9GdgoT4muOl9Cfhil6AhbQ== 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=sU+yhH0QaeGhQcso9cQQ+8ojai5q9tnJlJGZheXjqwE=; b=MFnvC0B9y6i88h37vBx1R/VQTVSgSYNM4GjcdSYFfpW46o2ZcBOGxkob9wdTIr2s12GuARKeyK6IM5IqELOHHkX2Y8RYv7g/6we1QaPRa65XbDnzBHcH0o+DQ+SE8LUQQhTApgaYj5dRGuozOt6MtIRL3O/6e+U6ACb0JkZ+hIXyeoDu7vl3JeOr/7QFZwx7XZTYZmmjg+3IjfBU9uB6vtmvL7chfimMaCWjPMn1Cox9CKVQ7PMPXlwqFqOAzf5kLeefkwkSfIQPDN+IZLRNhgRqox5KOKBJSfgP7Ap5ITEwnVW0Y+mxtt7xDrYKVtMP8w0UKoTaWVWpfF3UpRw/Mw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=sU+yhH0QaeGhQcso9cQQ+8ojai5q9tnJlJGZheXjqwE=; b=UHK70A2NJjt8w+9EQ+ZRro4OxBvN2BXOfKF8TgNUhwpJ8vcoTP7s4WlGiXWItyD34lPxlg0izNzUsr7wrxvasPx4Mqwnzacvb5MVAUE0zMKFjfe9INxz3SmXxOAXTFMaOK2wiE47BpOjYz6qBDXlz9IejLmboic2f+xLArb/Mkg= Received: from BYAPR11MB3143.namprd11.prod.outlook.com (2603:10b6:a03:92::32) by BYAPR11MB3541.namprd11.prod.outlook.com (2603:10b6:a03:f5::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2878.15; Fri, 3 Apr 2020 11:58:08 +0000 Received: from BYAPR11MB3143.namprd11.prod.outlook.com ([fe80::5917:9757:49b0:3c49]) by BYAPR11MB3143.namprd11.prod.outlook.com ([fe80::5917:9757:49b0:3c49%5]) with mapi id 15.20.2878.014; Fri, 3 Apr 2020 11:58:08 +0000 From: "Van Haaren, Harry" To: 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" , "Honnappa.Nagarahalli@arm.com" , "gavin.hu@arm.com" , "ruifeng.wang@arm.com" , "joyce.kong@arm.com" , "nd@arm.com" , Honnappa Nagarahalli , "stable@dpdk.org" Thread-Topic: [PATCH v3 09/12] service: avoid race condition for MT unsafe service Thread-Index: AQHV+/one+9ki+cHakWt/XJq7A5CNahnSwOQ Date: Fri, 3 Apr 2020 11:58:08 +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: <1584407863-774-10-git-send-email-phil.yang@arm.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.2.0.6 authentication-results: spf=none (sender IP is ) smtp.mailfrom=harry.van.haaren@intel.com; x-originating-ip: [192.198.151.168] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 2e4aa2a8-1b9a-4584-216c-08d7d7c64698 x-ms-traffictypediagnostic: BYAPR11MB3541: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:792; x-forefront-prvs: 0362BF9FDB x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BYAPR11MB3143.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(10019020)(396003)(136003)(376002)(346002)(366004)(39860400002)(76116006)(53546011)(8676002)(478600001)(9686003)(7696005)(66476007)(81156014)(81166006)(55016002)(5660300002)(64756008)(4326008)(66556008)(186003)(52536014)(66446008)(2906002)(66946007)(26005)(8936002)(33656002)(86362001)(110136005)(54906003)(6506007)(71200400001)(7416002)(316002); DIR:OUT; SFP:1102; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: /jci7akszHqaH6CUs0mwu8g9XFIWDvKJKdVUCUHmMXibCzcgiziKDHxO6LjlRv7laVxc50YtUR8jgQGIBEFifLB553QQHyLh0OTjjSrOIzRFUIFkJxbXzIxGFHxXiofLF9XDyRB5PWY5X137ds5/OxMJ5i2PZFwlb0bHDC3g4EFIW5l5DKMQknluj2KTN/dO6nCvlxoinHUpMHpKYDB3gQhopOgrbQTsI9nB4yEoWxVwtlXLxMIRxi1+32kyJliMLB2GKuG+MfYc54w9a/Vc0LbnTy88D2H6haQK46jtJFcFpZOcZ7/vF34eBVoBfdFUT6u9yRz8u300rllBtKzaSFlx23FGDwfKDsKGJfMUA5eC/zGb/Jmj9o8VQR7uf9i83v8HVl3g0cz2K5kC85GrSrXkhli2gniMPcZ9zPGyWIQaZPuEFPfZJWYXfTGMHGdE x-ms-exchange-antispam-messagedata: Xp9bFv4xVoShYiuVPbIeX8yMcqzKgG7ZsRI9wfYrJwdjIvrdUVF/Bq9iXmmaH8sB5i7CT5CGVEO2OKdaO0ZeGEBxr6uzLXha+rj6ECUG2slk9QJUE2B7WyKcoUwV0vZczjS4S6jONaBH14XE8uoAJw== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 2e4aa2a8-1b9a-4584-216c-08d7d7c64698 X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Apr 2020 11:58:08.2736 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: D7BHQ9NTCH1GSDSOYARczZsCdeiyERHdMmM3zStP73uc9quA6fxd16vAyrMrJmE+ZkFPsA5SKPARxA7AvJLTwXuBRt1mmYPxY4DW++24yvE= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3541 X-OriginatorOrg: intel.com 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" > From: Phil Yang > Sent: Tuesday, March 17, 2020 1:18 AM > To: thomas@monjalon.net; Van Haaren, Harry ; > Ananyev, Konstantin ; > stephen@networkplumber.org; maxime.coquelin@redhat.com; dev@dpdk.org > Cc: david.marchand@redhat.com; jerinj@marvell.com; hemant.agrawal@nxp.com= ; > Honnappa.Nagarahalli@arm.com; gavin.hu@arm.com; ruifeng.wang@arm.com; > joyce.kong@arm.com; nd@arm.com; Honnappa Nagarahalli > ; stable@dpdk.org > Subject: [PATCH v3 09/12] service: avoid race condition for MT unsafe ser= vice >=20 > From: Honnappa Nagarahalli >=20 > 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. >=20 > Fixes: e9139a32f6e8 ("service: add function to run on app lcore") > Cc: stable@dpdk.org >=20 > Signed-off-by: Honnappa Nagarahalli > Reviewed-by: Phil Yang > Reviewed-by: Gavin Hu We should put "fix" in the title, once converged on an implementation. 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. 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. In this particular case, we have a counter for number of iterations that a 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. 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 confidence. Thoughts on such an approach? > --- > lib/librte_eal/common/rte_service.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) >=20 > 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; >=20 > /* 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, uint6= 4_t > service_mask, >=20 > cs->service_active_on_lcore[i] =3D 1; >=20 > - /* 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; >=20 > -- > 2.7.4