03/11/2019 21:35, Ray Kinsella: > On 29/10/2019 14:27, Ferruh Yigit wrote: > > On 10/26/2019 5:23 PM, Thomas Monjalon wrote: > >> 26/10/2019 11:23, Wang, Haiyue: > >>> From: Thomas Monjalon [mailto:tho...@monjalon.net] > >>>> 26/10/2019 06:40, Wang, Haiyue: > >>>>> From: Thomas Monjalon [mailto:tho...@monjalon.net] > >>>>>> 25/10/2019 18:02, Jerin Jacob: > >>>>>>> On Fri, Oct 25, 2019 at 9:15 PM Thomas Monjalon <tho...@monjalon.net> > >>>>>>> wrote: > >>>>>>>> 25/10/2019 16:08, Ferruh Yigit: > >>>>>>>>> On 10/25/2019 10:36 AM, Thomas Monjalon wrote: > >>>>>>>>>> 15/10/2019 09:51, Haiyue Wang: > >>>>>>>>>>> Some PMDs have more than one RX/TX burst paths, add the ethdev API > >>>>>>>>>>> that allows an application to retrieve the mode information about > >>>>>>>>>>> Rx/Tx packet burst such as Scalar or Vector, and Vector technology > >>>>>>>>>>> like AVX2. > >>>>>>>>>> > >>>>>>>>>> I missed this patch. I and Andrew, maintainers of ethdev, were not > >>>>>>>>>> CC'ed. > >>>>>>>>>> Ferruh, I would expect to be Cc'ed and/or get a notification > >>>>>>>>>> before merging. > >>>>>>>>> > >>>>>>>>> It has been discussed in the mail list and went through multiple > >>>>>>>>> discussions, > >>>>>>>>> patch is out since the August, +1 to cc all maintainers I missed > >>>>>>>>> that part, > >>>>>>>>> but when the patch is reviewed and there is no objection, why block > >>>>>>>>> the merge? > >>>>>>>> > >>>>>>>> I'm not saying blocking the merge. > >>>>>>>> My bad is that I missed the patch and I am asking for help with a > >>>>>>>> notification > >>>>>>>> in this case. Same for Andrew I guess. > >>>>>>>> Note: it is merged in master and I am looking to improve this > >>>>>>>> feature. > >>>>>>> > >>>>>>>>>>> +/** > >>>>>>>>>>> + * Ethernet device RX/TX queue packet burst mode information > >>>>>>>>>>> structure. > >>>>>>>>>>> + * Used to retrieve information about packet burst mode setting. > >>>>>>>>>>> + */ > >>>>>>>>>>> +struct rte_eth_burst_mode { > >>>>>>>>>>> + uint64_t options; > >>>>>>>>>>> +}; > >>>>>>>>>> > >>>>>>>>>> Why a struct for an integer? > >>>>>>>>> > >>>>>>>>> Again by a request from me, to not need to break the API if we need > >>>>>>>>> to add more > >>>>>>>>> thing in the future. > >>>>>>>> > >>>>>>>> I would replace it with a string. This is the most flexible API. > >>>>>>> > >>>>>>> IMO, Probably, best of both worlds make a good option here, > >>>>>>> as Haiyue suggested if we have an additional dev_specific[1] in > >>>>>>> structure. > >>>>>>> and when a pass to the application, let common code make final string > >>>>>>> as > >>>>>>> (options flags to string + dev_specific) > >>>>>>> > >>>>>>> options flag can be zero if PMD does not have any generic flags nor > >>>>>>> interested in such a scheme. > >>>>>>> Generic flags will help at least to have some common code. > >>>>>>> > >>>>>>> [1] > >>>>>>> struct rte_eth_burst_mode { > >>>>>>> uint64_t options; > >>>>>>> char dev_specific[128]; /* PMD has specific burst mode > >>>>>>> information */ > >>>>>>> }; > >>>>>> > >>>>>> I really don't see how we can have generic flags. > >>>>>> The flags which are proposed are just matching > >>>>>> the functions implemented in Intel PMDs. > >>>>>> And this is a complicate solution. > >>>>>> Why not just returning a name for the selected Rx/Tx mode? > >>>>> > >>>>> Intel PMDs use the *generic* methods like x86 SSE, AVX2, ARM NEON, PPC > >>>>> ALTIVEC, > >>>>> 'dev->data->scattered_rx' etc for the target : "DPDK is the Data Plane > >>>>> Development Kit > >>>>> that consists of libraries to accelerate packet processing workloads > >>>>> running on a wide > >>>>> variety of CPU architectures." > >>>> > >>>> How RTE_ETH_BURST_SCATTERED and RTE_ETH_BURST_BULK_ALLOC are generic? > >>>> They just match some features of the Intel PMDs. > >>>> Why not exposing other optimizations of the Rx/Tx implementations? > >>>> You totally missed the point of generic burst mode description. > >>>> > >>>>> If understand these new experimental APIs from above, then bit options > >>>>> is the best, > >>>>> and we didn't invent new words to describe them, just from the CPU & > >>>>> other *generic* > >>>>> technology. And the application can loop to check which kind of burst > >>>>> is running by > >>>>> just simple bit test. > >>>>> > >>>>> If PMDs missed these, they can update them in future roadmaps to > >>>>> enhance their PMDs, > >>>>> like MLX5 supports ARM NEON, x86 SSE. > >>>> > >>>> I have no word! > >>>> You really think other PMDs should learn from Intel how to "enhance" > >>>> their PMD? > >>>> You talk about mlx5, did you look at its code? Did you see the burst > >>>> modes > >>>> depending on which specific hardware path is used (MPRQ, EMPW, inline)? > >>>> Or depending on which offloads are handled? > >>>> > >>>> Again, the instruction set used by the function is a small part > >>>> of the burst mode optimization. > >>>> > >>>> So you did not reply to my question: > >>>> Why not just returning a name for the selected Rx/Tx mode? > >>> > >>> In fact, RFC v1/v2 returns the *name*, but the *name* is hard for > >>> application to do further processing, strcmp, strstr ? Not so nice > >>> for C code, and it is not so standard, So switch it to bit definition. > >> > >> Again, please answer my question: why do you need it? > >> I think it is just informative, that's why a string should be enough. > >> I am clearly against the bitmap because it is way too much restrictive. > >> I disagree that knowing it is using AVX2 or AVX512 is so interesting. > >> What you would like to know is whether it is processing packets 4 by 4, > >> for instance, or to know which offload is supported, or what hardware trick > >> is used in the datapath design. > >> There are so many options in a datapath design that it cannot be > >> represented with a bitmap. And it makes no sense to have some design > >> criterias more important than others. > >> I Cc an Intel architect (Edwin) who could explain you how much > >> a datapath design is more complicate than just using AVX instructions. > > > > 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 know 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.
No! The name of this API is "burst mode information", not "vector instructions used". I think the main error is that in Intel PMDs, each Rx/Tx function use different vector instructions. So you generalize that knowing the vectors instructions will give you a good information about the performance. But this is generally wrong! The right level of infos is much more complex. > > There are many optimization in the data path, I agree we may not represent > > all > > of them, and agreed existing enum having "RTE_ETH_BURST_BULK_ALLOC" and > > similar > > causing this confusion, perhaps we can remove them. > > > > And if the requirement from the application is just informative, I would > > agree > > that free text string will be better, right now > > 'rte_eth_rx/tx_burst_mode_get()' > > is the main API to provide the information and > > 'rte_eth_burst_mode_option_name()' is a helper for application/driver to log > > this information. > > > > Well look we have a general deficit of information about what is happening > under > the covers in DPDK. The end user may get wildly different performance > characteristics > based on the DPDK configuration. Simple example is using flow director causes > the i40e > PMD to switch to using a scalar code path, and performance may as much as > half. > > This can cause no end of head-scratching in consuming products, I have done > some > of that head scratching myself, it is a usability nightmare. > > FD.io VPP tries to work around this by mining the call stack, to give the > user _some_ > kind of information about what is happening. These kind of heroics should not > be necessary. > > For exactly the same reasons as telemetry, we should be trying to give the > users as much > information as possible, in as standard as format as possible. Otherwise DPDK > becomes arcane leaving the user running gdb to understand what is going on, > as I > frequently do. I agree we must provide a clue to understand the performance result. As Stephen commented at the very beginning, a log is enough for such debug. But his comment was ignored. You wanted an API, fine. I am OK to have an API to request infos which are also in logs. > Finally, again for the same reasons as telemetry, I would say that machine > readable is the > ideal here. I disagree here. There is no need to make this info machine readable. We want a clue about the optimizations which are all about creativity. And we cannot make creativity of developers "machine readable".
