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 CD4C6A0352; Sun, 3 Nov 2019 12:38:36 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B50FE1D417; Sun, 3 Nov 2019 12:38:35 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 7254C1D414 for ; Sun, 3 Nov 2019 12:38:34 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Nov 2019 03:38:33 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,263,1569308400"; d="scan'208";a="191474848" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga007.jf.intel.com with ESMTP; 03 Nov 2019 03:38:32 -0800 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 3 Nov 2019 03:38:32 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 3 Nov 2019 03:38:32 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.108]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.60]) with mapi id 14.03.0439.000; Sun, 3 Nov 2019 19:38:30 +0800 From: "Wang, Haiyue" To: Slava Ovsiienko , "Liu, Yu Y" , Thomas Monjalon CC: "dev@dpdk.org" , "arybchenko@solarflare.com" , "Yigit, Ferruh" , "jerinjacobk@gmail.com" , "Ye, Xiaolong" , "Kinsella, Ray" , "Sun, Chenmin" , "Damjan Marion (damarion)" Thread-Topic: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode information Thread-Index: AQHVkA85NRUGBSr/iU+DJBHluQAAgad2ZiSAgAD2BdD//5LDgIAAHLKAgAGgxyD///dfAIAAoh0Q Date: Sun, 3 Nov 2019 11:38:29 +0000 Message-ID: References: <20191031171139.105110-1-haiyue.wang@intel.com> <20191031171139.105110-3-haiyue.wang@intel.com> <20693558.VL3dRorq05@xps> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDY4NTUyNDctMGIyNy00MWUzLTllNDYtZDVkZTM3ZDM3YjU1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiczRvZHUxSmdOV21YS1lVVUxnU3lBV1d0WEVvSFpSSXQ5Zk42aW56ZjdCSE9rd3IzbCtnMGRNOWFsS2ROUEhnViJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v1 3/3] ethdev: enhance the API for getting burst mode information 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 Slava, > -----Original Message----- > From: Slava Ovsiienko > Sent: Sunday, November 3, 2019 16:59 > To: Wang, Haiyue ; Liu, Yu Y ;= Thomas Monjalon > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh ; > jerinjacobk@gmail.com; Ye, Xiaolong ; Kinsella, Ra= y ; > Sun, Chenmin ; Damjan Marion (damarion) > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst mod= e information >=20 > > -----Original Message----- > > From: Wang, Haiyue > > Sent: Sunday, November 3, 2019 4:34 > > To: Slava Ovsiienko ; Liu, Yu Y > > ; Thomas Monjalon > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh > > ; jerinjacobk@gmail.com; Ye, Xiaolong > > ; Kinsella, Ray ; Sun, > > Chenmin ; Damjan Marion (damarion) > > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst m= ode > > information > > > > Hi Thomas, Slava, > > > > Please see the inline reply in one place. > > > > > -----Original Message----- > > > From: Slava Ovsiienko > > > Sent: Saturday, November 2, 2019 16:39 > > > To: Liu, Yu Y ; Wang, Haiyue > > > ; Thomas Monjalon > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh > > > ; jerinjacobk@gmail.com; Ye, Xiaolong > > > ; Kinsella, Ray ; Sun, > > > Chenmin ; Damjan Marion (damarion) > > > > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst > > > mode information > > > > > > Hi > > > > -----Original Message----- > > > > From: Liu, Yu Y > > > > Sent: Saturday, November 2, 2019 8:56 > > > > To: Wang, Haiyue ; Thomas Monjalon > > > > > > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh > > > > ; jerinjacobk@gmail.com; Ye, Xiaolong > > > > ; Kinsella, Ray ; > > > > Sun, Chenmin ; Slava Ovsiienko > > > > ; Damjan Marion (damarion) > > > > ; Liu, Yu Y > > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting > > > > burst mode information > > > > > > > > Add Damjan from FD.io for awareness... > > > > > > > > Hi Thomas, > > > > > > > > Long time no see. Sorry I use outlook which is not friendly to > > > > community email. > > > > > > > > >Anyway I will propose to replace this API in the next release. > > > > Will your plan be affected by API/ABI stable plan? > > > > BTW, if you propose new change in next release, it will make DPDK > > > > consumer(FD.io) to change again. > > > > So even if it is not affected to the API/ABI stable plan, do we > > > > still have time to get a solution for everyone in DPDK 19.11 with > > > > your contribution/acceleration? > > > > > > > > > I suspect a real hidden issue in Intel CPUs that you try to mitig= ate. > > > > Please be rest assured it is not the case. > > > > This request is just from one FD.io project internal bug " tx/rx > > > > burst function is shown as nil" reported by Chenmin. > > > > > > Why just the presenting string with function name (possible with suff= ix) is > > not enough? > > > I would like to see this API (strings approach) in mlx5 either, > > > dropping the entire feature does not look nice, as for me. > > > > > > We could consider some requirements for the name suffices to > > > distinguish whether function uses vector instructions and which ones = if any. > > > > > > > My understanding is DPDK behavior was taken as bug for someone in > > > > FD.io project and potentially will mislead other DPDK consumer. > > > > > > Why does FD.io code want to know which vector extension is used by bu= rst > > routines? > > > Is it going to share/preserve some resources (registers, etc.)? Is it= robust ? > > > Burst routines might not know whether vector extensions is used (they > > > might call libraries, even rte_memcpy() can use vectors in implicit f= ashion). > > > > > > > 1. > > > > The original issue description is: > > "VPP uses dladdr() to translate a function address to name, however, so= me > > tx/rx functions in DPDK are invisible for dladdr(), which is because t= hey are > > defined as static." > > > > 2. > > > > So the RFC design is: one function, one description, like: > > https://eur03.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fpat= ch > > work.dpdk.org%2Fpatch%2F57644%2F&data=3D02%7C01%7Cviacheslavo > > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7 > > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat > > a=3D4re5GOXPSwGk5BTOYLglafzgjBzRLk1gXyWKT47o8o0%3D&reserved=3D > > 0 > > > > +#ifdef RTE_ARCH_X86 > > + else if (dev->rx_pkt_burst =3D=3D ice_recv_scattered_pkts_vec_avx2) > > + len =3D snprintf(buf, sz, "AVX2 Vector Scattered Rx"); > > + else if (dev->rx_pkt_burst =3D=3D ice_recv_scattered_pkts_vec) > > + len =3D snprintf(buf, sz, "Vector Scattered Rx"); > > + else if (dev->rx_pkt_burst =3D=3D ice_recv_pkts_vec_avx2) > > + len =3D snprintf(buf, sz, "AVX2 Vector Rx"); > > + else if (dev->rx_pkt_burst =3D=3D ice_recv_pkts_vec) > > + len =3D snprintf(buf, sz, "Vector Rx"); #endif > The application gets the routine names with dladdr(). OK, it happens. > It is not clear for me why instead of direct replacement/extension of dla= ddr > functionality some new names were introduced and then converted to flags. >=20 Sorry, can you explain more ? Who 'direct replacement/extension of dladdr' = ? VPP, or DPDK ? > > 3. Since the main issue is as Damjan replied in another thread: > > "people are reporting lower performance caused by DPDK deciding for > > variety > > of reasons to switch from vector PMD to scalar one." > > > > And Ferruh replied also: > > "As I understand this is to let applications to give informed decis= ion based > > on what vectorization is used in the driver, currently this is not= known by > > the application. > > > > And as previously replied, the main target of the API is to define= the vector > > path, not all optimizations, so the number is limited." > > > > So we enhanced it with bit, example detail is (Yes, we defined a l= it more, > > so we removed it in this patch): >=20 > There are might be a lot of various burst functions, vectorized or not, > with various sets of supported offloads. Yes, identifying the engaged bur= st > routine is meaningful, but it is not clear for me, why the vectorizing ty= pe > should have dedicated means (flags) to identify ? >=20 The new 'rte_eth_rx/tx_burst_mode_get' works like logging, but in fact, the= log message is something special, like "Vector Neon/AltiVec/SSE/AVX2" and the d= evice specific offloads as you said. This kind of string "Vector Neon/AltiVec/SSE/AVX2" can be common, we not tr= eat it as 'flag', it is a normal bit like macro definition, and it will be transla= ted into string later. And we want to make PMD's string format life to be easy, don'= t need to call 'snprintf/sprintf' with the copied string format. So now, the log message format is: device specific (if have) + "Vector ..."= (if have, this is not MUST, if the PMD doesn't use vector, but at least, this i= s not hardware specific, it is some common from arch: lib/librte_eal/common/arch/= arm,ppc_64,x86). Further, as a SDK, the API exposes these common bit data for application ea= sily access if it may need, DON'T NEED TO BREAK THE ABI/API. Compared to 'strstr/strcas= estr', 'mode->options & RTE_ETH_BURST_VECTOR' is more friendly ? > > > > https://eur03.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fpat= ch > > work.dpdk.org%2Fpatch%2F61196%2F&data=3D02%7C01%7Cviacheslavo > > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7 > > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat > > a=3Dnm80Pt0fFWqmmrJcKY6ks4qRTJ7cjGJWEG1Wv6gxfSw%3D&reserved > > =3D0 > > > > 4. And thanks Jerin's suggestion, I think his word can be more accurate= : "This > > would > > help to reuse some of the flags to name conversion logic across all = PMDs" > > for the > > reason we try to use bit to reduce some string format effort, it wil= l be > > handled > > by the API internally "burst_mode_options_append(struct > > rte_eth_burst_mode *mode)". > > Now the new API will return the string finally: > > > > #define RTE_ETH_BURST_MODE_ALT_OPT_SIZE 1024 struct > > rte_eth_burst_mode { > > uint64_t options; > > > > /**< Each PMD can fill specific burst mode information into this, and > > * ethdev APIs will append the 'options' string format at its end. > > */ > > char alternate_options[RTE_ETH_BURST_MODE_ALT_OPT_SIZE]; > > }; > > > > So MLX PMD can add 'full_empw', 'mtsc_empw' etc into 'alternate_options= ' > > firstly, assign 'RTE_ETH_BURST_VECTOR | RTE_ETH_BURST_SSE' to 'options' > > as needed, then finally, 'alternate_options' will be "full_empw, Vector= SSE". >=20 > For mlx5 tx_burst these flags have no meaning. All information regarding = routine > is encoded within the name, mtsc stands for: > m - multisegment > t - TSO > s - software tunnel parser > c - check sum >=20 > There are no two versions of mtsc_empw - "mtsc_empw, Vector SSE", "mtsc_e= mpw, Vector Neon". > If we developed vectorized version, I would prefer "mtsc_empw_sse". >=20 > To summarize: > - application uses routine names, gets with dladdr(). Nice. > - compatible API providing names of internal routines is proposed. Nice. > - users now are able to identify the engaged burst routine. Nice. > - proposed API is extended, some vector related flags were added. Hmmm...= . Questionable. > Why vector related only? Why do we change the string format? (name -> n= ame, options) Again, vector is not "only", it is just 'main' characteristic "tree ./drive= rs/net/ | grep rxtx_". we can design the Rx/Tx burst function mainly by vector type, it is straigh= tforward. Why 'name -> name' ? 1.) [v4,4/4] app/testpmd: show the Rx/Tx burst mode description https://patchwork.dpdk.org/patch/61198/ This is handled by the application itself, not so friendly, many lines of c= ode to show. 2). [PATCH v1 3/3] ethdev: enhance the API for getting burst mode informati= on if (rte_eth_tx_burst_mode_get(port_id, queue_id, &mode) =3D=3D 0) printf("\nBurst mode: %s", mode.alternate_options); This design may meet your question above if I understand correctly. "It is not clear for me why instead of direct replacement/extension of dla= ddr functionality some new names were introduced and then converted to f= lags". Last, again, we define the bits 'RTE_ETH_BURST_XXX' for making the log mess= age generation process easily if you agree vector type is common, the vector ca= n be used to improve the performance. And if new burst design can be used for mo= st PMDs, use it as bit, the API helps to translate it to string. And the appli= cation can use the bit to do other kind of information display. We define it a little more than 'simple string' for just making life easy. = In fact, the patch comes from "simple string", RFC v1, v2, v3, PATCH v1 v2 v3 v4. >=20 >=20 > > Intel PMD can just assign "options", then finally, 'alternate_options' = will be > > "Vector SSE". >=20 > As I see from initial patch, Intel PMD has dedicated routines with unique= names for > each type of vectorization. Is there some burst routine with single name= which could > operate with different vectorization types, depending on configuration? >=20 > With best regards, Slava >=20 > > > > How about the design idea ? Again, this 'options' is not to do standard= ization, > > just want to reduce the duplicated name string things. > > > > > With best regards, Slava > > > > > > > Haiyue is working with Chenmin to address the issue and with your > > > > support it will be even better. > > > > > > > > Your support will be highly appreciated! > > > > > > > > Thanks & Regards, > > > > Yu Liu > > > > > > > > -----Original Message----- > > > > From: dev On Behalf Of Wang, Haiyue > > > > Sent: Saturday, November 2, 2019 1:30 PM > > > > To: Thomas Monjalon > > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh > > > > ; jerinjacobk@gmail.com; Ye, Xiaolong > > > > ; Kinsella, Ray ; > > > > Sun, Chenmin ; Slava Ovsiienko > > > > > > > > Subject: Re: [dpdk-dev] [PATCH v1 3/3] ethdev: enhance the API for > > > > getting burst mode information > > > > > > > > > -----Original Message----- > > > > > From: Thomas Monjalon > > > > > Sent: Saturday, November 2, 2019 06:46 > > > > > To: Wang, Haiyue > > > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; Yigit, Ferruh > > > > > ; jerinjacobk@gmail.com; Ye, Xiaolong > > > > > ; Kinsella, Ray ; > > > > > Sun, Chenmin ; Slava Ovsiienko > > > > > > > > > > Subject: Re: [PATCH v1 3/3] ethdev: enhance the API for getting > > > > > burst mode information > > > > > > > > > > Thank you for trying to address comments done late. > > > > > > > > > > 31/10/2019 18:11, Haiyue Wang: > > > > > > --- a/lib/librte_ethdev/rte_ethdev.h > > > > > > +++ b/lib/librte_ethdev/rte_ethdev.h > > > > > > > > > > > > > > +#define RTE_ETH_BURST_ALTIVEC (1ULL << 2) > > > > > > +#define RTE_ETH_BURST_NEON (1ULL << 3) > > > > > > +#define RTE_ETH_BURST_SSE (1ULL << 4) > > > > > > +#define RTE_ETH_BURST_AVX2 (1ULL << 5) > > > > > > +#define RTE_ETH_BURST_AVX512 (1ULL << 6) > > > > > > > > > > Of course, I still believe that giving a special treatment to > > > > > vector instructions is wrong. > > > > > You did not justify why it needs to be defined in bits instead of > > > > > string. I am not asking again because anyway you don't really > > > > > reply. I think you are executing an order you received and I don'= t > > > > > want to blame you more. > > > > > I suspect a real hidden issue in Intel CPUs that you try to mitig= ate. > > > > > No need to reply to this comment. > > > > > Anyway I will propose to replace this API in the next release. > > > > > > > > Never mind, if this design is truly ugly, drop it all now. I also > > > > prefer to do the best, that's why open source is amazing, thanks! > > > > ;-)