[RFC] ethdev: add VLAN attributes to ETH item

  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 item struct, to clearly define the
required characteristic of a packet, and enable precise match criteria.

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

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

On 8/4/2020 6:36 PM, Dekel Peled 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 item struct, to clearly define the
> required characteristic of a packet, and enable precise match criteria.
>
> [1] http://mails.dpdk.org/archives/dev/2020-May/166257.html
>
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> ---
>   lib/librte_ethdev/rte_flow.h | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index cf0eccb..345feb5 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -726,11 +726,20 @@ struct rte_flow_item_raw {
>    * 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 fields @p cvlan_exist and @p svlan_exist can be used to match specific
> + * packet types, instead of using the @p type field. This can be used to match
> + * on packets that do/don't contain either cvlan, svlan, or both.
> + * The field @p num_of_vlans can be used to match packets by the exact number
> + * of VLANs in header.
>    */
>   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 cvlan_exist:1; /**< C-tag VLAN exist in header. */
> +	uint32_t svlan_exist:1; /**< S-tag VLAN exist in header. */
> +	uint32_t reserved:14; /**< Reserved, must be zero. */
> +	uint32_t num_of_vlans:16; /**< Number of VLANs in header. */
We can deduct from num_of_vlans the values of cvlan_exist/svlan_exist, 
so those are redundant fields. Keeping them introduce a conflicting 
match. For example num_of_vlans=0 and cvlan_exist=1.
>   };
>   
>   /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
  

On Tue,  4 Aug 2020 18:36:20 +0300
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 item struct, to clearly define the
> required characteristic of a packet, and enable precise match criteria.
> 
> [1] http://mails.dpdk.org/archives/dev/2020-May/166257.html
> 
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> ---
>  lib/librte_ethdev/rte_flow.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index cf0eccb..345feb5 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -726,11 +726,20 @@ struct rte_flow_item_raw {
>   * 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 fields @p cvlan_exist and @p svlan_exist can be used to match specific
> + * packet types, instead of using the @p type field. This can be used to match
> + * on packets that do/don't contain either cvlan, svlan, or both.
> + * The field @p num_of_vlans can be used to match packets by the exact number
> + * of VLANs in header.
>   */
>  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 cvlan_exist:1; /**< C-tag VLAN exist in header. */
> +	uint32_t svlan_exist:1; /**< S-tag VLAN exist in header. */
> +	uint32_t reserved:14; /**< Reserved, must be zero. */
> +	uint32_t num_of_vlans:16; /**< Number of VLANs in header. */
>  };
>  
>  /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */

You are making life easy for you but harder for existing
users of flow_item_eth API.

Why not add new flow_item for vlan and handle multiple levels there.
  

Thanks, PSB.

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, August 4, 2020 7:22 PM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: ferruh.yigit@intel.com; arybchenko@solarflare.com; Ori Kam
> <orika@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>; Asaf
> Penso <asafp@mellanox.com>; Matan Azrad <matan@mellanox.com>; Eli
> Britstein <elibr@mellanox.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
> 
> On Tue,  4 Aug 2020 18:36:20 +0300
> 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 item struct, to clearly define
> > the required characteristic of a packet, and enable precise match criteria.
> >
> > [1]
> >
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails
> > .dpdk.org%2Farchives%2Fdev%2F2020-
> May%2F166257.html&amp;data=02%7C01%7
> >
> Cdekelp%40mellanox.com%7Cc1ca8148f6ad42ad532708d838928c72%7Ca6529
> 71c7d
> >
> 2e4d9ba6a4d149256f461b%7C0%7C0%7C637321549359549952&amp;sdata=W
> kklrEiL
> > KzWsd4UJCt5MKlWw40qIhiVn3vUPEjzE%2BNo%3D&amp;reserved=0
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > ---
> >  lib/librte_ethdev/rte_flow.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index cf0eccb..345feb5 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -726,11 +726,20 @@ struct rte_flow_item_raw {
> >   * 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 fields @p cvlan_exist and @p svlan_exist can be used to match
> > + specific
> > + * packet types, instead of using the @p type field. This can be used
> > + to match
> > + * on packets that do/don't contain either cvlan, svlan, or both.
> > + * The field @p num_of_vlans can be used to match packets by the
> > + exact number
> > + * of VLANs in header.
> >   */
> >  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 cvlan_exist:1; /**< C-tag VLAN exist in header. */
> > +	uint32_t svlan_exist:1; /**< S-tag VLAN exist in header. */
> > +	uint32_t reserved:14; /**< Reserved, must be zero. */
> > +	uint32_t num_of_vlans:16; /**< Number of VLANs in header. */
> >  };
> >
> >  /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
> 
> You are making life easy for you but harder for existing users of
> flow_item_eth API.
> 
I think new and existing users of this item can benefit from the new fields.

