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 79940A0597; Thu, 9 Apr 2020 21:29:12 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DCCE21C2AF; Thu, 9 Apr 2020 21:29:11 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id A61041C136 for ; Thu, 9 Apr 2020 21:29:08 +0200 (CEST) IronPort-SDR: Qr0PUmwyVBUXeqINbGa50KmApkwkbMJjAP2pSO+3yxDm5XPatzGqokF/kynvKFr2041cN5hSHT lT0DV3PR3orQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Apr 2020 12:29:07 -0700 IronPort-SDR: wVD2ym7LzHX+09ra9CVZPTLGwdIIUbduMzot1/k2UEAEpXivXzpyXuHEvw/VVvSulT41S5eMlJ vh92SBH3eLWA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,363,1580803200"; d="scan'208";a="270188323" Received: from orsmsx105.amr.corp.intel.com ([10.22.225.132]) by orsmga002.jf.intel.com with ESMTP; 09 Apr 2020 12:29:07 -0700 Received: from orsmsx163.amr.corp.intel.com (10.22.240.88) by ORSMSX105.amr.corp.intel.com (10.22.225.132) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 9 Apr 2020 12:29:07 -0700 Received: from ORSEDG001.ED.cps.intel.com (10.7.248.4) by ORSMSX163.amr.corp.intel.com (10.22.240.88) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 9 Apr 2020 12:29:07 -0700 Received: from NAM04-BN3-obe.outbound.protection.outlook.com (104.47.46.52) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 9 Apr 2020 12:29:07 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fmHXEziJ/BTLY3BYG+UFyNNRVRs1dEts13MZT4BDaIIejkjFDAXrcyG9ysWO9zuwgQVVuqFnfnYswgUzWQlzIJTQacGDHwJGIQUcUq/mT3p7pgyoh4QoWhUJjX0p5PnOOAcWc+sASsv6hQqPLCPz3h3vchhrfrJm0Y5sYOw0mKZTzLXJa04oqJNfYzZZQI5bR/TFWWAdtpOvXOvRXOEP76kvbOaMHJMylFdlH03Eqgi7gIPWeP/0vFnY/DDXLVxugYfmz1kyiM3HZwi2OLAK7KoOkT7s41jxiX16fTLkUav6JtW8Knqayoj7mkPxr/YVYVAGLk5P14aQgt2qzBGI0A== 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=BB2GBvwVDrQtSGsY++hBt99W4jJR4IDOXi/YquaVKcA=; b=WBUsIvzFyORBv1Pmff/22qdsy058FvvPxtta0Wi+eYr6pFiwB9dg/Nrzy9QJc8nfS1LFvs3CVnMYUG4ov1Gec+JBq+TOI6vXVbxBwcw3clpoqWynE1ODuQHZ3wbdRL8W/RoIdVdnJF9CP1/pU7bbKVM78/0stn+2z060Q935V8q+0ReYzijdqWeZkz2sa0TcdQeb+SD5qw+EXhtMhR8hcjdG5yFytYmA7syN+wTCod1jMRSUigbiscE8v9TcxEI6JMuBVZDZQP87YXQTfRALbj8cX9MAzpabjgF3FkRsuO/Dz9HaZGtdnkxVUvsT0k4Y0OR4MEnT3VxwmxdcuSK5FQ== 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=BB2GBvwVDrQtSGsY++hBt99W4jJR4IDOXi/YquaVKcA=; b=Q/oaJRHkZjzMs1kXeoP4YzjXOiGwEcPJb5BMKPkjXBJCsh8Gc3KfmdLM/NEto2aP9hVJbLbXqw+Q8WYf8T0B0niIAEl5wVUYay6L9e2vNupMXRcWd0oVVHPifNzutboEnntzhywxW0Al7qGITPgTsESuE02JSXGZNXo4evUz3vw= Received: from SA0PR11MB4656.namprd11.prod.outlook.com (2603:10b6:806:96::23) by SA0PR11MB4542.namprd11.prod.outlook.com (2603:10b6:806:9f::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2878.15; Thu, 9 Apr 2020 19:29:05 +0000 Received: from SA0PR11MB4656.namprd11.prod.outlook.com ([fe80::6400:b873:7752:50b4]) by SA0PR11MB4656.namprd11.prod.outlook.com ([fe80::6400:b873:7752:50b4%4]) with mapi id 15.20.2878.018; Thu, 9 Apr 2020 19:29:05 +0000 From: "Carrillo, Erik G" To: Honnappa Nagarahalli , Phil Yang , "rsanford@akamai.com" , "dev@dpdk.org" CC: "david.marchand@redhat.com" , "Burakov, Anatoly" , "thomas@monjalon.net" , "jerinj@marvell.com" , "hemant.agrawal@nxp.com" , Gavin Hu , nd , nd Thread-Topic: [PATCH 2/2] lib/timer: relax barrier for status update Thread-Index: AQHV6t2yBm1KhrbDIU2DdShJp9K8ZKhv+ljQgAAFyYCAAACa4IAACpkAgAFgUcA= Date: Thu, 9 Apr 2020 19:29:05 +0000 Message-ID: References: <1582526539-14360-1-git-send-email-phil.yang@arm.com> <1582526539-14360-2-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: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.2.0.6 authentication-results: spf=none (sender IP is ) smtp.mailfrom=erik.g.carrillo@intel.com; x-originating-ip: [192.55.52.201] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 82c194bb-744d-46ab-01d2-08d7dcbc4454 x-ms-traffictypediagnostic: SA0PR11MB4542: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-forefront-prvs: 0368E78B5B x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:SA0PR11MB4656.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(10019020)(366004)(39860400002)(346002)(396003)(136003)(376002)(54906003)(110136005)(81156014)(15650500001)(81166007)(76116006)(8676002)(52536014)(478600001)(7416002)(86362001)(26005)(66476007)(7696005)(71200400001)(5660300002)(66446008)(66946007)(64756008)(53546011)(55016002)(2906002)(66556008)(9686003)(316002)(186003)(4326008)(6506007)(8936002)(33656002); DIR:OUT; SFP:1102; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: Qk6X2LMBX1rKLBwRrfU3uHRHwlMm37eKJpozdz/S0ITeCnOT1+K5m3J8H8+vc6+g3jM+cIpjriZOiRHT83ZRyegHfloMS7R04wGiU4GGms7qn88GtG2VMpVPMPX6Trf9PlwYFka/UUiS838REnQUYnUX/p5tm7b/mdc1/HCjG0WZFnmikvMgfKJIVtRgfCYeBurBKAl7aawhQTj33IsyNTtmaAiHtXTdAysscxY4IS2XXbg50FQzW1XrNyGxmMVDuS5z8i0aqK25qAYQR7csQHWIg1z3pO/XSwyQN5HuNIIhfRe2wfMY7g4iNWL96fFRn2EOjfEb7t0NIXFrnGCWF8hh15CTLRSamJfjux9Z0LlDQt/jgcsEKhNVEDH7jBYZTYdxusPB3aIL7XlBkEdZHPIX+Iel7UCJwqHO6+Gh8EshNzV/4ktrla0rmzfBCH27 x-ms-exchange-antispam-messagedata: THQIMtgff053u2Z423fMwcs8H5rPO2u6pIhtgjFE/NfGc/AJ8Cm1CzZuZZ/sfCH0zVcB84WanEm2ruLHWEeFxgegh5eC7BAO8GOq1s1+lBjC9Dv4Qen3szApFacSNKUFrWglk4S99ah9rUB6aMGePA== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 82c194bb-744d-46ab-01d2-08d7dcbc4454 X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Apr 2020 19:29:05.3517 (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: 9a7UhfVWjywiGQscSWyoZzSTImB42tkr79s48A+0/z8YuZ6Wh63D5BLlunRgbcIlaW9Ctc+zCBlqomk8kr4G8uqUD1qU9g05sGjN3fSTNko= X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4542 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update 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" > -----Original Message----- > From: Honnappa Nagarahalli > Sent: Wednesday, April 8, 2020 4:56 PM > To: Carrillo, Erik G ; Phil Yang > ; rsanford@akamai.com; dev@dpdk.org > Cc: david.marchand@redhat.com; Burakov, Anatoly > ; thomas@monjalon.net; jerinj@marvell.com; > hemant.agrawal@nxp.com; Gavin Hu ; nd > ; Honnappa Nagarahalli ; > nd > Subject: RE: [PATCH 2/2] lib/timer: relax barrier for status update >=20 > >=20 > > > > > > > > Subject: [PATCH 2/2] lib/timer: relax barrier for status update > > > > > > > > > > Volatile has no ordering semantics. The rte_timer structure > > > > > defines timer status as a volatile variable and uses the > > > > > rte_r/wmb barrier to guarantee inter-thread visibility. > > > > > > > > > > This patch optimized the volatile operation with c11 atomic > > > > > operations and one-way barrier to save the performance penalty. > > > > > According to the timer_perf_autotest benchmarking results, this > > > > > patch can uplift 10%~16% timer appending performance, 3%~20% > > > > > timer resetting performance and 45% timer callbacks scheduling > > > > > performance on aarch64 and no loss in performance for x86. > > > > > > > > > > Suggested-by: Honnappa Nagarahalli > > > > > > > > > > Signed-off-by: Phil Yang > > > > > Reviewed-by: Gavin Hu > > > > > > > > Hi Phil, > > > > > > > > It seems like the consensus is to generally avoid replacing rte_ato= mic_* > > > > interfaces with the GCC builtins directly. In other areas of DPDK= that > are > > > > being patched, are the C11 APIs going to be > investigated? > > > It > > > > seems like that decision will apply here as well. > > > Agree. The new APIs are going to be 1 to 1 mapped with the built-in > > > intrinsics (the memory orderings used themselves will not change). > > > We should go ahead with the review and conclude any issues. Once the > > > decision is made on what APIs to use, we can submit the next version > > > using > > the APIs decided. > > > > > Thanks, Honnappa. > > > > I have reviewed the memory orderings and I see no issues with them. I= do > > have a question regarding a comment - I'll pose it inline: > Fantastic, thank you. > I have an unrelated (to this patch) question for you below. >=20 > > > > > > > > > > Thanks, > > > > Erik > > > > > > > > > --- > > > > > lib/librte_timer/rte_timer.c | 90 > > > > > +++++++++++++++++++++++++++++++---- > > > > > --------- > > > > > lib/librte_timer/rte_timer.h | 2 +- > > > > > 2 files changed, 65 insertions(+), 27 deletions(-) > > > > > > > > > > diff --git a/lib/librte_timer/rte_timer.c > > > > > b/lib/librte_timer/rte_timer.c index 269e921..be0262d 100644 > > > > > --- a/lib/librte_timer/rte_timer.c > > > > > +++ b/lib/librte_timer/rte_timer.c > > > > > @@ -10,7 +10,6 @@ > > > > > #include > > > > > #include > > > > > > > > > > -#include > > > > > #include > > > > > #include > > > > > #include > > > > > @@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim) > > > > > > > > > > status.state =3D RTE_TIMER_STOP; > > > > > status.owner =3D RTE_TIMER_NO_OWNER; > > > > > - tim->status.u32 =3D status.u32; > > > > > + __atomic_store_n(&tim->status.u32, status.u32, > > > > > __ATOMIC_RELAXED); > > > > > } > > > > > > > > > > /* <... snipped ...> > > > > > @@ -258,9 +257,20 @@ timer_set_config_state(struct rte_timer > *tim, > > > > > * mark it atomically as being configured */ > > > > > status.state =3D RTE_TIMER_CONFIG; > > > > > status.owner =3D (int16_t)lcore_id; > > > > > - success =3D rte_atomic32_cmpset(&tim->status.u32, > > > > > - prev_status.u32, > > > > > - status.u32); > > > > > + /* If status is observed as RTE_TIMER_CONFIG > earlier, > > > > > + * that's not going to cause any issues because the > > > > > + * pattern is read for status then read the other > members. > > > > I don't follow the above comment. What is meant by "earlier"? > > > > Thanks, > > Erik > I would rather change this comment to something similar to what is > mentioned while changing to 'RUNNING' state. > 'CONFIG' is also a locking state. I think it is much easier to understand= . >=20 Ok, thanks - that makes sense. < ... snipped ...> > > > > > 748,8 +774,12 @@ __rte_timer_manage(struct rte_timer_data > > > *timer_data) > > > > > status.state =3D RTE_TIMER_PENDING; > Is it better to set this to STOPPED since it is out of the run list? I th= ink it is > better for the understanding as well. >=20 In this location, we are dealing with periodic timers, and we are about to = restart the current timer after it just expired and its callback was execut= ed. As I understand it, setting the state back to PENDING here will cause = the timer_reset() call below to remove this timer from the list (run list) = it's still in (and fix up the links from the previous to the next elements)= , update other bits of the data structure, and update stats. That behavio= r would change if we set the state to STOPPED. At least to me, it also see= ms like the PENDING state is still accurate conceptually since the periodic= timer wasn't explicitly stopped by this processing. Thanks, Erik > > > > > __TIMER_STAT_ADD(priv_timer, pending, 1); > > > > > status.owner =3D (int16_t)lcore_id; > > > > > - rte_wmb(); > > > > > - tim->status.u32 =3D status.u32; > > > > > + /* The "RELEASE" ordering guarantees the > memory > > > > > + * operations above the status update are > observed > > > > > + * before the update by all threads > > > > > + */ > > > > > + __atomic_store_n(&tim->status.u32, > status.u32, > > > > > + __ATOMIC_RELEASE); > > > > > __rte_timer_reset(tim, tim->expire + tim- > >period, > > > > > tim->period, lcore_id, tim->f, tim- > >arg, 1, > > > > > timer_data); > > > > > @@ -919,8 +949,12 @@ rte_timer_alt_manage(uint32_t > timer_data_id, > > > > > /* remove from done list and mark timer as > stopped > > > > */ > > > > > status.state =3D RTE_TIMER_STOP; > > > > > status.owner =3D RTE_TIMER_NO_OWNER; > > > > > - rte_wmb(); > > > > > - tim->status.u32 =3D status.u32; > > > > > + /* The "RELEASE" ordering guarantees the > memory > > > > > + * operations above the status update are > observed > > > > > + * before the update by all threads > > > > > + */ > > > > > + __atomic_store_n(&tim->status.u32, > status.u32, > > > > > + __ATOMIC_RELEASE); > > > > > } else { > > > > > /* keep it in list and mark timer as pending */ > > > > > rte_spinlock_lock( > > > > > @@ -928,8 +962,12 @@ rte_timer_alt_manage(uint32_t > timer_data_id, > > > > > status.state =3D RTE_TIMER_PENDING; > > > > > __TIMER_STAT_ADD(data->priv_timer, > pending, 1); > > > > > status.owner =3D (int16_t)this_lcore; > > > > > - rte_wmb(); > > > > > - tim->status.u32 =3D status.u32; > > > > > + /* The "RELEASE" ordering guarantees the > memory > > > > > + * operations above the status update are > observed > > > > > + * before the update by all threads > > > > > + */ > > > > > + __atomic_store_n(&tim->status.u32, > status.u32, > > > > > + __ATOMIC_RELEASE); > > > > > __rte_timer_reset(tim, tim->expire + tim- > >period, > > > > > tim->period, this_lcore, tim->f, tim- > >arg, 1, > > > > > data); > > > > > diff --git a/lib/librte_timer/rte_timer.h > > > > > b/lib/librte_timer/rte_timer.h index c6b3d45..df533fa 100644 > > > > > --- a/lib/librte_timer/rte_timer.h > > > > > +++ b/lib/librte_timer/rte_timer.h > > > > > @@ -101,7 +101,7 @@ struct rte_timer { > > > > > uint64_t expire; /**< Time when timer expire. */ > > > > > struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH]; > > > > > - volatile union rte_timer_status status; /**< Status of timer. > */ > > > > > + union rte_timer_status status; /**< Status of timer. */ > > > > > uint64_t period; /**< Period of timer (0 if not periodic)= . */ > > > > > rte_timer_cb_t f; /**< Callback function. */ > > > > > void *arg; /**< Argument to callback function. */ > > > > > -- > > > > > 2.7.4