Re: [dpdk-dev] [RFC 2/7] eth: make drivers to use new API for Rx

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Ferruh,

> 
> Overall this enables us hiding the ethdev internals, which is good. But it
> duplicates most of the datapath function (rx burst for this patch) per each PMD ops.

Yes, same as right now rte_eth_rx/tx_burst() code can be duplicated in dozen places 
inside user-level code. And as any other 'static inline' function that we define and use inside DPDK.
Personally I don't see why it is a problem.

> 
> I wonder if we can have the callbacks ('_rte_eth_rx_epilog()') as separate
> function, this still enables us to hide the structs. Of course additional
> function call will bring some overhead, but if we enabled callbacks and calling
> them per packet, do we really care about additional function call?

Callbacks are not per packet, but per burst of packets - same as actual RX/TX.
A drawback with such approach -  we either have to keep
post_rx_burst_cbs[RTE_MAX_QUEUES_PER_PORT] visible to the user
(which I'd prefer not to), or call epilolg() unconditionally - which means
performance drop. 
 
> 
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> <...>
> 
> > @@ -3229,7 +3289,7 @@ int
> >  ice_rx_burst_mode_get(struct rte_eth_dev *dev, __rte_unused uint16_t queue_id,
> >  		      struct rte_eth_burst_mode *mode)
> >  {
> > -	eth_rx_burst_t pkt_burst = dev->rx_pkt_burst;
> > +	rte_eth_rx_burst_t pkt_burst = rte_eth_get_rx_burst(dev->data->port_id);
> 
> Does it makes easier to orginanise the patchset to have a separate patch to
> switch first to 'rte_eth_get_rx_burst()' / 'rte_eth_set_rx_burst()' with old
> implementation ('dev->rx_pkt_burst' get/set), and later just change the
> 'rte_eth_get_rx_burst()' / 'rte_eth_set_rx_burst()' implementation when
> structure is updated.

This is doable, don't know would be there any benefit from that or not. 

> 
> <...>
> 
> > --- a/drivers/net/ice/ice_rxtx_vec_sse.c
> > +++ b/drivers/net/ice/ice_rxtx_vec_sse.c
> > @@ -587,13 +587,15 @@ _ice_recv_raw_pkts_vec(struct ice_rx_queue *rxq, struct rte_mbuf **rx_pkts,
> >   * - nb_pkts > ICE_VPMD_RX_BURST, only scan ICE_VPMD_RX_BURST
> >   *   numbers of DD bits
> >   */
> > -uint16_t
> > +static inline uint16_t
> 
> These functions eventually will be called via a function pointer, so is there a
> benefit to request them to 'inline', why not just 'static' ?

Agree.
 
> <...>
> 
> > +_RTE_ETH_RX_DEF(ice_recv_scattered_pkts_vec)
> > +
> 
> This will duplicate most of the Rx burst function for each PMD Rx ops.
> 
> <...>
> 
> > +
> > +#define _RTE_ETH_FUNC(fn)	_rte_eth_##fn
> > +
> 
> Do we need this macro? The functions are still 'static', so they won't be
> visible to application and there won't be a namespace problem.

Not all RX/TX burst functions are defined as 'static'.
 
> Dropping and just use the original fucntion name may reduce the changes in the
> drivers.

It allows to keep existing RX/TX functions intact - no need to change prototype, add prolog/epilog, etc. manually.
Instead these macros help to create a wrapper functions around existing ones, that will
become new public entry points. 
All that should help to make changes faster and in a safer manner.
Though these macros are just helper ones to simplify the transition.
if someone will prefer to make changes in all their RX/TX function by hand - that is still possible. 
 
> <...>
> 
> > +__rte_experimental
> > +rte_eth_rx_burst_t rte_eth_get_rx_burst(uint16_t port_id);
> > +
> > +__rte_experimental
> > +int rte_eth_set_rx_burst(uint16_t port_id, rte_eth_rx_burst_t rxf);
> 
> can s/__rte_experimental/__rte_internal/

OK.
 
> <...>
> 
> > +
> > +__rte_experimental
> > +rte_eth_rx_burst_t
> > +rte_eth_get_rx_burst(uint16_t port_id)
> > +{
> > +	if (port_id >= RTE_DIM(rte_eth_burst_api)) {
> > +		rte_errno = EINVAL;
> > +		return NULL;
> > +	}
> > +	return rte_eth_burst_api[port_id].rx_pkt_burst;
> > +}
> > +
> > +__rte_experimental
> > +int
> > +rte_eth_set_rx_burst(uint16_t port_id, rte_eth_rx_burst_t rxf)
> > +{
> > +	if (port_id >= RTE_DIM(rte_eth_burst_api))
> > +		return -EINVAL;
> > +
> > +	rte_eth_burst_api[port_id].rx_pkt_burst = rxf;
> > +	return 0;
> > +}
> 
> Since these are internal functions for drivers, it can be easier for drivers to
> use directly with 'struct rte_eth_dev *eth_dev', instead of 'port_id'.
> 
> So instead of APIs getting 'port_id' as parameter, they can get 'struct
> rte_eth_dev *eth_dev'? Drivers for sure will have 'eth_dev' references for their
> device.

I am fine either way - it is a control path internal function.
 
> Overall, I think make sense for all public APIs to have handler ('port_id') as
> parameter, and all driver APIs to have 'eth_device' as paramter.
> 
> <...>
> 
> > @@ -4981,44 +4981,11 @@ static inline uint16_t
> >  rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> >  		 struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
> >  {
> > -	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > -	uint16_t nb_rx;
> > -
> > -#ifdef RTE_ETHDEV_DEBUG_RX
> > -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > -	RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
> > -
> > -	if (queue_id >= dev->data->nb_rx_queues) {
> > -		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", queue_id);
> > +	if (port_id >= RTE_MAX_ETHPORTS)
> 
> As an API, it makes sense to validate the input. But not sure to add these
> checks as part of this set, as previous versions don't have it. Perhaps we can
> add them with separate patch and discussion.

You mean 'if (port_id >= RTE_MAX_ETHPORTS)'?
No, this check is not present in current version.
Obviously, it can be removed, though I think it would be good to have it:
will help to keep thigs less error-prone.
I don’t think it would really impact the performance, in some cases
compiler will even be able to optimise out such check.  

> <...>
> 
> > +++ b/lib/ethdev/version.map
> > @@ -249,6 +249,11 @@ EXPERIMENTAL {
> >  	rte_mtr_meter_policy_delete;
> >  	rte_mtr_meter_policy_update;
> >  	rte_mtr_meter_policy_validate;
> > +
> > +	# added in 21.11
> > +	rte_eth_burst_api;
> > +	rte_eth_get_rx_burst;
> > +	rte_eth_set_rx_burst;
> 
> I think these APIs intented to use only by drivers, so instead of experimental,
> they can be added as 'INTERNAL'.

OK.