[v4,1/2] ethdev: add VLAN attributes to ETH and VLAN items

  This patch implements the change proposes in RFC [1], adding dedicated
fields to ETH and VLAN items structs, to clearly define the required
characteristic of a packet, and enable precise match criteria.
Documentation is updated accordingly.

[1] https://mails.dpdk.org/archives/dev/2020-August/177536.html

Signed-off-by: Dekel Peled <dekelp@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst     | 14 ++++++++++++--
 doc/guides/rel_notes/deprecation.rst   |  5 -----
 doc/guides/rel_notes/release_20_11.rst |  7 +++++++
 lib/librte_ethdev/rte_flow.h           | 18 +++++++++++++++---
 4 files changed, 34 insertions(+), 10 deletions(-)
  

14/10/2020 20:53, Dekel Peled:
> @@ -292,6 +292,13 @@ API Changes
>  
>  * vhost: Moved vDPA APIs from experimental to stable.
>  
> +* ethdev: Added new field ``has_vlan`` to structure ``rte_flow_item_eth``,
> +  indicating that packet header contains at least one VLAN.
> +
> +* ethdev: Added new field ``has_more_vlan`` to structure
> +  ``rte_flow_item_vlan``, indicating that packet header contains at least one
> +  more VLAN, after this VLAN.
> +

The ethdev changes should be grouped with others above vhost one.

[...]
> - * If the @p ETH item is the only item in the pattern, and the @p type field
> - * is not specified, then both tagged and untagged packets will match the
> - * pattern.
> + * The field @p has_vlan can be used to match specific packet types, instead
> + * of using the @p type field.
> + * This can be used to match any type of tagged packets.

If I understand well, these two sentences could be merged in one:
The field @p has_vlan can be used to match tagged packets,
instead of using the @p type field.

> + * If the @p type and @p has_vlan fields are not specified, then both tagged
> + * and untagged packets will match the pattern.

OK

[...]
> + * The field @p has_more_vlan can be used to match specific packet types,
> + * instead of using the @p inner_type field.
> + * This can be used to match any type of tagged packets.

Here as well, "specific" can be replaced with "QinQ" or "inner tagged".

> + * If the @p inner_type and @p has_more_vlan fields are not specified,
> + * then any tagged packets will match the pattern.

OK
  

On 10/14/20 9:53 PM, Dekel Peled wrote:
> This patch implements the change proposes in RFC [1], adding dedicated
> fields to ETH and VLAN items structs, to clearly define the required
> characteristic of a packet, and enable precise match criteria.
> Documentation is updated accordingly.
> 
> [1] https://mails.dpdk.org/archives/dev/2020-August/177536.html
> 
> Signed-off-by: Dekel Peled <dekelp@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst     | 14 ++++++++++++--
>  doc/guides/rel_notes/deprecation.rst   |  5 -----
>  doc/guides/rel_notes/release_20_11.rst |  7 +++++++
>  lib/librte_ethdev/rte_flow.h           | 18 +++++++++++++++---
>  4 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index f26a6c2..40230d3 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -908,12 +908,16 @@ order as on the wire.
>  If the ``type`` field contains a TPID value, then only tagged packets with the
>  specified TPID will match the pattern.
>  Otherwise, only untagged packets will match the pattern.

Is the above sentence still valid? It looks like no
if has_vlan is set in a mask and set in spec.

> -If the ``ETH`` item is the only item in the pattern, and the ``type`` field is
> -not specified, then both tagged and untagged packets will match the pattern.
> +The field ``has_vlan`` can be used to match specific packet types, instead
> +of using the ``type`` field.
> +This can be used to match any type of tagged packets.
> +If the ``type`` and ``has_vlan`` fields are not specified, then both tagged
> +and untagged packets will match the pattern.

Consider to highlight that has_vlan field in mask
controls if the field in spec is taken into account.
If the field is unset in mask, it could be either
tagged or untagged (if there is no VLAN item in spec).
If VLAN item is present, basically it is the same
as mask.has_vlan==1 and spec.has_vlan==1.

