[RFC,1/3] ethdev: support GRE optional fields
Checks
Commit Message
Add flow pattern items and header format for matching optional fields
(checksum/key/sequence) in GRE header. And the flags in gre item should
be correspondingly set with the new added items.
Signed-off-by: Sean Zhang <xiazhang@nvidia.com>
---
doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
lib/ethdev/rte_flow.c | 1 +
lib/ethdev/rte_flow.h | 18 ++++++++++++++++++
3 files changed, 35 insertions(+)
Comments
Hi Sean,
> -----Original Message-----
> From: Sean Zhang <xiazhang@nvidia.com>
> Subject: [RFC 1/3] ethdev: support GRE optional fields
>
> Add flow pattern items and header format for matching optional fields
> (checksum/key/sequence) in GRE header. And the flags in gre item should
> be correspondingly set with the new added items.
>
> Signed-off-by: Sean Zhang <xiazhang@nvidia.com>
> ---
> doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
> lib/ethdev/rte_flow.c | 1 +
> lib/ethdev/rte_flow.h | 18 ++++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index c51ed88..48d5685 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1113,6 +1113,22 @@ This should be preceded by item ``GRE``.
> - Value to be matched is a big-endian 32 bit integer.
> - When this item present it implicitly match K bit in default mask as "1"
>
> +Item: ``GRE_OPTION``
> +^^^^^^^^^^^^^^^^^^^^
> +
> +Matches a GRE optional fields (checksum/key/sequence).
> +This should be preceded by item ``GRE``.
> +
> +- ``checksum``: checksum.
> +- ``key``: key.
> +- ``sequence``: sequence.
> +- The items in GRE_OPTION do not change bit flags(c_bit/k_bit/s_bit) in GRE
> + item. The bit flags need be set with GRE item by application. When the items
> + present, the corresponding bits in GRE spec and mask should be set "1" by
> + application, it means to match specified value of the fields. When the items
> + no present, but the corresponding bits in GRE spec and mask is "1", it means
> + to match any value of the fields.
> +
> Item: ``FUZZY``
> ^^^^^^^^^^^^^^^
>
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index a93f68a..03bd1df 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -139,6 +139,7 @@ struct rte_flow_desc_data {
> MK_FLOW_ITEM(META, sizeof(struct rte_flow_item_meta)),
> MK_FLOW_ITEM(TAG, sizeof(struct rte_flow_item_tag)),
> MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),
> + MK_FLOW_ITEM(GRE_OPTION, sizeof(struct rte_gre_hdr_option)),
I think that this new item is making the gre_key redundant,
why not deprecate it?
> MK_FLOW_ITEM(GTP_PSC, sizeof(struct rte_flow_item_gtp_psc)),
> MK_FLOW_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)),
> MK_FLOW_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)),
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 1031fb2..27b4140 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -660,6 +660,13 @@ enum rte_flow_item_type {
> * See struct rte_flow_item_ppp.
> */
> RTE_FLOW_ITEM_TYPE_PPP,
> +
> + /**
> + * Matches GRE optional fields.
> + *
> + * See struct rte_gre_hdr_option.
> + */
> + RTE_FLOW_ITEM_TYPE_GRE_OPTION,
> };
>
> /**
> @@ -1196,6 +1203,17 @@ struct rte_flow_item_gre {
> #endif
>
> /**
> + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> + *
> + * Matches GRE optional fields in header.
> + */
> +struct rte_gre_hdr_option {
> + rte_be16_t checksum;
> + rte_be32_t key;
> + rte_be32_t sequence;
> +};
> +
> +/**
> * RTE_FLOW_ITEM_TYPE_FUZZY
> *
> * Fuzzy pattern match, expect faster than default.
> --
> 1.8.3.1
Best,
Ori
Hi Ori,
> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Sunday, January 9, 2022 8:30 PM
> To: Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org
> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
>
> Hi Sean,
>
>
> > -----Original Message-----
> > From: Sean Zhang <xiazhang@nvidia.com>
> > Subject: [RFC 1/3] ethdev: support GRE optional fields
> >
> > Add flow pattern items and header format for matching optional fields
> > (checksum/key/sequence) in GRE header. And the flags in gre item
> > should be correspondingly set with the new added items.
> >
> > Signed-off-by: Sean Zhang <xiazhang@nvidia.com>
> > ---
> > doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
> > lib/ethdev/rte_flow.c | 1 +
> > lib/ethdev/rte_flow.h | 18 ++++++++++++++++++
> > 3 files changed, 35 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index c51ed88..48d5685 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -1113,6 +1113,22 @@ This should be preceded by item ``GRE``.
> > - Value to be matched is a big-endian 32 bit integer.
> > - When this item present it implicitly match K bit in default mask as "1"
> >
> > +Item: ``GRE_OPTION``
> > +^^^^^^^^^^^^^^^^^^^^
> > +
> > +Matches a GRE optional fields (checksum/key/sequence).
> > +This should be preceded by item ``GRE``.
> > +
> > +- ``checksum``: checksum.
> > +- ``key``: key.
> > +- ``sequence``: sequence.
> > +- The items in GRE_OPTION do not change bit flags(c_bit/k_bit/s_bit)
> > +in GRE
> > + item. The bit flags need be set with GRE item by application. When
> > +the items
> > + present, the corresponding bits in GRE spec and mask should be set
> > +"1" by
> > + application, it means to match specified value of the fields. When
> > +the items
> > + no present, but the corresponding bits in GRE spec and mask is "1",
> > +it means
> > + to match any value of the fields.
> > +
> > Item: ``FUZZY``
> > ^^^^^^^^^^^^^^^
> >
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> > a93f68a..03bd1df 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -139,6 +139,7 @@ struct rte_flow_desc_data {
> > MK_FLOW_ITEM(META, sizeof(struct rte_flow_item_meta)),
> > MK_FLOW_ITEM(TAG, sizeof(struct rte_flow_item_tag)),
> > MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),
> > + MK_FLOW_ITEM(GRE_OPTION, sizeof(struct rte_gre_hdr_option)),
>
> I think that this new item is making the gre_key redundant, why not
> deprecate it?
Do you mean to add description like bellow?
Item: ``GRE_KEY``
^^^^^^^^^^^^^^^
+This action is deprecated. Consider `Item: GRE_OPTION`.
>
> > MK_FLOW_ITEM(GTP_PSC, sizeof(struct rte_flow_item_gtp_psc)),
> > MK_FLOW_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)),
> > MK_FLOW_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)), diff -
> -git
> > a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index 1031fb2..27b4140
> > 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -660,6 +660,13 @@ enum rte_flow_item_type {
> > * See struct rte_flow_item_ppp.
> > */
> > RTE_FLOW_ITEM_TYPE_PPP,
> > +
> > + /**
> > + * Matches GRE optional fields.
> > + *
> > + * See struct rte_gre_hdr_option.
> > + */
> > + RTE_FLOW_ITEM_TYPE_GRE_OPTION,
> > };
> >
> > /**
> > @@ -1196,6 +1203,17 @@ struct rte_flow_item_gre { #endif
> >
> > /**
> > + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> > + *
> > + * Matches GRE optional fields in header.
> > + */
> > +struct rte_gre_hdr_option {
> > + rte_be16_t checksum;
> > + rte_be32_t key;
> > + rte_be32_t sequence;
> > +};
> > +
> > +/**
> > * RTE_FLOW_ITEM_TYPE_FUZZY
> > *
> > * Fuzzy pattern match, expect faster than default.
> > --
> > 1.8.3.1
>
> Best,
> Ori
Thanks,
Sean
Hi Sean,
> -----Original Message-----
> From: Sean Zhang (Networking SW) <xiazhang@nvidia.com>
> Sent: Tuesday, January 11, 2022 5:45 AM
> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
>
> Hi Ori,
>
> > -----Original Message-----
> > From: Ori Kam <orika@nvidia.com>
> > Sent: Sunday, January 9, 2022 8:30 PM
> > To: Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
> > <matan@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> > Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > Cc: dev@dpdk.org
> > Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
> >
> > Hi Sean,
> >
> >
> > > -----Original Message-----
> > > From: Sean Zhang <xiazhang@nvidia.com>
> > > Subject: [RFC 1/3] ethdev: support GRE optional fields
> > >
> > > Add flow pattern items and header format for matching optional fields
> > > (checksum/key/sequence) in GRE header. And the flags in gre item
> > > should be correspondingly set with the new added items.
> > >
> > > Signed-off-by: Sean Zhang <xiazhang@nvidia.com>
> > > ---
> > > doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
> > > lib/ethdev/rte_flow.c | 1 +
> > > lib/ethdev/rte_flow.h | 18 ++++++++++++++++++
> > > 3 files changed, 35 insertions(+)
> > >
> > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > > b/doc/guides/prog_guide/rte_flow.rst
> > > index c51ed88..48d5685 100644
> > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > @@ -1113,6 +1113,22 @@ This should be preceded by item ``GRE``.
> > > - Value to be matched is a big-endian 32 bit integer.
> > > - When this item present it implicitly match K bit in default mask as "1"
> > >
> > > +Item: ``GRE_OPTION``
> > > +^^^^^^^^^^^^^^^^^^^^
> > > +
> > > +Matches a GRE optional fields (checksum/key/sequence).
> > > +This should be preceded by item ``GRE``.
> > > +
> > > +- ``checksum``: checksum.
> > > +- ``key``: key.
> > > +- ``sequence``: sequence.
> > > +- The items in GRE_OPTION do not change bit flags(c_bit/k_bit/s_bit)
> > > +in GRE
> > > + item. The bit flags need be set with GRE item by application. When
> > > +the items
> > > + present, the corresponding bits in GRE spec and mask should be set
> > > +"1" by
> > > + application, it means to match specified value of the fields. When
> > > +the items
> > > + no present, but the corresponding bits in GRE spec and mask is "1",
> > > +it means
> > > + to match any value of the fields.
> > > +
> > > Item: ``FUZZY``
> > > ^^^^^^^^^^^^^^^
> > >
> > > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> > > a93f68a..03bd1df 100644
> > > --- a/lib/ethdev/rte_flow.c
> > > +++ b/lib/ethdev/rte_flow.c
> > > @@ -139,6 +139,7 @@ struct rte_flow_desc_data {
> > > MK_FLOW_ITEM(META, sizeof(struct rte_flow_item_meta)),
> > > MK_FLOW_ITEM(TAG, sizeof(struct rte_flow_item_tag)),
> > > MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),
> > > + MK_FLOW_ITEM(GRE_OPTION, sizeof(struct rte_gre_hdr_option)),
> >
> > I think that this new item is making the gre_key redundant, why not
> > deprecate it?
>
> Do you mean to add description like bellow?
>
> Item: ``GRE_KEY``
> ^^^^^^^^^^^^^^^
> +This action is deprecated. Consider `Item: GRE_OPTION`.
Yes and also add the depreciation notice in the release notes,
there is also need to see if other PMD are using the GRE_KEY.
But to be clear do not remove this support now just send notice that it should
be removed.
>
> >
> > > MK_FLOW_ITEM(GTP_PSC, sizeof(struct rte_flow_item_gtp_psc)),
> > > MK_FLOW_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)),
> > > MK_FLOW_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)), diff -
> > -git
> > > a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index 1031fb2..27b4140
> > > 100644
> > > --- a/lib/ethdev/rte_flow.h
> > > +++ b/lib/ethdev/rte_flow.h
> > > @@ -660,6 +660,13 @@ enum rte_flow_item_type {
> > > * See struct rte_flow_item_ppp.
> > > */
> > > RTE_FLOW_ITEM_TYPE_PPP,
> > > +
> > > + /**
> > > + * Matches GRE optional fields.
> > > + *
> > > + * See struct rte_gre_hdr_option.
> > > + */
> > > + RTE_FLOW_ITEM_TYPE_GRE_OPTION,
> > > };
> > >
> > > /**
> > > @@ -1196,6 +1203,17 @@ struct rte_flow_item_gre { #endif
> > >
> > > /**
> > > + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> > > + *
> > > + * Matches GRE optional fields in header.
> > > + */
> > > +struct rte_gre_hdr_option {
> > > + rte_be16_t checksum;
> > > + rte_be32_t key;
> > > + rte_be32_t sequence;
> > > +};
> > > +
> > > +/**
> > > * RTE_FLOW_ITEM_TYPE_FUZZY
> > > *
> > > * Fuzzy pattern match, expect faster than default.
> > > --
> > > 1.8.3.1
> >
> > Best,
> > Ori
>
> Thanks,
> Sean
Hi Ori,
> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Tuesday, January 11, 2022 3:24 PM
> To: Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org
> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
>
> Hi Sean,
>
> > -----Original Message-----
> > From: Sean Zhang (Networking SW) <xiazhang@nvidia.com>
> > Sent: Tuesday, January 11, 2022 5:45 AM
> > Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
> >
> > Hi Ori,
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@nvidia.com>
> > > Sent: Sunday, January 9, 2022 8:30 PM
> > > To: Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
> > > <matan@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> > > Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > > Cc: dev@dpdk.org
> > > Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
> > >
> > > Hi Sean,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Sean Zhang <xiazhang@nvidia.com>
> > > > Subject: [RFC 1/3] ethdev: support GRE optional fields
> > > >
> > > > Add flow pattern items and header format for matching optional
> > > > fields
> > > > (checksum/key/sequence) in GRE header. And the flags in gre item
> > > > should be correspondingly set with the new added items.
> > > >
> > > > Signed-off-by: Sean Zhang <xiazhang@nvidia.com>
> > > > ---
> > > > doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
> > > > lib/ethdev/rte_flow.c | 1 +
> > > > lib/ethdev/rte_flow.h | 18 ++++++++++++++++++
> > > > 3 files changed, 35 insertions(+)
> > > >
> > > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > > > b/doc/guides/prog_guide/rte_flow.rst
> > > > index c51ed88..48d5685 100644
> > > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > > @@ -1113,6 +1113,22 @@ This should be preceded by item ``GRE``.
> > > > - Value to be matched is a big-endian 32 bit integer.
> > > > - When this item present it implicitly match K bit in default mask as "1"
> > > >
> > > > +Item: ``GRE_OPTION``
> > > > +^^^^^^^^^^^^^^^^^^^^
> > > > +
> > > > +Matches a GRE optional fields (checksum/key/sequence).
> > > > +This should be preceded by item ``GRE``.
> > > > +
> > > > +- ``checksum``: checksum.
> > > > +- ``key``: key.
> > > > +- ``sequence``: sequence.
> > > > +- The items in GRE_OPTION do not change bit
> > > > +flags(c_bit/k_bit/s_bit) in GRE
> > > > + item. The bit flags need be set with GRE item by application.
> > > > +When the items
> > > > + present, the corresponding bits in GRE spec and mask should be
> > > > +set "1" by
> > > > + application, it means to match specified value of the fields.
> > > > +When the items
> > > > + no present, but the corresponding bits in GRE spec and mask is
> > > > +"1", it means
> > > > + to match any value of the fields.
> > > > +
> > > > Item: ``FUZZY``
> > > > ^^^^^^^^^^^^^^^
> > > >
> > > > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> > > > a93f68a..03bd1df 100644
> > > > --- a/lib/ethdev/rte_flow.c
> > > > +++ b/lib/ethdev/rte_flow.c
> > > > @@ -139,6 +139,7 @@ struct rte_flow_desc_data {
> > > > MK_FLOW_ITEM(META, sizeof(struct rte_flow_item_meta)),
> > > > MK_FLOW_ITEM(TAG, sizeof(struct rte_flow_item_tag)),
> > > > MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),
> > > > + MK_FLOW_ITEM(GRE_OPTION, sizeof(struct rte_gre_hdr_option)),
> > >
> > > I think that this new item is making the gre_key redundant, why not
> > > deprecate it?
> >
> > Do you mean to add description like bellow?
> >
> > Item: ``GRE_KEY``
> > ^^^^^^^^^^^^^^^
> > +This action is deprecated. Consider `Item: GRE_OPTION`.
>
> Yes and also add the depreciation notice in the release notes, there is also
> need to see if other PMD are using the GRE_KEY.
> But to be clear do not remove this support now just send notice that it
> should be removed.
Got it, and will update in v2 patch.
Thanks,
Sean
>
> >
> > >
> > > > MK_FLOW_ITEM(GTP_PSC, sizeof(struct rte_flow_item_gtp_psc)),
> > > > MK_FLOW_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)),
> > > > MK_FLOW_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)), diff -
> > > -git
> > > > a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> > > > 1031fb2..27b4140
> > > > 100644
> > > > --- a/lib/ethdev/rte_flow.h
> > > > +++ b/lib/ethdev/rte_flow.h
> > > > @@ -660,6 +660,13 @@ enum rte_flow_item_type {
> > > > * See struct rte_flow_item_ppp.
> > > > */
> > > > RTE_FLOW_ITEM_TYPE_PPP,
> > > > +
> > > > + /**
> > > > + * Matches GRE optional fields.
> > > > + *
> > > > + * See struct rte_gre_hdr_option.
> > > > + */
> > > > + RTE_FLOW_ITEM_TYPE_GRE_OPTION,
> > > > };
> > > >
> > > > /**
> > > > @@ -1196,6 +1203,17 @@ struct rte_flow_item_gre { #endif
> > > >
> > > > /**
> > > > + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> > > > + *
> > > > + * Matches GRE optional fields in header.
> > > > + */
> > > > +struct rte_gre_hdr_option {
> > > > + rte_be16_t checksum;
> > > > + rte_be32_t key;
> > > > + rte_be32_t sequence;
> > > > +};
> > > > +
> > > > +/**
> > > > * RTE_FLOW_ITEM_TYPE_FUZZY
> > > > *
> > > > * Fuzzy pattern match, expect faster than default.
> > > > --
> > > > 1.8.3.1
> > >
> > > Best,
> > > Ori
> >
> > Thanks,
> > Sean
On 12/30/2021 3:08 AM, Sean Zhang wrote:
> Add flow pattern items and header format for matching optional fields
> (checksum/key/sequence) in GRE header. And the flags in gre item should
> be correspondingly set with the new added items.
>
> Signed-off-by: Sean Zhang <xiazhang@nvidia.com>
> ---
> doc/guides/prog_guide/rte_flow.rst | 16 ++++++++++++++++
> lib/ethdev/rte_flow.c | 1 +
> lib/ethdev/rte_flow.h | 18 ++++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index c51ed88..48d5685 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1113,6 +1113,22 @@ This should be preceded by item ``GRE``.
> - Value to be matched is a big-endian 32 bit integer.
> - When this item present it implicitly match K bit in default mask as "1"
>
> +Item: ``GRE_OPTION``
> +^^^^^^^^^^^^^^^^^^^^
> +
> +Matches a GRE optional fields (checksum/key/sequence).
> +This should be preceded by item ``GRE``.
> +
> +- ``checksum``: checksum.
> +- ``key``: key.
> +- ``sequence``: sequence.
> +- The items in GRE_OPTION do not change bit flags(c_bit/k_bit/s_bit) in GRE
> + item. The bit flags need be set with GRE item by application. When the items
> + present, the corresponding bits in GRE spec and mask should be set "1" by
> + application, it means to match specified value of the fields. When the items
> + no present, but the corresponding bits in GRE spec and mask is "1", it means
> + to match any value of the fields.
> +
> Item: ``FUZZY``
> ^^^^^^^^^^^^^^^
>
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index a93f68a..03bd1df 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -139,6 +139,7 @@ struct rte_flow_desc_data {
> MK_FLOW_ITEM(META, sizeof(struct rte_flow_item_meta)),
> MK_FLOW_ITEM(TAG, sizeof(struct rte_flow_item_tag)),
> MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),
> + MK_FLOW_ITEM(GRE_OPTION, sizeof(struct rte_gre_hdr_option)),
> MK_FLOW_ITEM(GTP_PSC, sizeof(struct rte_flow_item_gtp_psc)),
> MK_FLOW_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)),
> MK_FLOW_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)),
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 1031fb2..27b4140 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -660,6 +660,13 @@ enum rte_flow_item_type {
> * See struct rte_flow_item_ppp.
> */
> RTE_FLOW_ITEM_TYPE_PPP,
> +
> + /**
> + * Matches GRE optional fields.
> + *
> + * See struct rte_gre_hdr_option.
> + */
> + RTE_FLOW_ITEM_TYPE_GRE_OPTION,
> };
>
> /**
> @@ -1196,6 +1203,17 @@ struct rte_flow_item_gre {
> #endif
>
> /**
> + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> + *
> + * Matches GRE optional fields in header.
> + */
> +struct rte_gre_hdr_option {
> + rte_be16_t checksum;
> + rte_be32_t key;
> + rte_be32_t sequence;
> +};
> +
Hi Ori, Andrew,
The decision was to have protocol structs in the net library and flow structs
use from there, wasn't it?
(Btw, a deprecation notice is still pending to clear some existing ones)
So for the GRE optional fields, what about having a struct in the 'rte_gre.h'?
(Also perhaps an GRE extended protocol header can be defined combining
'rte_gre_hdr' and optional fields struct.)
Later flow API struct can embed that struct.
19/01/2022 10:53, Ferruh Yigit:
> On 12/30/2021 3:08 AM, Sean Zhang wrote:
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > /**
> > + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> > + *
> > + * Matches GRE optional fields in header.
> > + */
> > +struct rte_gre_hdr_option {
> > + rte_be16_t checksum;
> > + rte_be32_t key;
> > + rte_be32_t sequence;
> > +};
> > +
>
> Hi Ori, Andrew,
>
> The decision was to have protocol structs in the net library and flow structs
> use from there, wasn't it?
> (Btw, a deprecation notice is still pending to clear some existing ones)
>
> So for the GRE optional fields, what about having a struct in the 'rte_gre.h'?
> (Also perhaps an GRE extended protocol header can be defined combining
> 'rte_gre_hdr' and optional fields struct.)
> Later flow API struct can embed that struct.
+1 for using librte_net.
This addition in rte_flow looks to be a mistake.
Please fix in the next version.
Hi,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
>
> 19/01/2022 10:53, Ferruh Yigit:
> > On 12/30/2021 3:08 AM, Sean Zhang wrote:
> > > --- a/lib/ethdev/rte_flow.h
> > > +++ b/lib/ethdev/rte_flow.h
> > > /**
> > > + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> > > + *
> > > + * Matches GRE optional fields in header.
> > > + */
> > > +struct rte_gre_hdr_option {
> > > + rte_be16_t checksum;
> > > + rte_be32_t key;
> > > + rte_be32_t sequence;
> > > +};
> > > +
> >
> > Hi Ori, Andrew,
> >
> > The decision was to have protocol structs in the net library and flow structs
> > use from there, wasn't it?
> > (Btw, a deprecation notice is still pending to clear some existing ones)
> >
> > So for the GRE optional fields, what about having a struct in the 'rte_gre.h'?
> > (Also perhaps an GRE extended protocol header can be defined combining
> > 'rte_gre_hdr' and optional fields struct.)
> > Later flow API struct can embed that struct.
>
> +1 for using librte_net.
> This addition in rte_flow looks to be a mistake.
> Please fix the next version.
>
Nice idea,
but my main concern is that the header should have the header is defined.
Since some of the fields are optional this will look something like this:
gre_hdr_option_checksum {
rte_be_16_t checksum;
}
gre_hdr_option_key {
rte_be_32_t key;
}
gre_hdr_option_ sequence {
rte_be_32_t sequence;
}
I don't want to have so many rte_flow_items,
Has more and more protocols have optional data it doesn't make sense to create the item for each.
If I'm looking at it from an ideal place, I would like that the optional fields will be part of the original item.
For example in test pmd I would like to write:
Eth / ipv4 / udp / gre flags is key & checksum checksum is yyy key is xxx / end
And not
Eth / ipv4 / udp / gre flags is key & checksum / gre_option checksum is yyy key is xxx / end
This means that the structure will look like this:
struct rte_flow_item_gre {
union {
struct {
/**
* Checksum (1b), reserved 0 (12b), version (3b).
* Refer to RFC 2784.
*/
rte_be16_t c_rsvd0_ver;
rte_be16_t protocol; /**< Protocol type. */
}
struct rte_gre_hdr hdr
}
rte_be_16_t checksum;
rte_be_32_t key;
rte_be_32_t sequence;
};
The main issue with this is that it breaks ABI,
Maybe to solve this we can create a new structure gre_ext?
In any way I think we should think how we allow adding members to structures without
ABI breakage.
Best,
Ori
Hi,
> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Wednesday, January 19, 2022 6:57 PM
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
>
> Hi,
>
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
> >
> > 19/01/2022 10:53, Ferruh Yigit:
> > > On 12/30/2021 3:08 AM, Sean Zhang wrote:
> > > > --- a/lib/ethdev/rte_flow.h
> > > > +++ b/lib/ethdev/rte_flow.h
> > > > /**
> > > > + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> > > > + *
> > > > + * Matches GRE optional fields in header.
> > > > + */
> > > > +struct rte_gre_hdr_option {
> > > > + rte_be16_t checksum;
> > > > + rte_be32_t key;
> > > > + rte_be32_t sequence;
> > > > +};
> > > > +
> > >
> > > Hi Ori, Andrew,
> > >
> > > The decision was to have protocol structs in the net library and
> > > flow structs use from there, wasn't it?
> > > (Btw, a deprecation notice is still pending to clear some existing
> > > ones)
> > >
> > > So for the GRE optional fields, what about having a struct in the
> 'rte_gre.h'?
> > > (Also perhaps an GRE extended protocol header can be defined
> > > combining 'rte_gre_hdr' and optional fields struct.) Later flow API
> > > struct can embed that struct.
> >
> > +1 for using librte_net.
> > This addition in rte_flow looks to be a mistake.
> > Please fix the next version.
> >
> Nice idea,
> but my main concern is that the header should have the header is defined.
> Since some of the fields are optional this will look something like this:
> gre_hdr_option_checksum {
> rte_be_16_t checksum;
> }
>
> gre_hdr_option_key {
> rte_be_32_t key;
> }
>
> gre_hdr_option_ sequence {
> rte_be_32_t sequence;
> }
>
> I don't want to have so many rte_flow_items, Has more and more protocols
> have optional data it doesn't make sense to create the item for each.
>
> If I'm looking at it from an ideal place, I would like that the optional fields will
> be part of the original item.
> For example in test pmd I would like to write:
> Eth / ipv4 / udp / gre flags is key & checksum checksum is yyy key is xxx / end
> And not Eth / ipv4 / udp / gre flags is key & checksum / gre_option checksum
> is yyy key is xxx / end This means that the structure will look like this:
> struct rte_flow_item_gre {
> union {
> struct {
> /**
> * Checksum (1b), reserved 0 (12b), version (3b).
> * Refer to RFC 2784.
> */
> rte_be16_t c_rsvd0_ver;
> rte_be16_t protocol; /**< Protocol type. */
> }
> struct rte_gre_hdr hdr
> }
> rte_be_16_t checksum;
> rte_be_32_t key;
> rte_be_32_t sequence;
> };
> The main issue with this is that it breaks ABI, Maybe to solve this we can
> create a new structure gre_ext?
>
> In any way I think we should think how we allow adding members to
> structures without ABI breakage.
>
> Best,
> Ori
Thanks for the comments and suggestion.
So the acceptable solution is to have new structs define in rte_gre.h?
struct gre_hdr_opt_checksum {
rte_be_16_t checksum;
}
struct gre_hdr_opt_key {
rte_be_32_t key;
}
struct gre_hdr_opt_ sequence {
rte_be_32_t sequence;
}
And to add new struct gre_ext defined in rte_flow.h:
struct gre_ext {
struct rte_gre_hdr hdr;
struct gre_hdr_opt_checkum checksum;
struct rte_hdr_opt_key key;
struct rte_hdr_opt_seq seq;
};
And we use struct gre_ext for this new added flow item gre_option.
Correct me if my understanding is not right.
Thanks,
Sean
On 1/25/2022 9:49 AM, Sean Zhang (Networking SW) wrote:
> Hi,
>
>> -----Original Message-----
>> From: Ori Kam <orika@nvidia.com>
>> Sent: Wednesday, January 19, 2022 6:57 PM
>> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
>> Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
>> <matan@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>
>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
>> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
>>
>> Hi,
>>
>>> -----Original Message-----
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
>>>
>>> 19/01/2022 10:53, Ferruh Yigit:
>>>> On 12/30/2021 3:08 AM, Sean Zhang wrote:
>>>>> --- a/lib/ethdev/rte_flow.h
>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>> /**
>>>>> + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
>>>>> + *
>>>>> + * Matches GRE optional fields in header.
>>>>> + */
>>>>> +struct rte_gre_hdr_option {
>>>>> + rte_be16_t checksum;
>>>>> + rte_be32_t key;
>>>>> + rte_be32_t sequence;
>>>>> +};
>>>>> +
>>>>
>>>> Hi Ori, Andrew,
>>>>
>>>> The decision was to have protocol structs in the net library and
>>>> flow structs use from there, wasn't it?
>>>> (Btw, a deprecation notice is still pending to clear some existing
>>>> ones)
>>>>
>>>> So for the GRE optional fields, what about having a struct in the
>> 'rte_gre.h'?
>>>> (Also perhaps an GRE extended protocol header can be defined
>>>> combining 'rte_gre_hdr' and optional fields struct.) Later flow API
>>>> struct can embed that struct.
>>>
>>> +1 for using librte_net.
>>> This addition in rte_flow looks to be a mistake.
>>> Please fix the next version.
>>>
>> Nice idea,
>> but my main concern is that the header should have the header is defined.
>> Since some of the fields are optional this will look something like this:
>> gre_hdr_option_checksum {
>> rte_be_16_t checksum;
>> }
>>
>> gre_hdr_option_key {
>> rte_be_32_t key;
>> }
>>
>> gre_hdr_option_ sequence {
>> rte_be_32_t sequence;
>> }
>>
>> I don't want to have so many rte_flow_items, Has more and more protocols
>> have optional data it doesn't make sense to create the item for each.
>>
>> If I'm looking at it from an ideal place, I would like that the optional fields will
>> be part of the original item.
>> For example in test pmd I would like to write:
>> Eth / ipv4 / udp / gre flags is key & checksum checksum is yyy key is xxx / end
>> And not Eth / ipv4 / udp / gre flags is key & checksum / gre_option checksum
>> is yyy key is xxx / end This means that the structure will look like this:
>> struct rte_flow_item_gre {
>> union {
>> struct {
>> /**
>> * Checksum (1b), reserved 0 (12b), version (3b).
>> * Refer to RFC 2784.
>> */
>> rte_be16_t c_rsvd0_ver;
>> rte_be16_t protocol; /**< Protocol type. */
>> }
>> struct rte_gre_hdr hdr
>> }
>> rte_be_16_t checksum;
>> rte_be_32_t key;
>> rte_be_32_t sequence;
>> };
>> The main issue with this is that it breaks ABI, Maybe to solve this we can
>> create a new structure gre_ext?
>>
>> In any way I think we should think how we allow adding members to
>> structures without ABI breakage.
>>
>> Best,
>> Ori
>
> Thanks for the comments and suggestion.
> So the acceptable solution is to have new structs define in rte_gre.h?
> struct gre_hdr_opt_checksum {
> rte_be_16_t checksum;
> }
>
> struct gre_hdr_opt_key {
> rte_be_32_t key;
> }
>
> struct gre_hdr_opt_ sequence {
> rte_be_32_t sequence;
> }
>
> And to add new struct gre_ext defined in rte_flow.h:
> struct gre_ext {
> struct rte_gre_hdr hdr;
> struct gre_hdr_opt_checkum checksum;
> struct rte_hdr_opt_key key;
> struct rte_hdr_opt_seq seq;
> };
>
> And we use struct gre_ext for this new added flow item gre_option.
>
What about having a struct for 'options' and use in in flow item for options,
like:
struct gre_hdr_opt {
struct gre_hdr_opt_checkum checksum;
struct rte_hdr_opt_key key;
struct rte_hdr_opt_seq seq;
}
struct gre_hdr_ext {
struct rte_gre_hdr hdr;
struct gre_hdr_opt;
}
struct rte_flow_item_gre_opt {
struct gre_hdr_opt hdr;
}
> Correct me if my understanding is not right.
>
> Thanks,
> Sean
>
>
Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
>
> On 1/25/2022 9:49 AM, Sean Zhang (Networking SW) wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Ori Kam <orika@nvidia.com>
> >> Sent: Wednesday, January 19, 2022 6:57 PM
> >> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> >> Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
> >> <matan@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>
> >> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
> >> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
> >>
> >> Hi,
> >>
> >>> -----Original Message-----
> >>> From: Thomas Monjalon <thomas@monjalon.net>
> >>> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
> >>>
> >>> 19/01/2022 10:53, Ferruh Yigit:
> >>>> On 12/30/2021 3:08 AM, Sean Zhang wrote:
> >>>>> --- a/lib/ethdev/rte_flow.h
> >>>>> +++ b/lib/ethdev/rte_flow.h
> >>>>> /**
> >>>>> + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> >>>>> + *
> >>>>> + * Matches GRE optional fields in header.
> >>>>> + */
> >>>>> +struct rte_gre_hdr_option {
> >>>>> + rte_be16_t checksum;
> >>>>> + rte_be32_t key;
> >>>>> + rte_be32_t sequence;
> >>>>> +};
> >>>>> +
> >>>>
> >>>> Hi Ori, Andrew,
> >>>>
> >>>> The decision was to have protocol structs in the net library and
> >>>> flow structs use from there, wasn't it?
> >>>> (Btw, a deprecation notice is still pending to clear some existing
> >>>> ones)
> >>>>
> >>>> So for the GRE optional fields, what about having a struct in the
> >> 'rte_gre.h'?
> >>>> (Also perhaps an GRE extended protocol header can be defined
> >>>> combining 'rte_gre_hdr' and optional fields struct.) Later flow API
> >>>> struct can embed that struct.
> >>>
> >>> +1 for using librte_net.
> >>> This addition in rte_flow looks to be a mistake.
> >>> Please fix the next version.
> >>>
> >> Nice idea,
> >> but my main concern is that the header should have the header is defined.
> >> Since some of the fields are optional this will look something like this:
> >> gre_hdr_option_checksum {
> >> rte_be_16_t checksum;
> >> }
> >>
> >> gre_hdr_option_key {
> >> rte_be_32_t key;
> >> }
> >>
> >> gre_hdr_option_ sequence {
> >> rte_be_32_t sequence;
> >> }
> >>
> >> I don't want to have so many rte_flow_items, Has more and more protocols
> >> have optional data it doesn't make sense to create the item for each.
> >>
> >> If I'm looking at it from an ideal place, I would like that the optional fields will
> >> be part of the original item.
> >> For example in test pmd I would like to write:
> >> Eth / ipv4 / udp / gre flags is key & checksum checksum is yyy key is xxx / end
> >> And not Eth / ipv4 / udp / gre flags is key & checksum / gre_option checksum
> >> is yyy key is xxx / end This means that the structure will look like this:
> >> struct rte_flow_item_gre {
> >> union {
> >> struct {
> >> /**
> >> * Checksum (1b), reserved 0 (12b), version (3b).
> >> * Refer to RFC 2784.
> >> */
> >> rte_be16_t c_rsvd0_ver;
> >> rte_be16_t protocol; /**< Protocol type. */
> >> }
> >> struct rte_gre_hdr hdr
> >> }
> >> rte_be_16_t checksum;
> >> rte_be_32_t key;
> >> rte_be_32_t sequence;
> >> };
> >> The main issue with this is that it breaks ABI, Maybe to solve this we can
> >> create a new structure gre_ext?
> >>
> >> In any way I think we should think how we allow adding members to
> >> structures without ABI breakage.
> >>
> >> Best,
> >> Ori
> >
> > Thanks for the comments and suggestion.
> > So the acceptable solution is to have new structs define in rte_gre.h?
> > struct gre_hdr_opt_checksum {
> > rte_be_16_t checksum;
> > }
> >
> > struct gre_hdr_opt_key {
> > rte_be_32_t key;
> > }
> >
> > struct gre_hdr_opt_ sequence {
> > rte_be_32_t sequence;
> > }
> >
> > And to add new struct gre_ext defined in rte_flow.h:
> > struct gre_ext {
> > struct rte_gre_hdr hdr;
> > struct gre_hdr_opt_checkum checksum;
> > struct rte_hdr_opt_key key;
> > struct rte_hdr_opt_seq seq;
> > };
> >
> > And we use struct gre_ext for this new added flow item gre_option.
> >
>
> What about having a struct for 'options' and use in in flow item for options,
> like:
>
> struct gre_hdr_opt {
> struct gre_hdr_opt_checkum checksum;
> struct rte_hdr_opt_key key;
> struct rte_hdr_opt_seq seq;
> }
>
> struct gre_hdr_ext {
> struct rte_gre_hdr hdr;
> struct gre_hdr_opt;
> }
>
> struct rte_flow_item_gre_opt {
> struct gre_hdr_opt hdr;
> }
Fom my understanding the header should reflect structures
as they appear in the spec.
If we look at the spec, from my understanding each of those items is stand-alone.
It is possible to have just key or key and seq or any other combination.
So the struct you suggested is not valid struct in gre header.
If you are O.K with adding such a struct to the gre file I will also be O.K with it.
Best,
Ori
>
> > Correct me if my understanding is not right.
> >
> > Thanks,
> > Sean
> >
> >
On 1/25/2022 1:06 PM, Ori Kam wrote:
> Hi Ferruh,
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
>>
>> On 1/25/2022 9:49 AM, Sean Zhang (Networking SW) wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Ori Kam <orika@nvidia.com>
>>>> Sent: Wednesday, January 19, 2022 6:57 PM
>>>> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
>>>> Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
>>>> <matan@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
>>>> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
>>>>
>>>> Hi,
>>>>
>>>>> -----Original Message-----
>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
>>>>>
>>>>> 19/01/2022 10:53, Ferruh Yigit:
>>>>>> On 12/30/2021 3:08 AM, Sean Zhang wrote:
>>>>>>> --- a/lib/ethdev/rte_flow.h
>>>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>>>> /**
>>>>>>> + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
>>>>>>> + *
>>>>>>> + * Matches GRE optional fields in header.
>>>>>>> + */
>>>>>>> +struct rte_gre_hdr_option {
>>>>>>> + rte_be16_t checksum;
>>>>>>> + rte_be32_t key;
>>>>>>> + rte_be32_t sequence;
>>>>>>> +};
>>>>>>> +
>>>>>>
>>>>>> Hi Ori, Andrew,
>>>>>>
>>>>>> The decision was to have protocol structs in the net library and
>>>>>> flow structs use from there, wasn't it?
>>>>>> (Btw, a deprecation notice is still pending to clear some existing
>>>>>> ones)
>>>>>>
>>>>>> So for the GRE optional fields, what about having a struct in the
>>>> 'rte_gre.h'?
>>>>>> (Also perhaps an GRE extended protocol header can be defined
>>>>>> combining 'rte_gre_hdr' and optional fields struct.) Later flow API
>>>>>> struct can embed that struct.
>>>>>
>>>>> +1 for using librte_net.
>>>>> This addition in rte_flow looks to be a mistake.
>>>>> Please fix the next version.
>>>>>
>>>> Nice idea,
>>>> but my main concern is that the header should have the header is defined.
>>>> Since some of the fields are optional this will look something like this:
>>>> gre_hdr_option_checksum {
>>>> rte_be_16_t checksum;
>>>> }
>>>>
>>>> gre_hdr_option_key {
>>>> rte_be_32_t key;
>>>> }
>>>>
>>>> gre_hdr_option_ sequence {
>>>> rte_be_32_t sequence;
>>>> }
>>>>
>>>> I don't want to have so many rte_flow_items, Has more and more protocols
>>>> have optional data it doesn't make sense to create the item for each.
>>>>
>>>> If I'm looking at it from an ideal place, I would like that the optional fields will
>>>> be part of the original item.
>>>> For example in test pmd I would like to write:
>>>> Eth / ipv4 / udp / gre flags is key & checksum checksum is yyy key is xxx / end
>>>> And not Eth / ipv4 / udp / gre flags is key & checksum / gre_option checksum
>>>> is yyy key is xxx / end This means that the structure will look like this:
>>>> struct rte_flow_item_gre {
>>>> union {
>>>> struct {
>>>> /**
>>>> * Checksum (1b), reserved 0 (12b), version (3b).
>>>> * Refer to RFC 2784.
>>>> */
>>>> rte_be16_t c_rsvd0_ver;
>>>> rte_be16_t protocol; /**< Protocol type. */
>>>> }
>>>> struct rte_gre_hdr hdr
>>>> }
>>>> rte_be_16_t checksum;
>>>> rte_be_32_t key;
>>>> rte_be_32_t sequence;
>>>> };
>>>> The main issue with this is that it breaks ABI, Maybe to solve this we can
>>>> create a new structure gre_ext?
>>>>
>>>> In any way I think we should think how we allow adding members to
>>>> structures without ABI breakage.
>>>>
>>>> Best,
>>>> Ori
>>>
>>> Thanks for the comments and suggestion.
>>> So the acceptable solution is to have new structs define in rte_gre.h?
>>> struct gre_hdr_opt_checksum {
>>> rte_be_16_t checksum;
>>> }
>>>
>>> struct gre_hdr_opt_key {
>>> rte_be_32_t key;
>>> }
>>>
>>> struct gre_hdr_opt_ sequence {
>>> rte_be_32_t sequence;
>>> }
>>>
>>> And to add new struct gre_ext defined in rte_flow.h:
>>> struct gre_ext {
>>> struct rte_gre_hdr hdr;
>>> struct gre_hdr_opt_checkum checksum;
>>> struct rte_hdr_opt_key key;
>>> struct rte_hdr_opt_seq seq;
>>> };
>>>
>>> And we use struct gre_ext for this new added flow item gre_option.
>>>
>>
>> What about having a struct for 'options' and use in in flow item for options,
>> like:
>>
>> struct gre_hdr_opt {
>> struct gre_hdr_opt_checkum checksum;
>> struct rte_hdr_opt_key key;
>> struct rte_hdr_opt_seq seq;
>> }
>>
>> struct gre_hdr_ext {
>> struct rte_gre_hdr hdr;
>> struct gre_hdr_opt;
>> }
>>
>> struct rte_flow_item_gre_opt {
>> struct gre_hdr_opt hdr;
>> }
>
> Fom my understanding the header should reflect structures
> as they appear in the spec.
>
> If we look at the spec, from my understanding each of those items is stand-alone.
> It is possible to have just key or key and seq or any other combination.
> So the struct you suggested is not valid struct in gre header.
>
If it is not valid header representation, please forget about it.
But this means initially suggested 'struct gre_ext' is wrong, right?
So should 'rte_flow_item_gre_opt' use separate structs, like:
struct rte_flow_item_gre_opt {
struct gre_hdr_opt_checkum checksum;
struct rte_hdr_opt_key key;
struct rte_hdr_opt_seq seq;
}
> If you are O.K with adding such a struct to the gre file I will also be O.K with it.
>
> Best,
> Ori
>>
>>> Correct me if my understanding is not right.
>>>
>>> Thanks,
>>> Sean
>>>
>>>
>
Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, January 25, 2022 4:29 PM
> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
>
> On 1/25/2022 1:06 PM, Ori Kam wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
> >>
> >> On 1/25/2022 9:49 AM, Sean Zhang (Networking SW) wrote:
> >>> Hi,
> >>>
> >>>> -----Original Message-----
> >>>> From: Ori Kam <orika@nvidia.com>
> >>>> Sent: Wednesday, January 19, 2022 6:57 PM
> >>>> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> >>>> Sean Zhang (Networking SW) <xiazhang@nvidia.com>; Matan Azrad
> >>>> <matan@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
> >>>> Subject: RE: [RFC 1/3] ethdev: support GRE optional fields
> >>>>
> >>>> Hi,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>> Subject: Re: [RFC 1/3] ethdev: support GRE optional fields
> >>>>>
> >>>>> 19/01/2022 10:53, Ferruh Yigit:
> >>>>>> On 12/30/2021 3:08 AM, Sean Zhang wrote:
> >>>>>>> --- a/lib/ethdev/rte_flow.h
> >>>>>>> +++ b/lib/ethdev/rte_flow.h
> >>>>>>> /**
> >>>>>>> + * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
> >>>>>>> + *
> >>>>>>> + * Matches GRE optional fields in header.
> >>>>>>> + */
> >>>>>>> +struct rte_gre_hdr_option {
> >>>>>>> + rte_be16_t checksum;
> >>>>>>> + rte_be32_t key;
> >>>>>>> + rte_be32_t sequence;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>
> >>>>>> Hi Ori, Andrew,
> >>>>>>
> >>>>>> The decision was to have protocol structs in the net library and
> >>>>>> flow structs use from there, wasn't it?
> >>>>>> (Btw, a deprecation notice is still pending to clear some existing
> >>>>>> ones)
> >>>>>>
> >>>>>> So for the GRE optional fields, what about having a struct in the
> >>>> 'rte_gre.h'?
> >>>>>> (Also perhaps an GRE extended protocol header can be defined
> >>>>>> combining 'rte_gre_hdr' and optional fields struct.) Later flow API
> >>>>>> struct can embed that struct.
> >>>>>
> >>>>> +1 for using librte_net.
> >>>>> This addition in rte_flow looks to be a mistake.
> >>>>> Please fix the next version.
> >>>>>
> >>>> Nice idea,
> >>>> but my main concern is that the header should have the header is defined.
> >>>> Since some of the fields are optional this will look something like this:
> >>>> gre_hdr_option_checksum {
> >>>> rte_be_16_t checksum;
> >>>> }
> >>>>
> >>>> gre_hdr_option_key {
> >>>> rte_be_32_t key;
> >>>> }
> >>>>
> >>>> gre_hdr_option_ sequence {
> >>>> rte_be_32_t sequence;
> >>>> }
> >>>>
> >>>> I don't want to have so many rte_flow_items, Has more and more protocols
> >>>> have optional data it doesn't make sense to create the item for each.
> >>>>
> >>>> If I'm looking at it from an ideal place, I would like that the optional fields will
> >>>> be part of the original item.
> >>>> For example in test pmd I would like to write:
> >>>> Eth / ipv4 / udp / gre flags is key & checksum checksum is yyy key is xxx / end
> >>>> And not Eth / ipv4 / udp / gre flags is key & checksum / gre_option checksum
> >>>> is yyy key is xxx / end This means that the structure will look like this:
> >>>> struct rte_flow_item_gre {
> >>>> union {
> >>>> struct {
> >>>> /**
> >>>> * Checksum (1b), reserved 0 (12b), version (3b).
> >>>> * Refer to RFC 2784.
> >>>> */
> >>>> rte_be16_t c_rsvd0_ver;
> >>>> rte_be16_t protocol; /**< Protocol type. */
> >>>> }
> >>>> struct rte_gre_hdr hdr
> >>>> }
> >>>> rte_be_16_t checksum;
> >>>> rte_be_32_t key;
> >>>> rte_be_32_t sequence;
> >>>> };
> >>>> The main issue with this is that it breaks ABI, Maybe to solve this we can
> >>>> create a new structure gre_ext?
> >>>>
> >>>> In any way I think we should think how we allow adding members to
> >>>> structures without ABI breakage.
> >>>>
> >>>> Best,
> >>>> Ori
> >>>
> >>> Thanks for the comments and suggestion.
> >>> So the acceptable solution is to have new structs define in rte_gre.h?
> >>> struct gre_hdr_opt_checksum {
> >>> rte_be_16_t checksum;
> >>> }
> >>>
> >>> struct gre_hdr_opt_key {
> >>> rte_be_32_t key;
> >>> }
> >>>
> >>> struct gre_hdr_opt_ sequence {
> >>> rte_be_32_t sequence;
> >>> }
> >>>
> >>> And to add new struct gre_ext defined in rte_flow.h:
> >>> struct gre_ext {
> >>> struct rte_gre_hdr hdr;
> >>> struct gre_hdr_opt_checkum checksum;
> >>> struct rte_hdr_opt_key key;
> >>> struct rte_hdr_opt_seq seq;
> >>> };
> >>>
> >>> And we use struct gre_ext for this new added flow item gre_option.
> >>>
> >>
> >> What about having a struct for 'options' and use in in flow item for options,
> >> like:
> >>
> >> struct gre_hdr_opt {
> >> struct gre_hdr_opt_checkum checksum;
> >> struct rte_hdr_opt_key key;
> >> struct rte_hdr_opt_seq seq;
> >> }
> >>
> >> struct gre_hdr_ext {
> >> struct rte_gre_hdr hdr;
> >> struct gre_hdr_opt;
> >> }
> >>
> >> struct rte_flow_item_gre_opt {
> >> struct gre_hdr_opt hdr;
> >> }
> >
> > Fom my understanding the header should reflect structures
> > as they appear in the spec.
> >
> > If we look at the spec, from my understanding each of those items is stand-alone.
> > It is possible to have just key or key and seq or any other combination.
> > So the struct you suggested is not valid struct in gre header.
> >
>
> If it is not valid header representation, please forget about it.
>
> But this means initially suggested 'struct gre_ext' is wrong, right?
>
> So should 'rte_flow_item_gre_opt' use separate structs, like:
>
> struct rte_flow_item_gre_opt {
> struct gre_hdr_opt_checkum checksum;
> struct rte_hdr_opt_key key;
> struct rte_hdr_opt_seq seq;
> }
>
Yes this is the last suggestion from Sean, the only difference is that
he created a new item gre_ext that holds the struct that you listed above
and added the gre header. This means that from rte_flow thre is only one GRE item
(the old one can be deprecated)
Ori
>
> > If you are O.K with adding such a struct to the gre file I will also be O.K with it.
> >
> > Best,
> > Ori
> >>
> >>> Correct me if my understanding is not right.
> >>>
> >>> Thanks,
> >>> Sean
> >>>
> >>>
> >
@@ -1113,6 +1113,22 @@ This should be preceded by item ``GRE``.
- Value to be matched is a big-endian 32 bit integer.
- When this item present it implicitly match K bit in default mask as "1"
+Item: ``GRE_OPTION``
+^^^^^^^^^^^^^^^^^^^^
+
+Matches a GRE optional fields (checksum/key/sequence).
+This should be preceded by item ``GRE``.
+
+- ``checksum``: checksum.
+- ``key``: key.
+- ``sequence``: sequence.
+- The items in GRE_OPTION do not change bit flags(c_bit/k_bit/s_bit) in GRE
+ item. The bit flags need be set with GRE item by application. When the items
+ present, the corresponding bits in GRE spec and mask should be set "1" by
+ application, it means to match specified value of the fields. When the items
+ no present, but the corresponding bits in GRE spec and mask is "1", it means
+ to match any value of the fields.
+
Item: ``FUZZY``
^^^^^^^^^^^^^^^
@@ -139,6 +139,7 @@ struct rte_flow_desc_data {
MK_FLOW_ITEM(META, sizeof(struct rte_flow_item_meta)),
MK_FLOW_ITEM(TAG, sizeof(struct rte_flow_item_tag)),
MK_FLOW_ITEM(GRE_KEY, sizeof(rte_be32_t)),
+ MK_FLOW_ITEM(GRE_OPTION, sizeof(struct rte_gre_hdr_option)),
MK_FLOW_ITEM(GTP_PSC, sizeof(struct rte_flow_item_gtp_psc)),
MK_FLOW_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)),
MK_FLOW_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)),
@@ -660,6 +660,13 @@ enum rte_flow_item_type {
* See struct rte_flow_item_ppp.
*/
RTE_FLOW_ITEM_TYPE_PPP,
+
+ /**
+ * Matches GRE optional fields.
+ *
+ * See struct rte_gre_hdr_option.
+ */
+ RTE_FLOW_ITEM_TYPE_GRE_OPTION,
};
/**
@@ -1196,6 +1203,17 @@ struct rte_flow_item_gre {
#endif
/**
+ * RTE_FLOW_ITEM_TYPE_GRE_OPTION.
+ *
+ * Matches GRE optional fields in header.
+ */
+struct rte_gre_hdr_option {
+ rte_be16_t checksum;
+ rte_be32_t key;
+ rte_be32_t sequence;
+};
+
+/**
* RTE_FLOW_ITEM_TYPE_FUZZY
*
* Fuzzy pattern match, expect faster than default.