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 289E6A0350; Tue, 23 Jun 2020 21:39:03 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BC1531D6D0; Tue, 23 Jun 2020 21:39:01 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 074861D6CB for ; Tue, 23 Jun 2020 21:38:59 +0200 (CEST) IronPort-SDR: oJqvLaj0iKC36HETY1AER5e4jLx9GvC3vG2m5kPzr6v0cjGhZIuBVYb91u5VsFMrol9KBy4D3p c2r/QY/Y292g== X-IronPort-AV: E=McAfee;i="6000,8403,9661"; a="145689103" X-IronPort-AV: E=Sophos;i="5.75,272,1589266800"; d="scan'208";a="145689103" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jun 2020 12:38:58 -0700 IronPort-SDR: avTAzGkoxaTfJruol1BRTFzeb7qDFRPz1Ql5EaQlyGoDlD2cbwTtnodx0zhBNqi6yRO5mLki4/ I08IHH8LJ3SQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,272,1589266800"; d="scan'208";a="319248999" Received: from orsmsx102.amr.corp.intel.com ([10.22.225.129]) by FMSMGA003.fm.intel.com with ESMTP; 23 Jun 2020 12:38:58 -0700 Received: from orsmsx153.amr.corp.intel.com (10.22.226.247) by ORSMSX102.amr.corp.intel.com (10.22.225.129) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 23 Jun 2020 12:38:58 -0700 Received: from ORSEDG001.ED.cps.intel.com (10.7.248.4) by ORSMSX153.amr.corp.intel.com (10.22.226.247) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 23 Jun 2020 12:38:57 -0700 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.169) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 23 Jun 2020 12:38:57 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ffAcdDY3khCGML+idrCYcsWEjzApWTGW8BueFkrqJtejvszUDvk426njDtt2OqHzQu25HO3usGjvxDZAITe9nj0jb7vAUqnH/aKVKL3JT/Oyrh7kn0+KQrne6BvkAvk5+0NuVcv4d0iGAq62K16ys/ZmglFp4tuJHuoV8F9Q4OQQZcX3mjYggjiKXc7Xlbj69NZdBHD51cjzMG3qUYbEtTTfkrxZyoOiWg7uk+vZ4LVrbGwSP87wn/Lg1AE4qmrUvhwr+52H3rSWUA0VmfotjEJvL3KDxE1wxk74xRUslEuTqHjoHacXt03FjMiFFSdR93whnMNuDyRDwa95PoDnqw== 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=mCV8GkbHZa3oL2/ON484jJTMXwIA8B5jHecAC7OwVUs=; b=iPBoxZmb3rcnC65dPiUU/1OH4OaBsouOOHtg0ySYjrFqDFX9qzCKOzYhxlI0ShdVuXxzaxhVSCyxhP7RkyGGZiWSmmyXVq1PEDgyaBTy9rtHX8LufWqQNqe+ZMMoNB7LhV4LYG2t0IQyvr1C11MzmtOJRn6b6n3WoBWI0S0ZyQOj142/8AlIxHE05XgN1Q/c/1tmQorhxu5sCohEfUagOSo/KQcOrOsaWpHfZQG6z0C4aeBmtsUDtGyxvlTy6EX15hPcmXGeVvOoA3kOvbYOa1JhTaJ3h1e0n01zo1iT5Jl/sViiHokphkWOOIA78/oSXVgAROw58ejJOmtXT29Flg== 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=mCV8GkbHZa3oL2/ON484jJTMXwIA8B5jHecAC7OwVUs=; b=vmQWP9kyVR5lbUnI2DXglfqiwgf54jdQazBW3ynssBgiR8bYI9dbFyLVa/bK+Ks65c+6TX6PsQuzhnilmutBHSBCsOWmPOBxj9uY+ywp2DM0bwCLP5WEbDMNCpUfHbQ4Y8VAH0dmtgxbTIfusQ5Wlm20OcL6wcUQbR6go4tOW90= Received: from CY4PR1101MB2118.namprd11.prod.outlook.com (2603:10b6:910:1f::10) by CY4PR11MB1688.namprd11.prod.outlook.com (2603:10b6:903:25::23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3109.21; Tue, 23 Jun 2020 19:38:56 +0000 Received: from CY4PR1101MB2118.namprd11.prod.outlook.com ([fe80::410b:7807:181:1620]) by CY4PR1101MB2118.namprd11.prod.outlook.com ([fe80::410b:7807:181:1620%4]) with mapi id 15.20.3109.027; Tue, 23 Jun 2020 19:38:56 +0000 From: "Carrillo, Erik G" To: Phil Yang , "dev@dpdk.org" CC: "drc@linux.vnet.ibm.com" , Honnappa Nagarahalli , Ruifeng Wang , "Dharmik Thakkar" , nd Thread-Topic: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with c11 atomics Thread-Index: AQHWQKuLMcN95WYLzky6aL2QD3WEIajkeeaAgAItOBA= Date: Tue, 23 Jun 2020 19:38:56 +0000 Message-ID: References: <1591960798-24024-1-git-send-email-phil.yang@arm.com> <1591960798-24024-3-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: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [136.49.135.17] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: b046ec31-cabb-430d-2e1f-08d817ad11a7 x-ms-traffictypediagnostic: CY4PR11MB1688: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-forefront-prvs: 04433051BF x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: vX/l8+m4aBGhqs0UH4009iWeXOauxnAEaiOLVguc1KKtBABjV+G5rOUT2xWEZ6riB62BI/XEWWrYwWOeHwYwm12RBKswhrE/Ld6x1Ip71ZS7l23GWv+GXGw19wE1gPLkRvWTlZNyduh8R79frbFmsrrnDfawGXYpiCJf/XbEDQ+bO7S4Msfu37iX3jcKcdvGSyoQq/OVElxOBOItgVcoSBDetzDTwdaFo5DFJGLPxBqRrUgzcYdAIYM4LvUyDUDD1YO/kHkmPDaSnCzZ03ZElGwg9g6JJ7ItCfE1QNEtIZDd9/ZPaALG0WkhjTwei5QL5OJK2VrpDftdapmVpzoHCA== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:CY4PR1101MB2118.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(4636009)(346002)(136003)(376002)(396003)(39860400002)(366004)(110136005)(83380400001)(8676002)(53546011)(26005)(54906003)(7696005)(55016002)(9686003)(2906002)(52536014)(316002)(186003)(478600001)(4326008)(5660300002)(71200400001)(66946007)(6506007)(76116006)(66556008)(64756008)(8936002)(86362001)(33656002)(66446008)(66476007); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: ZilTWXY4A2hvWER+gy4YrN0htJSUsfB9W81BGuzd9XsBwn9+WtLHP5DTZI7oEodrFkZxxweAAXN6D6pE2aBCHyWKS8XnXbm3mnW6YwWsK0azKlg3S8KCmhgQs7nS1I+/xvLBr+Kih1J8UawHKQdQRMxb4GCrGwkWXk+Iy/dfmJAayXVY7MeWjrudOtoZF8iB2F1jZ4zfAP5ii7XaI6XnCPPoh2ft2FEhVYUk4HxXUrMtlp+H9OLg9/fZPnHFmkMHMHf4nZbmp03uOKpK+4WGnSDdgmI43rNsBaOVmxkhg3Jum48hhx7JrdXPJlJI/y/0ZetjcZgcTouTeZpaRFgSXU+iLU9r+F3jWVPt3fio1EbdwTpM4+MmN30y3n0+DgIE/On0eDWhVaohaewtCTzPG3Sn1jzZhGhPkocQWtsvN847Wf8nLSO1AhPm0HO5r0OsZl24fk8TRloZu5Wwz4+Tj7shVFTkoVsQWs8/InrFVbw= x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: b046ec31-cabb-430d-2e1f-08d817ad11a7 X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jun 2020 19:38:56.4345 (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: PdmbVwpNJ40DSTP+isUeXe499GNWwKwLDCm8UcBNSyxvv8V288kKG3h2/fzemGm9cTP9q64Z8QF/6s0YIAfHDYxR+v4FeNZD8vvKQoT/iWM= X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR11MB1688 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with c11 atomics 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" Hi Phil, Comment in-line: > -----Original Message----- > From: Phil Yang > Sent: Monday, June 22, 2020 5:12 AM > To: Phil Yang ; dev@dpdk.org; Carrillo, Erik G > > Cc: drc@linux.vnet.ibm.com; Honnappa Nagarahalli > ; Ruifeng Wang > ; Dharmik Thakkar > ; nd > Subject: RE: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with c11 > atomics >=20 > > -----Original Message----- > > From: dev On Behalf Of Phil Yang > > Sent: Friday, June 12, 2020 7:20 PM > > To: dev@dpdk.org; erik.g.carrillo@intel.com > > Cc: drc@linux.vnet.ibm.com; Honnappa Nagarahalli > > ; Ruifeng Wang > ; > > Dharmik Thakkar ; nd > > Subject: [dpdk-dev] [PATCH 3/3] eventdev: relax smp barriers with c11 > > atomics > > > > The implementation-specific opaque data is shared between arm and > > cancel operations. The state flag acts as a guard variable to make > > sure the update of opaque data is synchronized. This patch uses c11 > > atomics with explicit one way memory barrier instead of full barriers > > rte_smp_w/rmb() to synchronize the opaque data between timer arm and > cancel threads. > > > > Signed-off-by: Phil Yang > > Reviewed-by: Dharmik Thakkar > > Reviewed-by: Ruifeng Wang > > --- > > lib/librte_eventdev/rte_event_timer_adapter.c | 55 > > ++++++++++++++++++--------- > > lib/librte_eventdev/rte_event_timer_adapter.h | 2 +- > > 2 files changed, 38 insertions(+), 19 deletions(-) > > > > diff --git a/lib/librte_eventdev/rte_event_timer_adapter.c > > b/lib/librte_eventdev/rte_event_timer_adapter.c > > index 6947efb..0a26501 100644 > > --- a/lib/librte_eventdev/rte_event_timer_adapter.c > > +++ b/lib/librte_eventdev/rte_event_timer_adapter.c > > @@ -629,7 +629,8 @@ swtim_callback(struct rte_timer *tim) > > sw->expired_timers[sw->n_expired_timers++] =3D tim; > > sw->stats.evtim_exp_count++; > > > > - evtim->state =3D RTE_EVENT_TIMER_NOT_ARMED; > > + __atomic_store_n(&evtim->state, > > RTE_EVENT_TIMER_NOT_ARMED, > > + __ATOMIC_RELEASE); > > } > > > > if (event_buffer_batch_ready(&sw->buffer)) { @@ -1020,6 +1021,7 > @@ > > __swtim_arm_burst(const struct rte_event_timer_adapter *adapter, > > int n_lcores; > > /* Timer is not armed state */ > > int16_t exp_state =3D 0; > > + enum rte_event_timer_state n_state; > > > > #ifdef RTE_LIBRTE_EVENTDEV_DEBUG > > /* Check that the service is running. */ @@ -1060,30 +1062,36 @@ > > __swtim_arm_burst(const struct rte_event_timer_adapter *adapter, > > } > > > > for (i =3D 0; i < nb_evtims; i++) { > > - /* Don't modify the event timer state in these cases */ > > - if (evtims[i]->state =3D=3D RTE_EVENT_TIMER_ARMED) { > > + n_state =3D __atomic_load_n(&evtims[i]->state, > > __ATOMIC_RELAXED); > > + if (n_state =3D=3D RTE_EVENT_TIMER_ARMED) { > > rte_errno =3D EALREADY; > > break; > > - } else if (!(evtims[i]->state =3D=3D > > RTE_EVENT_TIMER_NOT_ARMED || > > - evtims[i]->state =3D=3D > > RTE_EVENT_TIMER_CANCELED)) { > > + } else if (!(n_state =3D=3D RTE_EVENT_TIMER_NOT_ARMED || > > + n_state =3D=3D RTE_EVENT_TIMER_CANCELED)) { > > rte_errno =3D EINVAL; > > break; > > } > > > > ret =3D check_timeout(evtims[i], adapter); > > if (unlikely(ret =3D=3D -1)) { > > - evtims[i]->state =3D > > RTE_EVENT_TIMER_ERROR_TOOLATE; > > + __atomic_store_n(&evtims[i]->state, > > + > RTE_EVENT_TIMER_ERROR_TOOLATE, > > + __ATOMIC_RELAXED); > > rte_errno =3D EINVAL; > > break; > > } else if (unlikely(ret =3D=3D -2)) { > > - evtims[i]->state =3D > > RTE_EVENT_TIMER_ERROR_TOOEARLY; > > + __atomic_store_n(&evtims[i]->state, > > + > > RTE_EVENT_TIMER_ERROR_TOOEARLY, > > + __ATOMIC_RELAXED); > > rte_errno =3D EINVAL; > > break; > > } > > > > if (unlikely(check_destination_event_queue(evtims[i], > > adapter) < 0)) { > > - evtims[i]->state =3D RTE_EVENT_TIMER_ERROR; > > + __atomic_store_n(&evtims[i]->state, > > + RTE_EVENT_TIMER_ERROR, > > + __ATOMIC_RELAXED); > > rte_errno =3D EINVAL; > > break; > > } > > @@ -1099,13 +1107,18 @@ __swtim_arm_burst(const struct > > rte_event_timer_adapter *adapter, > > SINGLE, lcore_id, NULL, evtims[i]); > > if (ret < 0) { > > /* tim was in RUNNING or CONFIG state */ > > - evtims[i]->state =3D RTE_EVENT_TIMER_ERROR; > > + __atomic_store_n(&evtims[i]->state, > > + RTE_EVENT_TIMER_ERROR, > > + __ATOMIC_RELEASE); > > break; > > } > > > > - rte_smp_wmb(); > > EVTIM_LOG_DBG("armed an event timer"); > > - evtims[i]->state =3D RTE_EVENT_TIMER_ARMED; > > + /* RELEASE ordering guarantees the adapter specific value > > + * changes observed before the update of state. > > + */ > > + __atomic_store_n(&evtims[i]->state, > > RTE_EVENT_TIMER_ARMED, > > + __ATOMIC_RELEASE); > > } > > > > if (i < nb_evtims) > > @@ -1132,6 +1145,7 @@ swtim_cancel_burst(const struct > > rte_event_timer_adapter *adapter, > > struct rte_timer *timp; > > uint64_t opaque; > > struct swtim *sw =3D swtim_pmd_priv(adapter); > > + enum rte_event_timer_state n_state; > > > > #ifdef RTE_LIBRTE_EVENTDEV_DEBUG > > /* Check that the service is running. */ @@ -1143,16 +1157,18 @@ > > swtim_cancel_burst(const struct rte_event_timer_adapter *adapter, > > > > for (i =3D 0; i < nb_evtims; i++) { > > /* Don't modify the event timer state in these cases */ > > - if (evtims[i]->state =3D=3D RTE_EVENT_TIMER_CANCELED) { > > + /* ACQUIRE ordering guarantees the access of > > implementation > > + * specific opague data under the correct state. > > + */ > > + n_state =3D __atomic_load_n(&evtims[i]->state, > > __ATOMIC_ACQUIRE); > > + if (n_state =3D=3D RTE_EVENT_TIMER_CANCELED) { > > rte_errno =3D EALREADY; > > break; > > - } else if (evtims[i]->state !=3D RTE_EVENT_TIMER_ARMED) { > > + } else if (n_state !=3D RTE_EVENT_TIMER_ARMED) { > > rte_errno =3D EINVAL; > > break; > > } > > > > - rte_smp_rmb(); > > - > > opaque =3D evtims[i]->impl_opaque[0]; > > timp =3D (struct rte_timer *)(uintptr_t)opaque; > > RTE_ASSERT(timp !=3D NULL); > > @@ -1166,11 +1182,14 @@ swtim_cancel_burst(const struct > > rte_event_timer_adapter *adapter, > > > > rte_mempool_put(sw->tim_pool, (void **)timp); > > > > - evtims[i]->state =3D RTE_EVENT_TIMER_CANCELED; > > + __atomic_store_n(&evtims[i]->state, > > RTE_EVENT_TIMER_CANCELED, > > + __ATOMIC_RELAXED); > > evtims[i]->impl_opaque[0] =3D 0; > > evtims[i]->impl_opaque[1] =3D 0; >=20 > Is that safe to remove impl_opaque cleanup above? >=20 > Once the soft timer canceled, the __swtim_arm_burst cannot access these > two fields under the RTE_EVENT_TIMER_CANCELED state. > After new timer armed, it refills these two fields in the __swtim_arm_bur= st > thread, which is the only producer of these two fields. > I think the only risk is that the values of these two field might be unkn= ow > after swtim_cancel_burst. > So it should be safe to remove them if no other thread access them after > canceling the timer. >=20 > Any comments on this? > If we remove these two instructions, we can also remove the > __atomic_thread_fence below to save performance penalty. >=20 > Thanks, > Phil >=20 In this case, I see the fence as (more importantly) ensuring the state upda= te is visible to other threads... do I misunderstand? I suppose we could = also update the state with an __atomic_store(..., __ATOMIC_RELEASE), but pe= rhaps that roughly equivalent? > > - > > - rte_smp_wmb(); > > + /* The RELEASE fence make sure the clean up > > + * of opaque data observed between threads. > > + */ > > + __atomic_thread_fence(__ATOMIC_RELEASE); > > } > > > > return i; > > diff --git a/lib/librte_eventdev/rte_event_timer_adapter.h > > b/lib/librte_eventdev/rte_event_timer_adapter.h > > index d2ebcb0..6f64b90 100644 > > --- a/lib/librte_eventdev/rte_event_timer_adapter.h > > +++ b/lib/librte_eventdev/rte_event_timer_adapter.h > > @@ -467,7 +467,7 @@ struct rte_event_timer { > > * - op: RTE_EVENT_OP_NEW > > * - event_type: RTE_EVENT_TYPE_TIMER > > */ > > - volatile enum rte_event_timer_state state; > > + enum rte_event_timer_state state; > > /**< State of the event timer. */ > > uint64_t timeout_ticks; > > /**< Expiry timer ticks expressed in number of *timer_ticks_ns* > from > > -- > > 2.7.4