Re: [PATCH v9 1/4] ethdev: introduce protocol header API

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 6/13/22 13:25, wenxuanx.wu@intel.com wrote:
> From: Wenxuan Wu <wenxuanx.wu@intel.com>
> 
> This patch added new ethdev API to retrieve supported protocol header mask

This patch added -> Add

> of a PMD, which helps to configure protocol header based buffer split.

I'd like to see motivation why single mask is considered sufficient.
I.e. why don't we follow ptypes approach which is move flexible, but
a bit more complicated.

Looking at RTE_PTYPE_* defines carefully it looks like below
API simply cannot provide information that we can split after
TCP or UDP.

> 
> Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>

[snip]

>   /**
>    * @internal
>    * Dump private info from device to a file.
> @@ -1281,6 +1296,9 @@ struct eth_dev_ops {
>   	/** Set IP reassembly configuration */
>   	eth_ip_reassembly_conf_set_t ip_reassembly_conf_set;
>   
> +	/** Get supported ptypes to split */
> +	eth_buffer_split_hdr_ptype_get_t hdrs_supported_ptypes_get;
> +

It is better to be consistent with naming. I.e. just cut prefix "eth_"
and suffix "_t".

Also the type name sounds like it get current split configuration,
not supported one.

>   	/** Dump private info from device */
>   	eth_dev_priv_dump_t eth_dev_priv_dump;
>   };
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 29a3d80466..e1f2a0ffe3 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1636,9 +1636,10 @@ rte_eth_dev_is_removed(uint16_t port_id)
>   }
>   
>   static int
> -rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg,
> -			     uint16_t n_seg, uint32_t *mbp_buf_size,
> -			     const struct rte_eth_dev_info *dev_info)
> +rte_eth_rx_queue_check_split(uint16_t port_id,
> +				const struct rte_eth_rxseg_split *rx_seg,
> +				int16_t n_seg, uint32_t *mbp_buf_size,
> +			    const struct rte_eth_dev_info *dev_info)
>   {
>   	const struct rte_eth_rxseg_capa *seg_capa = &dev_info->rx_seg_capa;
>   	struct rte_mempool *mp_first;
> @@ -1694,13 +1695,7 @@ rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg,
>   		}
>   		offset += seg_idx != 0 ? 0 : RTE_PKTMBUF_HEADROOM;
>   		*mbp_buf_size = rte_pktmbuf_data_room_size(mpl);
> -		length = length != 0 ? length : *mbp_buf_size;
> -		if (*mbp_buf_size < length + offset) {

I don't understand why the check goes away completely.

> -			RTE_ETHDEV_LOG(ERR,
> -				       "%s mbuf_data_room_size %u < %u (segment length=%u + segment offset=%u)\n",
> -				       mpl->name, *mbp_buf_size,
> -				       length + offset, length, offset);
> -			return -EINVAL;
> +

Unnecessary empty line

>   		}

Shouldn't the curly bracket go away as well together with its 'if'

>   	}
>   	return 0;
> @@ -1779,7 +1774,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>   		n_seg = rx_conf->rx_nseg;
>   
>   		if (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) {
> -			ret = rte_eth_rx_queue_check_split(rx_seg, n_seg,
> +			ret = rte_eth_rx_queue_check_split(port_id, rx_seg, n_seg,
>   							   &mbp_buf_size,
>   							   &dev_info);
>   			if (ret != 0)
> @@ -5844,6 +5839,20 @@ rte_eth_ip_reassembly_conf_set(uint16_t port_id,
>   		       (*dev->dev_ops->ip_reassembly_conf_set)(dev, conf));
>   }
>   
> +int
> +rte_eth_supported_hdrs_get(uint16_t port_id, uint32_t *ptypes)
> +{
> +	struct rte_eth_dev *dev;
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];

ptypes must be checked vs NULL

> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->hdrs_supported_ptypes_get,
> +				-ENOTSUP);
> +
> +	return eth_err(port_id,
> +		       (*dev->dev_ops->hdrs_supported_ptypes_get)(dev, ptypes));
> +}
> +
>   int
>   rte_eth_dev_priv_dump(uint16_t port_id, FILE *file)
>   {
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 04cff8ee10..72cac1518e 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -6152,6 +6152,28 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t queue_id,
>   	return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
>   }
>   
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get supported header protocols to split supported by PMD.

"supported" twice above.
Get supported header protocols to split on Rx.

> + * The API will return error if the device is not valid.

Above sentence is obvious and does not add any value. Please, remove.

> + *
> + * @param port_id
> + *   The port identifier of the device.
> + * @param ptype

Why do you use out annotation for the callback description and does not
use it here?

> + *   Supported protocol headers of driver.
> + * @return
> + *   - (-ENOTSUP) if header protocol is not supported by device.
> + *   - (-ENODEV) if *port_id* invalid.

EINVAL in the case of invalid ptypes argument

> + *   - (-EIO) if device is removed.
> + *   - (0) on success.
> + */
> +__rte_experimental
> +int rte_eth_supported_hdrs_get(uint16_t port_id,
> +		uint32_t *ptype);
> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 20391ab29e..7705c0364a 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -279,6 +279,9 @@ EXPERIMENTAL {
>   	rte_flow_async_action_handle_create;
>   	rte_flow_async_action_handle_destroy;
>   	rte_flow_async_action_handle_update;
> +
> +	# added in 22.07

It hopefully will be in 22.11

> +	rte_eth_supported_hdrs_get;
>   };
>   
>   INTERNAL {