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 55CC7A052B; Wed, 29 Jul 2020 10:08:20 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0DBC11BFFE; Wed, 29 Jul 2020 10:08:17 +0200 (CEST) Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05on2067.outbound.protection.outlook.com [40.107.21.67]) by dpdk.org (Postfix) with ESMTP id AC439F04 for ; Wed, 29 Jul 2020 10:08:15 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=AidSfys533tBRKEEhlhbEqluYtqQqNuwJne+Y8lg7Culhf4LA5KFfajzcXx4Bq6KEsbcGLa0+xcbtMBz+g8KuXAiol1qEERyirXlSVCCkpN+84R4vSW6K09R+z7jNxfYqiu64aSiN6WKS1+Nld8G36vLnFhgDLwCLubsDGkVv6e4f1MBcpznYxNtduPLV+AflyuPTFhgjBtmxiEibzjMs8Avhqy28eitF87T8wvCwKXPpdPzyncmgu3dt3CLw55onUDm2kiKDFCFg8P1nDuPzC6H9rdDGXn6AwRqHDsRqQUYAgXIUFUlEFaAgDHf7jF+PtK2mpIcInKTDCaGDROKZw== 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=AE87WSskh86N89XrbrYstBL1JYgqPkme8qvAj1MNIA4=; b=k5o5NEHzAb22PIBPl8whpjEkvcMSMDPc0HPbTd8obGPjANow3AshuyfScP5ZFlTzu0F2a1u4NT1vzKkuMexuhZ5WD8fYdoaM+5HCJH8ibr63cfNTH1YH1wXenVZF7lM3w2AjJmm7QtxPMhMbpbEOhSjqCcEPU7qARqP0Je9vsWa5mHSQOX1PqqRbVZUrGa7aN0MKhvqUxMWI2hIMT1gJFW4PR/xGfS/l48avgAd7RIOempujQIvzcUfFKZH9N1GSPWQWOCt2Hr4jasGXcMuixMRCqLsAqDPhzWC92KXIZrEhkR7b7EOQiIyRLyfDI6jVJqL5jAk4kz39C2D4Vb2iwg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=mellanox.com; dmarc=pass action=none header.from=mellanox.com; dkim=pass header.d=mellanox.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=AE87WSskh86N89XrbrYstBL1JYgqPkme8qvAj1MNIA4=; b=qOR33K6yabh2OklyvRSFkgLFeFp3WvPV0wjaVIpHpgD4d6JaaOqLEMLOoSzVaXKxok+RQkOXdaRKuE4frgiDcZG7vQaGEiBBG6x+AtaUUVj/QAxm2hDWV9tfN2f+lEaQMC5erPMfirbkGwV3pA1v8lp20RrVs+nGsvqyXn0TubQ= Received: from AM4PR05MB3265.eurprd05.prod.outlook.com (2603:10a6:205:8::26) by AM0PR0502MB4018.eurprd05.prod.outlook.com (2603:10a6:208:b::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3239.16; Wed, 29 Jul 2020 08:08:14 +0000 Received: from AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::194e:dc46:7543:50ed]) by AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::194e:dc46:7543:50ed%2]) with mapi id 15.20.3216.034; Wed, 29 Jul 2020 08:08:14 +0000 From: Slava Ovsiienko To: Phil Yang CC: Matan Azrad , Raslan Darawsheh , Thomas Monjalon , "ferruh.yigit@intel.com" , Honnappa Nagarahalli , nd , "dev@dpdk.org" , nd Thread-Topic: [dpdk-dev] [PATCH] app/testpmd: fix txonly mode timestamp intitialization Thread-Index: AQHWZCpYP0HUZxwY+026oorc6Dk9F6kdLpgAgAD/GDA= Date: Wed, 29 Jul 2020 08:08:14 +0000 Message-ID: References: <1595863595-11344-1-git-send-email-viacheslavo@mellanox.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=mellanox.com; x-originating-ip: [95.164.10.10] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 331f4af2-9ff3-4a50-e6d8-08d833968b04 x-ms-traffictypediagnostic: AM0PR0502MB4018: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr,ExtFwd x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: hTxB+6+Nj3/IKQUm0J/y+psHklf/jQVfkYZo5igJMqCKNsyHcOSHNmemVd529RMWVM0SQe6WcYN1hrEqbL7X/JA6uFFFl/fIfXeJxdkn49Gh/Ngmud/Q7BWZl+mhgLIlcovfS5iGxzS0Aq8rNjks/Z8ZlEZTT1ovs7hRyUqBi9DW2Wi6qO7x9lv97/MC5TJztmGkpMsGwSosEznS0q9gp3M9+nz9vMadSHrOcTltPL8yQa5a6NU5m5sIgW+h+ascVSFuUfJ7Xbh0/OnabNKIiHBxF7v5wABa6JNe79pRsr/px4YNKyfgsYpbqlBnpqsQJkqoif+1gDPi/kMBu7ap1A== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:AM4PR05MB3265.eurprd05.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(4636009)(366004)(346002)(376002)(39860400002)(396003)(136003)(6916009)(2906002)(55016002)(9686003)(478600001)(8936002)(33656002)(86362001)(54906003)(186003)(66946007)(66476007)(66556008)(66446008)(64756008)(316002)(71200400001)(52536014)(4326008)(83380400001)(53546011)(6506007)(5660300002)(26005)(76116006)(7696005); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: K5OM4Mih4+ZvBJ2sFCnflKy3Bf0olzuWcxgMLSBUt43annCBjV65tazOMrkhklHuBdH5HLdsjbPPhYh3vON8K2fVKVMIfHKeEIStDqhkjDZFv4u8UkSKIupQJhnGY6dgZOcDM/izKQP7S0JY3kVv9msVCj2YAUAk7uJIrKq8ojZaxScjhF2YFCkUwdpF3cYh+4P/Fb2nrDOz9zD2s7wBpuQ6b0JEKMMX221R8qzUIwQCFg2BXeRrJjcag+QDcPi/zaZk14bUGUHKNIuY6j6gjH9JlsLhEbWgFBUcFokXWhx94XIfqVL4Y2oMSTvONI9ozMsw5LZ7HRfQZ6oAbvak3wu6ZMGrSi92mhwAPwhVlvciDSxCcsGkYlKMGKjE6tglAcmP1AJCmIGX3ZKxuW/zz8LCs3T5m8m6jI8Rc6TMUpNHebXQMYOpnZUO+vhju+/BFa6liztCGRzEgUouSLJXhbqQZmtfYvchRB6JzKFn1GTIMj6VEqHvzri9RnS3M3gzNnZ6ohxbNzsmYaQ3hD+vk/AcAwFgOg7xVDmEgicdGTUP6RBlU0ZHvq9w+3b4Q/x7BZFtPOjwaWJO7n4yGcWB6c1wFmu3yKuAMJrEyWLhHYFrp3yk7cHe7BL4w5mIIevXRUZPZkgu5ANTSiuCQsoP9g== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: AM4PR05MB3265.eurprd05.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 331f4af2-9ff3-4a50-e6d8-08d833968b04 X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jul 2020 08:08:14.1701 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: dLN0/4l1c8XrI8dfynbGhLAHwBB3I0g2boW88fagqT8XLXibHjOPQaX65+Go1W3e5xab5U1ypgK/b3yer7TKm94uKJ3PpYU3iZfoYQg5t8U= X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR0502MB4018 Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix txonly mode timestamp intitialization 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 Very nice comment, I found it is useful, I'm on moving mlx5 PMD to use C11 = atomic primitives, thank you. But, don't you think it is an overkill for testpmd case and would make code= wordy and less readable ? How does testpmd start the forwarding? Let's have a glance at launch_packet= _forwarding(): - initialize forwarding with port_fwd_begin() - It is tx_only_begin for txo= nly mode, master core context - launch the forwarding cores rte_eal_remote_launch(), the forwarding will = happen in forward core context Let's reconsider: - tx_only_begin() is called once in master core context, forwarding is not= launched yet, there is NO any=20 concurrent access to variables, we can set ones in any way, NO atomic, vol= atile, etc. is needed at all. We should do setup and commit all our memory writes - rte_wmb() seems to b= e the very native way to do it once for all preceding writes. Performance can be neglected here -= it is not a datapath. - pkt_burst_prepare() is being executed in the forwarding core context, and= perform only READ access to the global variables, those are stable while forwarding lasts, and can = be considered as constants. No atomic, barriers, etc. are needed either. I agree - the volatile is red= undant ("over-reassurance"), let's remove it. But atomic access - not needed, would make code complicated. What do you think? Do you insist on atomic-barriered access implementing? With best regards,=20 Slava > -----Original Message----- > From: Phil Yang > Sent: Tuesday, July 28, 2020 19:24 > To: Slava Ovsiienko > Cc: Matan Azrad ; Raslan Darawsheh > ; Thomas Monjalon ; > ferruh.yigit@intel.com; Honnappa Nagarahalli > ; nd ; dev@dpdk.org; nd > > Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix txonly mode timestamp > intitialization >=20 > > -----Original Message----- > > From: dev On Behalf Of Viacheslav Ovsiienko > > Sent: Monday, July 27, 2020 11:27 PM > > To: dev@dpdk.org > > Cc: matan@mellanox.com; rasland@mellanox.com; > thomas@monjalon.net; > > ferruh.yigit@intel.com > > Subject: [dpdk-dev] [PATCH] app/testpmd: fix txonly mode timestamp > > intitialization > > > > The testpmd application forwards data in multiple threads. > > In the txonly mode the Tx timestamps must be initialized on per thread > > basis to provide phase shift for the packet burst being sent. This per > > thread initialization was performed on zero value of the variable in > > thread local storage and happened only once after testpmd forwarding > > start. Executing "start" and "stop" commands did not cause thread > > local variables zeroing and wrong timestamp values were used. >=20 > I think it is too heavy to use rte_wmb() to guarantee the visibility of > 'timestamp_init_req' updating for subsequent read operations. > We can use C11 atomics with explicit memory ordering instead of rte_wmb() > to achieve the same goal. >=20 > > > > Fixes: 4940344dab1d ("app/testpmd: add Tx scheduling command") > > > > Signed-off-by: Viacheslav Ovsiienko > > --- > > app/test-pmd/txonly.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c index > > 97f4a45..415431d 100644 > > --- a/app/test-pmd/txonly.c > > +++ b/app/test-pmd/txonly.c > > @@ -55,9 +55,13 @@ > > static struct rte_udp_hdr pkt_udp_hdr; /**< UDP header of tx packets. > > */ RTE_DEFINE_PER_LCORE(uint64_t, timestamp_qskew); > > /**< Timestamp offset per queue */ > > +RTE_DEFINE_PER_LCORE(uint32_t, timestamp_idone); /**< Timestamp > init > > done. */ > > + > > static uint64_t timestamp_mask; /**< Timestamp dynamic flag mask */ > > static int32_t timestamp_off; /**< Timestamp dynamic field offset */ > > static bool timestamp_enable; /**< Timestamp enable */ > > +static volatile uint32_t timestamp_init_req; >=20 > If we use C11 atomic builtins for 'timestamp_init_req' accessing, the vol= atile > key word becomes unnecessary. > Because they will generate same instructions. >=20 > > + /**< Timestamp initialization request. */ > > static uint64_t timestamp_initial[RTE_MAX_ETHPORTS]; > > > > static void > > @@ -229,7 +233,8 @@ > > rte_be64_t ts; > > } timestamp_mark; > > > > - if (unlikely(!skew)) { > > + if (unlikely(timestamp_init_req !=3D >=20 > if (unlikely(__atomic_load_n(×tamp_init_req, __ATOMIC_RELAXED) !=3D >=20 > > + RTE_PER_LCORE(timestamp_idone))) { > > struct rte_eth_dev *dev =3D &rte_eth_devices[fs- > > >tx_port]; > > unsigned int txqs_n =3D dev->data->nb_tx_queues; > > uint64_t phase =3D tx_pkt_times_inter * fs->tx_queue / > @@ -241,6 > > +246,7 @@ > > skew =3D timestamp_initial[fs->tx_port] + > > tx_pkt_times_inter + phase; > > RTE_PER_LCORE(timestamp_qskew) =3D skew; > > + RTE_PER_LCORE(timestamp_idone) =3D > > timestamp_init_req; >=20 >=20 > RTE_PER_LCORE(timestamp_idone) =3D > __atomic_load_n(×tamp_init_req, __ATOMIC_RELAXED); >=20 >=20 > > } > > timestamp_mark.pkt_idx =3D rte_cpu_to_be_16(idx); > > timestamp_mark.queue_idx =3D rte_cpu_to_be_16(fs- > > >tx_queue); > > @@ -426,6 +432,9 @@ > > timestamp_mask && > > timestamp_off >=3D 0 && > > !rte_eth_read_clock(pi, ×tamp_initial[pi]); > > + if (timestamp_enable) > > + timestamp_init_req++; >=20 >=20 > __atomic_add_fetch(×tamp_init_req, 1, __ATOMIC_ACQ_REL); >=20 >=20 > > + rte_wmb(); >=20 > We can remove it now. >=20 >=20 > Thanks, > Phil