[dpdk-dev,01/12] ethdev: add API to query what/if packet type is set

  Add a new API rte_eth_dev_get_ptype_info to query what/if packet type will
be set by current rx burst function.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
 lib/librte_ether/rte_ethdev.h | 22 ++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h    | 13 +++++++++++++
 3 files changed, 47 insertions(+)
  

I'm not sure about the usefulness of this new callback, but one issue I see
with rte_eth_dev_get_ptype_info() is that determining the proper size for
ptypes[] according to a mask is awkward. For instance suppose
RTE_PTYPE_L4_MASK is redefined to a different size at some point, the caller
must dynamically adjust its ptypes[] array size to avoid a possible
overflow, just in case.

I suggest one of these solutions:

- A callback to query for a single type at once instead (easiest method in
  my opinion).

- An additional argument with the number of entries in ptypes[], in which
  case rte_eth_dev_get_ptype_info() should return the number of entries that
  would have been filled regardless, a bit like snprintf().

On Thu, Dec 31, 2015 at 02:53:08PM +0800, Jianfeng Tan wrote:
> Add a new API rte_eth_dev_get_ptype_info to query what/if packet type will
> be set by current rx burst function.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
>  lib/librte_ether/rte_ethdev.h | 22 ++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h    | 13 +++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index ed971b4..1885374 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1614,6 +1614,18 @@ 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[])
> +{
> +	struct rte_eth_dev *dev;
> +
> +	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);
> +	return (*dev->dev_ops->dev_ptype_info_get)(dev, ptype_mask, ptypes);
> +}
> +
>  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..e97b632 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 ptype_mask, 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,23 @@ 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
> + * @return
> + *   - (>=0) if successful. Indicate number of valid values in ptypes array.
> + *   - (-ENOTSUP) if hardware-assisted VLAN stripping not configured.
> + *   - (-ENODEV) if *port_id* invalid.
> + */
> +extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
> +				 uint32_t ptype_mask, uint32_t ptypes[]);
> +
> +/**
>   * 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..21d4aa2 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -282,6 +282,8 @@ extern "C" {
>   * It is used for outer packet for tunneling cases.
>   */
>  #define RTE_PTYPE_L2_MASK                   0x0000000f
> +
> +#define RTE_PTYPE_L2_MAX_NUM				4
>  /**
>   * IP (Internet Protocol) version 4 packet type.
>   * It is used for outer packet for tunneling cases, and does not contain any
> @@ -349,6 +351,8 @@ extern "C" {
>   * It is used for outer packet for tunneling cases.
>   */
>  #define RTE_PTYPE_L3_MASK                   0x000000f0
> +
> +#define RTE_PTYPE_L3_MAX_NUM				6
>  /**
>   * TCP (Transmission Control Protocol) packet type.
>   * It is used for outer packet for tunneling cases.
> @@ -435,6 +439,8 @@ extern "C" {
>   * It is used for outer packet for tunneling cases.
>   */
>  #define RTE_PTYPE_L4_MASK                   0x00000f00
> +
> +#define RTE_PTYPE_L4_MAX_NUM				6
>  /**
>   * IP (Internet Protocol) in IP (Internet Protocol) tunneling packet type.
>   *
> @@ -508,6 +514,8 @@ extern "C" {
>   * Mask of tunneling packet types.
>   */
>  #define RTE_PTYPE_TUNNEL_MASK               0x0000f000
> +
> +#define RTE_PTYPE_TUNNEL_MAX_NUM			6
>  /**
>   * Ethernet packet type.
>   * It is used for inner packet type only.
> @@ -527,6 +535,8 @@ extern "C" {
>   * Mask of inner layer 2 packet types.
>   */
>  #define RTE_PTYPE_INNER_L2_MASK             0x000f0000
> +
> +#define RTE_PTYPE_INNER_L2_MAX_NUM			2
>  /**
>   * IP (Internet Protocol) version 4 packet type.
>   * It is used for inner packet only, and does not contain any header option.
> @@ -588,6 +598,8 @@ extern "C" {
>   * Mask of inner layer 3 packet types.
>   */
>  #define RTE_PTYPE_INNER_L3_MASK             0x00f00000
> +
> +#define RTE_PTYPE_INNER_L3_MAX_NUM			6
>  /**
>   * TCP (Transmission Control Protocol) packet type.
>   * It is used for inner packet only.
> @@ -666,6 +678,7 @@ extern "C" {
>   */
>  #define RTE_PTYPE_INNER_L4_MASK             0x0f000000
>  
> +#define RTE_PTYPE_INNER_L4_MAX_NUM			6
>  /**
>   * 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
> -- 
> 2.1.4
>
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Monday, January 04, 2016 11:38 AM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> 
> I'm not sure about the usefulness of this new callback, but one issue I see
> with rte_eth_dev_get_ptype_info() is that determining the proper size for
> ptypes[] according to a mask is awkward. For instance suppose
> RTE_PTYPE_L4_MASK is redefined to a different size at some point, the caller
> must dynamically adjust its ptypes[] array size to avoid a possible
> overflow, just in case.
> 
> I suggest one of these solutions:
> 
> - A callback to query for a single type at once instead (easiest method in
>   my opinion).
> 
> - An additional argument with the number of entries in ptypes[], in which
>   case rte_eth_dev_get_ptype_info() should return the number of entries that
>   would have been filled regardless, a bit like snprintf().

+1 for the second option.
Also not sure you really need: RTE_PTYPE_*_MAX_NUM macros.
Konstantin

> 
> On Thu, Dec 31, 2015 at 02:53:08PM +0800, Jianfeng Tan wrote:
> > Add a new API rte_eth_dev_get_ptype_info to query what/if packet type will
> > be set by current rx burst function.
> >
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > ---
> >  lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
> >  lib/librte_ether/rte_ethdev.h | 22 ++++++++++++++++++++++
> >  lib/librte_mbuf/rte_mbuf.h    | 13 +++++++++++++
> >  3 files changed, 47 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > index ed971b4..1885374 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1614,6 +1614,18 @@ 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[])
> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	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);
> > +	return (*dev->dev_ops->dev_ptype_info_get)(dev, ptype_mask, ptypes);
> > +}
> > +
> >  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..e97b632 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 ptype_mask, 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,23 @@ 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
> > + * @return
> > + *   - (>=0) if successful. Indicate number of valid values in ptypes array.
> > + *   - (-ENOTSUP) if hardware-assisted VLAN stripping not configured.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + */
> > +extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
> > +				 uint32_t ptype_mask, uint32_t ptypes[]);
> > +
> > +/**
> >   * 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..21d4aa2 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -282,6 +282,8 @@ extern "C" {
> >   * It is used for outer packet for tunneling cases.
> >   */
> >  #define RTE_PTYPE_L2_MASK                   0x0000000f
> > +
> > +#define RTE_PTYPE_L2_MAX_NUM				4
> >  /**
> >   * IP (Internet Protocol) version 4 packet type.
> >   * It is used for outer packet for tunneling cases, and does not contain any
> > @@ -349,6 +351,8 @@ extern "C" {
> >   * It is used for outer packet for tunneling cases.
> >   */
> >  #define RTE_PTYPE_L3_MASK                   0x000000f0
> > +
> > +#define RTE_PTYPE_L3_MAX_NUM				6
> >  /**
> >   * TCP (Transmission Control Protocol) packet type.
> >   * It is used for outer packet for tunneling cases.
> > @@ -435,6 +439,8 @@ extern "C" {
> >   * It is used for outer packet for tunneling cases.
> >   */
> >  #define RTE_PTYPE_L4_MASK                   0x00000f00
> > +
> > +#define RTE_PTYPE_L4_MAX_NUM				6
> >  /**
> >   * IP (Internet Protocol) in IP (Internet Protocol) tunneling packet type.
> >   *
> > @@ -508,6 +514,8 @@ extern "C" {
> >   * Mask of tunneling packet types.
> >   */
> >  #define RTE_PTYPE_TUNNEL_MASK               0x0000f000
> > +
> > +#define RTE_PTYPE_TUNNEL_MAX_NUM			6
> >  /**
> >   * Ethernet packet type.
> >   * It is used for inner packet type only.
> > @@ -527,6 +535,8 @@ extern "C" {
> >   * Mask of inner layer 2 packet types.
> >   */
> >  #define RTE_PTYPE_INNER_L2_MASK             0x000f0000
> > +
> > +#define RTE_PTYPE_INNER_L2_MAX_NUM			2
> >  /**
> >   * IP (Internet Protocol) version 4 packet type.
> >   * It is used for inner packet only, and does not contain any header option.
> > @@ -588,6 +598,8 @@ extern "C" {
> >   * Mask of inner layer 3 packet types.
> >   */
> >  #define RTE_PTYPE_INNER_L3_MASK             0x00f00000
> > +
> > +#define RTE_PTYPE_INNER_L3_MAX_NUM			6
> >  /**
> >   * TCP (Transmission Control Protocol) packet type.
> >   * It is used for inner packet only.
> > @@ -666,6 +678,7 @@ extern "C" {
> >   */
> >  #define RTE_PTYPE_INNER_L4_MASK             0x0f000000
> >
> > +#define RTE_PTYPE_INNER_L4_MAX_NUM			6
> >  /**
> >   * 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
> > --
> > 2.1.4
> >
> 
> --
> Adrien Mazarguil
> 6WIND
  

On Mon, Jan 04, 2016 at 02:36:14PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> > Sent: Monday, January 04, 2016 11:38 AM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > 
> > I'm not sure about the usefulness of this new callback, but one issue I see
> > with rte_eth_dev_get_ptype_info() is that determining the proper size for
> > ptypes[] according to a mask is awkward. For instance suppose
> > RTE_PTYPE_L4_MASK is redefined to a different size at some point, the caller
> > must dynamically adjust its ptypes[] array size to avoid a possible
> > overflow, just in case.
> > 
> > I suggest one of these solutions:
> > 
> > - A callback to query for a single type at once instead (easiest method in
> >   my opinion).
> > 
> > - An additional argument with the number of entries in ptypes[], in which
> >   case rte_eth_dev_get_ptype_info() should return the number of entries that
> >   would have been filled regardless, a bit like snprintf().
> 
> +1 for the second option.
> Also not sure you really need: RTE_PTYPE_*_MAX_NUM macros.
> Konstantin

+1 for the second option.  But see below.

> > 
> > On Thu, Dec 31, 2015 at 02:53:08PM +0800, Jianfeng Tan wrote:
> > > Add a new API rte_eth_dev_get_ptype_info to query what/if packet type will
> > > be set by current rx burst function.
> > >
> > > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
> > >  lib/librte_ether/rte_ethdev.h | 22 ++++++++++++++++++++++
> > >  lib/librte_mbuf/rte_mbuf.h    | 13 +++++++++++++
> > >  3 files changed, 47 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > > index ed971b4..1885374 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -1614,6 +1614,18 @@ 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[])
> > > +{
> > > +	struct rte_eth_dev *dev;
> > > +
> > > +	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);
> > > +	return (*dev->dev_ops->dev_ptype_info_get)(dev, ptype_mask, ptypes);
> > > +}
> > > +
> > >  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..e97b632 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 ptype_mask, 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,23 @@ 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
> > > + * @return
> > > + *   - (>=0) if successful. Indicate number of valid values in ptypes array.
> > > + *   - (-ENOTSUP) if hardware-assisted VLAN stripping not configured.
> > > + *   - (-ENODEV) if *port_id* invalid.
> > > + */
> > > +extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
> > > +				 uint32_t ptype_mask, uint32_t ptypes[]);
> > > +
> > > +/**
> > >   * 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..21d4aa2 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -282,6 +282,8 @@ extern "C" {
> > >   * It is used for outer packet for tunneling cases.
> > >   */
> > >  #define RTE_PTYPE_L2_MASK                   0x0000000f
> > > +
> > > +#define RTE_PTYPE_L2_MAX_NUM				4
> > >  /**
> > >   * IP (Internet Protocol) version 4 packet type.
> > >   * It is used for outer packet for tunneling cases, and does not contain any
> > > @@ -349,6 +351,8 @@ extern "C" {
> > >   * It is used for outer packet for tunneling cases.
> > >   */
> > >  #define RTE_PTYPE_L3_MASK                   0x000000f0
> > > +
> > > +#define RTE_PTYPE_L3_MAX_NUM				6
> > >  /**
> > >   * TCP (Transmission Control Protocol) packet type.
> > >   * It is used for outer packet for tunneling cases.
> > > @@ -435,6 +439,8 @@ extern "C" {
> > >   * It is used for outer packet for tunneling cases.
> > >   */
> > >  #define RTE_PTYPE_L4_MASK                   0x00000f00
> > > +
> > > +#define RTE_PTYPE_L4_MAX_NUM				6
> > >  /**
> > >   * IP (Internet Protocol) in IP (Internet Protocol) tunneling packet type.
> > >   *
> > > @@ -508,6 +514,8 @@ extern "C" {
> > >   * Mask of tunneling packet types.
> > >   */
> > >  #define RTE_PTYPE_TUNNEL_MASK               0x0000f000
> > > +
> > > +#define RTE_PTYPE_TUNNEL_MAX_NUM			6
> > >  /**
> > >   * Ethernet packet type.
> > >   * It is used for inner packet type only.
> > > @@ -527,6 +535,8 @@ extern "C" {
> > >   * Mask of inner layer 2 packet types.
> > >   */
> > >  #define RTE_PTYPE_INNER_L2_MASK             0x000f0000
> > > +
> > > +#define RTE_PTYPE_INNER_L2_MAX_NUM			2
> > >  /**
> > >   * IP (Internet Protocol) version 4 packet type.
> > >   * It is used for inner packet only, and does not contain any header option.
> > > @@ -588,6 +598,8 @@ extern "C" {
> > >   * Mask of inner layer 3 packet types.
> > >   */
> > >  #define RTE_PTYPE_INNER_L3_MASK             0x00f00000
> > > +
> > > +#define RTE_PTYPE_INNER_L3_MAX_NUM			6
> > >  /**
> > >   * TCP (Transmission Control Protocol) packet type.
> > >   * It is used for inner packet only.
> > > @@ -666,6 +678,7 @@ extern "C" {
> > >   */
> > >  #define RTE_PTYPE_INNER_L4_MASK             0x0f000000
> > >
> > > +#define RTE_PTYPE_INNER_L4_MAX_NUM			6
> > >  /**
> > >   * 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

I think we miss a comment here in how those 2/6/4 values are chosen
because, according to the mask, I expect 16 possibilities but I get
less.  It will help a lot anyone who needs to add a new type.

Extending the snprintf behavior above, it is best to remove the mask
argument altogether and have rte_eth_dev_get_ptype_info() return the
entire list every time.  Applications need to iterate on the result in
any case.

  rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[],
                             size_t max_entries)



Another point, I have read the example patch (l3fwd) but I don't
understand why the PMD is not responsible for filling the packet type in
the MBUF (packet parsing is done by parse_packet_type()).  Why the extra
computation?

I see it more like an offload request (as checksum, etc...) and if the
NIC does not support it then the application does the necessary overload.

Best regards,
  

Hi Neilo,

> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Tuesday, January 05, 2016 4:14 PM
> To: Tan, Jianfeng
> Cc: Adrien Mazarguil; Ananyev, Konstantin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> 
> On Mon, Jan 04, 2016 at 02:36:14PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> > > Sent: Monday, January 04, 2016 11:38 AM
> > > To: Tan, Jianfeng
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > >
> > > I'm not sure about the usefulness of this new callback, but one issue I see
> > > with rte_eth_dev_get_ptype_info() is that determining the proper size for
> > > ptypes[] according to a mask is awkward. For instance suppose
> > > RTE_PTYPE_L4_MASK is redefined to a different size at some point, the caller
> > > must dynamically adjust its ptypes[] array size to avoid a possible
> > > overflow, just in case.
> > >
> > > I suggest one of these solutions:
> > >
> > > - A callback to query for a single type at once instead (easiest method in
> > >   my opinion).
> > >
> > > - An additional argument with the number of entries in ptypes[], in which
> > >   case rte_eth_dev_get_ptype_info() should return the number of entries that
> > >   would have been filled regardless, a bit like snprintf().
> >
> > +1 for the second option.
> > Also not sure you really need: RTE_PTYPE_*_MAX_NUM macros.
> > Konstantin
> 
> +1 for the second option.  But see below.
> 
> > >
> > > On Thu, Dec 31, 2015 at 02:53:08PM +0800, Jianfeng Tan wrote:
> > > > Add a new API rte_eth_dev_get_ptype_info to query what/if packet type will
> > > > be set by current rx burst function.
> > > >
> > > > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > > > ---
> > > >  lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
> > > >  lib/librte_ether/rte_ethdev.h | 22 ++++++++++++++++++++++
> > > >  lib/librte_mbuf/rte_mbuf.h    | 13 +++++++++++++
> > > >  3 files changed, 47 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > > > index ed971b4..1885374 100644
> > > > --- a/lib/librte_ether/rte_ethdev.c
> > > > +++ b/lib/librte_ether/rte_ethdev.c
> > > > @@ -1614,6 +1614,18 @@ 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[])
> > > > +{
> > > > +	struct rte_eth_dev *dev;
> > > > +
> > > > +	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);
> > > > +	return (*dev->dev_ops->dev_ptype_info_get)(dev, ptype_mask, ptypes);
> > > > +}
> > > > +
> > > >  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..e97b632 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 ptype_mask, 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,23 @@ 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
> > > > + * @return
> > > > + *   - (>=0) if successful. Indicate number of valid values in ptypes array.
> > > > + *   - (-ENOTSUP) if hardware-assisted VLAN stripping not configured.
> > > > + *   - (-ENODEV) if *port_id* invalid.
> > > > + */
> > > > +extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
> > > > +				 uint32_t ptype_mask, uint32_t ptypes[]);
> > > > +
> > > > +/**
> > > >   * 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..21d4aa2 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > > @@ -282,6 +282,8 @@ extern "C" {
> > > >   * It is used for outer packet for tunneling cases.
> > > >   */
> > > >  #define RTE_PTYPE_L2_MASK                   0x0000000f
> > > > +
> > > > +#define RTE_PTYPE_L2_MAX_NUM				4
> > > >  /**
> > > >   * IP (Internet Protocol) version 4 packet type.
> > > >   * It is used for outer packet for tunneling cases, and does not contain any
> > > > @@ -349,6 +351,8 @@ extern "C" {
> > > >   * It is used for outer packet for tunneling cases.
> > > >   */
> > > >  #define RTE_PTYPE_L3_MASK                   0x000000f0
> > > > +
> > > > +#define RTE_PTYPE_L3_MAX_NUM				6
> > > >  /**
> > > >   * TCP (Transmission Control Protocol) packet type.
> > > >   * It is used for outer packet for tunneling cases.
> > > > @@ -435,6 +439,8 @@ extern "C" {
> > > >   * It is used for outer packet for tunneling cases.
> > > >   */
> > > >  #define RTE_PTYPE_L4_MASK                   0x00000f00
> > > > +
> > > > +#define RTE_PTYPE_L4_MAX_NUM				6
> > > >  /**
> > > >   * IP (Internet Protocol) in IP (Internet Protocol) tunneling packet type.
> > > >   *
> > > > @@ -508,6 +514,8 @@ extern "C" {
> > > >   * Mask of tunneling packet types.
> > > >   */
> > > >  #define RTE_PTYPE_TUNNEL_MASK               0x0000f000
> > > > +
> > > > +#define RTE_PTYPE_TUNNEL_MAX_NUM			6
> > > >  /**
> > > >   * Ethernet packet type.
> > > >   * It is used for inner packet type only.
> > > > @@ -527,6 +535,8 @@ extern "C" {
> > > >   * Mask of inner layer 2 packet types.
> > > >   */
> > > >  #define RTE_PTYPE_INNER_L2_MASK             0x000f0000
> > > > +
> > > > +#define RTE_PTYPE_INNER_L2_MAX_NUM			2
> > > >  /**
> > > >   * IP (Internet Protocol) version 4 packet type.
> > > >   * It is used for inner packet only, and does not contain any header option.
> > > > @@ -588,6 +598,8 @@ extern "C" {
> > > >   * Mask of inner layer 3 packet types.
> > > >   */
> > > >  #define RTE_PTYPE_INNER_L3_MASK             0x00f00000
> > > > +
> > > > +#define RTE_PTYPE_INNER_L3_MAX_NUM			6
> > > >  /**
> > > >   * TCP (Transmission Control Protocol) packet type.
> > > >   * It is used for inner packet only.
> > > > @@ -666,6 +678,7 @@ extern "C" {
> > > >   */
> > > >  #define RTE_PTYPE_INNER_L4_MASK             0x0f000000
> > > >
> > > > +#define RTE_PTYPE_INNER_L4_MAX_NUM			6
> > > >  /**
> > > >   * 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
> 
> I think we miss a comment here in how those 2/6/4 values are chosen
> because, according to the mask, I expect 16 possibilities but I get
> less.  It will help a lot anyone who needs to add a new type.
> 
> Extending the snprintf behavior above, it is best to remove the mask
> argument altogether and have rte_eth_dev_get_ptype_info() return the
> entire list every time.  Applications need to iterate on the result in
> any case.

I think we'd better keep mask argument.
In many cases upper layer only interested in some particular  subset of
all packet types that HW can recognise.
Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,
tunnelling packet types, etc.
If caller needs to know all recognised ptypes, he can set mask==-1,
In that case all supported packet types will be returned.

> 
>   rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[],
>                              size_t max_entries)
> 
> 
> 
> Another point, I have read the example patch (l3fwd) but I don't
> understand why the PMD is not responsible for filling the packet type in
> the MBUF (packet parsing is done by parse_packet_type()).  Why the extra
> computation?

As I understand there are 3 possibilities now:
1. HW supports ptype recognition and SW ptype parsing is never done
(--parse-ptype is not specified).
2. HW supports ptype recognition, but and SW ptype parsing is done anyway
(--parse-ptype is specified).
3. HW doesn't support and ptype recognition, and and SW ptype parsing is done
(--parse-ptype is specified).

I suppose the question is what for introduce '--parse-ptype' at all?
My thought because of #2, so people can easily check what will be the performance impact of SW parsing. 

Konstantin

> 
> I see it more like an offload request (as checksum, etc...) and if the
> NIC does not support it then the application does the necessary overload.
> 
> Best regards,
> 
> --
> Nélio Laranjeiro
> 6WIND
  

On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote:
[...]
> > -----Original Message-----
> > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
[...]
> > I think we miss a comment here in how those 2/6/4 values are chosen
> > because, according to the mask, I expect 16 possibilities but I get
> > less.  It will help a lot anyone who needs to add a new type.
> > 
> > Extending the snprintf behavior above, it is best to remove the mask
> > argument altogether and have rte_eth_dev_get_ptype_info() return the
> > entire list every time.  Applications need to iterate on the result in
> > any case.
> 
> I think we'd better keep mask argument.
> In many cases upper layer only interested in some particular  subset of
> all packet types that HW can recognise.
> Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,
> tunnelling packet types, etc.
> If caller needs to know all recognised ptypes, he can set mask==-1,
> In that case all supported packet types will be returned.

There are other drawbacks to the mask argument in my opinion. The API will
have to be updated again as soon as 32 bits aren't enough to represent all
possible masks. We can't predict it will be large enough forever but on the
other hand, using uint64_t seems overkill at this point.

I think this use for masks should be avoided when performance does not
matter much, as in this case, user application cannot know the number of
entries in advance and must rely on the returned value to iterate.

A helper function can be added to convert a RTE_PTYPE_* value to the layer
it belongs to (using enum to define possible values).

If we absolutely want a mean to filter returned values, I suggest we use
this enum instead of the mask argument. Since it won't be a mask, it won't
have to be updated every time a new protocol requires extending one.

> >   rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[],
> >                              size_t max_entries)
> > 
> > 
> > 
> > Another point, I have read the example patch (l3fwd) but I don't
> > understand why the PMD is not responsible for filling the packet type in
> > the MBUF (packet parsing is done by parse_packet_type()).  Why the extra
> > computation?
> 
> As I understand there are 3 possibilities now:
> 1. HW supports ptype recognition and SW ptype parsing is never done
> (--parse-ptype is not specified).
> 2. HW supports ptype recognition, but and SW ptype parsing is done anyway
> (--parse-ptype is specified).
> 3. HW doesn't support and ptype recognition, and and SW ptype parsing is done
> (--parse-ptype is specified).
> 
> I suppose the question is what for introduce '--parse-ptype' at all?
> My thought because of #2, so people can easily check what will be the performance impact of SW parsing. 
> 
> Konstantin
> 
> > 
> > I see it more like an offload request (as checksum, etc...) and if the
> > NIC does not support it then the application does the necessary overload.
> > 
> > Best regards,
> > 
> > --
> > Nélio Laranjeiro
> > 6WIND
  

> -----Original Message-----

> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]

> Sent: Wednesday, January 06, 2016 10:01 AM

> To: Ananyev, Konstantin

> Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set

> 

> On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote:

> [...]

> > > -----Original Message-----

> > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]

> [...]

> > > I think we miss a comment here in how those 2/6/4 values are chosen

> > > because, according to the mask, I expect 16 possibilities but I get

> > > less.  It will help a lot anyone who needs to add a new type.

> > >

> > > Extending the snprintf behavior above, it is best to remove the mask

> > > argument altogether and have rte_eth_dev_get_ptype_info() return the

> > > entire list every time.  Applications need to iterate on the result in

> > > any case.

> >

> > I think we'd better keep mask argument.

> > In many cases upper layer only interested in some particular  subset of

> > all packet types that HW can recognise.

> > Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,

> > tunnelling packet types, etc.

> > If caller needs to know all recognised ptypes, he can set mask==-1,

> > In that case all supported packet types will be returned.

> 

> There are other drawbacks to the mask argument in my opinion. The API will

> have to be updated again as soon as 32 bits aren't enough to represent all

> possible masks. We can't predict it will be large enough forever but on the

> other hand, using uint64_t seems overkill at this point.


Inside rte_mbuf packet_type itself is a 32 bit value.
These 32 bits are divided into several fields to mark packet types,
i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc.
As long as packet_type itself is 32bits, 32bit mask is sufficient. 
If we'll ever run out of 32 bits in packet_type itself, it will be ABI change anyway.

> 

> I think this use for masks should be avoided when performance does not

> matter much, as in this case, user application cannot know the number of

> entries in advance and must rely on the returned value to iterate.


User doesn't know numbers of entries in advance anyway (with and without the mask).
That's why this function was introduced at first place.
With mask it just a bit more handy, in case user cares only about particular subset of supported
packet types (only L2 let say). 

> 

> A helper function can be added to convert a RTE_PTYPE_* value to the layer

> it belongs to (using enum to define possible values).


Not sure what for?

> 

> If we absolutely want a mean to filter returned values, I suggest we use

> this enum instead of the mask argument.

> Since it won't be a mask, it won't

> have to be updated every time a new protocol requires extending one.


Number of bits PTYPE_L2/L3/L4,... layers are already defined.
So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype -
there are few reserved values right now.  
if one day we'll run out bits in let say RTE_PTYPE_L2_MASK  and will have to increase its size -
it would mean change of the packet_type layout and possible ABI breakage anyway. 
Konstantin

> 

> > >   rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[],

> > >                              size_t max_entries)

> > >

> > >

> > >

> > > Another point, I have read the example patch (l3fwd) but I don't

> > > understand why the PMD is not responsible for filling the packet type in

> > > the MBUF (packet parsing is done by parse_packet_type()).  Why the extra

> > > computation?

> >

> > As I understand there are 3 possibilities now:

> > 1. HW supports ptype recognition and SW ptype parsing is never done

> > (--parse-ptype is not specified).

> > 2. HW supports ptype recognition, but and SW ptype parsing is done anyway

> > (--parse-ptype is specified).

> > 3. HW doesn't support and ptype recognition, and and SW ptype parsing is done

> > (--parse-ptype is specified).

> >

> > I suppose the question is what for introduce '--parse-ptype' at all?

> > My thought because of #2, so people can easily check what will be the performance impact of SW parsing.

> >

> > Konstantin

> >

> > >

> > > I see it more like an offload request (as checksum, etc...) and if the

> > > NIC does not support it then the application does the necessary overload.

> > >

> > > Best regards,

> > >

> > > --

> > > Nélio Laranjeiro

> > > 6WIND

> 

> --

> Adrien Mazarguil

> 6WIND
  

On Wed, Jan 06, 2016 at 02:29:07PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Wednesday, January 06, 2016 10:01 AM
> > To: Ananyev, Konstantin
> > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > 
> > On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote:
> > [...]
> > > > -----Original Message-----
> > > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > [...]
> > > > I think we miss a comment here in how those 2/6/4 values are chosen
> > > > because, according to the mask, I expect 16 possibilities but I get
> > > > less.  It will help a lot anyone who needs to add a new type.
> > > >
> > > > Extending the snprintf behavior above, it is best to remove the mask
> > > > argument altogether and have rte_eth_dev_get_ptype_info() return the
> > > > entire list every time.  Applications need to iterate on the result in
> > > > any case.
> > >
> > > I think we'd better keep mask argument.
> > > In many cases upper layer only interested in some particular  subset of
> > > all packet types that HW can recognise.
> > > Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,
> > > tunnelling packet types, etc.
> > > If caller needs to know all recognised ptypes, he can set mask==-1,
> > > In that case all supported packet types will be returned.
> > 
> > There are other drawbacks to the mask argument in my opinion. The API will
> > have to be updated again as soon as 32 bits aren't enough to represent all
> > possible masks. We can't predict it will be large enough forever but on the
> > other hand, using uint64_t seems overkill at this point.
> 
> Inside rte_mbuf packet_type itself is a 32 bit value.
> These 32 bits are divided into several fields to mark packet types,
> i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc.
> As long as packet_type itself is 32bits, 32bit mask is sufficient. 
> If we'll ever run out of 32 bits in packet_type itself, it will be ABI change anyway.

Sure, however why not do it now this issue has been raised so this function
doesn't need updating the day it breaks? I know there's a million other
places with a similar problem but I'm all for making new code future proof.

Perhaps in this particular case there is no way to hit the limit (although
there are only four unused bits left to extend RTE_PTYPE masks) but we've
seen this happen too many times with subsequent ABI breakage.

> > I think this use for masks should be avoided when performance does not
> > matter much, as in this case, user application cannot know the number of
> > entries in advance and must rely on the returned value to iterate.
> 
> User doesn't know numbers of entries in advance anyway (with and without the mask).
> That's why this function was introduced at first place.
> 
> With mask it just a bit more handy, in case user cares only about particular subset of supported
> packet types (only L2 let say). 

OK, so we definitely need something to let applications know the layer a
given packet type belongs to, I'm sure it can be done in a convenient way
that won't be limited to the underlying type of the mask.

> > A helper function can be added to convert a RTE_PTYPE_* value to the layer
> > it belongs to (using enum to define possible values).
> 
> Not sure what for?

This is assuming rte_eth_dev_get_ptype_info() doesn't filter anything (no
"mask" argument). In that case a separate function must be added to convert
RTE_PTYPE_* values to a layer, so applications can look for interesting
packet types while parsing plist[] on their own.

This layer information could be defined as an enum, i.e.:

 enum rte_ptype_info {
     RTE_PTYPE_UNKNOWN,
     RTE_PTYPE_L2,
     RTE_PTYPE_L3,
    ...
 };

Or even an int value (2 standing for for "layer 2" etc. Tunnel encapsulation
wouldn't be described easily that way though). It's just an idea.

> > If we absolutely want a mean to filter returned values, I suggest we use
> > this enum instead of the mask argument.
> > Since it won't be a mask, it won't
> > have to be updated every time a new protocol requires extending one.
> 
> Number of bits PTYPE_L2/L3/L4,... layers are already defined.
> So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype -
> there are few reserved values right now.  
> if one day we'll run out bits in let say RTE_PTYPE_L2_MASK  and will have to increase its size -
> it would mean change of the packet_type layout and possible ABI breakage anyway. 

I'm aware of this, only pointing out we tend to rely on masks and type
boundaries a bit too much when there are other methods that are as (if not
more) convenient.

Perhaps some sort of tunneled packet types beyond inner L4 consuming the
four remaining bits will be added? That could happen soon.

> Konstantin
> 
> > 
> > > >   rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[],
> > > >                              size_t max_entries)
> > > >
> > > >
> > > >
> > > > Another point, I have read the example patch (l3fwd) but I don't
> > > > understand why the PMD is not responsible for filling the packet type in
> > > > the MBUF (packet parsing is done by parse_packet_type()).  Why the extra
> > > > computation?
> > >
> > > As I understand there are 3 possibilities now:
> > > 1. HW supports ptype recognition and SW ptype parsing is never done
> > > (--parse-ptype is not specified).
> > > 2. HW supports ptype recognition, but and SW ptype parsing is done anyway
> > > (--parse-ptype is specified).
> > > 3. HW doesn't support and ptype recognition, and and SW ptype parsing is done
> > > (--parse-ptype is specified).
> > >
> > > I suppose the question is what for introduce '--parse-ptype' at all?
> > > My thought because of #2, so people can easily check what will be the performance impact of SW parsing.
> > >
> > > Konstantin
> > >
> > > >
> > > > I see it more like an offload request (as checksum, etc...) and if the
> > > > NIC does not support it then the application does the necessary overload.
> > > >
> > > > Best regards,
> > > >
> > > > --
> > > > Nélio Laranjeiro
> > > > 6WIND
> > 
> > --
> > Adrien Mazarguil
> > 6WIND
  

> -----Original Message-----

> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]

> Sent: Wednesday, January 06, 2016 3:45 PM

> To: Ananyev, Konstantin

> Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set

> 

> On Wed, Jan 06, 2016 at 02:29:07PM +0000, Ananyev, Konstantin wrote:

> >

> >

> > > -----Original Message-----

> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]

> > > Sent: Wednesday, January 06, 2016 10:01 AM

> > > To: Ananyev, Konstantin

> > > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org

> > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set

> > >

> > > On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote:

> > > [...]

> > > > > -----Original Message-----

> > > > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]

> > > [...]

> > > > > I think we miss a comment here in how those 2/6/4 values are chosen

> > > > > because, according to the mask, I expect 16 possibilities but I get

> > > > > less.  It will help a lot anyone who needs to add a new type.

> > > > >

> > > > > Extending the snprintf behavior above, it is best to remove the mask

> > > > > argument altogether and have rte_eth_dev_get_ptype_info() return the

> > > > > entire list every time.  Applications need to iterate on the result in

> > > > > any case.

> > > >

> > > > I think we'd better keep mask argument.

> > > > In many cases upper layer only interested in some particular  subset of

> > > > all packet types that HW can recognise.

> > > > Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,

> > > > tunnelling packet types, etc.

> > > > If caller needs to know all recognised ptypes, he can set mask==-1,

> > > > In that case all supported packet types will be returned.

> > >

> > > There are other drawbacks to the mask argument in my opinion. The API will

> > > have to be updated again as soon as 32 bits aren't enough to represent all

> > > possible masks. We can't predict it will be large enough forever but on the

> > > other hand, using uint64_t seems overkill at this point.

> >

> > Inside rte_mbuf packet_type itself is a 32 bit value.

> > These 32 bits are divided into several fields to mark packet types,

> > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc.

> > As long as packet_type itself is 32bits, 32bit mask is sufficient.

> > If we'll ever run out of 32 bits in packet_type itself, it will be ABI change anyway.

> 

> Sure, however why not do it now this issue has been raised so this function

> doesn't need updating the day it breaks? I know there's a million other

> places with a similar problem but I'm all for making new code future proof.


If rte_mbuf packet_type will have to be increased to 64bit long, then
this function will have to change anyway (with or without mask parameter).
It will have to become:

rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...)

So I think we don't have to worry about mask parameter itself.

> 

> Perhaps in this particular case there is no way to hit the limit (although

> there are only four unused bits left to extend RTE_PTYPE masks) but we've

> seen this happen too many times with subsequent ABI breakage.


When ptype was introduced we tried to reserve some free space for each layer (L2/L3/L4/...),
so it wouldn't be overrun immediately.
But of course if there would be a new HW that can recognise dozen new packet types - it is possible.
Do you have any particular use-case in mind? 

> 

> > > I think this use for masks should be avoided when performance does not

> > > matter much, as in this case, user application cannot know the number of

> > > entries in advance and must rely on the returned value to iterate.

> >

> > User doesn't know numbers of entries in advance anyway (with and without the mask).

> > That's why this function was introduced at first place.

> >

> > With mask it just a bit more handy, in case user cares only about particular subset of supported

> > packet types (only L2 let say).

> 

> OK, so we definitely need something to let applications know the layer a

> given packet type belongs to, I'm sure it can be done in a convenient way

> that won't be limited to the underlying type of the mask.

> 

> > > A helper function can be added to convert a RTE_PTYPE_* value to the layer

> > > it belongs to (using enum to define possible values).

> >

> > Not sure what for?

> 

> This is assuming rte_eth_dev_get_ptype_info() doesn't filter anything (no

> "mask" argument). In that case a separate function must be added to convert

> RTE_PTYPE_* values to a layer, so applications can look for interesting

> packet types while parsing plist[] on their own.


Honestly, I don't see why do you need that.
You already do know that  let say RTE_PTYPE_L3_IPV4 belongs to L3.
Why do you need some extra enum here?
From my thought - the only purpose of mask parameter was to limit number of elements in the ptypes[] at return.
So let say user would need to iterate over 10 elements, instead of 100 to find
the ones he is interested in.

> 

> This layer information could be defined as an enum, i.e.:

> 

>  enum rte_ptype_info {

>      RTE_PTYPE_UNKNOWN,

>      RTE_PTYPE_L2,

>      RTE_PTYPE_L3,

>     ...

>  };

> 

> Or even an int value (2 standing for for "layer 2" etc. Tunnel encapsulation

> wouldn't be described easily that way though). It's just an idea.

> 

> > > If we absolutely want a mean to filter returned values, I suggest we use

> > > this enum instead of the mask argument.

> > > Since it won't be a mask, it won't

> > > have to be updated every time a new protocol requires extending one.

> >

> > Number of bits PTYPE_L2/L3/L4,... layers are already defined.

> > So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype -

> > there are few reserved values right now.

> > if one day we'll run out bits in let say RTE_PTYPE_L2_MASK  and will have to increase its size -

> > it would mean change of the packet_type layout and possible ABI breakage anyway.

> 

> I'm aware of this, only pointing out we tend to rely on masks and type

> boundaries a bit too much when there are other methods that are as (if not

> more) convenient.


Yes, we do rely on masks in ptype.
That's how ptype was defined.
Let say to check that incoming packet is Ether/IPv4(no extentions)/UDP,
you probably would do:

if (mbuf->packet_type & (RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK | RTE_PTYPE_L4_MASK) ==
(RTE_PTYPE_L2_ETHER  | RTE_PTYPE_L3_IPV4 |  RTE_PTYPE_L4_UDP)) {...}

> 

> Perhaps some sort of tunneled packet types beyond inner L4 consuming the

> four remaining bits will be added? That could happen soon.


As I said above: do you have particular scenario in mind when 32bits for packet_type
might be not enough?
If yes, then it is probably a good idea to submit an RFC for extending it to 64 bit,
or introduce packet_type2, or whatever would be your preferred way to deal with it.

Konstantin
  

On Wed, Jan 06, 2016 at 04:44:43PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Wednesday, January 06, 2016 3:45 PM
> > To: Ananyev, Konstantin
> > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > 
> > On Wed, Jan 06, 2016 at 02:29:07PM +0000, Ananyev, Konstantin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > Sent: Wednesday, January 06, 2016 10:01 AM
> > > > To: Ananyev, Konstantin
> > > > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > > >
> > > > On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote:
> > > > [...]
> > > > > > -----Original Message-----
> > > > > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > > > [...]
> > > > > > I think we miss a comment here in how those 2/6/4 values are chosen
> > > > > > because, according to the mask, I expect 16 possibilities but I get
> > > > > > less.  It will help a lot anyone who needs to add a new type.
> > > > > >
> > > > > > Extending the snprintf behavior above, it is best to remove the mask
> > > > > > argument altogether and have rte_eth_dev_get_ptype_info() return the
> > > > > > entire list every time.  Applications need to iterate on the result in
> > > > > > any case.
> > > > >
> > > > > I think we'd better keep mask argument.
> > > > > In many cases upper layer only interested in some particular  subset of
> > > > > all packet types that HW can recognise.
> > > > > Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,
> > > > > tunnelling packet types, etc.
> > > > > If caller needs to know all recognised ptypes, he can set mask==-1,
> > > > > In that case all supported packet types will be returned.
> > > >
> > > > There are other drawbacks to the mask argument in my opinion. The API will
> > > > have to be updated again as soon as 32 bits aren't enough to represent all
> > > > possible masks. We can't predict it will be large enough forever but on the
> > > > other hand, using uint64_t seems overkill at this point.
> > >
> > > Inside rte_mbuf packet_type itself is a 32 bit value.
> > > These 32 bits are divided into several fields to mark packet types,
> > > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc.
> > > As long as packet_type itself is 32bits, 32bit mask is sufficient.
> > > If we'll ever run out of 32 bits in packet_type itself, it will be ABI change anyway.
> > 
> > Sure, however why not do it now this issue has been raised so this function
> > doesn't need updating the day it breaks? I know there's a million other
> > places with a similar problem but I'm all for making new code future proof.
> 
> If rte_mbuf packet_type will have to be increased to 64bit long, then
> this function will have to change anyway (with or without mask parameter).
> It will have to become:
> 
> rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...)
> 
> So I think we don't have to worry about mask parameter itself.

Well, yes, besides I overlooked ptypes[] itself is 32 bit, working around
the type width of the mask wouldn't help much.

> > Perhaps in this particular case there is no way to hit the limit (although
> > there are only four unused bits left to extend RTE_PTYPE masks) but we've
> > seen this happen too many times with subsequent ABI breakage.
> 
> When ptype was introduced we tried to reserve some free space for each layer (L2/L3/L4/...),
> so it wouldn't be overrun immediately.
> But of course if there would be a new HW that can recognise dozen new packet types - it is possible.
> Do you have any particular use-case in mind? 

No, that was just to illustrate my point.

> > > > I think this use for masks should be avoided when performance does not
> > > > matter much, as in this case, user application cannot know the number of
> > > > entries in advance and must rely on the returned value to iterate.
> > >
> > > User doesn't know numbers of entries in advance anyway (with and without the mask).
> > > That's why this function was introduced at first place.
> > >
> > > With mask it just a bit more handy, in case user cares only about particular subset of supported
> > > packet types (only L2 let say).
> > 
> > OK, so we definitely need something to let applications know the layer a
> > given packet type belongs to, I'm sure it can be done in a convenient way
> > that won't be limited to the underlying type of the mask.
> > 
> > > > A helper function can be added to convert a RTE_PTYPE_* value to the layer
> > > > it belongs to (using enum to define possible values).
> > >
> > > Not sure what for?
> > 
> > This is assuming rte_eth_dev_get_ptype_info() doesn't filter anything (no
> > "mask" argument). In that case a separate function must be added to convert
> > RTE_PTYPE_* values to a layer, so applications can look for interesting
> > packet types while parsing plist[] on their own.
> 
> Honestly, I don't see why do you need that.
> You already do know that  let say RTE_PTYPE_L3_IPV4 belongs to L3.
> Why do you need some extra enum here?
> From my thought - the only purpose of mask parameter was to limit number of elements in the ptypes[] at return.
> So let say user would need to iterate over 10 elements, instead of 100 to find
> the ones he is interested in.

Since this is already a slow manner for retrieving types, 10 or 100 doesn't
make much difference. Such a function shouldn't be used in the data path
directly.

My point is, since we're dealing with a slow function, let's keep its API as
simple as possible. By having a mask to match, a large number of checks are
added in all PMDs while they could just fill the array without
bothering. The filtering logic is an application requirement that could be
useful in its own function as well (converting any random value to its
related layer or mask).

> > This layer information could be defined as an enum, i.e.:
> > 
> >  enum rte_ptype_info {
> >      RTE_PTYPE_UNKNOWN,
> >      RTE_PTYPE_L2,
> >      RTE_PTYPE_L3,
> >     ...
> >  };
> > 
> > Or even an int value (2 standing for for "layer 2" etc. Tunnel encapsulation
> > wouldn't be described easily that way though). It's just an idea.
> > 
> > > > If we absolutely want a mean to filter returned values, I suggest we use
> > > > this enum instead of the mask argument.
> > > > Since it won't be a mask, it won't
> > > > have to be updated every time a new protocol requires extending one.
> > >
> > > Number of bits PTYPE_L2/L3/L4,... layers are already defined.
> > > So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype -
> > > there are few reserved values right now.
> > > if one day we'll run out bits in let say RTE_PTYPE_L2_MASK  and will have to increase its size -
> > > it would mean change of the packet_type layout and possible ABI breakage anyway.
> > 
> > I'm aware of this, only pointing out we tend to rely on masks and type
> > boundaries a bit too much when there are other methods that are as (if not
> > more) convenient.
> 
> Yes, we do rely on masks in ptype.
> That's how ptype was defined.
> Let say to check that incoming packet is Ether/IPv4(no extentions)/UDP,
> you probably would do:
> 
> if (mbuf->packet_type & (RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK | RTE_PTYPE_L4_MASK) ==
> (RTE_PTYPE_L2_ETHER  | RTE_PTYPE_L3_IPV4 |  RTE_PTYPE_L4_UDP)) {...}

All right, let's not use a different method to filter packet types.

> > Perhaps some sort of tunneled packet types beyond inner L4 consuming the
> > four remaining bits will be added? That could happen soon.
> 
> As I said above: do you have particular scenario in mind when 32bits for packet_type
> might be not enough?
> If yes, then it is probably a good idea to submit an RFC for extending it to 64 bit,
> or introduce packet_type2, or whatever would be your preferred way to deal with it.

No, really, I guess we'll extend ptype to 64 bit when necessary. My point on
filtering separately still stands.

> Konstantin
>
  

> -----Original Message-----

> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]

> Sent: Wednesday, January 06, 2016 5:23 PM

> To: Ananyev, Konstantin

> Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set

> 

> On Wed, Jan 06, 2016 at 04:44:43PM +0000, Ananyev, Konstantin wrote:

> >

> >

> > > -----Original Message-----

> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]

> > > Sent: Wednesday, January 06, 2016 3:45 PM

> > > To: Ananyev, Konstantin

> > > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org

> > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set

> > >

> > > On Wed, Jan 06, 2016 at 02:29:07PM +0000, Ananyev, Konstantin wrote:

> > > >

> > > >

> > > > > -----Original Message-----

> > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]

> > > > > Sent: Wednesday, January 06, 2016 10:01 AM

> > > > > To: Ananyev, Konstantin

> > > > > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org

> > > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set

> > > > >

> > > > > On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote:

> > > > > [...]

> > > > > > > -----Original Message-----

> > > > > > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]

> > > > > [...]

> > > > > > > I think we miss a comment here in how those 2/6/4 values are chosen

> > > > > > > because, according to the mask, I expect 16 possibilities but I get

> > > > > > > less.  It will help a lot anyone who needs to add a new type.

> > > > > > >

> > > > > > > Extending the snprintf behavior above, it is best to remove the mask

> > > > > > > argument altogether and have rte_eth_dev_get_ptype_info() return the

> > > > > > > entire list every time.  Applications need to iterate on the result in

> > > > > > > any case.

> > > > > >

> > > > > > I think we'd better keep mask argument.

> > > > > > In many cases upper layer only interested in some particular  subset of

> > > > > > all packet types that HW can recognise.

> > > > > > Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,

> > > > > > tunnelling packet types, etc.

> > > > > > If caller needs to know all recognised ptypes, he can set mask==-1,

> > > > > > In that case all supported packet types will be returned.

> > > > >

> > > > > There are other drawbacks to the mask argument in my opinion. The API will

> > > > > have to be updated again as soon as 32 bits aren't enough to represent all

> > > > > possible masks. We can't predict it will be large enough forever but on the

> > > > > other hand, using uint64_t seems overkill at this point.

> > > >

> > > > Inside rte_mbuf packet_type itself is a 32 bit value.

> > > > These 32 bits are divided into several fields to mark packet types,

> > > > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc.

> > > > As long as packet_type itself is 32bits, 32bit mask is sufficient.

> > > > If we'll ever run out of 32 bits in packet_type itself, it will be ABI change anyway.

> > >

> > > Sure, however why not do it now this issue has been raised so this function

> > > doesn't need updating the day it breaks? I know there's a million other

> > > places with a similar problem but I'm all for making new code future proof.

> >

> > If rte_mbuf packet_type will have to be increased to 64bit long, then

> > this function will have to change anyway (with or without mask parameter).

> > It will have to become:

> >

> > rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...)

> >

> > So I think we don't have to worry about mask parameter itself.

> 

> Well, yes, besides I overlooked ptypes[] itself is 32 bit, working around

> the type width of the mask wouldn't help much.

> 

> > > Perhaps in this particular case there is no way to hit the limit (although

> > > there are only four unused bits left to extend RTE_PTYPE masks) but we've

> > > seen this happen too many times with subsequent ABI breakage.

> >

> > When ptype was introduced we tried to reserve some free space for each layer (L2/L3/L4/...),

> > so it wouldn't be overrun immediately.

> > But of course if there would be a new HW that can recognise dozen new packet types - it is possible.

> > Do you have any particular use-case in mind?

> 

> No, that was just to illustrate my point.

> 

> > > > > I think this use for masks should be avoided when performance does not

> > > > > matter much, as in this case, user application cannot know the number of

> > > > > entries in advance and must rely on the returned value to iterate.

> > > >

> > > > User doesn't know numbers of entries in advance anyway (with and without the mask).

> > > > That's why this function was introduced at first place.

> > > >

> > > > With mask it just a bit more handy, in case user cares only about particular subset of supported

> > > > packet types (only L2 let say).

> > >

> > > OK, so we definitely need something to let applications know the layer a

> > > given packet type belongs to, I'm sure it can be done in a convenient way

> > > that won't be limited to the underlying type of the mask.

> > >

> > > > > A helper function can be added to convert a RTE_PTYPE_* value to the layer

> > > > > it belongs to (using enum to define possible values).

> > > >

> > > > Not sure what for?

> > >

> > > This is assuming rte_eth_dev_get_ptype_info() doesn't filter anything (no

> > > "mask" argument). In that case a separate function must be added to convert

> > > RTE_PTYPE_* values to a layer, so applications can look for interesting

> > > packet types while parsing plist[] on their own.

> >

> > Honestly, I don't see why do you need that.

> > You already do know that  let say RTE_PTYPE_L3_IPV4 belongs to L3.

> > Why do you need some extra enum here?

> > From my thought - the only purpose of mask parameter was to limit number of elements in the ptypes[] at return.

> > So let say user would need to iterate over 10 elements, instead of 100 to find

> > the ones he is interested in.

> 

> Since this is already a slow manner for retrieving types, 10 or 100 doesn't

> make much difference. Such a function shouldn't be used in the data path

> directly.

 
Yes, it is not supposed to be called from data-path.

> My point is, since we're dealing with a slow function, let's keep its API as

> simple as possible. 


Well, API should be simple, but from other side it has to be flexible and convenient
for the user.
As I user, I would prefer to have an ability to select the layers here - that's
why I suggested to add the mask parameter. 

>By having a mask to match, a large number of checks are

> added in all PMDs while they could just fill the array without

> bothering. 


That's a valid point.
We could move filter point into rte_ethdev layer.
So PMD would always return an array of all supported ptypes, and
then rte_ethdev layer will filter it based on mask parameter.
Does it sound reasonable to you?

Konstantin 

>The filtering logic is an application requirement that could be

> useful in its own function as well (converting any random value to its

> related layer or mask).

> 

> > > This layer information could be defined as an enum, i.e.:

> > >

> > >  enum rte_ptype_info {

> > >      RTE_PTYPE_UNKNOWN,

> > >      RTE_PTYPE_L2,

> > >      RTE_PTYPE_L3,

> > >     ...

> > >  };

> > >

> > > Or even an int value (2 standing for for "layer 2" etc. Tunnel encapsulation

> > > wouldn't be described easily that way though). It's just an idea.

> > >

> > > > > If we absolutely want a mean to filter returned values, I suggest we use

> > > > > this enum instead of the mask argument.

> > > > > Since it won't be a mask, it won't

> > > > > have to be updated every time a new protocol requires extending one.

> > > >

> > > > Number of bits PTYPE_L2/L3/L4,... layers are already defined.

> > > > So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype -

> > > > there are few reserved values right now.

> > > > if one day we'll run out bits in let say RTE_PTYPE_L2_MASK  and will have to increase its size -

> > > > it would mean change of the packet_type layout and possible ABI breakage anyway.

> > >

> > > I'm aware of this, only pointing out we tend to rely on masks and type

> > > boundaries a bit too much when there are other methods that are as (if not

> > > more) convenient.

> >

> > Yes, we do rely on masks in ptype.

> > That's how ptype was defined.

> > Let say to check that incoming packet is Ether/IPv4(no extentions)/UDP,

> > you probably would do:

> >

> > if (mbuf->packet_type & (RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK | RTE_PTYPE_L4_MASK) ==

> > (RTE_PTYPE_L2_ETHER  | RTE_PTYPE_L3_IPV4 |  RTE_PTYPE_L4_UDP)) {...}

> 

> All right, let's not use a different method to filter packet types.

> 

> > > Perhaps some sort of tunneled packet types beyond inner L4 consuming the

> > > four remaining bits will be added? That could happen soon.

> >

> > As I said above: do you have particular scenario in mind when 32bits for packet_type

> > might be not enough?

> > If yes, then it is probably a good idea to submit an RFC for extending it to 64 bit,

> > or introduce packet_type2, or whatever would be your preferred way to deal with it.

> 

> No, really, I guess we'll extend ptype to 64 bit when necessary. My point on

> filtering separately still stands.

> 

> > Konstantin

> >

> 

> --

> Adrien Mazarguil

> 6WIND
  

On Thu, Jan 07, 2016 at 10:24:19AM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Wednesday, January 06, 2016 5:23 PM
> > To: Ananyev, Konstantin
> > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > 
> > On Wed, Jan 06, 2016 at 04:44:43PM +0000, Ananyev, Konstantin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > Sent: Wednesday, January 06, 2016 3:45 PM
> > > > To: Ananyev, Konstantin
> > > > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > > >
> > > > On Wed, Jan 06, 2016 at 02:29:07PM +0000, Ananyev, Konstantin wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > > > Sent: Wednesday, January 06, 2016 10:01 AM
> > > > > > To: Ananyev, Konstantin
> > > > > > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@dpdk.org
> > > > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > > > > >
> > > > > > On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote:
> > > > > > [...]
> > > > > > > > -----Original Message-----
> > > > > > > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > > > > > [...]
> > > > > > > > I think we miss a comment here in how those 2/6/4 values are chosen
> > > > > > > > because, according to the mask, I expect 16 possibilities but I get
> > > > > > > > less.  It will help a lot anyone who needs to add a new type.
> > > > > > > >
> > > > > > > > Extending the snprintf behavior above, it is best to remove the mask
> > > > > > > > argument altogether and have rte_eth_dev_get_ptype_info() return the
> > > > > > > > entire list every time.  Applications need to iterate on the result in
> > > > > > > > any case.
> > > > > > >
> > > > > > > I think we'd better keep mask argument.
> > > > > > > In many cases upper layer only interested in some particular  subset of
> > > > > > > all packet types that HW can recognise.
> > > > > > > Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,
> > > > > > > tunnelling packet types, etc.
> > > > > > > If caller needs to know all recognised ptypes, he can set mask==-1,
> > > > > > > In that case all supported packet types will be returned.
> > > > > >
> > > > > > There are other drawbacks to the mask argument in my opinion. The API will
> > > > > > have to be updated again as soon as 32 bits aren't enough to represent all
> > > > > > possible masks. We can't predict it will be large enough forever but on the
> > > > > > other hand, using uint64_t seems overkill at this point.
> > > > >
> > > > > Inside rte_mbuf packet_type itself is a 32 bit value.
> > > > > These 32 bits are divided into several fields to mark packet types,
> > > > > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc.
> > > > > As long as packet_type itself is 32bits, 32bit mask is sufficient.
> > > > > If we'll ever run out of 32 bits in packet_type itself, it will be ABI change anyway.
> > > >
> > > > Sure, however why not do it now this issue has been raised so this function
> > > > doesn't need updating the day it breaks? I know there's a million other
> > > > places with a similar problem but I'm all for making new code future proof.
> > >
> > > If rte_mbuf packet_type will have to be increased to 64bit long, then
> > > this function will have to change anyway (with or without mask parameter).
> > > It will have to become:
> > >
> > > rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...)
> > >
> > > So I think we don't have to worry about mask parameter itself.
> > 
> > Well, yes, besides I overlooked ptypes[] itself is 32 bit, working around
> > the type width of the mask wouldn't help much.
> > 
> > > > Perhaps in this particular case there is no way to hit the limit (although
> > > > there are only four unused bits left to extend RTE_PTYPE masks) but we've
> > > > seen this happen too many times with subsequent ABI breakage.
> > >
> > > When ptype was introduced we tried to reserve some free space for each layer (L2/L3/L4/...),
> > > so it wouldn't be overrun immediately.
> > > But of course if there would be a new HW that can recognise dozen new packet types - it is possible.
> > > Do you have any particular use-case in mind?
> > 
> > No, that was just to illustrate my point.
> > 
> > > > > > I think this use for masks should be avoided when performance does not
> > > > > > matter much, as in this case, user application cannot know the number of
> > > > > > entries in advance and must rely on the returned value to iterate.
> > > > >
> > > > > User doesn't know numbers of entries in advance anyway (with and without the mask).
> > > > > That's why this function was introduced at first place.
> > > > >
> > > > > With mask it just a bit more handy, in case user cares only about particular subset of supported
> > > > > packet types (only L2 let say).
> > > >
> > > > OK, so we definitely need something to let applications know the layer a
> > > > given packet type belongs to, I'm sure it can be done in a convenient way
> > > > that won't be limited to the underlying type of the mask.
> > > >
> > > > > > A helper function can be added to convert a RTE_PTYPE_* value to the layer
> > > > > > it belongs to (using enum to define possible values).
> > > > >
> > > > > Not sure what for?
> > > >
> > > > This is assuming rte_eth_dev_get_ptype_info() doesn't filter anything (no
> > > > "mask" argument). In that case a separate function must be added to convert
> > > > RTE_PTYPE_* values to a layer, so applications can look for interesting
> > > > packet types while parsing plist[] on their own.
> > >
> > > Honestly, I don't see why do you need that.
> > > You already do know that  let say RTE_PTYPE_L3_IPV4 belongs to L3.
> > > Why do you need some extra enum here?
> > > From my thought - the only purpose of mask parameter was to limit number of elements in the ptypes[] at return.
> > > So let say user would need to iterate over 10 elements, instead of 100 to find
> > > the ones he is interested in.
> > 
> > Since this is already a slow manner for retrieving types, 10 or 100 doesn't
> > make much difference. Such a function shouldn't be used in the data path
> > directly.
>  
> Yes, it is not supposed to be called from data-path.
> 
> > My point is, since we're dealing with a slow function, let's keep its API as
> > simple as possible. 
> 
> Well, API should be simple, but from other side it has to be flexible and convenient
> for the user.
> As I user, I would prefer to have an ability to select the layers here - that's
> why I suggested to add the mask parameter. 
> 
> >By having a mask to match, a large number of checks are
> > added in all PMDs while they could just fill the array without
> > bothering. 
> 
> That's a valid point.
> We could move filter point into rte_ethdev layer.
> So PMD would always return an array of all supported ptypes, and
> then rte_ethdev layer will filter it based on mask parameter.
> Does it sound reasonable to you?

Yes, I think it's fine that way, thanks.

> >The filtering logic is an application requirement that could be
> > useful in its own function as well (converting any random value to its
> > related layer or mask).
> > 
> > > > This layer information could be defined as an enum, i.e.:
> > > >
> > > >  enum rte_ptype_info {
> > > >      RTE_PTYPE_UNKNOWN,
> > > >      RTE_PTYPE_L2,
> > > >      RTE_PTYPE_L3,
> > > >     ...
> > > >  };
> > > >
> > > > Or even an int value (2 standing for for "layer 2" etc. Tunnel encapsulation
> > > > wouldn't be described easily that way though). It's just an idea.
> > > >
> > > > > > If we absolutely want a mean to filter returned values, I suggest we use
> > > > > > this enum instead of the mask argument.
> > > > > > Since it won't be a mask, it won't
> > > > > > have to be updated every time a new protocol requires extending one.
> > > > >
> > > > > Number of bits PTYPE_L2/L3/L4,... layers are already defined.
> > > > > So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype -
> > > > > there are few reserved values right now.
> > > > > if one day we'll run out bits in let say RTE_PTYPE_L2_MASK  and will have to increase its size -
> > > > > it would mean change of the packet_type layout and possible ABI breakage anyway.
> > > >
> > > > I'm aware of this, only pointing out we tend to rely on masks and type
> > > > boundaries a bit too much when there are other methods that are as (if not
> > > > more) convenient.
> > >
> > > Yes, we do rely on masks in ptype.
> > > That's how ptype was defined.
> > > Let say to check that incoming packet is Ether/IPv4(no extentions)/UDP,
> > > you probably would do:
> > >
> > > if (mbuf->packet_type & (RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK | RTE_PTYPE_L4_MASK) ==
> > > (RTE_PTYPE_L2_ETHER  | RTE_PTYPE_L3_IPV4 |  RTE_PTYPE_L4_UDP)) {...}
> > 
> > All right, let's not use a different method to filter packet types.
> > 
> > > > Perhaps some sort of tunneled packet types beyond inner L4 consuming the
> > > > four remaining bits will be added? That could happen soon.
> > >
> > > As I said above: do you have particular scenario in mind when 32bits for packet_type
> > > might be not enough?
> > > If yes, then it is probably a good idea to submit an RFC for extending it to 64 bit,
> > > or introduce packet_type2, or whatever would be your preferred way to deal with it.
> > 
> > No, really, I guess we'll extend ptype to 64 bit when necessary. My point on
> > filtering separately still stands.
> > 
> > > Konstantin
> > >
> > 
> > --
> > Adrien Mazarguil
> > 6WIND
  

Hi,

According to the proposal, I'm going to fix the definition of this API 
as below:
/**
  * 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
  * Size of ptypes[]
  * @return
  *   - (>=0) if successful. Indicate number of valid values in ptypes 
array.
  *   - (-ENOTSUP) if hardware-assisted VLAN stripping not configured.
  *   - (-ENODEV) if *port_id* invalid.
  */
extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
                                 uint32_t ptype_mask, uint32_t ptypes[], 
uint32_t num);

Unresolved issues:
1) When num is exceeded, we just stop there and return num, or return 
-ENOMEM?
The first way has a bug when: what app is exactly asking for is not 
filled in ptypes[] because of
exceeding num, but app believes this API returns with success.

2) if RTE_PTYPE_*_MAX_NUM macros necessary? Without them, we could calculate
num through 2^(number of bit 1 in RTE_PTPE_*_MASK).

Thanks,
Jianfeng
  

Hi Jianfeng,

> -----Original Message-----

> From: Tan, Jianfeng

> Sent: Monday, January 11, 2016 7:39 AM

> To: Ananyev, Konstantin; Nélio Laranjeiro; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set

> 

> Hi,

> 

> According to the proposal, I'm going to fix the definition of this API

> as below:

> /**

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

>   * Size of ptypes[]

>   * @return

>   *   - (>=0) if successful. Indicate number of valid values in ptypes

> array.

>   *   - (-ENOTSUP) if hardware-assisted VLAN stripping not configured.

>   *   - (-ENODEV) if *port_id* invalid.

>   */

> extern int rte_eth_dev_get_ptype_info(uint8_t port_id,

>                                  uint32_t ptype_mask, uint32_t ptypes[],

> uint32_t num);

> 

> Unresolved issues:

> 1) When num is exceeded, we just stop there and return num, or return

> -ENOMEM?


I think when num is exceeded it should return number of entries enough to
return all requested packet types.
Same as snprintf() does when it has to truncate the output buffer.

> The first way has a bug when: what app is exactly asking for is not

> filled in ptypes[] because of

> exceeding num, but app believes this API returns with success.


It is a caller responsibility to check the return value and handle it properly.
When return value  exceeds num - caller can  resize ptypes[] and call get_ptype_info() again.

> 

> 2) if RTE_PTYPE_*_MAX_NUM macros necessary? Without them, we could calculate

> num through 2^(number of bit 1 in RTE_PTPE_*_MASK).


I don't think caller has to guess somehow what number of entries in ptypes[] he need .
He can retrieve that information from get_ptype_info() itself.
Something like that for example:

num = rte_eth_dev_get_ptype_info(port, UINT32_MAX, NULL, 0);
if (num < 0) return num;
ptypes = alloca(num * sizeof(ptypes[0]);
ret = rte_eth_dev_get_ptype_info(port, UINT32_MAX, ptypes, num);
if (ret != num) return -1;
....

Konstantin

> 

> Thanks,

> Jianfeng