> Why not add new flow_item for vlan and handle multiple levels there.
There is already rte_flow_item_vlan available.
The proposed update is intended to clarify the use of this item.
For example pattern 'eth / ipv4 / end' can be interpreted as no-vlan, or as any-vlan.
The pattern 'eth / vlan / ipv4 / end' can be interpreted as one-vlan, or many-vlans.
The proposed update will allow specific match on vlan attributes of packet, without ambiguity.

Regards,
Dekel
  

Thanks, PSB.

> -----Original Message-----
> From: Eli Britstein <elibr@mellanox.com>
> Sent: Tuesday, August 4, 2020 6:47 PM
> To: Dekel Peled <dekelp@mellanox.com>; ferruh.yigit@intel.com;
> arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> Monjalon <thomas@monjalon.net>
> Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> <matan@mellanox.com>; dev@dpdk.org
> Subject: Re: [RFC] ethdev: add VLAN attributes to ETH item
> 
> 
> On 8/4/2020 6:36 PM, Dekel Peled 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 item struct, to clearly define
> > the required characteristic of a packet, and enable precise match criteria.
> >
> > [1] http://mails.dpdk.org/archives/dev/2020-May/166257.html
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > ---
> >   lib/librte_ethdev/rte_flow.h | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index cf0eccb..345feb5 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -726,11 +726,20 @@ struct rte_flow_item_raw {
> >    * 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 fields @p cvlan_exist and @p svlan_exist can be used to match
> > + specific
> > + * packet types, instead of using the @p type field. This can be used
> > + to match
> > + * on packets that do/don't contain either cvlan, svlan, or both.
> > + * The field @p num_of_vlans can be used to match packets by the
> > + exact number
> > + * of VLANs in header.
> >    */
> >   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 cvlan_exist:1; /**< C-tag VLAN exist in header. */
> > +	uint32_t svlan_exist:1; /**< S-tag VLAN exist in header. */
> > +	uint32_t reserved:14; /**< Reserved, must be zero. */
> > +	uint32_t num_of_vlans:16; /**< Number of VLANs in header. */
> We can deduct from num_of_vlans the values of cvlan_exist/svlan_exist, so
> those are redundant fields. Keeping them introduce a conflicting match. For
> example num_of_vlans=0 and cvlan_exist=1.

Such conflict is simple to validate and reject.
Even if num_of_vlans is removed, we can still get conflict svlan_exist=1, cvlan_exist=0.
The different fields are proposed to allow flexible match on different VLAN attributes.
Every PMD can choose to support any or none of them.

> >   };
> >
> >   /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
  

But if the user want to force only one vlan and don't care about others?


