Thanks, PSB. > -----Original Message----- > From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> > Sent: Thursday, October 15, 2020 1:35 PM > To: Dekel Peled <dek...@nvidia.com>; Ori Kam <or...@nvidia.com>; > wenzhuo...@intel.com; beilei.x...@intel.com; > bernard.iremon...@intel.com; m...@ashroe.eu; nhor...@tuxdriver.com; > NBU-Contact-Thomas Monjalon <tho...@monjalon.net>; > ferruh.yi...@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&data=04%7C > > > 01%7Cdekelp%40nvidia.com%7C8fbd9db835b1498cbddf08d870f5f866%7C430 > 83d15 > > > 727340c1b7db39efd9ccc17a%7C0%7C0%7C637383549029697005%7CUnknow > n%7CTWFp > > > bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC > I6Mn > > > 0%3D%7C1000&sdata=LiZ9UIHl0Kz18Ck94B6daV%2BhByfMyhZJDpYz7Zn > NDL8%3D > > &reserved=0 > > > > Signed-off-by: Dekel Peled <dek...@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&data=04%7C01%7Cdekelp%40nvidia.com%7C8f > bd9db835b1498cbddf08d870f5f866%7C43083d15727340c1b7db39efd9ccc17a > %7C0%7C0%7C637383549029697005%7CUnknown%7CTWFpbGZsb3d8eyJWIj > oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1 > 000&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. */ > >
