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 AB19EA0556; Wed, 25 May 2022 14:59:05 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 48C5240146; Wed, 25 May 2022 14:59:05 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 84257400EF for ; Wed, 25 May 2022 14:59:03 +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 CA43987; Wed, 25 May 2022 15:59:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru CA43987 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1653483542; bh=SeqOo0Yi143zhVtVSwMfr6ic4ZO+VTDuc2YulMeM5Bc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=jeKWnO7vatYnD0FtwGTdFQSL44muZmR7uTABEXK6Nf9E4eGDmyxww+WrHBmlBnjp6 VnIpVscB7V7AXgRXobS3uLiIiHJ6IIXem1WAk8Sg1j4nFNxA97cp4k8+b4/rfT1pPa ph70XOAKQcr2TWNo+bvfvA0UcUqqoVCwG/l3HdAw= Message-ID: Date: Wed, 25 May 2022 15:59:02 +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> <1836405.jbyF5MZJ3u@thomas> <31797171.aRNtrjHk3s@thomas> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <31797171.aRNtrjHk3s@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/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. If so, I'd wait for the next ABI breaking release and do not touch reserved fields.