ethdev: add VLAN attributes to ETH and VLAN items

  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://mails.dpdk.org/archives/dev/2020-August/177536.html

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(-)
  

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://mails.dpdk.org/archives/dev/2020-August/177536.html
>
> 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 ?

With this new specification, I think you address all the use cases.
That's great !

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

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

Regards,

Maxime
  

02/10/2020 14:39, Maxime Leroy:
> 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.

Please add more explanations, without relying on what is in RFC.
We need to clearly understand the motivation and why
this implementation is chosen.

If you correctly thread your patch with previous ones,
it should not be needed to mention RFC.

> >
> > [1] https://mails.dpdk.org/archives/dev/2020-August/177536.html
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> 
> 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.

You're right Maxime, a NOT operator looks better for the user,
but it is expected to be very hard to implement and offload.
  

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.

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

> 
> Regards,
> 
> Maxime
  

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
  

Sorry for jumping late,


> -----Original Message-----
> From: Maxime Leroy <maxime.leroy@6wind.com>
> Sent: Wednesday, October 7, 2020 2:52 PM
> Subject: Re: [PATCH] ethdev: add VLAN attributes to ETH and VLAN items
> 
> 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

First I'm all in favor of adding a not item, but it is very tricky and should be designed very carefully.
Also using a not will get the rules to be very complicated.
For example think about the following case:
Application want only packets without any extensions, using the suggested API it is very simple
just set exits = 0 with mask = 1.
While if we use the not I'm not sure how it should look, since we need number of not,
it is not just enough to say next proto is not XXX since we need to cover all possible
extensions, also there might be ordering issue which the not can't support.

So my thinking is that we should go with the suggested approach and regardless see
how can we add the not.

In any case the exit should not be the goto solution but again in case of extension it make
sense since order is not guaranteed.

What do you think?

Best,
Ori
  

Thanks, PSB.

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Wednesday, October 7, 2020 3:14 PM
> To: Maxime Leroy <maxime.leroy@6wind.com>; Dekel Peled
> <dekelp@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>
> Cc: ferruh.yigit@intel.com; arybchenko@solarflare.com; dev@dpdk.org
> Subject: RE: [PATCH] ethdev: add VLAN attributes to ETH and VLAN items
> 
> Sorry for jumping late,

Please note v2 of this patch is in ML since Oct. 1st.
We should continue the discussion on the latest thread.
More comments below.

> 
> 
> > -----Original Message-----
> > From: Maxime Leroy <maxime.leroy@6wind.com>
> > Sent: Wednesday, October 7, 2020 2:52 PM
> > Subject: Re: [PATCH] ethdev: add VLAN attributes to ETH and VLAN items
> >
> > 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%2F
> > > > mail
> > > > > 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 ?

I will send v3 with added description.

> >
> > >
> > > > >  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
> 
> First I'm all in favor of adding a not item, but it is very tricky and should be
> designed very carefully.
> Also using a not will get the rules to be very complicated.
> For example think about the following case:
> Application want only packets without any extensions, using the suggested
> API it is very simple just set exits = 0 with mask = 1.
> While if we use the not I'm not sure how it should look, since we need
> number of not, it is not just enough to say next proto is not XXX since we
> need to cover all possible extensions, also there might be ordering issue
> which the not can't support.
> 
> So my thinking is that we should go with the suggested approach and
> regardless see how can we add the not.
> 
> In any case the exit should not be the goto solution but again in case of
> extension it make sense since order is not guaranteed.
> 
> What do you think?
> 
> Best,
> Ori
>