Hi Slava, Thanks for your reply. I've no more description to talk about the code design. And I agree with your thinking, all roads lead to Rome, please don't hesitate to submit a patch to clean up things. Your rich experience is very appreciated to speed up the design, thanks! ;-)
BR, Haiyue > -----Original Message----- > From: Slava Ovsiienko <viachesl...@mellanox.com> > Sent: Monday, November 4, 2019 03:31 > To: Wang, Haiyue <haiyue.w...@intel.com>; Liu, Yu Y <yu.y....@intel.com>; > Thomas Monjalon > <tho...@monjalon.net> > Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh > <ferruh.yi...@intel.com>; > jerinjac...@gmail.com; Ye, Xiaolong <xiaolong...@intel.com>; Kinsella, Ray > <ray.kinse...@intel.com>; > Sun, Chenmin <chenmin....@intel.com>; Damjan Marion (damarion) > <damar...@cisco.com> > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode > information > > Hi, Haiyue > > > -----Original Message----- > > From: Wang, Haiyue <haiyue.w...@intel.com> > > Sent: Sunday, November 3, 2019 13:38 > > To: Slava Ovsiienko <viachesl...@mellanox.com>; Liu, Yu Y > > <yu.y....@intel.com>; Thomas Monjalon <tho...@monjalon.net> > > Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh > > <ferruh.yi...@intel.com>; jerinjac...@gmail.com; Ye, Xiaolong > > <xiaolong...@intel.com>; Kinsella, Ray <ray.kinse...@intel.com>; Sun, > > Chenmin <chenmin....@intel.com>; Damjan Marion (damarion) > > <damar...@cisco.com> > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst mode > > information > > > > Hi Slava, > > > > > -----Original Message----- > > > From: Slava Ovsiienko <viachesl...@mellanox.com> > > > Sent: Sunday, November 3, 2019 16:59 > > > To: Wang, Haiyue <haiyue.w...@intel.com>; Liu, Yu Y > > <yu.y....@intel.com>; Thomas Monjalon > > > <tho...@monjalon.net> > > > Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh > > <ferruh.yi...@intel.com>; > > > jerinjac...@gmail.com; Ye, Xiaolong <xiaolong...@intel.com>; Kinsella, > > Ray <ray.kinse...@intel.com>; > > > Sun, Chenmin <chenmin....@intel.com>; Damjan Marion (damarion) > > <damar...@cisco.com> > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst > > mode information > > > > > > > -----Original Message----- > > > > From: Wang, Haiyue <haiyue.w...@intel.com> > > > > Sent: Sunday, November 3, 2019 4:34 > > > > To: Slava Ovsiienko <viachesl...@mellanox.com>; Liu, Yu Y > > > > <yu.y....@intel.com>; Thomas Monjalon <tho...@monjalon.net> > > > > Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh > > > > <ferruh.yi...@intel.com>; jerinjac...@gmail.com; Ye, Xiaolong > > > > <xiaolong...@intel.com>; Kinsella, Ray <ray.kinse...@intel.com>; Sun, > > > > Chenmin <chenmin....@intel.com>; Damjan Marion (damarion) > > > > <damar...@cisco.com> > > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst > > mode > > > > information > > > > > > > > Hi Thomas, Slava, > > > > > > > > Please see the inline reply in one place. > > > > > > > > > -----Original Message----- > > > > > From: Slava Ovsiienko <viachesl...@mellanox.com> > > > > > Sent: Saturday, November 2, 2019 16:39 > > > > > To: Liu, Yu Y <yu.y....@intel.com>; Wang, Haiyue > > > > > <haiyue.w...@intel.com>; Thomas Monjalon > > <tho...@monjalon.net> > > > > > Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh > > > > > <ferruh.yi...@intel.com>; jerinjac...@gmail.com; Ye, Xiaolong > > > > > <xiaolong...@intel.com>; Kinsella, Ray <ray.kinse...@intel.com>; Sun, > > > > > Chenmin <chenmin....@intel.com>; Damjan Marion (damarion) > > > > > <damar...@cisco.com> > > > > > Subject: RE: [PATCH v1 3/3] ethdev: enhance the API for getting burst > > > > > mode information > > > > > > > > > > Hi > > > > > > -----Original Message----- > > > > > > From: Liu, Yu Y <yu.y....@intel.com> > > > > > > Sent: Saturday, November 2, 2019 8:56 > > > > > > To: Wang, Haiyue <haiyue.w...@intel.com>; Thomas Monjalon > > > > > > <tho...@monjalon.net> > > > > > > Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh > > > > > > <ferruh.yi...@intel.com>; jerinjac...@gmail.com; Ye, Xiaolong > > > > > > <xiaolong...@intel.com>; Kinsella, Ray <ray.kinse...@intel.com>; > > > > > > Sun, Chenmin <chenmin....@intel.com>; Slava Ovsiienko > > > > > > <viachesl...@mellanox.com>; Damjan Marion (damarion) > > > > > > <damar...@cisco.com>; Liu, Yu Y <yu.y....@intel.com> > > > > > > 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 > > > > > > > mitigate. > > > > > > 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 > > > > > suffix) > > 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 > > burst > > > > 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 > > fashion). > > > > > > > > > > > > > 1. > > > > > > > > The original issue description is: > > > > "VPP uses dladdr() to translate a function address to name, however, > > some > > > > tx/rx functions in DPDK are invisible for dladdr(), which is because > > > > they > > are > > > > defined as static." > > > > > > > > 2. > > > > > > > > So the RFC design is: one function, one description, like: > > > > > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch > > > > > > work.dpdk.org%2Fpatch%2F57644%2F&data=02%7C01%7Cviacheslavo > > > > > > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7 > > > > > > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat > > > > > > a=4re5GOXPSwGk5BTOYLglafzgjBzRLk1gXyWKT47o8o0%3D&reserved= > > > > 0 > > > > > > > > +#ifdef RTE_ARCH_X86 > > > > + else if (dev->rx_pkt_burst == ice_recv_scattered_pkts_vec_avx2) > > > > + len = snprintf(buf, sz, "AVX2 Vector Scattered Rx"); > > > > + else if (dev->rx_pkt_burst == ice_recv_scattered_pkts_vec) > > > > + len = snprintf(buf, sz, "Vector Scattered Rx"); > > > > + else if (dev->rx_pkt_burst == ice_recv_pkts_vec_avx2) > > > > + len = snprintf(buf, sz, "AVX2 Vector Rx"); > > > > + else if (dev->rx_pkt_burst == ice_recv_pkts_vec) > > > > + len = 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 > > > dladdr > > > functionality some new names were introduced and then converted to > > flags. > > > > > > > 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 > > > > decision > > 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 > > > > lit > > more, > > > > so we removed it in this patch): > > > > > > There are might be a lot of various burst functions, vectorized or not, > > > with various sets of supported offloads. Yes, identifying the engaged > > > burst > > > routine is meaningful, but it is not clear for me, why the vectorizing > > > type > > > should have dedicated means (flags) to identify ? > > > > > > > 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 > > device > > specific offloads as you said. > > > > This kind of string "Vector Neon/AltiVec/SSE/AVX2" can be common, we not > > treat it > > as 'flag', it is a normal bit like macro definition, and it will be > > translated 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 > > is 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 > > easily access > > if it may need, DON'T NEED TO BREAK THE ABI/API. Compared to > > 'strstr/strcasestr', > > 'mode->options & RTE_ETH_BURST_VECTOR' is more friendly ? > > Yes, it is more friendly for app. But there is quite another question: do we > really > need these flags for logging purposes ? There are no explicitly expressed > requirements from applications for these flags. > > > > > > > > > > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch > > > > > > work.dpdk.org%2Fpatch%2F61196%2F&data=02%7C01%7Cviacheslavo > > > > > > %40mellanox.com%7Ca99632b4e2444ec00b1f08d760065041%7Ca652971c7 > > > > > > d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083452540980873&sdat > > > > > > a=nm80Pt0fFWqmmrJcKY6ks4qRTJ7cjGJWEG1Wv6gxfSw%3D&reserved > > > > =0 > > > > > > > > 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 > > > > will 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". > > > > > > 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 > > > > > > There are no two versions of mtsc_empw - "mtsc_empw, Vector SSE", > > "mtsc_empw, Vector Neon". > > > If we developed vectorized version, I would prefer "mtsc_empw_sse". > > > > > > 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 -> > > name, options) > > > > Again, vector is not "only", it is just 'main' characteristic "tree > > ./drivers/net/ | > > grep rxtx_". > > we can design the Rx/Tx burst function mainly by vector type, it is > > straightforward. > > OK, we have some flag field proposed. Saying "why vector only" I meant > that vectorizing is just one of optimizing technics. I do not see it as > "main tree characteristics", sorry. > > What if some other vendor would like to add its own flags? For example, > mlx5 could add at least 8 optimizing flags for tx_burst and 4 flags for > rx_burst > (besides vector related ones). Why not? Why do we decide to add vector flags > only? > Other vendors might come into play and add its own flags describing the burst > routine > features and optimizations. And then say - "hey, these parameters define our > internal rxtx tree. It is very critical for performance, user must know about > ones". > > > > > Why 'name -> name' ? > Sorry for the MS Outlook (and I'm on the way to Mutt now), > it is not community friendly. > Correct sentence: "name" to "name, options" > > > 1.) [v4,4/4] app/testpmd: show the Rx/Tx burst mode description > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F% > > 2Fpatchwork.dpdk.org%2Fpatch%2F61198%2F&data=02%7C01%7Cviac > > heslavo%40mellanox.com%7C0ba4e73594684f944b7608d760525c97%7Ca6 > > 52971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637083779173049231&a > > mp;sdata=2NCHJJpsAYsb07FTEmvr4EiIVudm7ZCVhemh2g1MBG0%3D&r > > eserved=0 > > This is handled by the application itself, not so friendly, many lines of > > code to > > show. > Yes, it does not look nice, as for me. String should be simple and just > provided by PMD, > without any extra flags/options, IMHO. > > > > > > 2). [PATCH v1 3/3] ethdev: enhance the API for getting burst mode > > information > > if (rte_eth_tx_burst_mode_get(port_id, queue_id, &mode) == 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 dladdr > > functionality some new names were introduced and then converted to > > flags". > > > > > > Last, again, we define the bits 'RTE_ETH_BURST_XXX' for making the log > > message > > generation process easily if you agree vector type is common, the vector can > > be > Simple string returned by PMD eliminates "message process generation" at all. > No flags/options - no generation needed at all. In my opinion, PMD just should > return strings like "rx_burst_vector_sse", "rx_burst_vector_neon", etc. > > > used to improve the performance. And if new burst design can be used for > > most > > PMDs, use it as bit, the API helps to translate it to string. And the > > application > > 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. > > Applications live OK with dladdr(). The returned name is used for logging. > There is NO explicit requirements from application to provide some extra > options, > besides the name (or, at least, these ones are not visible for me). > > Sorry, it is not clear for me, how by introducing extra flags and the extra > "easy message generation process" we make life easier. If PMD just provides > the simple string "rx_burst_vector_sse", everyone seeing this string in the > log > understands what and how the named rx_burst is doing, right? Do you think > the message like "rx_burst, Vector SSE" looks better? OK, your PMD > is free to return it. > > Honestly, I do not mind against flags strictly, but I do not see any new > meanings > introduced by flags. It requires extra code, it might introduce some > ambiguity, > it would be ridiculous to see something like that: > "rx_burst_vector_neon, Vector_AVX" > And, the last, the flag field is a potential scarce resource for vendors. > > With best regards, Slava > > > > > > > > > > > Intel PMD can just assign "options", then finally, 'alternate_options' > > > > will > > be > > > > "Vector SSE". > > > > > > 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? > > > > > > With best regards, Slava > > > > > > > > > > > How about the design idea ? Again, this 'options' is not to do > > standardization, > > > > 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 <dev-boun...@dpdk.org> On Behalf Of Wang, Haiyue > > > > > > Sent: Saturday, November 2, 2019 1:30 PM > > > > > > To: Thomas Monjalon <tho...@monjalon.net> > > > > > > Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh > > > > > > <ferruh.yi...@intel.com>; jerinjac...@gmail.com; Ye, Xiaolong > > > > > > <xiaolong...@intel.com>; Kinsella, Ray <ray.kinse...@intel.com>; > > > > > > Sun, Chenmin <chenmin....@intel.com>; Slava Ovsiienko > > > > > > <viachesl...@mellanox.com> > > > > > > Subject: Re: [dpdk-dev] [PATCH v1 3/3] ethdev: enhance the API for > > > > > > getting burst mode information > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Thomas Monjalon <tho...@monjalon.net> > > > > > > > Sent: Saturday, November 2, 2019 06:46 > > > > > > > To: Wang, Haiyue <haiyue.w...@intel.com> > > > > > > > Cc: dev@dpdk.org; arybche...@solarflare.com; Yigit, Ferruh > > > > > > > <ferruh.yi...@intel.com>; jerinjac...@gmail.com; Ye, Xiaolong > > > > > > > <xiaolong...@intel.com>; Kinsella, Ray <ray.kinse...@intel.com>; > > > > > > > Sun, Chenmin <chenmin....@intel.com>; Slava Ovsiienko > > > > > > > <viachesl...@mellanox.com> > > > > > > > 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 > > > > > > > mitigate. > > > > > > > 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! > > > > > > ;-)
