From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54]) by dpdk.org (Postfix) with ESMTP id 785B8968 for ; Thu, 7 Sep 2017 14:20:00 +0200 (CEST) Received: by mail-wm0-f54.google.com with SMTP id 137so33522796wmj.1 for ; Thu, 07 Sep 2017 05:20:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=07D5HgE1eevWVGnJ9ZXfJCDdOVc8ZN3PhWN/6Vy9wTo=; b=T7G7PpUghA5/sGmFnLXm8uJ7ZCWta+93ZQNGPif1DWQQP7nmfWFPj2VUH5aV+DHnr1 zOVMOK5xcm91Nqq7UE5Nq6daslZ5FQYblqlLeAv0mDykfgZunTrKiX+HfixCFQxsxSVP B25QHZTY+0AbRy00fCARZH9sJeSgRJAZaBXR7FyxyDqKCmVPBhOOI7VIogCxSr28YDnI Pr1nXptidv/xSHto8vTarlbMtCoP2JmFQtrCkFm5PK3Bfib2YuEMBXELfkBIjZ6O9Q3D 7Ikp5X0xbwUvO9yTLw6S1Z92ZWlFaqfq7F0NstUiLECLXdsKTg6ZJeKIuvEZbbvygDra FZRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=07D5HgE1eevWVGnJ9ZXfJCDdOVc8ZN3PhWN/6Vy9wTo=; b=h58ynvAu5CnhA/4fPxCBWxl6aScrAQsdxrFz5Lxvvb0KlATS3U6y7fBsGtziaCLP5M 4QcleMhUEIaf17ZaakTUa9rA/+E5FVtZ5HjZP4J3tzWgfD73g24ovpDMshrC9sAW5uia /5AE7CueW+a2NyqnFtEYDKt9SDh3eMg8rJj1qMFdwPuiZK9dEurLPgGMCHFkmIjXIL1t S3YXtOwLdPy7WAjtxfg+uhKejZQwaTMkW/SH71xO8cjKnJWXiKJFip2xV/bbo7JaDW8F 2LorQmwSHddO3beq9mKCr9wtJwYeg93qLy8Ay4rM8gxP1ycE5mbmTvcPVbSL0mYRNg+e nxiQ== X-Gm-Message-State: AHPjjUjybq0DNh4dGnrPywOLnv/pHjGihBz5D0SpS9NJHtxZoxbz42aw juGOQWo8izkCIVt3 X-Google-Smtp-Source: AOwi7QAnKQPEdkVgvrYISuTNbJLLVxhsnDkj9ZVknwfEP1Oisw0C3I+nR89HgdYkXVhizJlXN8wsMQ== X-Received: by 10.28.66.65 with SMTP id p62mr336212wma.159.1504786800115; Thu, 07 Sep 2017 05:20:00 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id w82sm416120wme.5.2017.09.07.05.19.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 Sep 2017 05:19:59 -0700 (PDT) Date: Thu, 7 Sep 2017 14:19:49 +0200 From: Adrien Mazarguil To: Beilei Xing Cc: jingjing.wu@intel.com, andrey.chilikin@intel.com, dev@dpdk.org Message-ID: <20170907121949.GI4301@6wind.com> References: <1503647430-93905-2-git-send-email-beilei.xing@intel.com> <1504783263-20575-1-git-send-email-beilei.xing@intel.com> <1504783263-20575-3-git-send-email-beilei.xing@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1504783263-20575-3-git-send-email-beilei.xing@intel.com> Subject: Re: [dpdk-dev] [PATCH v2 2/6] ethdev: add GTPC and GTPU items X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Sep 2017 12:20:00 -0000 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 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