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

[Dekel Peled <dekelp@nvidia.com>]

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