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

[Maxime Leroy <maxime.leroy@6wind.com>]

On Mon, Oct 5, 2020 at 11:37 AM Dekel Peled <dekelp@nvidia.com> wrote:
>
> Thanks, PSB.
>
> > -----Original Message-----
> > From: Maxime Leroy <maxime.leroy@6wind.com>
> > Sent: Friday, October 2, 2020 3:39 PM
> > To: Dekel Peled <dekelp@nvidia.com>
> > Cc: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon
> > <thomas@monjalon.net>; ferruh.yigit@intel.com;
> > arybchenko@solarflare.com; dev@dpdk.org; Dekel Peled
> > <dekelp@mellanox.com>
> > Subject: Re: [PATCH] ethdev: add VLAN attributes to ETH and VLAN items
> >
> > Hi Dekel,
> >
> > On Thu, Oct 1, 2020 at 8:49 PM Dekel Peled <dekelp@nvidia.com> wrote:
> > >
> > > From: Dekel Peled <dekelp@mellanox.com>
> > >
> > > 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.
> > >
> > > [1]
> > >
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail
> > > s.dpdk.org%2Farchives%2Fdev%2F2020-
> > August%2F177536.html&amp;data=02%7C
> > >
> > 01%7Cdekelp%40nvidia.com%7Cc12bfd3f662747f7b7c408d866d0376f%7C430
> > 83d15
> > >
> > 727340c1b7db39efd9ccc17a%7C0%7C0%7C637372391779092411&amp;sdata=
> > yeOKvc
> > > 4r0dL09UZ65%2Bt4qWJqJmcp21VyPSK%2FhbablKI%3D&amp;reserved=0
> > >
> > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > ---
> > >  doc/guides/rel_notes/release_20_11.rst |  7 +++++++
> > >  lib/librte_ethdev/rte_flow.h           | 16 +++++++++++++---
> > >  2 files changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/doc/guides/rel_notes/release_20_11.rst
> > > b/doc/guides/rel_notes/release_20_11.rst
> > > index 7f9d0dd..199c60b 100644
> > > --- a/doc/guides/rel_notes/release_20_11.rst
> > > +++ b/doc/guides/rel_notes/release_20_11.rst
> > > @@ -173,6 +173,13 @@ API Changes
> > >    * ``_rte_eth_dev_callback_process()`` ->
> > ``rte_eth_dev_callback_process()``
> > >    * ``_rte_eth_dev_reset`` -> ``rte_eth_dev_internal_reset()``
> > >
> > > +* ethdev: Added new field ``vlan_exist`` to structure
> > > +``rte_flow_item_eth``,
> > > +  indicating that at least one VLAN exists in the packet header.
> > > +
> > > +* ethdev: Added new field ``more_vlans_exist`` to structure
> > > +  ``rte_flow_item_vlan``, indicating that at least one more VLAN
> > > +exists in
> > > +  packet header, following 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 da8bfa5..39d04ef 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 vlan_exist 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 vlan_exist fields are not specified, then
> > > + both tagged
> > > + * and untagged packets will match 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 vlan_exist:1; /**< At least one VLAN exist in header. */
> > > +       uint32_t reserved:31; /**< Reserved, must be zero. */
> > >  };
> >
> > To resume:
> > - type and vlan_exists fields not specified:  tag and untagged matched
> > - with vlan_exists, match only tag or untagged
> > - with type matching specific ethernet type
> > - vlan_exists and type should not setted at the same time ?
>
> PMD should validate they don't conflict.
>
> >
> > With this new specification, I think you address all the use cases.
> > That's great !
> >
>
> Glad to see we agree on this.
>
> > >
> > >  /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */ @@ -752,10 +756,16
> > @@
> > > 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 more_vlans_exist 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.
> > >   */
> >
> > Could you please specify what the expected behavior when inner_type and
> > more_vlans_exist are not specified .
> > What is the default behavior ?
> >
>
> You wrote above for the eth item, if the user didn't specify it means don't-care.
Could you please add the same comment for the vlan pattern ?

>
> > >  struct rte_flow_item_vlan {
> > >         rte_be16_t tci; /**< Tag control information. */
> > >         rte_be16_t inner_type; /**< Inner EtherType or TPID. */
> > > +       uint32_t more_vlans_exist:1;
> > > +       /**< At least one more VLAN exist in header, following this VLAN. */
> > > +       uint32_t reserved:31; /**< Reserved, must be zero. */
> > >  };
> > >
> > >  /** Default mask for RTE_FLOW_ITEM_TYPE_VLAN. */
> > > --
> > > 1.8.3.1
> > >
> >
> > I am still wondering, why not using a new item 'NOT' for example to match
> > only eth packet not tagged ?
> > example: eth / not vlan. It's a more generic solution.
> >
> > Here in this commit, we add a reference on VLAN fields on ethernet header.
> > But tomorrow, we could do the same for mpls by adding mpls_exists in the
> > eth item and so on.
> >
> > In fact, we  have the same needs for IPv6 options. To match for example,
> > ipv6 packet with no fragment option.
> > With a NOT field, it can be easily done: > eth / ipv6 / no ipv6_frag.
> >
> > Adding new fields 'item'_exists into eth and ipv6 do the jobs, but having a
> > NOT attribute is a more generic solution.
> >
> > It could address many other use cases like matching any udp packets that are
> > not vxlan ( eth / ipv4 / vxlan / not udp),
> >
> > Let me know what you think about that.
>
> I agree with Thomas Monjalon response on this.

RTE_FLOW pattern is here to have a generic way to describe a flow.

Of course, hardware nics don't need to support any type of pattern.
It's why we have rte_flow_validate functions to be sure that the
hardware can match this type of pattern.

For example, the not attribute could be only supported for vlan item
with mlx5 nics. (i.e. eth / not vlan).

When a user adds a flow with a pattern including a not attribute, if
the pmd doesn't support it, it should return -ENOTSUP.

Later, if we add support of not attribute with mpls (i.e. eth / not
mpls) in mlx5 pmd, modification can be done on the pmd side, without
any API changes.

You are already adding new '_exits' fields in IPv6 item. It's why I
think having a generic solution like a NOT attribute, it's a better
solution.

If we continue to add '_exists' fields in each item (like you are
doing with IPv6 item), I think we will need to do an API rework to
have a generic solution.

Regards,

Maxime