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 5BB77A32A4 for ; Fri, 25 Oct 2019 17:45:34 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 518971D155; Fri, 25 Oct 2019 17:45:33 +0200 (CEST) Received: from new2-smtp.messagingengine.com (new2-smtp.messagingengine.com [66.111.4.224]) by dpdk.org (Postfix) with ESMTP id 108AE1C25E for ; Fri, 25 Oct 2019 17:45:30 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailnew.nyi.internal (Postfix) with ESMTP id 769566DAC; Fri, 25 Oct 2019 11:45:29 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Fri, 25 Oct 2019 11:45:29 -0400 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=Q6srfDIhdRJErQ+s5SKcwEbI/XzG9Ec87rkHIsYGzfk=; b=YtvL93lweIPO 0BzHAPse9hrky+DmlPSBd+ug7kzC9oOKwLAurxQ6QFjwaaYOzctk0SBIIfdIdyxp vXUl3Yh1vJUjkj57i1yE7naoFNvbqzo5zkJ/1ymDN8Reh3Ismb0kIjpLZnH2Ipw/ 6tOZtpmf7ifw1Ztb9Mbj3SaUxdQmrn4= 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=Q6srfDIhdRJErQ+s5SKcwEbI/XzG9Ec87rkHIsYGz fk=; b=BVLxRaIyr7EPJS8IjMnDA8tWm/9kU0KpN6vj5o0AntBJ7Ye3EVweVYtYn Tki1bGNdGHUlumOY1OUFqcN10EmYeaygGeQeGYvSuUhtDMCO7zR39WFuOg+wvrnj 5iqtPQR+xaQ1S7zuVf1M22Hv91cX4FpQ/Ps55HQ4JLBdUlqvYFCzSI7x3QShyiP5 AsRWnXVH3hm6pULazhkJ7V7++EFFddnty3vkUmOPUmKGCU8xPaZxFHPTwqQ2r+oq YKNL69MskA1GcYC9puQj7XxLb3udLqpSnyiGNJgzI6u/irEii2Xft1fG7RXjlWJe xjx6FQqsMbVwzqPDLU8+x9RWT+SBg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrleefgdelfecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho mhgrshesmhhonhhjrghlohhnrdhnvghtnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 799B7D60057; Fri, 25 Oct 2019 11:45:25 -0400 (EDT) From: Thomas Monjalon To: Ferruh Yigit Cc: Haiyue Wang , dev@dpdk.org, xiaolong.ye@intel.com, ray.kinsella@intel.com, bernard.iremonger@intel.com, chenmin.sun@intel.com, arybchenko@solarflare.com, viacheslavo@mellanox.com, stephen@networkplumber.org, david.marchand@redhat.com, jerinj@marvell.com Date: Fri, 25 Oct 2019 17:45:23 +0200 Message-ID: <1811898.7XjjD7ZjLQ@xps> In-Reply-To: <419163b4-2fe9-443c-0796-e928cdf697d6@intel.com> References: <20191015075133.38560-1-haiyue.wang@intel.com> <1682096.FjkMPolNrE@xps> <419163b4-2fe9-443c-0796-e928cdf697d6@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" 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. > > Hopefully it is not too late to fix this API before releasing 19.11. > > > > I think the idea of getting infos from PMD internal mode is not bad. > > But I strongly disagree with standardizing the names. More below. > > > > [...] > >> +enum rte_eth_burst_mode_option { > >> + RTE_ETH_BURST_SCALAR = (1 << 0), > >> + RTE_ETH_BURST_VECTOR = (1 << 1), > > > > 2 bits for a boolean value? > > > >> + > >> + /**< bits[15:2] are reserved for each vector type */ > >> + RTE_ETH_BURST_ALTIVEC = (1 << 2), > >> + RTE_ETH_BURST_NEON = (1 << 3), > >> + RTE_ETH_BURST_SSE = (1 << 4), > >> + RTE_ETH_BURST_AVX2 = (1 << 5), > >> + RTE_ETH_BURST_AVX512 = (1 << 6), > >> + > >> + RTE_ETH_BURST_SCATTERED = (1 << 16), /**< Support scattered packets */ > >> + RTE_ETH_BURST_BULK_ALLOC = (1 << 17), /**< Support mbuf bulk alloc */ > >> + RTE_ETH_BURST_SIMPLE = (1 << 18), > > > > What means simple? Looks meaningless. > > It is used in some drivers as simple scalar path with most features are missing. This is a weak definition... > >> + RTE_ETH_BURST_PER_QUEUE = (1 << 19), /**< Support per queue burst */ > > > > What is per-queue burst? No need to add a comment if not adding any info. > > The burst API is *already* per-queue. > > That was a comment by me, most PMDs doesn't support different vector path per > queue, for those PMDs, to make application life easy PMD can say if it supports > per queue or not with this flag. > If PMD supports only per port data path, application doesn't need to call this > per queue if it want. It would require some doxygen explanations. > >> +}; > > > > How can we imagine standardizing the PMD optimizations? > > PMD developers are free to have as many burst implementation as they want. > > If we want to report info about what is used, it can be only a free string. > > The main target of the API is to define the vector path, not all optimizations, > so the number is limited. > > When there is a standardized output it can be easier to be consumed by the > applications. Why application needs this info to be standardized? I think it should not. I am really against such standardization. I think it would hurt more than help because there are a lot more major infos than just knowing the ISA it is using. I think such info should be used only for debugging or have a clue about the expected performance. You cannot standardize the expectations of a specific implementation. > >> +/** > >> + * 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. > >> +/** > >> + * Retrieve information about the Rx packet burst mode. > >> + * > >> + * @param port_id > >> + * The port identifier of the Ethernet device. > >> + * @param queue_id > >> + * The Rx queue on the Ethernet device for which information > >> + * will be retrieved. > >> + * @param mode > >> + * A pointer to a structure of type *rte_eth_burst_mode* to be filled > >> + * with the information of the packet burst mode. > > > > No reference to the enum rte_eth_burst_mode_option or RTE_ETH_BURST_ prefix? > > > >> + * > >> + * @return > >> + * - 0: Success > >> + * - -ENOTSUP: routine is not supported by the device PMD. > >> + * - -EINVAL: The port_id or the queue_id is out of range. > >> + */ > >> +__rte_experimental > >> +int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id, > >> + struct rte_eth_burst_mode *mode); > > [...] > >> +/** > >> + * Retrieve name about burst mode option. > >> + * > >> + * @param mode > >> + * The burst mode option of type *rte_eth_burst_mode_option*. > >> + * > >> + * @return > >> + * - "": Not found > >> + * - "xxx": name of the mode option. > >> + */ > >> +__rte_experimental > >> +const char * > >> +rte_eth_burst_mode_option_name(uint64_t option); > > > > rte_eth_burst_mode_name would be a better function name. > > But anyway, this function should not exist. > > The string should be freely returned by the PMD > > in the burst_mode_get functions.