Hi Beilei, I assume this patch supersedes [1]?
[1] http://dpdk.org/ml/archives/dev/2017-August/073501.html Thanks for merging testpmd and the API change as a single patch, I still have a few comments, see below. (please add "flow API" somewhere in the title by the way) On Thu, Sep 07, 2017 at 07:20:59PM +0800, Beilei Xing wrote: > This patch adds GTPC and GTPU items to generic rte > flow, and also exposes the following item fields > through the flow command: > > - GTPC TEID > - GTPU TEID > > Signed-off-by: Beilei Xing <beilei.x...@intel.com> Won't there be a need to match nonspecific GTP traffic as well (both GTP-C and GTP-U a once), since they use the same structure? I'm not familiar with the protocol at all so I wonder if you should maybe leave the GTP item in addition to those two. > --- > app/test-pmd/cmdline_flow.c | 44 > +++++++++++++++++++++++++++++ > app/test-pmd/config.c | 2 ++ > doc/guides/prog_guide/rte_flow.rst | 26 +++++++++++++++++ > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 8 ++++++ > lib/librte_ether/rte_flow.h | 44 > +++++++++++++++++++++++++++++ > 5 files changed, 124 insertions(+) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index a17a004..72d159c 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_GTPC, > + ITEM_GTPC_TEID, > + ITEM_GTPU, > + ITEM_GTPU_TEID, You could refactor the TEID parameter since they use the same structure. Might be useful if you add nonspecific GTP: ITEM_GTP, ITEM_GTP_TEID, ITEM_GTPC, ITEM_GTPU, > > /* Validate/create actions. */ > ACTIONS, > @@ -451,6 +455,8 @@ static const enum index next_item[] = { > ITEM_MPLS, > ITEM_GRE, > ITEM_FUZZY, > + ITEM_GTPC, > + ITEM_GTPU, > ZERO, > }; > > @@ -588,6 +594,18 @@ static const enum index item_gre[] = { > ZERO, > }; > > +static const enum index item_gtpc[] = { > + ITEM_GTPC_TEID, > + ITEM_NEXT, > + ZERO, > +}; > + > +static const enum index item_gtpu[] = { > + ITEM_GTPU_TEID, > + ITEM_NEXT, > + ZERO, > +}; A single array is necessary, item_gtp[]. > + > static const enum index next_action[] = { > ACTION_END, > ACTION_VOID, > @@ -1421,6 +1439,32 @@ static const struct token token_list[] = { > .args = ARGS(ARGS_ENTRY(struct rte_flow_item_fuzzy, > thresh)), > }, > + [ITEM_GTPC] = { > + .name = "gtpc", > + .help = "match GTP header", > + .priv = PRIV_ITEM(GTPC, sizeof(struct rte_flow_item_gtp)), > + .next = NEXT(item_gtpc), > + .call = parse_vc, > + }, > + [ITEM_GTPC_TEID] = { > + .name = "teid", > + .help = "TUNNEL ENDPOINT IDENTIFIER", Please don't shout, "tunnel endpoint identifier" is fine. > + .next = NEXT(item_gtpc, NEXT_ENTRY(UNSIGNED), item_param), > + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gtp, teid)), > + }, > + [ITEM_GTPU] = { > + .name = "gtpu", > + .help = "match GTP header", > + .priv = PRIV_ITEM(GTPU, sizeof(struct rte_flow_item_gtp)), > + .next = NEXT(item_gtpu), > + .call = parse_vc, > + }, > + [ITEM_GTPU_TEID] = { > + .name = "teid", > + .help = "TUNNEL ENDPOINT IDENTIFIER", Same comment here, however the a single TEID entry is necessary as previously described. > + .next = NEXT(item_gtpu, NEXT_ENTRY(UNSIGNED), item_param), > + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gtp, teid)), > + }, > > /* Validate/create actions. */ > [ACTIONS] = { > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index 3ae3e1c..be4c3b9 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -947,6 +947,8 @@ 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)), Remember to add GTP here assuming it makes sense. > + 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..9e7179a 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -955,6 +955,32 @@ Usage example, fuzzy match a TCPv4 packets: > | 4 | END | > +-------+----------+ > > +Item: ``GTPC`` > +^^^^^^^^^^^^^^ > + > +Matches a GTP header. > + > +- ``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``: TEID. > +- Default ``mask`` matches teid only. > + > +Item: ``GTPU`` > +^^^^^^^^^^^^^^ > + > +Matches a GTP header. > + > +- ``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``: TEID. > +- Default ``mask`` matches teid only. > + You can use a single section to describe all three items at once since they map to a common structure: Item: ``GTP``, ``GTPC``, ``GTPU``: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Then elaborate a bit on the the differences between them. > Actions > ~~~~~~~ > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index 2ed62f5..2ca36ad 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -2696,6 +2696,14 @@ This section lists supported pattern items and their > attributes, if any. > > - ``thresh {unsigned}``: accuracy threshold. > > +- ``gtpc``: match GTP header. > + > + - ``teid {unsigned}``: Tunnel endpoint identifier. Tunnel => tunnel > + > +- ``gtpu``: match GTP header. > + > + - ``teid {unsigned}``: Tunnel endpoint identifier. You could also merge all three items here, e.g.: - ``gtp``, ``gtpc``, ``gtpu``: ... > + > Actions list > ^^^^^^^^^^^^ > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index bba6169..8b24cac 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -309,6 +309,24 @@ enum rte_flow_item_type { > * See struct rte_flow_item_fuzzy. > */ > RTE_FLOW_ITEM_TYPE_FUZZY, > + > + /** > + * Matches a GTP header. Write "GTP-U" to make clear this is not nonspecific "GTP" matching. > + * > + * Configure flow for GTP-C packets. > + * > + * See struct rte_flow_item_gtp. > + */ > + RTE_FLOW_ITEM_TYPE_GTPC, > + > + /** > + * Matches a GTP header. "GTP-C" here. > + * > + * Configure flow for GTP-U packets. > + * > + * See struct rte_flow_item_gtp. > + */ > + RTE_FLOW_ITEM_TYPE_GTPU, > }; > > /** > @@ -735,6 +753,32 @@ static const struct rte_flow_item_fuzzy > rte_flow_item_fuzzy_mask = { > #endif > > /** > + * RTE_FLOW_ITEM_TYPE_GTP. You need to mention the others, something like: RTE_FLOW_ITEM_TYPE_GTP, RTE_FLOW_ITEM_TYPE_GTPC and RTE_FLOW_ITEM_TYPE_GTPU. > + * > + * Matches a GTP header. Similarly: Matches a nonspecific GTP, a GTP-C or a GTP-U header. > + */ > +struct rte_flow_item_gtp { > + /** > + * Version (2b), protocol type (1b), reserved (1b), > + * Extension header flag (1b), > + * Sequence number flag (1b), Extension => extension sequence => sequence > + * 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 = { > + .msg_type = 0x00, The above field is not necessary since you're not initializing the entire structure, the rest is set to 0 by default. > + .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 6WIND
