Re: [PATCH v2 01/12] ethdev: add API to query packet type filling info

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Jianfeng, a few comments below.

On Fri, Jan 15, 2016 at 01:45:48PM +0800, Jianfeng Tan wrote:
> Add a new API rte_eth_dev_get_ptype_info to query wether/what packet type will
> be filled by given pmd rx burst function.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 20 ++++++++++++++++++++
>  lib/librte_ether/rte_ethdev.h | 27 +++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h    |  6 ++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index ed971b4..cd34f46 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1614,6 +1614,26 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
>  	dev_info->driver_name = dev->data->drv_name;
>  }
>  
> +int
> +rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptype_mask,
> +			   uint32_t ptypes[], int num)
> +{
> +	int ret, i, j;
> +	struct rte_eth_dev *dev;
> +	uint32_t all_ptypes[RTE_PTYPE_MAX_NUM];
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_ptype_info_get, -ENOTSUP);
> +	ret = (*dev->dev_ops->dev_ptype_info_get)(dev, all_ptypes);
> +
> +	for (i = 0, j = 0; i < ret && j < num; ++i)
> +		if (all_ptypes[i] & ptype_mask)
> +			ptypes[j++] = all_ptypes[i];
> +
> +	return ret;
> +}

It's a good thing that the size of ptypes[] can be provided, but I think num
should be passed to the dev_ptype_info_get() callback as well.

If you really do not want to pass the size, you have to force the array type
size onto callbacks using a pointer to the array otherwise they look unsafe
(and are actually unsafe when not called from the rte_eth_dev wrapper),
something like this:

 int (*dev_ptype_info_get)(uint8_t port_id, uint32_t (*ptypes)[RTE_PTYPE_MAX_NUM]);

In which case you might as well drop the num argument from
rte_eth_dev_get_ptype_info() to use the same syntax. That way there is no
need to allocate a ptypes array on the stack twice.

But since people usually do not like this syntax, I think passing num and
letting callbacks check for overflow themselves on the user-provided ptypes
array directly is better. They have to return the total number of packet
types supported even when num is 0 (ptypes may be NULL in that case).

I understand the result needs to be temporarily stored somewhere for
filtering and for that purpose the entire size must be known in advance,
hence my previous suggestion for rte_eth_dev_get_ptype_info() to return
the total number of ptypes and providing a separate function for filtering
a ptypes array for applications that need it:

 /* Return remaining number entries in ptypes[] after filtering it
  * according to ptype_mask. */
 int rte_eth_dev_ptypes_filter(uint32_t ptype_mask, uint32_t ptypes[], int num);

Usage would be like:

 int ptypes_num = rte_eth_dev_get_ptype_info(42, NULL, 0);

 if (ptypes_num <= 0)
     goto err;

 uint32_t ptypes[ptypes_num];

 rte_eth_dev_get_ptype_info(42, ptypes, ptypes_num);
 ptypes_num = rte_eth_dev_ptypes_filter(RTE_PTYPE_INNER_L4_MASK, ptypes, ptypes_num);

 if (ptypes_num <= 0)
    goto err;

 /* process ptypes... */

What about this?

> +
>  void
>  rte_eth_macaddr_get(uint8_t port_id, struct ether_addr *mac_addr)
>  {
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index bada8ad..42f8188 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1021,6 +1021,10 @@ typedef void (*eth_dev_infos_get_t)(struct rte_eth_dev *dev,
>  				    struct rte_eth_dev_info *dev_info);
>  /**< @internal Get specific informations of an Ethernet device. */
>  
> +typedef int (*eth_dev_ptype_info_get_t)(struct rte_eth_dev *dev,
> +					uint32_t ptypes[]);
> +/**< @internal Get ptype info of eth_rx_burst_t. */
> +
>  typedef int (*eth_queue_start_t)(struct rte_eth_dev *dev,
>  				    uint16_t queue_id);
>  /**< @internal Start rx and tx of a queue of an Ethernet device. */
> @@ -1347,6 +1351,7 @@ struct eth_dev_ops {
>  	eth_queue_stats_mapping_set_t queue_stats_mapping_set;
>  	/**< Configure per queue stat counter mapping. */
>  	eth_dev_infos_get_t        dev_infos_get; /**< Get device info. */
> +	eth_dev_ptype_info_get_t   dev_ptype_info_get; /** Get ptype info */
>  	mtu_set_t                  mtu_set; /**< Set MTU. */
>  	vlan_filter_set_t          vlan_filter_set;  /**< Filter VLAN Setup. */
>  	vlan_tpid_set_t            vlan_tpid_set;      /**< Outer VLAN TPID Setup. */
> @@ -2273,6 +2278,28 @@ extern void rte_eth_dev_info_get(uint8_t port_id,
>  				 struct rte_eth_dev_info *dev_info);
>  
>  /**
> + * Retrieve the contextual information of an Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param ptype_mask
> + *   A hint of what kind of packet type which the caller is interested in.
> + * @param ptypes
> + *   An array of packet types to be filled with.
> + * @param num
> + *   The size of ptypes array.
> + * @return
> + *   - (>0) Number of ptypes supported. May be greater than param num and
> + *	    caller needs to check that.
> + *   - (0 or -ENOTSUP) if PMD does not fill the specified ptype.
> + *   - (-ENODEV) if *port_id* invalid.
> + */
> +extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
> +				      uint32_t ptype_mask,
> +				      uint32_t ptypes[],
> +				      int num);
> +
> +/**
>   * Retrieve the MTU of an Ethernet device.
>   *
>   * @param port_id
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index f234ac9..d116711 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -667,6 +667,12 @@ extern "C" {
>  #define RTE_PTYPE_INNER_L4_MASK             0x0f000000
>  
>  /**
> +  * Total number of all kinds of RTE_PTYPE_*, except from *_MASK, is 37 for now
> +  * and reserve some space for new ptypes
> +  */
> +#define RTE_PTYPE_MAX_NUM		    64

This macro should not be needed if the num argument is kept. Applications
should only rely on returned values.

> +
> +/**
>   * Check if the (outer) L3 header is IPv4. To avoid comparing IPv4 types one by
>   * one, bit 4 is selected to be used for IPv4 only. Then checking bit 4 can
>   * determine if it is an IPV4 packet.
> -- 
> 2.1.4
> 

-- 
Adrien Mazarguil
6WIND