[RFC,v2] ethdev: add VLAN attributes to ETH and VLAN items

  In existing code the match on tagged/untagged packets is not explicit.
Recent documentation update [1] describes the different patterns and
clarifies the intended use of different patterns.

This patch proposes an update to ETH and VLAN items struct, to clearly
define the required characteristic of a packet, and enable precise
match criteria.

[1] https://mails.dpdk.org/archives/dev/2020-May/166257.html

Signed-off-by: Dekel Peled <dekelp@mellanox.com>
---
 lib/librte_ethdev/rte_flow.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
  

Hi Dekel,

On Thu, Aug 6, 2020 at 12:40 PM Dekel Peled <dekelp@mellanox.com> wrote:
>
> In existing code the match on tagged/untagged packets is not explicit.
> Recent documentation update [1] describes the different patterns and
> clarifies the intended use of different patterns.
>
> This patch proposes an update to ETH and VLAN items struct, to clearly
> define the required characteristic of a packet, and enable precise
> match criteria.
>
> [1] https://mails.dpdk.org/archives/dev/2020-May/166257.html

First, I still don't understand the initial change [1] done on RTE_FLOW API.
Before this change, it was possible to match any packets with or
without vlan encapsulations.
At least, it's not anymore possible the case with the mlx5 pmd since
this change.

For example, if I want to match any ssh packets whatever if it's
encapsulated with no vlan or N vlan headers:
testpmd> flow create 0 ingress  pattern  eth type spec 0 type mask 0 /
ipv4 / udp dst is 22  / end actions  mark id 2 / queue index 0 / end

By setting the ethernet type mask to 0x0, it means that ethernet type
should be ignored. It means if ethernet type is 0x800 (i.e. ipv4) or
0x8100 (i.e. vlan) or 0x88A8 (qinq), the packet should be matched.

Why is it not anymore supported to create a rule matching any ip
packets (with/without vlan header) ?
How is RFC handling this issue ?

>
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> ---
>  lib/librte_ethdev/rte_flow.h | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index cf0eccb..0e0b8d4 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. */
>  };

For vlan_exists=1, it can already be achieved with the following follow:
testpmd>  flow  create 0 ingress  pattern eth type spec 0x8100 type
mask 0xFFFF / vlan inner_type is 0x800 mask is 0xFFFF / end actions
mark id 2 / queue index 0 / end

For vlan_exists=0, first you can match ipv4 packets without vlan
header like that:
testpmd> flow create 0 ingress  pattern  eth type spec 0x800 type mask
0xffff / ipv4 / end actions  mark id 2 / queue index
0 / end

For matching ethernet packet without any vlan header, it's not
possible today with RTE_FLOW API.
But instead of adding a new field each structure for next fields, I
think we should introduce an attribute 'NOT' in the rte_flow API.

For example, we could add this flow in test pmd like that:
testpmd>  flow  create 0 ingress  pattern eth /  not vlan / end
actions  mark id 2 / queue index 0 / end

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

This can already be achieved with the following RTE_FLOW flows:
> flow create 0 ingress  pattern  eth  / vlan inner_type spec is 0x0 mask is 0x0 / ipv4  / end actions  mark id 2 / queue index 0 / end

By setting inner_type mask to 0, it means that we don't care about
inner_type, so inner_type can be any value. Thus it can be 0x8100 for
vlan or 0x800 for ipv4. At the end, this rule matches any ipv4 packets
with at least one vlan header.

Having more_vlan_exist in rte_flow_item_vlan is not useful.

Regards,

Maxime Leroy

>
>  /** Default mask for RTE_FLOW_ITEM_TYPE_VLAN. */
> --
> 1.8.3.1
>
  

Hi Maxime

