[dpdk-dev,v6,4/8] ethdev: add GTP items to support flow API

Message ID 1506662342-18966-5-git-send-email-beilei.xing@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Xing, Beilei Sept. 29, 2017, 5:18 a.m. UTC
  This patch adds GTP, GTPC and GTPU items for
generic flow API, and also exposes item fields
through the flow command.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 app/test-pmd/cmdline_flow.c                 | 40 ++++++++++++++++++++++
 app/test-pmd/config.c                       |  3 ++
 doc/guides/prog_guide/rte_flow.rst          | 17 ++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
 lib/librte_ether/rte_flow.h                 | 52 +++++++++++++++++++++++++++++
 5 files changed, 116 insertions(+)
  

Comments

Sean Harte Sept. 29, 2017, 8:15 a.m. UTC | #1
On 29 September 2017 at 06:18, Beilei Xing <beilei.xing@intel.com> wrote:
> This patch adds GTP, GTPC and GTPU items for
> generic flow API, and also exposes item fields
> through the flow command.
>
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  app/test-pmd/cmdline_flow.c                 | 40 ++++++++++++++++++++++
>  app/test-pmd/config.c                       |  3 ++
>  doc/guides/prog_guide/rte_flow.rst          | 17 ++++++++++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
>  lib/librte_ether/rte_flow.h                 | 52 +++++++++++++++++++++++++++++
>  5 files changed, 116 insertions(+)

<snip>

>  /**
> + * RTE_FLOW_ITEM_TYPE_GTP.
> + *
> + * Matches a GTPv1 header.
> + */
> +struct rte_flow_item_gtp {
> +       /**
> +        * Version (3b), protocol type (1b), reserved (1b),
> +        * Extension header flag (1b),
> +        * Sequence number flag (1b),
> +        * N-PDU number flag (1b).
> +        */
> +       uint8_t v_pt_rsv_flags;
> +       uint8_t msg_type; /**< Message type. */
> +       rte_be16_t msg_len; /**< Message length. */
> +       rte_be32_t teid; /**< Tunnel endpoint identifier. */
> +};

In future, you might add support for GTPv2 (which is used since LTE).
Maybe this structure should have v1 in its name to avoid confusion?

> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_GTP. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_gtp rte_flow_item_gtp_mask = {
> +       .teid = RTE_BE32(0xffffffff),
> +};
> +#endif
> +
> +/**
>   * Matching pattern item definition.
>   *
>   * A pattern is formed by stacking items starting from the lowest protocol
> --
> 2.5.5
>
  
Xing, Beilei Sept. 29, 2017, 8:54 a.m. UTC | #2
> -----Original Message-----

> From: Sean Harte [mailto:seanbh@gmail.com]

> Sent: Friday, September 29, 2017 4:15 PM

> To: Xing, Beilei <beilei.xing@intel.com>

> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Chilikin, Andrey

> <andrey.chilikin@intel.com>; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 4/8] ethdev: add GTP items to support

> flow API

> 

> On 29 September 2017 at 06:18, Beilei Xing <beilei.xing@intel.com> wrote:

> > This patch adds GTP, GTPC and GTPU items for generic flow API, and

> > also exposes item fields through the flow command.

> >

> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>

> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

> > ---

> >  app/test-pmd/cmdline_flow.c                 | 40 ++++++++++++++++++++++

> >  app/test-pmd/config.c                       |  3 ++

> >  doc/guides/prog_guide/rte_flow.rst          | 17 ++++++++++

> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++

> >  lib/librte_ether/rte_flow.h                 | 52

> +++++++++++++++++++++++++++++

> >  5 files changed, 116 insertions(+)

> 

> <snip>

> 

> >  /**

> > + * RTE_FLOW_ITEM_TYPE_GTP.

> > + *

> > + * Matches a GTPv1 header.

> > + */

> > +struct rte_flow_item_gtp {

> > +       /**

> > +        * Version (3b), protocol type (1b), reserved (1b),

> > +        * Extension header flag (1b),

> > +        * Sequence number flag (1b),

> > +        * N-PDU number flag (1b).

> > +        */

> > +       uint8_t v_pt_rsv_flags;

> > +       uint8_t msg_type; /**< Message type. */

> > +       rte_be16_t msg_len; /**< Message length. */

> > +       rte_be32_t teid; /**< Tunnel endpoint identifier. */ };

