Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add VLAN attributes to ETH and VLAN items

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

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