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 23DF6A0352; Sun, 3 Nov 2019 23:41:14 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 520F41BE95; Sun, 3 Nov 2019 23:41:13 +0100 (CET) Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by dpdk.org (Postfix) with ESMTP id 89F9C1BE93 for ; Sun, 3 Nov 2019 23:41:11 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 18669213CA; Sun, 3 Nov 2019 17:41:11 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Sun, 03 Nov 2019 17:41:11 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=5TkPKhtH+XWGBmNykEUpXHscZxSWujqeU8tvxIY9Qnw=; b=knM/5nk/brRa eDiOUuKNlm1imh+c9KWATRc6cAuxRNUCFVwwaPaV6L5TJb1zliq6kqus5xfXDW2H OiP4d/zvu75s7ittceb8UbdKHQE1PPbZSNLTPTVd8Wlzaxweh3yMeYd8gABgUx7M PTrnCELa3nXCJ3pPr2h8C/VUQPQng2U= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=5TkPKhtH+XWGBmNykEUpXHscZxSWujqeU8tvxIY9Q nw=; b=CdkelSK42VzD6VTEhiLTeJVWHYXyWN09C7q+wUsZqNWbQfJ4MeTTR6aPt CMP9AA1FXbQEDAeuoWA318E+qkX1HCH/Snsw3NJlGOY2uNV6FXhSrrOggoNhFZux PCu+CITrBQFSR5gURqWzk1RJn0uQAMTlobz89gomPCPqsweaGTjIpN9Q24ox67sp jYiCK8/HaoH9Oe22/cw17J0ts/7uaENQE0QoDAvUASSWQSPm2FnzUNjWHkSmKAX8 oA2OdRPLEpX5u4hcOEScTLjpdCIUP7pfBrxu7bw0/hY40sLy3zQ8eO3OL69ck3fF 0/4rJCg+b1DbDYK3FgiegULiWQU+Q== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrudduuddgudeikecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc fkphepleefrddvfedrvdegkedrvddtkeenucfrrghrrghmpehmrghilhhfrhhomhepthhh ohhmrghssehmohhnjhgrlhhonhdrnhgvthenucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from xps.localnet (208.248.23.93.rev.sfr.net [93.23.248.208]) by mail.messagingengine.com (Postfix) with ESMTPA id 594E33060057; Sun, 3 Nov 2019 17:41:07 -0500 (EST) From: Thomas Monjalon To: Ray Kinsella Cc: dev@dpdk.org, "Yigit, Ferruh" , 'Damjan Marion' , "Wang, Haiyue" , Jerin Jacob Kollanukkaran , viacheslavo@mellanox.com, stephen@networkplumber.org, arybchenko@solarflare.com Date: Sun, 03 Nov 2019 23:41:05 +0100 Message-ID: <6097021.DVUxeYL0oo@xps> In-Reply-To: References: <20191015075133.38560-1-haiyue.wang@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add 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" 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:thomas@monjalon.net] > >>>> 26/10/2019 06:40, Wang, Haiyue: > >>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net] > >>>>>> 25/10/2019 18:02, Jerin Jacob: > >>>>>>> On Fri, Oct 25, 2019 at 9:15 PM Thomas Monjalon 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".