From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id F31E1A0558; Wed, 25 May 2022 15:58:22 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9839D40146; Wed, 25 May 2022 15:58:22 +0200 (CEST) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by mails.dpdk.org (Postfix) with ESMTP id BE025400EF for ; Wed, 25 May 2022 15:58:21 +0200 (CEST) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 102635C00A8; Wed, 25 May 2022 09:58:21 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Wed, 25 May 2022 09:58:21 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm1; t=1653487101; x= 1653573501; bh=rCZzo35GEBqBj317ETUxKhmtGsxwl1eTxXE4JDvCgek=; b=X 1Dj0eX9F+8YiovYXDFHxj+G2lKJsfGvdIC1EhBLqNcFGulWouHAzNd/0OrgJlEW+ og1wHPlu6KDtgvHaM1wGaBd8tnw4CaITRddRXFro+P3v4v71fqDR6qYSGlopZf2Z 8fBUdf1E+gs2CkihxGdj5VSJxaBpzQdjgYuGaUcQswlfnxVMIQMUQL61XPlJy0Us 1mLwrmtiNamgsdnA0Gt2mp7669z0D+yezCA4wfrRopfYhQrARNhj0nkUH8A9eW56 RfdZ4JXudQbktLbTLYyAWNK/Koqdfdmg10O4v0yj/1wupOoFI22gbnDUn0iewLA3 FzW8dRRS6chLz/qr8UnyQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1653487101; x= 1653573501; bh=rCZzo35GEBqBj317ETUxKhmtGsxwl1eTxXE4JDvCgek=; b=F m0w3O+5PMOtrrKhAnsRQmlJv32jZZQsx79xaqHjJy+cQm2CPkZakI4xani+kWOTy 148JfLLWVwTl86k/qY1/1wGDp1JuNQJY3PBXtFqIYsbyd2BzjSqXVGNwFGgtQnCL Mz3YDeInddyIyX3rooX5YcrmJmg3xYAW+WZHLOLMyve6RD8HKJxIO1Oy0WuAUSTy McFinwEbW94zk/lCRZiKenzrFG9bSWQryB9T91RUsvaY0UbM/GbQM2CUUNajdjly WmhZLEJazgej5ktjSc4A0ml2PnZyhdjCdvMrikCi2Nb0oS4aPcOFiLkn6uBFw9x4 9Z0NXSbV6xFbAp9UheUjg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrjeehgdeilecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvfevufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepjeejffffgfffkeefffelgfekleetjeffleeludeghfehleffteeh veduffdugfdvnecuffhomhgrihhnpeguphgukhdrohhrghenucevlhhushhtvghrufhiii gvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghlohhn rdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 25 May 2022 09:58:18 -0400 (EDT) From: Thomas Monjalon To: Stephen Hemminger , Spike Du , Andrew Rybchenko Cc: "dev@dpdk.org" , Matan Azrad , Slava Ovsiienko , Ori Kam , Raslan Darawsheh , "ferruh.yigit@amd.com" Subject: Re: [RFC v2 3/7] ethdev: introduce Rx queue based limit watermark Date: Wed, 25 May 2022 15:58:09 +0200 Message-ID: <1740079.OQMrUlzKSU@thomas> In-Reply-To: References: <20220506035645.4101714-1-spiked@nvidia.com> <31797171.aRNtrjHk3s@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 25/05/2022 14:59, Andrew Rybchenko: > On 5/24/22 11:18, Thomas Monjalon wrote: > > 24/05/2022 04:50, Spike Du: > >> From: Thomas Monjalon > >>> 23/05/2022 05:01, Spike Du: > >>>> From: Stephen Hemminger > >>>>> Spike Du wrote: > >>>>>> --- a/lib/ethdev/rte_ethdev.h > >>>>>> +++ b/lib/ethdev/rte_ethdev.h > >>>>>> @@ -1249,7 +1249,16 @@ struct rte_eth_rxconf { > >>>>>> */ > >>>>>> union rte_eth_rxseg *rx_seg; > >>>>>> > >>>>>> - uint64_t reserved_64s[2]; /**< Reserved for future fields */ > >>>>>> + /** > >>>>>> + * Per-queue Rx limit watermark defined as percentage of Rx queue > >>>>>> + * size. If Rx queue receives traffic higher than this percentage, > >>>>>> + * the event RTE_ETH_EVENT_RX_LWM is triggered. > >>>>>> + */ > >>>>>> + uint8_t lwm; > >>>>>> + > >>>>>> + uint8_t reserved_bits[3]; > >>>>>> + uint32_t reserved_32s; > >>>>>> + uint64_t reserved_64s; > >>>>> > >>>>> Ok but, this is an ABI risk about this because reserved stuff was > >>>>> never required before. > >>> > >>> An ABI compatibility issue would be for an application compiled with an old > >>> DPDK, and loading a new DPDK at runtime. > >>> Let's think what would happen in such a case. > >>> > >>>>> Whenever is a reserved field is introduced the code (in this case > >>>>> rte_ethdev_configure). > >>> > >>> rte_eth_rx_queue_setup() is called with rx_conf->lwm not initialized. > >>> Then the library and drivers may interpret a wrong value. > >>> > >>>>> Best practice would have been to have the code require all reserved > >>>>> fields be > >>>>> 0 in earlier releases. In this case an application is like to define > >>>>> a watermark of zero; how will your code handle it. > >>>> > >>>> Having watermark of 0 is desired, which is the default. LWM of 0 means > >>>> the Rx Queue's watermark is not monitored, hence no LWM event is > >>> generated. > >>> > >>> The problem is to have a value not initialized. > >>> I think the best approach is to not expose the LWM value through this > >>> configuration structure. > >>> If the need is to get the current value, we should better add a field in the > >>> struct rte_eth_rxq_info. > >> > >> At least from all the dpdk app/example code, rxconf is initialized to 0 then setup > >> The Rx queue, if user follows these examples we should not have ABI issue. > >> Since many people are concerned about rxconf change, it's ok to remove the LWM > >> Field there. > >> Yes, I think we can add lwm into rte_eth_rxq_info. If we can set Rx queue's attribute, > >> We should have a way to get it. > > > > Unfortunately we cannot rely on examples for ABI compatibility. > > My suggestion of moving the field in rte_eth_rxq_info > > is not obvious because it could change the size of the struct. > > But thanks to __rte_cache_min_aligned, it is OK. > > Running pahole on this struct shows we have 50 bytes free: > > /* size: 128, cachelines: 2, members: 6 */ > > /* padding: 50 */ > > > > The other option would be to get the LWM value with a "get" function. > > > > What others prefer? > > If I'm not mistaken the changeset breaks ABI in any case since > it adds a new event and changes MAX. I think we can consider it as not a breakage (a rule should be added). Last time we had to update this enum, this was the conclusion: from https://git.dpdk.org/dpdk/commit/?id=44bf3c796be3f " The new event type addition in the enum is flagged as an ABI breakage, so an ignore rule is added for these reasons: - It is not changing value of existing types (except MAX) - The new value is not used by existing API if the event is not registered In general, it is safe adding new ethdev event types at the end of the enum, because of event callback registration mechanism. " > If so, I'd wait for the > next ABI breaking release and do not touch reserved fields. In any case, rte_eth_rxconf is not a good fit because we have a separate function for configuration. It should be either in rte_eth_rxq_info or a specific "get" function.