> -----Original Message-----
> From: Dekel Peled <dekelp@mellanox.com>
> Sent: Wednesday, August 5, 2020 9:54 AM
> To: Eli Britstein <elibr@mellanox.com>; ferruh.yigit@intel.com;
> arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> Monjalon <thomas@monjalon.net>
> Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> <matan@mellanox.com>; dev@dpdk.org
> Subject: RE: [RFC] ethdev: add VLAN attributes to ETH item
> 
> Thanks, PSB.
> 
> > -----Original Message-----
> > From: Eli Britstein <elibr@mellanox.com>
> > Sent: Tuesday, August 4, 2020 6:47 PM
> > To: Dekel Peled <dekelp@mellanox.com>; ferruh.yigit@intel.com;
> > arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> > Monjalon <thomas@monjalon.net>
> > Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> <matan@mellanox.com>;
> > dev@dpdk.org
> > Subject: Re: [RFC] ethdev: add VLAN attributes to ETH item
> >
> >
> > On 8/4/2020 6:36 PM, Dekel Peled 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 item struct, to clearly define
> > > the required characteristic of a packet, and enable precise match criteria.
> > >
> > > [1] http://mails.dpdk.org/archives/dev/2020-May/166257.html
> > >
> > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > ---
> > >   lib/librte_ethdev/rte_flow.h | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > b/lib/librte_ethdev/rte_flow.h index cf0eccb..345feb5 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -726,11 +726,20 @@ struct rte_flow_item_raw {
> > >    * 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 fields @p cvlan_exist and @p svlan_exist can be used to
> > > + match specific
> > > + * packet types, instead of using the @p type field. This can be
> > > + used to match
> > > + * on packets that do/don't contain either cvlan, svlan, or both.
> > > + * The field @p num_of_vlans can be used to match packets by the
> > > + exact number
> > > + * of VLANs in header.
> > >    */
> > >   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 cvlan_exist:1; /**< C-tag VLAN exist in header. */
> > > +	uint32_t svlan_exist:1; /**< S-tag VLAN exist in header. */
> > > +	uint32_t reserved:14; /**< Reserved, must be zero. */
> > > +	uint32_t num_of_vlans:16; /**< Number of VLANs in header. */
> > We can deduct from num_of_vlans the values of cvlan_exist/svlan_exist,
> > so those are redundant fields. Keeping them introduce a conflicting
> > match. For example num_of_vlans=0 and cvlan_exist=1.
> 
> Such conflict is simple to validate and reject.
> Even if num_of_vlans is removed, we can still get conflict svlan_exist=1,
> cvlan_exist=0.
> The different fields are proposed to allow flexible match on different VLAN
> attributes.
> Every PMD can choose to support any or none of them.
> 
> > >   };
> > >
> > >   /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
  

Hi Dekel,

First, I 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 the case with the mlx5 pmd.

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 if you wanted to match only ethernet packets (and not vlan/qinq
one), you can create the following flows:
testpmd> flow create 0 ingress  pattern  eth type spec 0x800 type mask
0xffff / ipv4 / udp dst is 22  / end actions  mark id 2 / queue index
0 / end

With your new RFC, first I don't understand the needs of the num_of_vlans field.

You can create the following follow if you want to match any qinq /
ipv4 packets (i.e. 2 vlan level) for example:
> flow create 0 ingress  pattern  eth type spec 0x88A8 type mask 0xffff / vlan inner_type spec is 0x8100 mask is 0xFFFF / vlan  / end actions  mark id 2 / queue index 0 / end

Could you explain to me the utility of this new field ?

The cvlan_exist, and svlan_exist seems useless for me. For me, you can
already do the same thing with type field. Because, by setting the
type mask to 0, you can already give the notion of any ethertype.

Let's take some example:

1. with svlan_exists=0, cvlan_exists=0, it can already be configured like that:
> flow  create 0 ingress  pattern  eth type spec 0x800 type mask 0xFFFF / ipv4 / end actions  mark id 2 / queue index 0 / end

2. With svlan_exists=0, cvlan_exists=1:
> flow  create 0 ingress  pattern eth type spec 0x8100 type mask 0xFFFF / vlan inner_type is 0x800 mask is 0xFFFF / ipv4 / end actions  mark id 2 / queue index 0 / end

