From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v2 1/3] app/testpmd: add support for MPLS and GRE items Date: Fri, 24 Mar 2017 11:00:12 +0100 Message-ID: <20170324100012.GT3790@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> <20170323193928.GR3790@6wind.com> <94479800C636CB44BD422CB454846E01315BCF52@SHSMSX101.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "Wu, Jingjing" , "Zhang, Helin" , "dev@dpdk.org" To: "Xing, Beilei" 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 26A24CFA6 for ; Fri, 24 Mar 2017 11:00:22 +0100 (CET) Received: by mail-wm0-f54.google.com with SMTP id t189so8922897wmt.1 for ; Fri, 24 Mar 2017 03:00:22 -0700 (PDT) Content-Disposition: inline In-Reply-To: <94479800C636CB44BD422CB454846E01315BCF52@SHSMSX101.ccr.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Mar 24, 2017 at 02:17:42AM +0000, Xing, Beilei wrote: > Hi Adrien, > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Friday, March 24, 2017 3:39 AM > > To: Xing, Beilei > > Cc: Wu, Jingjing ; Zhang, Helin > > ; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v2 1/3] app/testpmd: add support for MPLS > > and GRE items > > > > 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). > > > > OK, I see. > I think it's better to split this patch into two patches, one is for rte_flow, and the other is for testpmd, that'll be more clear. I prefer having the new API stuff, documentation and testpmd implementation at once especially since it's not a large patch, but it's your call. > > > > > > 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. > > > > Agree, thanks. > > > > + .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. > > > > Got it, thanks. > > > > +- 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. > > Yes. > > > > > > +[ > > > 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. > > Yes. > > > > > > 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). > > OK, will fix in next version. Another note, you could use uint8_t[] to mitigate the endianness issue, e.g.: struct rte_flow_item_mpls { /** * Label (20b), TC (3b), Bottom of Stack (1b). */ uint8_t label_tc_s[3]; uint8_t ttl; /** Time-to-Live. */ }; /** Default mask for RTE_FLOW_ITEM_TYPE_MPLS. */ static const struct rte_flow_item_mpls rte_flow_item_mpls_mask = { .label_tc_s = "\xff\xff\xf0", }; > > > > > > + > > > +/** > > > + * 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. > > OK. > > > > > > + > > > +/** > > > * Matching pattern item definition. > > > * > > > * A pattern is formed by stacking items starting from the lowest > > > protocol > > > -- > > > 2.5.5 > > > > > > > -- > > Adrien Mazarguil > > 6WIND -- Adrien Mazarguil 6WIND