[dpdk-dev] [PATCH v9 1/7] ethdev: add set ptype function

[Andrew Rybchenko]

On 10/7/19 9:51 AM, pbhagavatula at marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula at marvell.com>
>
> Add `rte_eth_dev_set_supported_ptypes` function that will allow the
> application to inform the PMD the packet types it is interested in.
> Based on the ptypes set PMDs can optimize their Rx path.
>
> -If application doesn’t want any ptype information it can call
> `rte_eth_dev_set_supported_ptypes(ethdev_id, RTE_PTYPE_UNKNOWN, NULL, 0)`
> and PMD may skip packet type processing and set rte_mbuf::packet_type to
> RTE_PTYPE_UNKNOWN.
>
> -If application doesn’t call `rte_eth_dev_set_supported_ptypes` PMD can
> return `rte_mbuf::packet_type` with `rte_eth_dev_get_supported_ptypes`.
>
> -If application is interested only in L2/L3 layer, it can inform the PMD
> to update `rte_mbuf::packet_type` with L2/L3 ptype by calling
> `rte_eth_dev_set_supported_ptypes(ethdev_id,
> 		RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK, NULL, 0)`.
>
> Suggested-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula at marvell.com>
> Reviewed-by: Andrew Rybchenko <arybchenko at solarflare.com>

Looks good except one thing below.

[snip]

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 17d183e1f..d2561272c 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -2602,6 +2602,79 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>   	return j;
>   }
>   
> +int
> +rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
> +				 uint32_t *set_ptypes, unsigned int num)
> +{
> +	const uint32_t *all_ptypes;
> +	uint32_t test_ptype_mask;
> +	struct rte_eth_dev *dev;
> +	unsigned int i, j;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (num > 0 && set_ptypes == NULL)
> +		return -EINVAL;
> +
> +	if (*dev->dev_ops->dev_supported_ptypes_get == NULL ||
> +			*dev->dev_ops->dev_supported_ptypes_set == NULL) {
> +		ret = 0;
> +		goto ptype_unknown;
> +	}
> +
> +	if (ptype_mask == 0) {
> +		ret = (*dev->dev_ops->dev_supported_ptypes_set)(dev,
> +				ptype_mask);
> +		goto ptype_unknown;
> +	}
> +
> +	test_ptype_mask = ptype_mask;
> +	while (test_ptype_mask) {
> +		uint8_t mask = test_ptype_mask & RTE_PTYPE_L2_MASK;
> +
> +		if (mask && (mask != RTE_PTYPE_L2_MASK)) {
> +			ret = -EINVAL;
> +			goto ptype_unknown;
> +		}
> +		test_ptype_mask >>= __builtin_popcount(RTE_PTYPE_L2_MASK);

It heavily relies on current MASK values - 4 bits in each mask one by one.
IMO const array of all supported masks is less evil here. Plus it should
be checked that no set bits remain when all masks are removed.

> +	}
> +
> +	all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev);
> +	if (all_ptypes == NULL) {
> +		ret = 0;
> +		goto ptype_unknown;
> +	}
> +
> +	/*
> +	 * Accodommodate as many set_ptypes as possible. If the supplied
> +	 * set_ptypes array is insufficient fill it partially.
> +	 */
> +	for (i = 0, j = 0; set_ptypes != NULL &&
> +				(all_ptypes[i] != RTE_PTYPE_UNKNOWN); ++i) {
> +		if (ptype_mask & all_ptypes[i]) {
> +			if (j < num - 1) {
> +				set_ptypes[j] = all_ptypes[i];
> +				j++;
> +				continue;
> +			}
> +			break;
> +		}
> +	}
> +
> +	if (set_ptypes != NULL && j < num)
> +		set_ptypes[j] = RTE_PTYPE_UNKNOWN;
> +
> +	return (*dev->dev_ops->dev_supported_ptypes_set)(dev, ptype_mask);
> +
> +ptype_unknown:
> +	if (num > 0)
> +		set_ptypes[0] = RTE_PTYPE_UNKNOWN;
> +
> +	return ret;
> +}
> +
>   void
>   rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr)
>   {

[snip]