3. With svlan_exists=1, cvlan_exists=0: it doesn't seem to be a real
use case. Anyway, you could have:
> flow create 0 ingress  pattern  eth type spec 0x88A8 type mask 0xffff / vlan inner_type spec is 0x800 mask is 0xFFFF / ipv4  / end actions  mark id 2 / queue index 0 / end

4. With svlan_exists=1, cvlan_exists=1:
 > flow create 0 ingress  pattern  eth type spec 0x88A8 type mask
0xffff / vlan inner_type spec is 0x8100 mask is 0xFFFF / vlan spec is
0x800 mask 0xFFFF / ipv4 / end actions  mark id 2 / queue index 0 /
end

Could you please explain to me what you try to achieve with this RFC ?

I would like to know why ether_type value setted by the user is
ignored when I create the following rule:
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
with the mlx5 pmd ? (i.e. why this change [1].)

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

Best Regards,

Maxime

On Wed, Aug 5, 2020 at 9:04 AM Matan Azrad <matan@mellanox.com> wrote:
>
> But if the user want to force only one vlan and don't care about others?
>
>
> > -----Original Message-----
> > From: Dekel Peled <dekelp@mellanox.com>
> > Sent: Wednesday, August 5, 2020 9:54 AM
> > To: Eli Britstein <elibr@mellanox.com>; ferruh.yigit@intel.com;
> > arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> > Monjalon <thomas@monjalon.net>
> > Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> > <matan@mellanox.com>; dev@dpdk.org
> > Subject: RE: [RFC] ethdev: add VLAN attributes to ETH item
> >
> > Thanks, PSB.
> >
> > > -----Original Message-----
> > > From: Eli Britstein <elibr@mellanox.com>
> > > Sent: Tuesday, August 4, 2020 6:47 PM
> > > To: Dekel Peled <dekelp@mellanox.com>; ferruh.yigit@intel.com;
> > > arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> > > Monjalon <thomas@monjalon.net>
> > > Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> > <matan@mellanox.com>;
> > > dev@dpdk.org
> > > Subject: Re: [RFC] ethdev: add VLAN attributes to ETH item
> > >
> > >
> > > On 8/4/2020 6:36 PM, Dekel Peled 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 item struct, to clearly define
> > > > the required characteristic of a packet, and enable precise match criteria.
> > > >
> > > > [1] http://mails.dpdk.org/archives/dev/2020-May/166257.html
> > > >
> > > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > > ---
> > > >   lib/librte_ethdev/rte_flow.h | 9 +++++++++
> > > >   1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > b/lib/librte_ethdev/rte_flow.h index cf0eccb..345feb5 100644
> > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > @@ -726,11 +726,20 @@ struct rte_flow_item_raw {
> > > >    * 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 fields @p cvlan_exist and @p svlan_exist can be used to
> > > > + match specific
> > > > + * packet types, instead of using the @p type field. This can be
> > > > + used to match
> > > > + * on packets that do/don't contain either cvlan, svlan, or both.
> > > > + * The field @p num_of_vlans can be used to match packets by the
> > > > + exact number
> > > > + * of VLANs in header.
> > > >    */
> > > >   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 cvlan_exist:1; /**< C-tag VLAN exist in header. */
> > > > + uint32_t svlan_exist:1; /**< S-tag VLAN exist in header. */
> > > > + uint32_t reserved:14; /**< Reserved, must be zero. */
> > > > + uint32_t num_of_vlans:16; /**< Number of VLANs in header. */
> > > We can deduct from num_of_vlans the values of cvlan_exist/svlan_exist,
> > > so those are redundant fields. Keeping them introduce a conflicting
> > > match. For example num_of_vlans=0 and cvlan_exist=1.
> >
> > Such conflict is simple to validate and reject.
> > Even if num_of_vlans is removed, we can still get conflict svlan_exist=1,
> > cvlan_exist=0.
> > The different fields are proposed to allow flexible match on different VLAN
> > attributes.
> > Every PMD can choose to support any or none of them.
> >
> > > >   };
> > > >
> > > >   /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
  

Thanks, PSB.