> 

> In future, you might add support for GTPv2 (which is used since LTE).

> Maybe this structure should have v1 in its name to avoid confusion?


I considered it before. But I think we can modify it when we support GTPv2 in future, and keep concise 'GTP' currently:)  since I have described it matches v1 header.

> 

> > +

> > +/** Default mask for RTE_FLOW_ITEM_TYPE_GTP. */ #ifndef __cplusplus

> > +static const struct rte_flow_item_gtp rte_flow_item_gtp_mask = {

> > +       .teid = RTE_BE32(0xffffffff),

> > +};

> > +#endif

> > +

> > +/**

> >   * Matching pattern item definition.

> >   *

> >   * A pattern is formed by stacking items starting from the lowest

> > protocol

> > --

> > 2.5.5

> >
  
Sean Harte Sept. 29, 2017, 9:29 a.m. UTC | #3
On 29 September 2017 at 09:54, Xing, Beilei <beilei.xing@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Sean Harte [mailto:seanbh@gmail.com]
>> Sent: Friday, September 29, 2017 4:15 PM
>> To: Xing, Beilei <beilei.xing@intel.com>
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Chilikin, Andrey
>> <andrey.chilikin@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v6 4/8] ethdev: add GTP items to support
>> flow API
>>
>> On 29 September 2017 at 06:18, Beilei Xing <beilei.xing@intel.com> wrote:
>> > This patch adds GTP, GTPC and GTPU items for generic flow API, and
>> > also exposes item fields through the flow command.
>> >
>> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
>> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>> > ---
>> >  app/test-pmd/cmdline_flow.c                 | 40 ++++++++++++++++++++++
>> >  app/test-pmd/config.c                       |  3 ++
>> >  doc/guides/prog_guide/rte_flow.rst          | 17 ++++++++++
>> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
>> >  lib/librte_ether/rte_flow.h                 | 52
>> +++++++++++++++++++++++++++++
>> >  5 files changed, 116 insertions(+)
>>
>> <snip>
>>
>> >  /**
>> > + * RTE_FLOW_ITEM_TYPE_GTP.
>> > + *
>> > + * Matches a GTPv1 header.
>> > + */
>> > +struct rte_flow_item_gtp {
>> > +       /**
>> > +        * Version (3b), protocol type (1b), reserved (1b),
>> > +        * Extension header flag (1b),
>> > +        * Sequence number flag (1b),
>> > +        * N-PDU number flag (1b).
>> > +        */
>> > +       uint8_t v_pt_rsv_flags;
>> > +       uint8_t msg_type; /**< Message type. */
>> > +       rte_be16_t msg_len; /**< Message length. */
>> > +       rte_be32_t teid; /**< Tunnel endpoint identifier. */ };
>>
>> In future, you might add support for GTPv2 (which is used since LTE).
>> Maybe this structure should have v1 in its name to avoid confusion?
>
> I considered it before. But I think we can modify it when we support GTPv2 in future, and keep concise 'GTP' currently:)  since I have described it matches v1 header.
>

You could rename v_pt_rsv_flags to version_flags to avoid some future
code changes to support GTPv2. There's still the issue that not all
GTPv2 messages have a TEID though.

>>
>> > +
>> > +/** Default mask for RTE_FLOW_ITEM_TYPE_GTP. */ #ifndef __cplusplus
>> > +static const struct rte_flow_item_gtp rte_flow_item_gtp_mask = {
>> > +       .teid = RTE_BE32(0xffffffff),
>> > +};
>> > +#endif
>> > +
>> > +/**
>> >   * Matching pattern item definition.
>> >   *
>> >   * A pattern is formed by stacking items starting from the lowest
>> > protocol
>> > --
>> > 2.5.5
>> >
  
Xing, Beilei Sept. 29, 2017, 9:37 a.m. UTC | #4
> -----Original Message-----

> From: Sean Harte [mailto:seanbh@gmail.com]

> Sent: Friday, September 29, 2017 5:30 PM

> To: Xing, Beilei <beilei.xing@intel.com>

> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Chilikin, Andrey

> <andrey.chilikin@intel.com>; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 4/8] ethdev: add GTP items to support

> flow API

> 

> On 29 September 2017 at 09:54, Xing, Beilei <beilei.xing@intel.com> wrote:

> >

> >

> >> -----Original Message-----

> >> From: Sean Harte [mailto:seanbh@gmail.com]

> >> Sent: Friday, September 29, 2017 4:15 PM

> >> To: Xing, Beilei <beilei.xing@intel.com>

> >> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Chilikin, Andrey

> >> <andrey.chilikin@intel.com>; dev@dpdk.org

> >> Subject: Re: [dpdk-dev] [PATCH v6 4/8] ethdev: add GTP items to

> >> support flow API

> >>

> >> On 29 September 2017 at 06:18, Beilei Xing <beilei.xing@intel.com> wrote:

> >> > This patch adds GTP, GTPC and GTPU items for generic flow API, and

> >> > also exposes item fields through the flow command.

> >> >

> >> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>

> >> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

> >> > ---

> >> >  app/test-pmd/cmdline_flow.c                 | 40

> ++++++++++++++++++++++

> >> >  app/test-pmd/config.c                       |  3 ++

> >> >  doc/guides/prog_guide/rte_flow.rst          | 17 ++++++++++

> >> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++

> >> >  lib/librte_ether/rte_flow.h                 | 52

> >> +++++++++++++++++++++++++++++

> >> >  5 files changed, 116 insertions(+)

> >>

> >> <snip>

> >>

> >> >  /**

> >> > + * RTE_FLOW_ITEM_TYPE_GTP.

> >> > + *

> >> > + * Matches a GTPv1 header.

> >> > + */

> >> > +struct rte_flow_item_gtp {

> >> > +       /**

> >> > +        * Version (3b), protocol type (1b), reserved (1b),

> >> > +        * Extension header flag (1b),

> >> > +        * Sequence number flag (1b),

> >> > +        * N-PDU number flag (1b).

> >> > +        */

> >> > +       uint8_t v_pt_rsv_flags;

> >> > +       uint8_t msg_type; /**< Message type. */

> >> > +       rte_be16_t msg_len; /**< Message length. */

> >> > +       rte_be32_t teid; /**< Tunnel endpoint identifier. */ };

> >>

> >> In future, you might add support for GTPv2 (which is used since LTE).

> >> Maybe this structure should have v1 in its name to avoid confusion?

> >

> > I considered it before. But I think we can modify it when we support GTPv2

> in future, and keep concise 'GTP' currently:)  since I have described it

> matches v1 header.

> >

> 

> You could rename v_pt_rsv_flags to version_flags to avoid some future code

> changes to support GTPv2. There's still the issue that not all

> GTPv2 messages have a TEID though.

> 


Yes, actually I have no a good idea for the compatibility between GTPv1 and GTPv2 currently...
Maybe we can consider it in future.

> >>

> >> > +

> >> > +/** Default mask for RTE_FLOW_ITEM_TYPE_GTP. */ #ifndef

> >> > +__cplusplus static const struct rte_flow_item_gtp

> rte_flow_item_gtp_mask = {

> >> > +       .teid = RTE_BE32(0xffffffff), }; #endif

> >> > +

> >> > +/**

> >> >   * Matching pattern item definition.

> >> >   *

> >> >   * A pattern is formed by stacking items starting from the lowest

> >> > protocol

> >> > --

> >> > 2.5.5

> >> >
  
Adrien Mazarguil Oct. 2, 2017, 12:27 p.m. UTC | #5
On Fri, Sep 29, 2017 at 10:29:55AM +0100, Sean Harte wrote:
> On 29 September 2017 at 09:54, Xing, Beilei <beilei.xing@intel.com> wrote:
<snip>
> >> >  /**
> >> > + * RTE_FLOW_ITEM_TYPE_GTP.
> >> > + *
> >> > + * Matches a GTPv1 header.
> >> > + */
> >> > +struct rte_flow_item_gtp {
> >> > +       /**
> >> > +        * Version (3b), protocol type (1b), reserved (1b),
> >> > +        * Extension header flag (1b),
> >> > +        * Sequence number flag (1b),
> >> > +        * N-PDU number flag (1b).
> >> > +        */
> >> > +       uint8_t v_pt_rsv_flags;
> >> > +       uint8_t msg_type; /**< Message type. */
> >> > +       rte_be16_t msg_len; /**< Message length. */
> >> > +       rte_be32_t teid; /**< Tunnel endpoint identifier. */ };
> >>
> >> In future, you might add support for GTPv2 (which is used since LTE).
> >> Maybe this structure should have v1 in its name to avoid confusion?
> >
> > I considered it before. But I think we can modify it when we support GTPv2 in future, and keep concise 'GTP' currently:)  since I have described it matches v1 header.
> >
> 
> You could rename v_pt_rsv_flags to version_flags to avoid some future
> code changes to support GTPv2. There's still the issue that not all
> GTPv2 messages have a TEID though.

Although they have the same size, the header of these two protocols
obviously differs. My suggestion would be to go with a separate GTPv2
pattern item using its own dedicated structure instead.
  
Sean Harte Oct. 3, 2017, 8:56 a.m. UTC | #6
On 2 October 2017 at 13:27, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> On Fri, Sep 29, 2017 at 10:29:55AM +0100, Sean Harte wrote:
>> On 29 September 2017 at 09:54, Xing, Beilei <beilei.xing@intel.com> wrote:
> <snip>
>> >> >  /**
>> >> > + * RTE_FLOW_ITEM_TYPE_GTP.
>> >> > + *
>> >> > + * Matches a GTPv1 header.
>> >> > + */
>> >> > +struct rte_flow_item_gtp {
>> >> > +       /**
>> >> > +        * Version (3b), protocol type (1b), reserved (1b),
>> >> > +        * Extension header flag (1b),
>> >> > +        * Sequence number flag (1b),
>> >> > +        * N-PDU number flag (1b).
>> >> > +        */
>> >> > +       uint8_t v_pt_rsv_flags;
>> >> > +       uint8_t msg_type; /**< Message type. */
>> >> > +       rte_be16_t msg_len; /**< Message length. */
>> >> > +       rte_be32_t teid; /**< Tunnel endpoint identifier. */ };
>> >>
>> >> In future, you might add support for GTPv2 (which is used since LTE).
>> >> Maybe this structure should have v1 in its name to avoid confusion?
>> >
>> > I considered it before. But I think we can modify it when we support GTPv2 in future, and keep concise 'GTP' currently:)  since I have described it matches v1 header.
>> >
>>
>> You could rename v_pt_rsv_flags to version_flags to avoid some future
>> code changes to support GTPv2. There's still the issue that not all
>> GTPv2 messages have a TEID though.
>
> Although they have the same size, the header of these two protocols
> obviously differs. My suggestion would be to go with a separate GTPv2
> pattern item using its own dedicated structure instead.
>
> --
> Adrien Mazarguil
> 6WIND

The 1st four bytes are the same (flags in first byte have different
meanings, but the bits indicating the version are in the same
location). After that, different fields in each version are optional,
and the headers have variable size. A single structure could be used
if the first field is renamed to something like "version_flags", and
then check that the teid field in item->mask is not set if
((version_flags >> 5 == 2) && ((version_flags >> 4) & 1) == 1). If
there's going to be two structures, it would be good to put v1 and v2
in the names, in my opinion.
  
Jingjing Wu Oct. 5, 2017, 8:06 a.m. UTC | #7
> -----Original Message-----

> From: Sean Harte [mailto:seanbh@gmail.com]

> Sent: Tuesday, October 3, 2017 4:57 PM

> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>

> Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Chilikin,

> Andrey <andrey.chilikin@intel.com>; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 4/8] ethdev: add GTP items to support flow API

> 

> On 2 October 2017 at 13:27, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:

> > On Fri, Sep 29, 2017 at 10:29:55AM +0100, Sean Harte wrote:

> >> On 29 September 2017 at 09:54, Xing, Beilei <beilei.xing@intel.com> wrote:

> > <snip>

> >> >> >  /**

> >> >> > + * RTE_FLOW_ITEM_TYPE_GTP.

> >> >> > + *

> >> >> > + * Matches a GTPv1 header.

> >> >> > + */

> >> >> > +struct rte_flow_item_gtp {

> >> >> > +       /**

> >> >> > +        * Version (3b), protocol type (1b), reserved (1b),

> >> >> > +        * Extension header flag (1b),

> >> >> > +        * Sequence number flag (1b),

> >> >> > +        * N-PDU number flag (1b).

> >> >> > +        */

> >> >> > +       uint8_t v_pt_rsv_flags;

> >> >> > +       uint8_t msg_type; /**< Message type. */

> >> >> > +       rte_be16_t msg_len; /**< Message length. */

> >> >> > +       rte_be32_t teid; /**< Tunnel endpoint identifier. */ };

> >> >>

> >> >> In future, you might add support for GTPv2 (which is used since LTE).

> >> >> Maybe this structure should have v1 in its name to avoid confusion?

> >> >

> >> > I considered it before. But I think we can modify it when we support GTPv2 in future,

> and keep concise 'GTP' currently:)  since I have described it matches v1 header.

> >> >

> >>

> >> You could rename v_pt_rsv_flags to version_flags to avoid some future

> >> code changes to support GTPv2. There's still the issue that not all

> >> GTPv2 messages have a TEID though.

> >

> > Although they have the same size, the header of these two protocols

> > obviously differs. My suggestion would be to go with a separate GTPv2

> > pattern item using its own dedicated structure instead.

> >

> > --

> > Adrien Mazarguil

> > 6WIND

> 

> The 1st four bytes are the same (flags in first byte have different

> meanings, but the bits indicating the version are in the same

> location). After that, different fields in each version are optional,

> and the headers have variable size. A single structure could be used

> if the first field is renamed to something like "version_flags", and

> then check that the teid field in item->mask is not set if

> ((version_flags >> 5 == 2) && ((version_flags >> 4) & 1) == 1). If

> there's going to be two structures, it would be good to put v1 and v2

> in the names, in my opinion.


I think the name GTP is OK for now. Due to v1 and v2 are different, why not rename them
when the v2 supporting are introduced?
  
Adrien Mazarguil Oct. 5, 2017, 8:30 a.m. UTC | #8
On Thu, Oct 05, 2017 at 08:06:38AM +0000, Wu, Jingjing wrote:
> 
> 
> > -----Original Message-----
> > From: Sean Harte [mailto:seanbh@gmail.com]
> > Sent: Tuesday, October 3, 2017 4:57 PM
> > To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Chilikin,
> > Andrey <andrey.chilikin@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v6 4/8] ethdev: add GTP items to support flow API
> > 
> > On 2 October 2017 at 13:27, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> > > On Fri, Sep 29, 2017 at 10:29:55AM +0100, Sean Harte wrote:
> > >> On 29 September 2017 at 09:54, Xing, Beilei <beilei.xing@intel.com> wrote:
> > > <snip>
> > >> >> >  /**
> > >> >> > + * RTE_FLOW_ITEM_TYPE_GTP.
> > >> >> > + *
> > >> >> > + * Matches a GTPv1 header.
> > >> >> > + */
> > >> >> > +struct rte_flow_item_gtp {
> > >> >> > +       /**
> > >> >> > +        * Version (3b), protocol type (1b), reserved (1b),
> > >> >> > +        * Extension header flag (1b),
> > >> >> > +        * Sequence number flag (1b),
> > >> >> > +        * N-PDU number flag (1b).
> > >> >> > +        */
> > >> >> > +       uint8_t v_pt_rsv_flags;
> > >> >> > +       uint8_t msg_type; /**< Message type. */
> > >> >> > +       rte_be16_t msg_len; /**< Message length. */
> > >> >> > +       rte_be32_t teid; /**< Tunnel endpoint identifier. */ };
> > >> >>
> > >> >> In future, you might add support for GTPv2 (which is used since LTE).
> > >> >> Maybe this structure should have v1 in its name to avoid confusion?
> > >> >
> > >> > I considered it before. But I think we can modify it when we support GTPv2 in future,
> > and keep concise 'GTP' currently:)  since I have described it matches v1 header.
> > >> >
> > >>
> > >> You could rename v_pt_rsv_flags to version_flags to avoid some future
> > >> code changes to support GTPv2. There's still the issue that not all
> > >> GTPv2 messages have a TEID though.
> > >
> > > Although they have the same size, the header of these two protocols
> > > obviously differs. My suggestion would be to go with a separate GTPv2
> > > pattern item using its own dedicated structure instead.
> > >
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> > 
> > The 1st four bytes are the same (flags in first byte have different
> > meanings, but the bits indicating the version are in the same
> > location). After that, different fields in each version are optional,
> > and the headers have variable size. A single structure could be used
> > if the first field is renamed to something like "version_flags", and
> > then check that the teid field in item->mask is not set if
> > ((version_flags >> 5 == 2) && ((version_flags >> 4) & 1) == 1). If
> > there's going to be two structures, it would be good to put v1 and v2
> > in the names, in my opinion.
> 
> I think the name GTP is OK for now. Due to v1 and v2 are different, why not rename them
> when the v2 supporting are introduced?

In any case I'd rather avoid renaming and modifying existing items and
structure contents once part of the API to avoid API/ABI breakage that
require deprecation notices, user applications updates and so on; rte_flow
has been created as a kind of append-only API for this reason (of course
there are exceptions, such as a bad design choice for the VLAN item I intend
to fix at some point).

I'm fine with the name "GTP" as defined now and documented as matching
GTPv1. We can add "GTPv2"-themed definitions later when some implementation
provides the ability to match this version. If you want to append the "v1"
suffix right now to be more explicit, I'm also fine with that. Your call.
  
Jingjing Wu Oct. 5, 2017, 8:39 a.m. UTC | #9
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, October 5, 2017 4:30 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: Sean Harte <seanbh@gmail.com>; Xing, Beilei <beilei.xing@intel.com>; Chilikin,
> Andrey <andrey.chilikin@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 4/8] ethdev: add GTP items to support flow API
> 
> On Thu, Oct 05, 2017 at 08:06:38AM +0000, Wu, Jingjing wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sean Harte [mailto:seanbh@gmail.com]
> > > Sent: Tuesday, October 3, 2017 4:57 PM
> > > To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Chilikin,
> > > Andrey <andrey.chilikin@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v6 4/8] ethdev: add GTP items to support flow API
> > >
> > > On 2 October 2017 at 13:27, Adrien Mazarguil <adrien.mazarguil@6wind.com>
> wrote:
> > > > On Fri, Sep 29, 2017 at 10:29:55AM +0100, Sean Harte wrote:
> > > >> On 29 September 2017 at 09:54, Xing, Beilei <beilei.xing@intel.com> wrote:
> > > > <snip>
> > > >> >> >  /**
> > > >> >> > + * RTE_FLOW_ITEM_TYPE_GTP.
> > > >> >> > + *
> > > >> >> > + * Matches a GTPv1 header.
> > > >> >> > + */
> > > >> >> > +struct rte_flow_item_gtp {
> > > >> >> > +       /**
> > > >> >> > +        * Version (3b), protocol type (1b), reserved (1b),
> > > >> >> > +        * Extension header flag (1b),
> > > >> >> > +        * Sequence number flag (1b),
> > > >> >> > +        * N-PDU number flag (1b).
> > > >> >> > +        */
> > > >> >> > +       uint8_t v_pt_rsv_flags;
> > > >> >> > +       uint8_t msg_type; /**< Message type. */
> > > >> >> > +       rte_be16_t msg_len; /**< Message length. */
> > > >> >> > +       rte_be32_t teid; /**< Tunnel endpoint identifier. */ };
> > > >> >>
> > > >> >> In future, you might add support for GTPv2 (which is used since LTE).
> > > >> >> Maybe this structure should have v1 in its name to avoid confusion?
> > > >> >
> > > >> > I considered it before. But I think we can modify it when we support GTPv2 in
> future,
> > > and keep concise 'GTP' currently:)  since I have described it matches v1 header.
> > > >> >
> > > >>
> > > >> You could rename v_pt_rsv_flags to version_flags to avoid some future
> > > >> code changes to support GTPv2. There's still the issue that not all
> > > >> GTPv2 messages have a TEID though.
> > > >
> > > > Although they have the same size, the header of these two protocols
> > > > obviously differs. My suggestion would be to go with a separate GTPv2
> > > > pattern item using its own dedicated structure instead.
> > > >
> > > > --
> > > > Adrien Mazarguil
> > > > 6WIND
> > >
> > > The 1st four bytes are the same (flags in first byte have different
> > > meanings, but the bits indicating the version are in the same
> > > location). After that, different fields in each version are optional,
> > > and the headers have variable size. A single structure could be used
> > > if the first field is renamed to something like "version_flags", and
> > > then check that the teid field in item->mask is not set if
> > > ((version_flags >> 5 == 2) && ((version_flags >> 4) & 1) == 1). If
> > > there's going to be two structures, it would be good to put v1 and v2
> > > in the names, in my opinion.
> >
> > I think the name GTP is OK for now. Due to v1 and v2 are different, why not rename
> them
> > when the v2 supporting are introduced?
> 
> In any case I'd rather avoid renaming and modifying existing items and
> structure contents once part of the API to avoid API/ABI breakage that
> require deprecation notices, user applications updates and so on; rte_flow
> has been created as a kind of append-only API for this reason (of course
> there are exceptions, such as a bad design choice for the VLAN item I intend
> to fix at some point).
> 
> I'm fine with the name "GTP" as defined now and documented as matching
> GTPv1. We can add "GTPv2"-themed definitions later when some implementation
> provides the ability to match this version. If you want to append the "v1"
> suffix right now to be more explicit, I'm also fine with that. Your call.
> 
Got your point, I'm also fine with the name now for GTPv1, and add "GTPv2" when
It is supported.

Thanks
Jingjing
  

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index a17a004..26c3e4f 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -171,6 +171,10 @@  enum index {
 	ITEM_GRE_PROTO,
 	ITEM_FUZZY,
 	ITEM_FUZZY_THRESH,
+	ITEM_GTP,
+	ITEM_GTP_TEID,
+	ITEM_GTPC,
+	ITEM_GTPU,
 
 	/* Validate/create actions. */
 	ACTIONS,
@@ -451,6 +455,9 @@  static const enum index next_item[] = {
 	ITEM_MPLS,
 	ITEM_GRE,
 	ITEM_FUZZY,
+	ITEM_GTP,
+	ITEM_GTPC,
+	ITEM_GTPU,
 	ZERO,
 };
 
@@ -588,6 +595,12 @@  static const enum index item_gre[] = {
 	ZERO,
 };
 
+static const enum index item_gtp[] = {
+	ITEM_GTP_TEID,
+	ITEM_NEXT,
+	ZERO,
+};
+
 static const enum index next_action[] = {
 	ACTION_END,
 	ACTION_VOID,
@@ -1421,6 +1434,33 @@  static const struct token token_list[] = {
 		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_fuzzy,
 					thresh)),
 	},
+	[ITEM_GTP] = {
+		.name = "gtp",
+		.help = "match GTP header",
+		.priv = PRIV_ITEM(GTP, sizeof(struct rte_flow_item_gtp)),
+		.next = NEXT(item_gtp),
+		.call = parse_vc,
+	},
+	[ITEM_GTP_TEID] = {
+		.name = "teid",
+		.help = "tunnel endpoint identifier",
+		.next = NEXT(item_gtp, NEXT_ENTRY(UNSIGNED), item_param),
+		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gtp, teid)),
+	},
+	[ITEM_GTPC] = {
+		.name = "gtpc",
+		.help = "match GTP header",
+		.priv = PRIV_ITEM(GTPC, sizeof(struct rte_flow_item_gtp)),
+		.next = NEXT(item_gtp),
+		.call = parse_vc,
+	},
+	[ITEM_GTPU] = {
+		.name = "gtpu",
+		.help = "match GTP header",
+		.priv = PRIV_ITEM(GTPU, sizeof(struct rte_flow_item_gtp)),
+		.next = NEXT(item_gtp),
+		.call = parse_vc,
+	},
 
 	/* Validate/create actions. */
 	[ACTIONS] = {
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index e8e311c..9b09bbd 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -949,6 +949,9 @@  static const struct {
 	MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
 	MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
 	MK_FLOW_ITEM(FUZZY, sizeof(struct rte_flow_item_fuzzy)),
+	MK_FLOW_ITEM(GTP, sizeof(struct rte_flow_item_gtp)),
+	MK_FLOW_ITEM(GTPC, sizeof(struct rte_flow_item_gtp)),
+	MK_FLOW_ITEM(GTPU, sizeof(struct rte_flow_item_gtp)),
 };
 
 /** Compute storage space needed by item specification. */
diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 662a912..73f12ee 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -955,6 +955,23 @@  Usage example, fuzzy match a TCPv4 packets:
    | 4     | END      |
    +-------+----------+
 
+Item: ``GTP``, ``GTPC``, ``GTPU``
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Matches a GTPv1 header.
+
+Note: GTP, GTPC and GTPU use the same structure. GTPC and GTPU item
+are defined for a user-friendly API when creating GTP-C and GTP-U
+flow rules.
+
+- ``v_pt_rsv_flags``: version (3b), protocol type (1b), reserved (1b),
+  extension header flag (1b), sequence number flag (1b), N-PDU number
+  flag (1b).
+- ``msg_type``: message type.
+- ``msg_len``: message length.
+- ``teid``: tunnel endpoint identifier.
+- Default ``mask`` matches teid only.
+
 Actions
 ~~~~~~~
 
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 2ed62f5..4c2facc 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -2696,6 +2696,10 @@  This section lists supported pattern items and their attributes, if any.
 
   - ``thresh {unsigned}``: accuracy threshold.
 
+- ``gtp``, ``gtpc``, ``gtpu``: match GTPv1 header.
+
+  - ``teid {unsigned}``: tunnel endpoint identifier.
+
 Actions list
 ^^^^^^^^^^^^
 
diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index bba6169..b1a1b97 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -309,6 +309,33 @@  enum rte_flow_item_type {
 	 * See struct rte_flow_item_fuzzy.
 	 */
 	RTE_FLOW_ITEM_TYPE_FUZZY,
+
+	/**
+	 * Matches a GTP header.
+	 *
+	 * Configure flow for GTP packets.
+	 *
+	 * See struct rte_flow_item_gtp.
+	 */
+	RTE_FLOW_ITEM_TYPE_GTP,
+
+	/**
+	 * Matches a GTP header.
+	 *
+	 * Configure flow for GTP-C packets.
+	 *
+	 * See struct rte_flow_item_gtp.
+	 */
+	RTE_FLOW_ITEM_TYPE_GTPC,
+
+	/**
+	 * Matches a GTP header.
+	 *
+	 * Configure flow for GTP-U packets.
+	 *
+	 * See struct rte_flow_item_gtp.
+	 */
+	RTE_FLOW_ITEM_TYPE_GTPU,
 };
 
 /**
@@ -735,6 +762,31 @@  static const struct rte_flow_item_fuzzy rte_flow_item_fuzzy_mask = {
 #endif
 
 /**
+ * RTE_FLOW_ITEM_TYPE_GTP.
+ *
+ * Matches a GTPv1 header.
+ */
+struct rte_flow_item_gtp {
+	/**
+	 * Version (3b), protocol type (1b), reserved (1b),
+	 * Extension header flag (1b),
+	 * Sequence number flag (1b),
+	 * N-PDU number flag (1b).
+	 */
+	uint8_t v_pt_rsv_flags;
+	uint8_t msg_type; /**< Message type. */
+	rte_be16_t msg_len; /**< Message length. */
+	rte_be32_t teid; /**< Tunnel endpoint identifier. */
+};
+
+/** Default mask for RTE_FLOW_ITEM_TYPE_GTP. */
+#ifndef __cplusplus
+static const struct rte_flow_item_gtp rte_flow_item_gtp_mask = {
+	.teid = RTE_BE32(0xffffffff),
+};
+#endif
+
+/**
  * Matching pattern item definition.
  *
  * A pattern is formed by stacking items starting from the lowest protocol