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 B3B5DA0562; Fri, 3 Apr 2020 13:58:32 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A9EA81C114; Fri, 3 Apr 2020 13:58:20 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 9D6CB1C0DC; Fri, 3 Apr 2020 13:58:19 +0200 (CEST) IronPort-SDR: H9QaZ9GUJ8SLHiuDzrgbVz75J/YOmY/Kq5DEeZlmQFiqdm0mJZ8ZU3UWzX7VndDDeaafndoQcp mPMREDdU6ROA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Apr 2020 04:58:18 -0700 IronPort-SDR: 6ExvzDiTBdjJ454WanF6Ap32Zbg6uJpbV7mQjeWSl0zZrMf2IAciV3amE7AeotC2xEFBfzg1az Jm8835kG3LBQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,339,1580803200"; d="scan'208";a="396715198" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga004.jf.intel.com with ESMTP; 03 Apr 2020 04:58:18 -0700 Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 3 Apr 2020 04:58:18 -0700 Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Fri, 3 Apr 2020 04:58:17 -0700 Received: from FMSEDG001.ED.cps.intel.com (10.1.192.133) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Fri, 3 Apr 2020 04:58:17 -0700 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.175) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 3 Apr 2020 04:58:17 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YIMyQbRdndx7QS6n6+BCRUwucarjaXyYJPMXeZF7DlX7z+LNvvjI11CSo1YJVmUFBFqIUG+p2PFX8deyZ7Fc8bsCrMgmh5xBmOTSseQ0+Z95NzdOaI/HmVdlM+hw8uA1eNazdnUELjVo1WCQxFeOzE3jxEOXO41J6B45XUJTj2x2AaBL/OTR5DZiIhzTm0sR3zhH2ULZSBiZjaNibA2QYc7tbOt7dnlq1XVw56/78jGYeXAh5XB5cK3Gi3aCdUhMeIRoUUUsss2T0u6YFgZkIUSV2W6N5FTkoYVsdZ4I/UZqoSGddDphFGw6f2o9clZIFZpTFPzVL6v72mDJyXwBCw== 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=syx0oOPmPVnajRjTBfNZAhHrPdA4LpgjwoU3kC0DTk8=; b=lu2jJBF1QJfTRN3RW5AQ7/hfvAY08t8gTJ6HB7C8n/RJoyhaBsGhiszuXjEc+E6+3+KsWVrd3jS9FMzyNa5SyKkTCqOmqpQDKbs+U3roVMyIoP3+5+D3mUHVZgO4oxFh98tqVcNpr+KnqxQ9bOtrLsewXvzKbB7HoBxXd+uoRSiKTo3LIbxaf5egJRYA+EWDhK7T1rpWUZpdAdVueaWB121Ff2DpS2mOgpiHbxcTs3AwCPquLNRzTQgRj3WmcgaA+VfX8GkL3R9CbBoanr56csJHA2eB6Av0YIpw9FXMTooUfTf6/y3IC8cUaunjwAlEA08qIy7lUVkGRdxOORbwjw== 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=syx0oOPmPVnajRjTBfNZAhHrPdA4LpgjwoU3kC0DTk8=; b=KfkdFEwA3zE415PGJkiZd1ljcndeOQZB7s5TaVR59++xZmvYVGTDNzYunrkqqlm8ut1S4XCoBeVHRvf9sLpsdLGW6ncfxMr+SN4DqtElLsket17xndNNJOwR9S8JNe/npLLoJ7jq6Xl6mFbLeJLz7JarLS4JWZoEmR86cRt9c8w= 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:16 +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:16 +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 10/12] service: identify service running on another core correctly Thread-Index: AQHV+/orBSL3IBQhjESAbL8UIwVqvqhnYzZA Date: Fri, 3 Apr 2020 11:58:16 +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-11-git-send-email-phil.yang@arm.com> In-Reply-To: <1584407863-774-11-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: 7cc48c5b-0cf0-4320-6857-08d7d7c64b57 x-ms-traffictypediagnostic: BYAPR11MB3541: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:1051; 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: ndyO4flpD59U45VthRxUC1CwGMtbUvRNH+fdmWmwMQbHsgY4t8sFq02EYbvKDqa4bOJaV7CnffHszl86/sLddMMde0bboNUnTkzn1KKomF7JKpBteuPnBHbj+EUjGXx7QtdSSwbaGMrKOTiAGaA4mnkAfnyzSgQf5ozvq7vkuJix/cGVANmrzDRuJTBpkQ7xfiASmv2rAdBihItgmrOv2JYTnKsFLir5vYUEPouZA/+2qTc9MO/KbIA3M8OH93L6aRO3Eo1ZOH6neqVeqwWM7azSJ/9SJ7FMpRERgFKcmcd+XgptPyDNy+OiRX6wf5wGCiZrsdEeRj2qpzRtRLswpU8ZJYdsSO+js5VRTb/qUsgKC8ZqEJkSKXFupM16FD2z7YbdXRYNbQrZEzchissM0udPf1H5cMEYyYr78o6QyCGieGA5emRgJ8+XRaXT/cCk x-ms-exchange-antispam-messagedata: NO4o9cfUH1B6Y9NEI0fJ6guZkTH5WRVxj7YOjwiixw67LiXXG2x2hK4bJwY1vKWmL0/hMRScTQHOocQGBT3JNNhQdWn5DgkLU9Cndf8JIElu5p/v4EL1n3W8QEems7Y3AfZVFUNeeubJ0+qmVkkW9Q== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 7cc48c5b-0cf0-4320-6857-08d7d7c64b57 X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Apr 2020 11:58:16.2331 (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: zQhSBTvBwQbLbtgWj7qrJc0kvvu9+u7DhV6KCecx4tdEvp9DMNLuOoRVOPyjJLX0Q5RG17Wbe4b9AkLXPj8k+nI2AWWKZE1FoAUmDOdq5Z8= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3541 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH v3 10/12] service: identify service running on another core correctly 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 10/12] service: identify service running on another co= re > correctly >=20 > From: Honnappa Nagarahalli >=20 > The logic to identify if the MT unsafe service is running on another > core can return -EBUSY spuriously. In such cases, running the service > becomes costlier than using atomic operations. Assume that the > application passes the right parameters and reduces the number of > instructions for all cases. >=20 > Cc: stable@dpdk.org >=20 > Signed-off-by: Honnappa Nagarahalli > Reviewed-by: Phil Yang > Reviewed-by: Gavin Hu Is this fixing broken functionality, or does it aim to only "optimize"? Lack of "fixes" tag suggests optimization. I'm cautious about the commit phrase "Assume that the application ...", if the code was previously checking things, we must not stop checking them now, this may introduce race-conditions in existing applications? It seems like the "serialize_mt_unsafe" branch is being pushed further down the callgraph, and instead of branching over atomics this patch forces always executing 2 atomics? This feels like too specific an optimization/tradeoff, without data to backup that there are no regressions on any DPDK supported platforms. DPDK today doesn't have a micro-benchmark to gather such perf data,=20 but I would welcome one and we can have a data-driven decision. Hope this point-of-view makes sense, -Harry > --- > lib/librte_eal/common/rte_service.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) >=20 > diff --git a/lib/librte_eal/common/rte_service.c > b/lib/librte_eal/common/rte_service.c > index 32a2f8a..0843c3c 100644 > --- a/lib/librte_eal/common/rte_service.c > +++ b/lib/librte_eal/common/rte_service.c > @@ -360,7 +360,7 @@ service_runner_do_callback(struct rte_service_spec_im= pl > *s, > /* Expects the service 's' is valid. */ > static int32_t > service_run(uint32_t i, struct core_state *cs, uint64_t service_mask, > - struct rte_service_spec_impl *s) > + struct rte_service_spec_impl *s, uint32_t serialize_mt_unsafe) > { > if (!s) > return -EINVAL; > @@ -374,7 +374,7 @@ service_run(uint32_t i, struct core_state *cs, uint64= _t > service_mask, >=20 > cs->service_active_on_lcore[i] =3D 1; >=20 > - if (service_mt_safe(s) =3D=3D 0) { > + if ((service_mt_safe(s) =3D=3D 0) && (serialize_mt_unsafe =3D=3D 1)) { > if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1)) > return -EBUSY; >=20 > @@ -412,24 +412,14 @@ rte_service_run_iter_on_app_lcore(uint32_t id, uint= 32_t > serialize_mt_unsafe) >=20 > SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL); >=20 > - /* Atomically add this core to the mapped cores first, then examine if > - * we can run the service. This avoids a race condition between > - * checking the value, and atomically adding to the mapped count. > + /* Increment num_mapped_cores to indicate that the service > + * is running on a core. > */ > - if (serialize_mt_unsafe) > - rte_atomic32_inc(&s->num_mapped_cores); > + rte_atomic32_inc(&s->num_mapped_cores); >=20 > - if (service_mt_safe(s) =3D=3D 0 && > - rte_atomic32_read(&s->num_mapped_cores) > 1) { > - if (serialize_mt_unsafe) > - rte_atomic32_dec(&s->num_mapped_cores); > - return -EBUSY; > - } > - > - int ret =3D service_run(id, cs, UINT64_MAX, s); > + int ret =3D service_run(id, cs, UINT64_MAX, s, serialize_mt_unsafe); >=20 > - if (serialize_mt_unsafe) > - rte_atomic32_dec(&s->num_mapped_cores); > + rte_atomic32_dec(&s->num_mapped_cores); >=20 > return ret; > } > @@ -449,7 +439,7 @@ service_runner_func(void *arg) > if (!service_valid(i)) > continue; > /* return value ignored as no change to code flow */ > - service_run(i, cs, service_mask, service_get(i)); > + service_run(i, cs, service_mask, service_get(i), 1); > } >=20 > cs->loops++; > -- > 2.7.4