> -----Original Message-----
> From: Maxime Leroy <maxime.leroy@6wind.com>
> Sent: Monday, September 7, 2020 7:13 PM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: Eli Britstein <elibr@mellanox.com>; ferruh.yigit@intel.com;
> arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; NBU-Contact-
> Thomas Monjalon <thomas@monjalon.net>; Asaf Penso
> <asafp@mellanox.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
> 
> Hi Dekel,
> 
> First, I don't understand the initial change [1] done on RTE_FLOW API.

As [1] commit log specifies, it is meant to clarify the required pattern to use, and reduce ambiguity.

> Before this change, it was possible to match any packets with or without vlan
> encapsulations.
> At least, it's not anymore the case with the mlx5 pmd.
> 
> 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 if you wanted to match only ethernet packets (and not vlan/qinq one),
> you can create the following flows:
> testpmd> flow create 0 ingress  pattern  eth type spec 0x800 type mask
> 0xffff / ipv4 / udp dst is 22  / end actions  mark id 2 / queue index
> 0 / end
> 
> With your new RFC, first I don't understand the needs of the num_of_vlans
> field.

Actually v2 of this RFC was sent already, please refer to https://mails.dpdk.org/archives/dev/2020-August/177536.html.

> 
> You can create the following follow if you want to match any qinq /
> ipv4 packets (i.e. 2 vlan level) for example:
> > flow create 0 ingress  pattern  eth type spec 0x88A8 type mask 0xffff
> > / vlan inner_type spec is 0x8100 mask is 0xFFFF / vlan  / end actions
> > mark id 2 / queue index 0 / end
> 
> Could you explain to me the utility of this new field ?
> 
> The cvlan_exist, and svlan_exist seems useless for me. For me, you can
> already do the same thing with type field. Because, by setting the type mask
> to 0, you can already give the notion of any ethertype.
> 
> Let's take some example:
> 
> 1. with svlan_exists=0, cvlan_exists=0, it can already be configured like that:
> > flow  create 0 ingress  pattern  eth type spec 0x800 type mask 0xFFFF
> > / ipv4 / end actions  mark id 2 / queue index 0 / end
> 
> 2. With svlan_exists=0, cvlan_exists=1:
> > flow  create 0 ingress  pattern eth type spec 0x8100 type mask 0xFFFF
> > / vlan inner_type is 0x800 mask is 0xFFFF / ipv4 / end actions  mark
> > id 2 / queue index 0 / end
> 
> 3. With svlan_exists=1, cvlan_exists=0: it doesn't seem to be a real use case.
> Anyway, you could have:
> > flow create 0 ingress  pattern  eth type spec 0x88A8 type mask 0xffff
> > / vlan inner_type spec is 0x800 mask is 0xFFFF / ipv4  / end actions
> > mark id 2 / queue index 0 / end
> 
> 4. With svlan_exists=1, cvlan_exists=1:
>  > flow create 0 ingress  pattern  eth type spec 0x88A8 type mask 0xffff / vlan
> inner_type spec is 0x8100 mask is 0xFFFF / vlan spec is
> 0x800 mask 0xFFFF / ipv4 / end actions  mark id 2 / queue index 0 / end
> 
> Could you please explain to me what you try to achieve with this RFC ?
> 
> I would like to know why ether_type value setted by the user is ignored
> when I create the following rule:
> 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 with the
> mlx5 pmd ? (i.e. why this change [1].)
> 
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails.
> dpdk.org%2Farchives%2Fdev%2F2020-
> May%2F166257.html&amp;data=02%7C01%7Cdekelp%40nvidia.com%7C7cb0
> 777ea0e841bdc19808d85348f520%7C43083d15727340c1b7db39efd9ccc17a%7
> C0%7C0%7C637350920106123048&amp;sdata=BWVQ0vSBMW2oDaGe0%2F6
> 6EsEY1z76jFVJLKKtrmTQHj0%3D&amp;reserved=0
> 
> Best Regards,
> 
> Maxime
> 
> On Wed, Aug 5, 2020 at 9:04 AM Matan Azrad <matan@mellanox.com>
> wrote:
> >
> > But if the user want to force only one vlan and don't care about others?
> >
> >
> > > -----Original Message-----
> > > From: Dekel Peled <dekelp@mellanox.com>
> > > Sent: Wednesday, August 5, 2020 9:54 AM
> > > To: Eli Britstein <elibr@mellanox.com>; ferruh.yigit@intel.com;
> > > arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> > > Monjalon <thomas@monjalon.net>
> > > Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> > > <matan@mellanox.com>; dev@dpdk.org
> > > Subject: RE: [RFC] ethdev: add VLAN attributes to ETH item
> > >
> > > Thanks, PSB.
> > >
> > > > -----Original Message-----
> > > > From: Eli Britstein <elibr@mellanox.com>
> > > > Sent: Tuesday, August 4, 2020 6:47 PM
> > > > To: Dekel Peled <dekelp@mellanox.com>; ferruh.yigit@intel.com;
> > > > arybchenko@solarflare.com; Ori Kam <orika@mellanox.com>; Thomas
> > > > Monjalon <thomas@monjalon.net>
> > > > Cc: Asaf Penso <asafp@mellanox.com>; Matan Azrad
> > > <matan@mellanox.com>;
> > > > dev@dpdk.org
> > > > Subject: Re: [RFC] ethdev: add VLAN attributes to ETH item
> > > >
> > > >
> > > > On 8/4/2020 6:36 PM, Dekel Peled 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 item struct, to clearly
> > > > > define the required characteristic of a packet, and enable precise
> match criteria.
> > > > >
> > > > > [1]
> > > > >
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2
> > > > > Fmails.dpdk.org%2Farchives%2Fdev%2F2020-
> May%2F166257.html&amp;da
> > > > >
> ta=02%7C01%7Cdekelp%40nvidia.com%7C7cb0777ea0e841bdc19808d85348f
> > > > >
> 520%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637350920106123
> > > > >
> 048&amp;sdata=BWVQ0vSBMW2oDaGe0%2F66EsEY1z76jFVJLKKtrmTQHj0%
> 3D&a
> > > > > mp;reserved=0
> > > > >
> > > > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > > > ---
> > > > >   lib/librte_ethdev/rte_flow.h | 9 +++++++++
> > > > >   1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > > > b/lib/librte_ethdev/rte_flow.h index cf0eccb..345feb5 100644
> > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > @@ -726,11 +726,20 @@ struct rte_flow_item_raw {
> > > > >    * 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 fields @p cvlan_exist and @p svlan_exist can be used to
> > > > > + match specific
> > > > > + * packet types, instead of using the @p type field. This can
> > > > > + be used to match
> > > > > + * on packets that do/don't contain either cvlan, svlan, or both.
> > > > > + * The field @p num_of_vlans can be used to match packets by
> > > > > + the exact number
> > > > > + * of VLANs in header.
> > > > >    */
> > > > >   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 cvlan_exist:1; /**< C-tag VLAN exist in header. */
> > > > > + uint32_t svlan_exist:1; /**< S-tag VLAN exist in header. */
> > > > > + uint32_t reserved:14; /**< Reserved, must be zero. */ uint32_t
> > > > > + num_of_vlans:16; /**< Number of VLANs in header. */
> > > > We can deduct from num_of_vlans the values of
> > > > cvlan_exist/svlan_exist, so those are redundant fields. Keeping
> > > > them introduce a conflicting match. For example num_of_vlans=0 and
> cvlan_exist=1.
> > >
> > > Such conflict is simple to validate and reject.
> > > Even if num_of_vlans is removed, we can still get conflict
> > > svlan_exist=1, cvlan_exist=0.
> > > The different fields are proposed to allow flexible match on
> > > different VLAN attributes.
> > > Every PMD can choose to support any or none of them.
> > >
> > > > >   };
> > > > >
> > > > >   /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */