RE: [PATCH v2 1/2] ethdev: remove unnecessary type conversion

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> Sent: Wednesday, 28 May 2025 11.14
> 
> >
> > > From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> > > Sent: Wednesday, 28 May 2025 10.24
> > >
> > > > >
> > > > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > > > Sent: Tuesday, 27 May 2025 17.07
> > > > > >
> > > > > > On Mon, 12 May 2025 20:37:19 +0530
> > > > > > <skori@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-
> skori@marvell.com/
> 
> So what we are saving with that patch: one cmp and one un-taken branch:
> @@ -6399,8 +6399,6 @@ rte_eth_rx_queue_count(uint16_t port_id, uint16_t
> queue_id)
>  		return -EINVAL;
>  #endif
> 
> -	if (p->rx_queue_count == NULL)
> -		return -ENOTSUP;
>  	return p->rx_queue_count(qd);
>  }
> 
> I wonder is how realistic (and measurable) is the gain?

Moving the performance discussion to the other thread [2].
[2]: https://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35E9FC9E@smartserver.smartshare.dk/

> 
> >
> > >
> > > > >
> > > > > 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