>  
>  - ``dst``: destination MAC.
>  - ``src``: source MAC.
>  - ``type``: EtherType or TPID.
> +- ``has_vlan``: packet header contains at least one VLAN.
>  - Default ``mask`` matches destination and source addresses only.
>  
>  Item: ``VLAN``
> @@ -926,9 +930,15 @@ The corresponding standard outer EtherType (TPID) values are
>  preceding pattern item.
>  If a ``VLAN`` item is present in the pattern, then only tagged packets will
>  match the pattern.
> +The field ``has_more_vlan`` can be used to match specific packet types,
> +instead of using the ``inner_type field``.
> +This can be used to match any type of tagged packets.
> +If the ``inner_type`` and ``has_more_vlan`` fields are not specified,
> +then any tagged packets will match the pattern.
>  
>  - ``tci``: tag control information.
>  - ``inner_type``: inner EtherType or TPID.
> +- ``has_more_vlan``: packet header contains at least one more VLAN, after this VLAN.
>  - Default ``mask`` matches the VID part of TCI only (lower 12 bits).
>  
>  Item: ``IPV4``
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 584e720..72011b0 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -154,11 +154,6 @@ Deprecation Notices
>    as deprecated in DPDK 20.11, along with the associated macros ``ETH_MIRROR_*``.
>    This API will be fully removed in DPDK 21.11.
>  
> -* ethdev: The ``struct rte_flow_item_eth`` and ``struct rte_flow_item_vlan``
> -  structs will be modified, to include an additional value, indicating existence
> -  or absence of a VLAN header following the current header, as proposed in RFC
> -  https://mails.dpdk.org/archives/dev/2020-August/177536.html.
> -
>  * ethdev: The ``struct rte_flow_item_ipv6`` struct will be modified to include
>    additional values, indicating existence or absence of IPv6 extension headers
>    following the IPv6 header, as proposed in RFC
> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> index 30db8f2..4932d82 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -292,6 +292,13 @@ API Changes
>  
>  * vhost: Moved vDPA APIs from experimental to stable.
>  
> +* ethdev: Added new field ``has_vlan`` to structure ``rte_flow_item_eth``,
> +  indicating that packet header contains at least one VLAN.
> +
> +* ethdev: Added new field ``has_more_vlan`` to structure
> +  ``rte_flow_item_vlan``, indicating that packet header contains at least one
> +  more VLAN, after this VLAN.
> +
>  * rawdev: Added a structure size parameter to the functions
>    ``rte_rawdev_queue_setup()``, ``rte_rawdev_queue_conf_get()``,
>    ``rte_rawdev_info_get()`` and ``rte_rawdev_configure()``,
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 3d5fb09..cb3bb5c 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -723,14 +723,18 @@ struct rte_flow_item_raw {
>   * If the @p type field contains a TPID value, then only tagged packets with the
>   * specified TPID will match the pattern.
>   * Otherwise, only untagged packets will match the pattern.
> - * If the @p ETH item is the only item in the pattern, and the @p type field
> - * is not specified, then both tagged and untagged packets will match the
> - * pattern.
> + * The field @p has_vlan can be used to match specific packet types, instead
> + * of using the @p type field.
> + * This can be used to match any type of tagged packets.
> + * If the @p type and @p has_vlan fields are not specified, then both tagged
> + * and untagged packets will match the pattern.

.. if there is no VLAN item in the pattern

>   */
>  struct rte_flow_item_eth {
>  	struct rte_ether_addr dst; /**< Destination MAC. */
>  	struct rte_ether_addr src; /**< Source MAC. */
>  	rte_be16_t type; /**< EtherType or TPID. */
> +	uint32_t has_vlan:1; /**< Packet header contains at least one VLAN. */
> +	uint32_t reserved:31; /**< Reserved, must be zero. */

Have you considered to make it uint16_t to keep the size of the
structure and have no holes?

>  };
>  
>  /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
> @@ -752,10 +756,18 @@ struct rte_flow_item_eth {
>   * the preceding pattern item.
>   * If a @p VLAN item is present in the pattern, then only tagged packets will
>   * match the pattern.
> + * The field @p has_more_vlan can be used to match specific packet types,
> + * instead of using the @p inner_type field.
> + * This can be used to match any type of tagged packets.
> + * If the @p inner_type and @p has_more_vlan fields are not specified,
> + * then any tagged packets will match the pattern.

... if there no VLAN items follow?

>   */
>  struct rte_flow_item_vlan {
>  	rte_be16_t tci; /**< Tag control information. */
>  	rte_be16_t inner_type; /**< Inner EtherType or TPID. */
> +	uint32_t has_more_vlan:1;
> +	/**< Packet header contains at least one more VLAN, after this VLAN. */
> +	uint32_t reserved:31; /**< Reserved, must be zero. */
>  };
>  
>  /** Default mask for RTE_FLOW_ITEM_TYPE_VLAN. */
>
  

Thanks, PSB.

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Thursday, October 15, 2020 1:35 PM
> To: Dekel Peled <dekelp@nvidia.com>; Ori Kam <orika@nvidia.com>;
> wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com; mdr@ashroe.eu; nhorman@tuxdriver.com;
> NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> ferruh.yigit@intel.com
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v4 1/2] ethdev: add VLAN attributes to ETH and VLAN
> items
> 
> On 10/14/20 9:53 PM, Dekel Peled wrote:
> > This patch implements the change proposes in RFC [1], adding dedicated
> > fields to ETH and VLAN items structs, to clearly define the required
> > characteristic of a packet, and enable precise match criteria.
> > Documentation is updated accordingly.
> >
> > [1]
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail
> > s.dpdk.org%2Farchives%2Fdev%2F2020-
> August%2F177536.html&amp;data=04%7C
> >
> 01%7Cdekelp%40nvidia.com%7C8fbd9db835b1498cbddf08d870f5f866%7C430
> 83d15
> >
> 727340c1b7db39efd9ccc17a%7C0%7C0%7C637383549029697005%7CUnknow
> n%7CTWFp
> >
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
> I6Mn
> >
> 0%3D%7C1000&amp;sdata=LiZ9UIHl0Kz18Ck94B6daV%2BhByfMyhZJDpYz7Zn
> NDL8%3D
> > &amp;reserved=0
> >
> > Signed-off-by: Dekel Peled <dekelp@nvidia.com>
> > ---
> >  doc/guides/prog_guide/rte_flow.rst     | 14 ++++++++++++--
> >  doc/guides/rel_notes/deprecation.rst   |  5 -----
> >  doc/guides/rel_notes/release_20_11.rst |  7 +++++++
> >  lib/librte_ethdev/rte_flow.h           | 18 +++++++++++++++---
> >  4 files changed, 34 insertions(+), 10 deletions(-)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index f26a6c2..40230d3 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -908,12 +908,16 @@ order as on the wire.
> >  If the ``type`` field contains a TPID value, then only tagged packets
> > with the  specified TPID will match the pattern.
> >  Otherwise, only untagged packets will match the pattern.
> 
> Is the above sentence still valid? It looks like no if has_vlan is set in a mask
> and set in spec.

IIUC in this sentence 'Otherwise' means 'type field contains Ethertype value and not TPID value'.
I agree it may be misleading, I'll remove this sentence.

> 
> > -If the ``ETH`` item is the only item in the pattern, and the ``type``
> > field is -not specified, then both tagged and untagged packets will match
> the pattern.
> > +The field ``has_vlan`` can be used to match specific packet types,
> > +instead of using the ``type`` field.
> > +This can be used to match any type of tagged packets.
> > +If the ``type`` and ``has_vlan`` fields are not specified, then both
> > +tagged and untagged packets will match the pattern.
> 
> Consider to highlight that has_vlan field in mask controls if the field in spec is
> taken into account.
> If the field is unset in mask, it could be either tagged or untagged (if there is
> no VLAN item in spec).
> If VLAN item is present, basically it is the same as mask.has_vlan==1 and
> spec.has_vlan==1.

Maybe I'm missing your intention here, but this is the standard functionality of spec and mask, it is not specific to this field.

> 
> >
> >  - ``dst``: destination MAC.
> >  - ``src``: source MAC.
> >  - ``type``: EtherType or TPID.
> > +- ``has_vlan``: packet header contains at least one VLAN.
> >  - Default ``mask`` matches destination and source addresses only.
> >
> >  Item: ``VLAN``
> > @@ -926,9 +930,15 @@ The corresponding standard outer EtherType
> (TPID)
> > values are  preceding pattern item.
> >  If a ``VLAN`` item is present in the pattern, then only tagged
> > packets will  match the pattern.
> > +The field ``has_more_vlan`` can be used to match specific packet
> > +types, instead of using the ``inner_type field``.
> > +This can be used to match any type of tagged packets.
> > +If the ``inner_type`` and ``has_more_vlan`` fields are not specified,
> > +then any tagged packets will match the pattern.
> >
> >  - ``tci``: tag control information.
> >  - ``inner_type``: inner EtherType or TPID.
> > +- ``has_more_vlan``: packet header contains at least one more VLAN,
> after this VLAN.
> >  - Default ``mask`` matches the VID part of TCI only (lower 12 bits).
> >
> >  Item: ``IPV4``
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index 584e720..72011b0 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -154,11 +154,6 @@ Deprecation Notices
> >    as deprecated in DPDK 20.11, along with the associated macros
> ``ETH_MIRROR_*``.
> >    This API will be fully removed in DPDK 21.11.
> >
> > -* ethdev: The ``struct rte_flow_item_eth`` and ``struct
> > rte_flow_item_vlan``
> > -  structs will be modified, to include an additional value,
> > indicating existence
> > -  or absence of a VLAN header following the current header, as
> > proposed in RFC
> > -
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail
> s.dpdk.org%2Farchives%2Fdev%2F2020-
> August%2F177536.html&amp;data=04%7C01%7Cdekelp%40nvidia.com%7C8f
> bd9db835b1498cbddf08d870f5f866%7C43083d15727340c1b7db39efd9ccc17a
> %7C0%7C0%7C637383549029697005%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1
> 000&amp;sdata=LiZ9UIHl0Kz18Ck94B6daV%2BhByfMyhZJDpYz7ZnNDL8%3D&
> amp;reserved=0.
> > -
> >  * ethdev: The ``struct rte_flow_item_ipv6`` struct will be modified to
> include
> >    additional values, indicating existence or absence of IPv6 extension
> headers
> >    following the IPv6 header, as proposed in RFC diff --git
> > a/doc/guides/rel_notes/release_20_11.rst
> > b/doc/guides/rel_notes/release_20_11.rst
> > index 30db8f2..4932d82 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -292,6 +292,13 @@ API Changes
> >
> >  * vhost: Moved vDPA APIs from experimental to stable.
> >
> > +* ethdev: Added new field ``has_vlan`` to structure
> > +``rte_flow_item_eth``,
> > +  indicating that packet header contains at least one VLAN.
> > +
> > +* ethdev: Added new field ``has_more_vlan`` to structure
> > +  ``rte_flow_item_vlan``, indicating that packet header contains at
> > +least one
> > +  more VLAN, after this VLAN.
> > +
> >  * rawdev: Added a structure size parameter to the functions
> >    ``rte_rawdev_queue_setup()``, ``rte_rawdev_queue_conf_get()``,
> >    ``rte_rawdev_info_get()`` and ``rte_rawdev_configure()``, diff
> > --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 3d5fb09..cb3bb5c 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -723,14 +723,18 @@ struct rte_flow_item_raw {
> >   * If the @p type field contains a TPID value, then only tagged packets with
> the
> >   * specified TPID will match the pattern.
> >   * Otherwise, only untagged packets will match the pattern.
> > - * If the @p ETH item is the only item in the pattern, and the @p
> > type field
> > - * is not specified, then both tagged and untagged packets will match
> > the
> > - * pattern.
> > + * The field @p has_vlan can be used to match specific packet types,
> > + instead
> > + * of using the @p type field.
> > + * This can be used to match any type of tagged packets.
> > + * If the @p type and @p has_vlan fields are not specified, then both
> > + tagged
> > + * and untagged packets will match the pattern.
> 
> .. if there is no VLAN item in the pattern

IIUC this description is for the ETH item only, w/o dependency on other items.

> 
> >   */
> >  struct rte_flow_item_eth {
> >  	struct rte_ether_addr dst; /**< Destination MAC. */
> >  	struct rte_ether_addr src; /**< Source MAC. */
> >  	rte_be16_t type; /**< EtherType or TPID. */
> > +	uint32_t has_vlan:1; /**< Packet header contains at least one VLAN.
> */
> > +	uint32_t reserved:31; /**< Reserved, must be zero. */
> 
> Have you considered to make it uint16_t to keep the size of the structure
> and have no holes?

Yes, but left it as uint32_t because:
- compiler issues a warning on 16-bit bitfield.
- need a 1 bit flag for item attribute, to align with recent change in ipv6 item, and for strict control with specific mask.
 
> 
> >  };
> >
> >  /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */ @@ -752,10 +756,18
> @@
> > struct rte_flow_item_eth {
> >   * the preceding pattern item.
> >   * If a @p VLAN item is present in the pattern, then only tagged packets
> will
> >   * match the pattern.
> > + * The field @p has_more_vlan can be used to match specific packet
> > + types,
> > + * instead of using the @p inner_type field.
> > + * This can be used to match any type of tagged packets.
> > + * If the @p inner_type and @p has_more_vlan fields are not
> > + specified,
> > + * then any tagged packets will match the pattern.
> 
> ... if there no VLAN items follow?

IIUC this description is for the VLAN item only, w/o dependency on other items.

> 
> >   */
> >  struct rte_flow_item_vlan {
> >  	rte_be16_t tci; /**< Tag control information. */
> >  	rte_be16_t inner_type; /**< Inner EtherType or TPID. */
> > +	uint32_t has_more_vlan:1;
> > +	/**< Packet header contains at least one more VLAN, after this
> VLAN. */
> > +	uint32_t reserved:31; /**< Reserved, must be zero. */
> >  };
> >
> >  /** Default mask for RTE_FLOW_ITEM_TYPE_VLAN. */
> >