> From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com] > Sent: Wednesday, 28 May 2025 10.24 > > > > > > > > From: Stephen Hemminger [mailto:step...@networkplumber.org] > > > > Sent: Tuesday, 27 May 2025 17.07 > > > > > > > > On Mon, 12 May 2025 20:37:19 +0530 > > > > <sk...@marvell.com> wrote: > > > > > > > > > /**@{@name Rx hardware descriptor states > > > > > diff --git a/lib/ethdev/rte_ethdev_core.h > > > > b/lib/ethdev/rte_ethdev_core.h > > > > > index e55fb42996..4ffae4921a 100644 > > > > > --- a/lib/ethdev/rte_ethdev_core.h > > > > > +++ b/lib/ethdev/rte_ethdev_core.h > > > > > @@ -45,7 +45,7 @@ typedef uint16_t (*eth_tx_prep_t)(void *txq, > > > > > > > > > > > > > > > /** @internal Get number of used descriptors on a receive > queue. */ > > > > > -typedef uint32_t (*eth_rx_queue_count_t)(void *rxq); > > > > > +typedef int (*eth_rx_queue_count_t)(void *rxq); > > > > > > > > > > /** @internal Check the status of a Rx descriptor */ > > > > > typedef int (*eth_rx_descriptor_status_t)(void *rxq, uint16_t > > > > offset); > > > > > > > > > > > > This gets reported as ABI breakage. The change will have to wait > until > > > > next LTS (25.11) > > > > > > The return type was weird (wrong) to begin with. > > > When used, it gets cast to int: > > > > https://elixir.bootlin.com/dpdk/v25.03/source/lib/ethdev/rte_ethdev.h#L > 6404 > > > > Personally, I don't see anything strange here. > > devops rx_queue_count() supposed to return uint, because it should > never failed for > > valid queue.
The main thing wrong is inconsistency with its sibling API for TX queue count: /** @internal Get number of used descriptors on a receive queue. */ typedef uint32_t (*eth_rx_queue_count_t)(void *rxq); /** @internal Get number of used descriptors on a transmit queue. */ typedef int (*eth_tx_queue_count_t)(void *txq); > > While rte_eth_rx_queue_count() itself can fail - wrong port/queue id, > etc. > > BTW, rx_queue_setup() accepts only uint16_t for number of rx > descritoirs: > int rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id, > uint16_t nb_rx_desc, unsigned int socket_id, > const struct rte_eth_rxconf *rx_conf, > struct rte_mempool *mb_pool); > > shouldn't dev->rx_queue_count() then also return uitn16_t (for > consistency)? If neither the RX or TX queue count callbacks can fail, then yes, both APIs could be updated to return uint16_t. But it would be more future proof to allow these callbacks to fail, even on a valid queue. The public APIs rte_eth_rx/tx_queue_count() can fail, so passing on failures from the callbacks would not be a change of the public API. (Assuming no new error codes are allowed.) And the "future" already arrived: The return type must be int (or have the same size as int), for the performance patch [1] replacing the ops->callback==NULL check with dummy callbacks returning -ENOTSUP. [1]: https://inbox.dpdk.org/dev/20250512150732.65743-2-sk...@marvell.com/ > > > > > > > You are right that it formally changes the ABI, and we should go > through the LTS motions. > > > But, for this change, I'd favor an exception. > > > > Again, from my opinion, there is nothing that urgent/important why > such changes (if needed) > > can't wait till next LTS. > > For now, we can simply do type conversion explicitly at > rte_eth_rx_queue_count(). OK. No objections from me. Just trying to accelerate some cleanup work. > > > > > PS: As a consequence of this change, a patch to update the return > type of the callback in all the ethdev drivers should be provided. > > > > > > > > > > > > > > > [C] 'rte_eth_fp_ops rte_eth_fp_ops[32]' was changed at > > > > rte_ethdev.c:47:1: > > > > type of variable changed: > > > > array element type 'struct rte_eth_fp_ops' changed: > > > > type size hasn't changed > > > > 1 data member change: > > > > type of 'eth_rx_queue_count_t rx_queue_count' changed: > > > > underlying type 'uint32_t (*)(void*)' changed: > > > > in pointed to type 'function type uint32_t > (void*)': > > > > return type changed: > > > > entity changed from 'typedef uint32_t' to 'int' > > > > type size hasn't changed > > > > type size hasn't changed
