From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 6362714E8 for ; Fri, 24 Mar 2017 03:19:49 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490321989; x=1521857989; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=Hl+t3ZlfFalqiBFsFZkQfvNVi5iOXFZxAGG+UjyauY0=; b=j9pkSi7ZPn+HFPnjdE1YJsIWuNd9jLtgsdPf14QJPq9ixQGQLV4aDua5 9YKbmfk0oc5s4/sF1b6EuylwpoBXTg==; Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Mar 2017 19:19:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,213,1486454400"; d="scan'208";a="947662736" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga003.jf.intel.com with ESMTP; 23 Mar 2017 19:19:47 -0700 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 23 Mar 2017 19:19:47 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 23 Mar 2017 19:19:46 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.224]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.82]) with mapi id 14.03.0248.002; Fri, 24 Mar 2017 10:17:43 +0800 From: "Xing, Beilei" To: Adrien Mazarguil CC: "Wu, Jingjing" , "Zhang, Helin" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2 1/3] app/testpmd: add support for MPLS and GRE items Thread-Index: AQHSpA1A7/o7Ysy72UmuxQoCW9qFBqGjPcRg Date: Fri, 24 Mar 2017 02:17:42 +0000 Message-ID: <94479800C636CB44BD422CB454846E01315BCF52@SHSMSX101.ccr.corp.intel.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> In-Reply-To: <20170323193928.GR3790@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Fri, 24 Mar 2017 02:19:50 -0000 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 >=20 > Hi Beilei, >=20 > 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. >=20 > Besides the typo, my previous comment about the title line still stands > ("app/testpmd" is not accurate regarding the intent of this patch). >=20 OK, I see. I think it's better to split this patch into two patches, one is for rte_fl= ow, and the other is for testpmd, that'll be more clear. > > > > 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[] =3D { > > ITEM_TCP, > > ITEM_SCTP, > > ITEM_VXLAN, > > + ITEM_MPLS, > > + ITEM_GRE, > > ZERO, > > }; > > > > @@ -538,6 +544,18 @@ static const enum index item_vxlan[] =3D { > > ZERO, > > }; > > > > +static const enum index item_mpls[] =3D { > > + ITEM_MPLS_LABEL, > > + ITEM_NEXT, > > + ZERO, > > +}; > > + > > +static const enum index item_gre[] =3D { > > + ITEM_GRE_PROTO, > > + ITEM_NEXT, > > + ZERO, > > +}; > > + > > static const enum index next_action[] =3D { > > ACTION_END, > > ACTION_VOID, > > @@ -1279,6 +1297,34 @@ static const struct token token_list[] =3D { > > .next =3D NEXT(item_vxlan, NEXT_ENTRY(UNSIGNED), > item_param), > > .args =3D ARGS(ARGS_ENTRY_HTON(struct > rte_flow_item_vxlan, vni)), > > }, > > + [ITEM_MPLS] =3D { > > + .name =3D "mpls", > > + .help =3D "match MPLS header", > > + .priv =3D PRIV_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)), > > + .next =3D NEXT(item_mpls), > > + .call =3D parse_vc, > > + }, > > + [ITEM_MPLS_LABEL] =3D { > > + .name =3D "label", >=20 > 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. >=20 > If you really want to name it "label" with the intent of only assigning t= he initial > 20b, have a look at ITEM_IPV6_TC and ITEM_IPV6_FLOW entries in the same > array. > Agree, thanks. =20 > > + .help =3D "MPLS label", > > + .next =3D NEXT(item_mpls, NEXT_ENTRY(UNSIGNED), > item_param), > > + .args =3D ARGS(ARGS_ENTRY_HTON(struct > rte_flow_item_mpls, > > + label_tc_s_ttl)), > > + }, > > + [ITEM_GRE] =3D { > > + .name =3D "gre", > > + .help =3D "match GRE header", > > + .priv =3D PRIV_ITEM(GRE, sizeof(struct rte_flow_item_gre)), > > + .next =3D NEXT(item_gre), > > + .call =3D parse_vc, > > + }, > > + [ITEM_GRE_PROTO] =3D { > > + .name =3D "protocol", > > + .help =3D "GRE protocol type", > > + .next =3D NEXT(item_gre, NEXT_ENTRY(UNSIGNED), > item_param), > > + .args =3D ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre, > > + protocol)), > > + }, > > /* Validate/create actions. */ > > [ACTIONS] =3D { > > .name =3D "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, INVER= T, > 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. >=20 > Typo, "Lable". Also a minor comment, this word should be lower case since= it > comes after a colon. >=20 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. >=20 > "Checksum" and "Protocol" should be lower case. >=20 > You must define a default mask as well. Yes. >=20 > > +[ > > 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. > > + >=20 > Indentation issue, also "Protocol" should be lower case. Yes. >=20 > > 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). >=20 > Typo, "Lable". >=20 > > + */ > > + 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 =3D { > > + .label_tc_s_ttl =3D 0xfffff, > > +}; >=20 > 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. >=20 > > + > > +/** > > + * 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. */ }; >=20 > Default mask is missing, you must add one. OK. >=20 > > + > > +/** > > * Matching pattern item definition. > > * > > * A pattern is formed by stacking items starting from the lowest > > protocol > > -- > > 2.5.5 > > >=20 > -- > Adrien Mazarguil > 6WIND