From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f173.google.com (mail-wr0-f173.google.com [209.85.128.173]) by dpdk.org (Postfix) with ESMTP id D789A14E8 for ; Thu, 23 Mar 2017 20:39:37 +0100 (CET) Received: by mail-wr0-f173.google.com with SMTP id u108so154424090wrb.3 for ; Thu, 23 Mar 2017 12:39:37 -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=etf9P8s6PgjkF7pp8xLQEr2v9frw46t2QGgVfBsKM3M=; b=pSbLGvn1bvqXtIZjvrfJjSSUtXIqbvpPZ0+pwlznRBc11A0j4/V76qH03KBcJIoqHw /kJbWPupu5PHJ4x88i9i0btN7OXLd/wd3FoHf9zmRasSBfONMMP5eH2XYlPOX6UwwpKB /96lYahbSWfyr8PcJPbqcGAqzR9GrxliXbnwcv520F7eBM1VUFYA+g88AIi2DS+s1WlG UTnsgodzx+N8ij7oHpFmzFx5+FdnEWf80kZxJE6mfhG+ZYnz6AgmuUa2bfKf5dCOvtGY btukTT+hrpPpGeUEqp+RnrvAhByrrtXIZ9FfuNIZx8u19Wndgmzt32OU5k2dZNikKncl 3kgw== 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=etf9P8s6PgjkF7pp8xLQEr2v9frw46t2QGgVfBsKM3M=; b=hXi5E3nnNEy7PPA6Y3xGlueIzBhWmG7iFJKVudxIecwUMYrX7NWxF8dUAD4yPwngC7 WYJ/dCp8izIQYrLoKxLiz0EIXuPm7izYrEjZWp3+dApJ2eYLVl+AzwvToYypqXrseRja p69Z58l+54z+4HtPcd/wMoqvHfb7sOnSGW6I6YJEQMQBXC94yjFd5qYM3WkKH2hetMYL elPMpxYQON+FZJ8IeROJ95Zou/tNmKhnQj3DID18kIjXnRduWrYQzKEtr1o1nOd3bW65 a/1bKCyt4+DWQBjmTzVo8fYT2mMi9KUGTfpJCjFL+7f4/+rVhlpAyTzAD0w4DBh24rjQ xmpQ== X-Gm-Message-State: AFeK/H15FmSSph7mk3hSB46zwk/KH7rL5eDbhxuX4UjnZsTlA7Muo495FjtTaqXWQqm3CFuC X-Received: by 10.223.150.123 with SMTP id c56mr4089363wra.202.1490297977439; Thu, 23 Mar 2017 12:39:37 -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 11sm9897wrb.10.2017.03.23.12.39.36 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 23 Mar 2017 12:39:36 -0700 (PDT) Date: Thu, 23 Mar 2017 20:39:28 +0100 From: Adrien Mazarguil To: Beilei Xing Cc: jingjing.wu@intel.com, helin.zhang@intel.com, dev@dpdk.org Message-ID: <20170323193928.GR3790@6wind.com> References: <1488534236-29904-1-git-send-email-beilei.xing@intel.com> <1490266669-122137-1-git-send-email-beilei.xing@intel.com> <1490266669-122137-2-git-send-email-beilei.xing@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1490266669-122137-2-git-send-email-beilei.xing@intel.com> Subject: Re: [dpdk-dev] [PATCH v2 1/3] app/testpmd: add support for MPLS and GRE 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, 23 Mar 2017 19:39:38 -0000 Hi Beilei, On Thu, Mar 23, 2017 at 06:57:47PM +0800, Beilei Xing wrote: > This patch adds sopport for MPLS and GRE items to generic filter > API. Besides the typo, my previous comment about the title line still stands ("app/testpmd" is not accurate regarding the intent of this patch). > > Signed-off-by: Beilei Xing > --- > app/test-pmd/cmdline_flow.c | 46 +++++++++++++++++++++++++++++ > app/test-pmd/config.c | 2 ++ > doc/guides/prog_guide/rte_flow.rst | 20 +++++++++++-- > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 8 +++++ > lib/librte_ether/rte_flow.h | 45 ++++++++++++++++++++++++++++ > 5 files changed, 119 insertions(+), 2 deletions(-) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index ff98690..ff1e47c 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -159,6 +159,10 @@ enum index { > ITEM_SCTP_CKSUM, > ITEM_VXLAN, > ITEM_VXLAN_VNI, > + ITEM_MPLS, > + ITEM_MPLS_LABEL, > + ITEM_GRE, > + ITEM_GRE_PROTO, > > /* Validate/create actions. */ > ACTIONS, > @@ -432,6 +436,8 @@ static const enum index next_item[] = { > ITEM_TCP, > ITEM_SCTP, > ITEM_VXLAN, > + ITEM_MPLS, > + ITEM_GRE, > ZERO, > }; > > @@ -538,6 +544,18 @@ static const enum index item_vxlan[] = { > ZERO, > }; > > +static const enum index item_mpls[] = { > + ITEM_MPLS_LABEL, > + ITEM_NEXT, > + ZERO, > +}; > + > +static const enum index item_gre[] = { > + ITEM_GRE_PROTO, > + ITEM_NEXT, > + ZERO, > +}; > + > static const enum index next_action[] = { > ACTION_END, > ACTION_VOID, > @@ -1279,6 +1297,34 @@ static const struct token token_list[] = { > .next = NEXT(item_vxlan, NEXT_ENTRY(UNSIGNED), item_param), > .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_vxlan, vni)), > }, > + [ITEM_MPLS] = { > + .name = "mpls", > + .help = "match MPLS header", > + .priv = PRIV_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)), > + .next = NEXT(item_mpls), > + .call = parse_vc, > + }, > + [ITEM_MPLS_LABEL] = { > + .name = "label", I didn't catch this during my previous review, naming it "label" is wrong because users will actually specify the entire "label_tc_s_ttl" field at once. If you really want to name it "label" with the intent of only assigning the initial 20b, have a look at ITEM_IPV6_TC and ITEM_IPV6_FLOW entries in the same array. > + .help = "MPLS label", > + .next = NEXT(item_mpls, NEXT_ENTRY(UNSIGNED), item_param), > + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_mpls, > + label_tc_s_ttl)), > + }, > + [ITEM_GRE] = { > + .name = "gre", > + .help = "match GRE header", > + .priv = PRIV_ITEM(GRE, sizeof(struct rte_flow_item_gre)), > + .next = NEXT(item_gre), > + .call = parse_vc, > + }, > + [ITEM_GRE_PROTO] = { > + .name = "protocol", > + .help = "GRE protocol type", > + .next = NEXT(item_gre, NEXT_ENTRY(UNSIGNED), item_param), > + .args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre, > + protocol)), > + }, > /* Validate/create actions. */ > [ACTIONS] = { > .name = "actions", > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index eb3d572..90d153e 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -963,6 +963,8 @@ static const struct { > MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)), > MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)), > MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)), > + MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)), > + MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)), > }; > > /** 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 3bf8871..45897cd 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -187,8 +187,8 @@ Pattern item > Pattern items fall in two categories: > > - Matching protocol headers and packet data (ANY, RAW, ETH, VLAN, IPV4, > - IPV6, ICMP, UDP, TCP, SCTP, VXLAN and so on), usually associated with a > - specification structure. > + IPV6, ICMP, UDP, TCP, SCTP, VXLAN, MPLS, GRE and so on), usually > + associated with a specification structure. > > - Matching meta-data or affecting pattern processing (END, VOID, INVERT, PF, > VF, PORT and so on), often without a specification structure. > @@ -863,6 +863,22 @@ Matches a VXLAN header (RFC 7348). > - ``rsvd1``: reserved, normally 0x00. > - Default ``mask`` matches VNI only. > > +Item: ``MPLS`` > +^^^^^^^^^^^^^^ > + > +Matches a MPLS header. > + > +- ``label_tc_s_ttl``: Lable, TC, Bottom of Stack and TTL. Typo, "Lable". Also a minor comment, this word should be lower case since it comes after a colon. > +- Default ``mask`` matches label only. > + > +Item: ``GRE`` > +^^^^^^^^^^^^^^ > + > +Matches a GRE header. > + > +- ``c_rsvd0_ver``: Checksum, reserved 0 and version. > +- ``protocol``: Protocol type. "Checksum" and "Protocol" should be lower case. You must define a default mask as well. > +[ > Actions > ~~~~~~~ > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index bdc6a14..f9fa5d6 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -2453,6 +2453,14 @@ This section lists supported pattern items and their attributes, if any. > > - ``vni {unsigned}``: VXLAN identifier. > > +- ``mpls``: match MPLS header. > + > + - ``label {unsigned}``: MPLS label. > + > +- ``gre``: match GRE header. > + > + - ``protocol {unsigned}``: Protocol type. > + Indentation issue, also "Protocol" should be lower case. > Actions list > ^^^^^^^^^^^^ > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index 171a569..f855090 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -282,6 +282,20 @@ enum rte_flow_item_type { > * See struct rte_flow_item_nvgre. > */ > RTE_FLOW_ITEM_TYPE_NVGRE, > + > + /** > + * Matches a MPLS header. > + * > + * See struct rte_flow_item_mpls. > + */ > + RTE_FLOW_ITEM_TYPE_MPLS, > + > + /** > + * Matches a GRE header. > + * > + * See struct rte_flow_item_gre. > + */ > + RTE_FLOW_ITEM_TYPE_GRE, > }; > > /** > @@ -599,6 +613,37 @@ struct rte_flow_item_nvgre { > }; > > /** > + * RTE_FLOW_ITEM_TYPE_MPLS. > + * > + * Matches a MPLS header. > + */ > +struct rte_flow_item_mpls { > + /** > + * Lable (20b), TC (3b), Bottom of Stack (1b), TTL (8b). Typo, "Lable". > + */ > + uint32_t label_tc_s_ttl; > +}; > + > +/** Default mask for RTE_FLOW_ITEM_TYPE_MPLS. */ > +static const struct rte_flow_item_mpls rte_flow_item_mpls_mask = { > + .label_tc_s_ttl = 0xfffff, > +}; This default mask is wrong, it has to be specified in network order (you can include rte_byteorder.h if you need some #ifdef). > + > +/** > + * RTE_FLOW_ITEM_TYPE_GRE. > + * > + * Matches a GRE header. > + */ > +struct rte_flow_item_gre { > + /** > + * Checksum (1b), reserved 0 (12b), version (3b). > + * Refer to RFC 2784. > + */ > + uint16_t c_rsvd0_ver; > + uint16_t protocol; /**< Protocol type. */ > +}; Default mask is missing, you must add one. > + > +/** > * Matching pattern item definition. > * > * A pattern is formed by stacking items starting from the lowest protocol > -- > 2.5.5 > -- Adrien Mazarguil 6WIND