DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
@ 2020-08-04 15:36 Dekel Peled
  2020-08-04 15:47 ` Eli Britstein
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Dekel Peled @ 2020-08-04 15:36 UTC (permalink / raw)
  To: ferruh.yigit, arybchenko, orika, thomas; +Cc: asafp, matan, elibr, dev

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. */
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
  2020-08-04 15:36 [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item Dekel Peled
@ 2020-08-04 15:47 ` Eli Britstein
  2020-08-05  6:53   ` Dekel Peled
  2020-08-04 16:22 ` Stephen Hemminger
  2020-08-06 10:36 ` [dpdk-dev] [RFC v2] ethdev: add VLAN attributes to ETH and VLAN items Dekel Peled
  2 siblings, 1 reply; 11+ messages in thread
From: Eli Britstein @ 2020-08-04 15:47 UTC (permalink / raw)
  To: Dekel Peled, ferruh.yigit, arybchenko, orika, thomas; +Cc: asafp, matan, dev


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
  2020-08-04 15:36 [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item Dekel Peled
  2020-08-04 15:47 ` Eli Britstein
@ 2020-08-04 16:22 ` Stephen Hemminger
  2020-08-05  6:48   ` Dekel Peled
  2020-08-06 10:36 ` [dpdk-dev] [RFC v2] ethdev: add VLAN attributes to ETH and VLAN items Dekel Peled
  2 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2020-08-04 16:22 UTC (permalink / raw)
  To: Dekel Peled
  Cc: ferruh.yigit, arybchenko, orika, thomas, asafp, matan, elibr, dev

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
  2020-08-04 16:22 ` Stephen Hemminger
@ 2020-08-05  6:48   ` Dekel Peled
  0 siblings, 0 replies; 11+ messages in thread
From: Dekel Peled @ 2020-08-05  6:48 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: ferruh.yigit, arybchenko, Ori Kam, Thomas Monjalon, Asaf Penso,
	Matan Azrad, Eli Britstein, dev

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
  2020-08-04 15:47 ` Eli Britstein
@ 2020-08-05  6:53   ` Dekel Peled
  2020-08-05  7:04     ` Matan Azrad
  0 siblings, 1 reply; 11+ messages in thread
From: Dekel Peled @ 2020-08-05  6:53 UTC (permalink / raw)
  To: Eli Britstein, ferruh.yigit, arybchenko, Ori Kam, Thomas Monjalon
  Cc: Asaf Penso, Matan Azrad, dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
  2020-08-05  6:53   ` Dekel Peled
@ 2020-08-05  7:04     ` Matan Azrad
  2020-09-07 16:13       ` Maxime Leroy
  0 siblings, 1 reply; 11+ messages in thread
From: Matan Azrad @ 2020-08-05  7:04 UTC (permalink / raw)
  To: Dekel Peled, Eli Britstein, ferruh.yigit, arybchenko, Ori Kam,
	Thomas Monjalon
  Cc: Asaf Penso, dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [RFC v2] ethdev: add VLAN attributes to ETH and VLAN items
  2020-08-04 15:36 [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item Dekel Peled
  2020-08-04 15:47 ` Eli Britstein
  2020-08-04 16:22 ` Stephen Hemminger
@ 2020-08-06 10:36 ` Dekel Peled
  2020-09-08  9:30   ` Maxime Leroy
  2 siblings, 1 reply; 11+ messages in thread
From: Dekel Peled @ 2020-08-06 10:36 UTC (permalink / raw)
  To: ferruh.yigit, arybchenko, orika, thomas; +Cc: asafp, matan, elibr, dev

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

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. */
 };
 
 /** 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. */
 };
 
 /** Default mask for RTE_FLOW_ITEM_TYPE_VLAN. */
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
  2020-08-05  7:04     ` Matan Azrad
@ 2020-09-07 16:13       ` Maxime Leroy
  2020-09-07 18:21         ` Dekel Peled
  0 siblings, 1 reply; 11+ messages in thread
From: Maxime Leroy @ 2020-09-07 16:13 UTC (permalink / raw)
  To: Dekel Peled
  Cc: Eli Britstein, ferruh.yigit, arybchenko, Ori Kam,
	Thomas Monjalon, Asaf Penso, dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item
  2020-09-07 16:13       ` Maxime Leroy
@ 2020-09-07 18:21         ` Dekel Peled
  0 siblings, 0 replies; 11+ messages in thread
From: Dekel Peled @ 2020-09-07 18:21 UTC (permalink / raw)
  To: Maxime Leroy
  Cc: Eli Britstein, ferruh.yigit, arybchenko, Ori Kam,
	NBU-Contact-Thomas Monjalon, Asaf Penso, dev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC v2] ethdev: add VLAN attributes to ETH and VLAN items
  2020-08-06 10:36 ` [dpdk-dev] [RFC v2] ethdev: add VLAN attributes to ETH and VLAN items Dekel Peled
@ 2020-09-08  9:30   ` Maxime Leroy
  2020-09-30 15:59     ` Matan Azrad
  0 siblings, 1 reply; 11+ messages in thread
From: Maxime Leroy @ 2020-09-08  9:30 UTC (permalink / raw)
  To: Dekel Peled
  Cc: ferruh.yigit, arybchenko, Ori Kam, Thomas Monjalon, Asaf Penso,
	matan, Eli Britstein, dev

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC v2] ethdev: add VLAN attributes to ETH and VLAN items
  2020-09-08  9:30   ` Maxime Leroy
@ 2020-09-30 15:59     ` Matan Azrad
  0 siblings, 0 replies; 11+ messages in thread
From: Matan Azrad @ 2020-09-30 15:59 UTC (permalink / raw)
  To: Maxime Leroy, Dekel Peled
  Cc: ferruh.yigit, arybchenko, Ori Kam, NBU-Contact-Thomas Monjalon,
	Asaf Penso, matan, Eli Britstein, dev


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-09-30 15:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-04 15:36 [dpdk-dev] [RFC] ethdev: add VLAN attributes to ETH item Dekel Peled
2020-08-04 15:47 ` Eli Britstein
2020-08-05  6:53   ` Dekel Peled
2020-08-05  7:04     ` Matan Azrad
2020-09-07 16:13       ` Maxime Leroy
2020-09-07 18:21         ` Dekel Peled
2020-08-04 16:22 ` Stephen Hemminger
2020-08-05  6:48   ` Dekel Peled
2020-08-06 10:36 ` [dpdk-dev] [RFC v2] ethdev: add VLAN attributes to ETH and VLAN items Dekel Peled
2020-09-08  9:30   ` Maxime Leroy
2020-09-30 15:59     ` Matan Azrad

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).