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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/20/2021 5:28 PM, Konstantin Ananyev wrote:
> ethdev:
>  - make changes so drivers can start using new API for rx_pkt_burst().
>  - provide helper functions/macros.
>  - remove rx_pkt_burst() from 'struct rte_eth_dev'.
> drivers/net:
>  - adjust to new rx_burst API.
> 

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.

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?


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

<...>

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

<...>

> +_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.

Dropping and just use the original fucntion name may reduce the changes in the
drivers.

<...>

> +__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/

<...>

> +
> +__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.

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.

<...>

> +++ 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'.