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 C40B8A0558; Wed, 25 May 2022 16:23:43 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6E6A940150; Wed, 25 May 2022 16:23:43 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 02870400EF for ; Wed, 25 May 2022 16:23:42 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 639FE8A; Wed, 25 May 2022 17:23:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 639FE8A DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1653488622; bh=PVIJez6f5fMu8ydjdqxSsgWjBiGZnbC9/9VhtDCNjbE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=HvqWS7HrGIO5yQ3jNvCbFMNPDSzGVOnHKWQ+9rqXycJYv0MtN+3woFbTeDJtWO1tE qFp+/OBT60inuU4CmmaLi0F4ss2Y1KfOBb0ju12OYuLtWFpldeidTasWOxQPJeg8NK BZfLqReVlB0gKWCMXDe8EoSURVQNnxdlowmpEfJg= Message-ID: <9134a987-5b7e-74df-7366-70bbdd4792e1@oktetlabs.ru> Date: Wed, 25 May 2022 17:23:41 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [RFC v2 3/7] ethdev: introduce Rx queue based limit watermark Content-Language: en-US To: Thomas Monjalon , Stephen Hemminger , Spike Du Cc: "dev@dpdk.org" , Matan Azrad , Slava Ovsiienko , Ori Kam , Raslan Darawsheh , "ferruh.yigit@amd.com" References: <20220506035645.4101714-1-spiked@nvidia.com> <31797171.aRNtrjHk3s@thomas> <1740079.OQMrUlzKSU@thomas> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <1740079.OQMrUlzKSU@thomas> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 On 5/25/22 16:58, Thomas Monjalon wrote: > 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. > " I see. Makes sense. Thanks for the information. >> 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. Yes, it is better to avoid two ways to configure the same thing. > It should be either in rte_eth_rxq_info or a specific "get" function. I see no point to introduce specific get function for a single value. I think that rte_eth_rxq_info is the right way to get current value.