From: Maxime Leroy:
> Hi Dekel,
> 
> On Thu, Aug 6, 2020 at 12:40 PM Dekel Peled <dekelp@mellanox.com>
> wrote:
> >
> > In existing code the match on tagged/untagged packets is not explicit.
> > Recent documentation update [1] describes the different patterns and
> > clarifies the intended use of different patterns.
> >
> > This patch proposes an update to ETH and VLAN items struct, 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-
> May%2F166257.html&amp;data=02%7C01%
> >
> 7Cmatan%40nvidia.com%7C380bfd614a574a3550e108d853d9f535%7C43083d
> 157273
> >
> 40c1b7db39efd9ccc17a%7C0%7C0%7C637351542877842049&amp;sdata=Sy0q
> isaTIw
> > ypjkF0dU2FoOwvlHBY%2FQ5SSNRaVk4jIIQ%3D&amp;reserved=0
> 
> First, I still don't understand the initial change [1] done on RTE_FLOW API.
> Before this change, it was possible to match any packets with or without vlan
> encapsulations.
> At least, it's not anymore possible the case with the mlx5 pmd since this
> change.

We are going with the all explicit approach.
User should say exactly what he want\doesn't want\doesn't-care of vlan attributes.
Previously, the user had no way to say "I want IPv4 but not with vlan".

> 
> For example, if I want to match any ssh packets whatever if it's encapsulated
> with no vlan or N vlan headers:
> testpmd> flow create 0 ingress  pattern  eth type spec 0 type mask 0 /
> ipv4 / udp dst is 22  / end actions  mark id 2 / queue index 0 / end
> 
> By setting the ethernet type mask to 0x0, it means that ethernet type should
> be ignored. It means if ethernet type is 0x800 (i.e. ipv4) or
> 0x8100 (i.e. vlan) or 0x88A8 (qinq), the packet should be matched.
> 

But you cannot say I don't want to match ethernet type=0x8100\0x88A8.

> Why is it not anymore supported to create a rule matching any ip packets
> (with/without vlan header) ?
> How is RFC handling this issue ?

In the current proposal you can match on all - including the don't-care option you mentioned above.

The new bit in item_eth allow you to say all options (yes - v 1 m 1,no v 0 m 1,don't-care v0\1 m 0) for the first vlan
Any more vlan item after can say the same on the next vlans by the new bit it item_vlan more_vlans_exist.

The default case will be don't care.

Matan

> 
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > ---
> >  lib/librte_ethdev/rte_flow.h | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index cf0eccb..0e0b8d4 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. */
> >  };
> 
> For vlan_exists=1, it can already be achieved with the following follow:
> testpmd>  flow  create 0 ingress  pattern eth type spec 0x8100 type
> mask 0xFFFF / vlan inner_type is 0x800 mask is 0xFFFF / end actions mark id 2
> / queue index 0 / end
> 
> For vlan_exists=0, first you can match ipv4 packets without vlan header like
> that:
> testpmd> flow create 0 ingress  pattern  eth type spec 0x800 type mask
> 0xffff / ipv4 / end actions  mark id 2 / queue index
> 0 / end
> 
> For matching ethernet packet without any vlan header, it's not possible
> today with RTE_FLOW API.
> But instead of adding a new field each structure for next fields, I think we
> should introduce an attribute 'NOT' in the rte_flow API.
> 
> For example, we could add this flow in test pmd like that:
> testpmd>  flow  create 0 ingress  pattern eth /  not vlan / end
> actions  mark id 2 / queue index 0 / end
> 
> >
> >  /** 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.
> >   */
> >  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. */
> >  };
> 
> This can already be achieved with the following RTE_FLOW flows:
> > flow create 0 ingress  pattern  eth  / vlan inner_type spec is 0x0
> > mask is 0x0 / ipv4  / end actions  mark id 2 / queue index 0 / end
> 
> By setting inner_type mask to 0, it means that we don't care about
> inner_type, so inner_type can be any value. Thus it can be 0x8100 for vlan or
> 0x800 for ipv4. At the end, this rule matches any ipv4 packets with at least
> one vlan header.
> 
> Having more_vlan_exist in rte_flow_item_vlan is not useful.
> 
> Regards,
> 
> Maxime Leroy
> 
> >
> >  /** Default mask for RTE_FLOW_ITEM_TYPE_VLAN. */
> > --
> > 1.8.3.1
> >