From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D8C37C3A59D for ; Mon, 19 Aug 2019 11:53:19 +0000 (UTC) Received: from dpdk.org (dpdk.org [92.243.14.124]) by mail.kernel.org (Postfix) with ESMTP id 619222085A for ; Mon, 19 Aug 2019 11:53:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 619222085A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=dev-bounces@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 66E221BEB9; Mon, 19 Aug 2019 13:53:18 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 4C4AB1BE82 for ; Mon, 19 Aug 2019 13:53:17 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Aug 2019 04:53:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,403,1559545200"; d="scan'208";a="168725886" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga007.jf.intel.com with ESMTP; 19 Aug 2019 04:53:15 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 19 Aug 2019 04:53:15 -0700 Received: from shsmsx105.ccr.corp.intel.com ([169.254.11.15]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.139]) with mapi id 14.03.0439.000; Mon, 19 Aug 2019 19:53:13 +0800 From: "Zhang, Qi Z" To: Adrien Mazarguil , "Wang, Ying A" CC: "Ye, Xiaolong" , "Yang, Qiming" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] ethdev: add more protocol support in flow API Thread-Index: AQHVUlICN/8/o5O8Fky/FdPloYtb7qb51NYAgAiNZzA= Date: Mon, 19 Aug 2019 11:53:12 +0000 Message-ID: <039ED4275CED7440929022BC67E7061153D80195@SHSMSX105.ccr.corp.intel.com> References: <20190814030018.373628-1-ying.a.wang@intel.com> <20190814032430.404190-1-ying.a.wang@intel.com> <20190814090810.GM4512@6wind.com> In-Reply-To: <20190814090810.GM4512@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjczMzhmNzUtODhhMy00ZWI1LWFiMDAtNzhlM2I2Y2YwMTllIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiaVRQQmdpYjhXV0hqQzhzcDc0WFFCRU44TElKWG9kQ05qSGI0Q0pTYTZ1Syt0cU5zSXdaTzhvOWMzYUZEQlQrTiJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action 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] ethdev: add more protocol support in flow API 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Wednesday, August 14, 2019 5:08 PM > To: Wang, Ying A > Cc: Zhang, Qi Z ; Ye, Xiaolong ; > Yang, Qiming ; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: add more protocol support in f= low > API >=20 > Hi Wang Ying, >=20 > On Wed, Aug 14, 2019 at 11:24:30AM +0800, Wang Ying A wrote: > > Add new protocol header match support as below > > > > RTE_FLOW_ITEM_TYPE_GTP_PSC > > - matches a GTP PDU extension header (type is 0x85: > > PDU Session Container) > > RTE_FLOW_ITEM_TYPE_PPPOES > > - matches a PPPoE Session header. > > RTE_FLOW_ITEM_TYPE_PPPOED > > - matches a PPPoE Discovery stage header. > > > > Signed-off-by: Wang Ying A >=20 > OK, please split this patch, one for GTP and the other for PPPoE to make = title > less vague than "add more protocol support". >=20 > For PPPoE, the distinction between session and discovery is not a bad ide= a but > since discovery packets typically have a different format (tags in place = of > protocol), I'm not sure they should share a common structure. >=20 > How about a single "PPPOE" item without proto_id to cover both session an= d > discovery, then later add separate tag items on a needed basis for each > possible/optional tag (e.g. PPPOE_TAG_SERVICE_NAME)? Likewise proto_id > would be provided through a separate optional item if relevant > (PPPOE_PROTO_ID). PPPoE Discovery had PPPoE Session has different ethertype 0x8863 , 0x8864, = so I think they should belong to separate match item,( image the case we only need to match a PPPoE Session flow but = don't care what kind of protocol payload it have ) but I agree they should only share the common part of the header, then for PPPoED, it can append optional PPPoE Tag item(s) and for PPPoES , = it can append optional PPPoE Protocol item(s) > Such an approach is already used for IPV6 and IPV6_EXT. >=20 > Another benefit is that applications could match PPPoE regardless of sess= ion or > discovery when they do not care, while PPPOED/PPPOES make that distinctio= n > mandatory. >=20 > Also by inserting new entries in the middle of the pattern items list, th= is patch > breaks ABI. I think it's not on purpose, so please move them to the end (= not > grouping them with existing GTP stuff is fine, ABI compat is more > important.) This must be reflected in rte_flow.h, rte_flow.c, testpmd and > documentation. >=20 > More nits below. >=20 > > --- > > --- > > v2: Remove Gerrit Change-Id's. > > --- > > app/test-pmd/cmdline_flow.c | 80 > +++++++++++++++++++++++++++++ > > doc/guides/prog_guide/rte_flow.rst | 25 +++++++++ > > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 10 ++++ > > lib/librte_ethdev/rte_flow.c | 3 ++ > > lib/librte_ethdev/rte_flow.h | 71 > +++++++++++++++++++++++++ > > 5 files changed, 189 insertions(+) > > > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > [...] > > + [ITEM_PPPOES] =3D { > > + .name =3D "pppoes", > > + .help =3D "match PPPoE Session header", >=20 > Session =3D> session >=20 > > + .priv =3D PRIV_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)), > > + .next =3D NEXT(item_pppoe), > > + .call =3D parse_vc, > > + }, > > + [ITEM_PPPOED] =3D { > > + .name =3D "pppoed", > > + .help =3D "match PPPoE Discovery stage header", >=20 > Discovery =3D> discovery >=20 > > + .priv =3D PRIV_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)), > > + .next =3D NEXT(item_pppoe), > > + .call =3D parse_vc, > > + }, > > + [ITEM_PPPOE_SEID] =3D { > > + .name =3D "seid", > > + .help =3D "Session identifier", >=20 > Session =3D> session >=20 > [...] > > diff --git a/doc/guides/prog_guide/rte_flow.rst > > b/doc/guides/prog_guide/rte_flow.rst > > index 821b524b3..d09c42071 100644 > > --- a/doc/guides/prog_guide/rte_flow.rst > > +++ b/doc/guides/prog_guide/rte_flow.rst > > @@ -1055,6 +1055,31 @@ flow rules. > > - ``teid``: tunnel endpoint identifier. > > - Default ``mask`` matches teid only. > > > > +Item: ``GTP_PSC`` > > +^^^^^^^^^^^^^^^^^^^^^^^ >=20 > Too many "^^^"'s. >=20 > [...] > > +Item: ``PPPOES``, ``PPPOED`` > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Matches a PPPOE header. >=20 > PPPOE =3D> PPPoE >=20 > > + > > +Note: PPPOES and PPPOED use the same structure. PPPOES and PPPOED > > +item >=20 > item =3D> items (or better, "pattern items") >=20 > > +are defined for a user-friendly API when creating PPPOE-Session and > > +PPPOE-Discovery flow rules. >=20 > Super nit: use "PPPoE" when mentioning the protocol itself and "PPPOE" wh= en > mentioning the pattern item. >=20 > > + > > +- ``v_t_flags``: version (4b), type (4b). >=20 > Why "flags"? I don't see any so you could name it "version_type" (same in > documentation). >=20 > > +- ``code``: Message type. >=20 > Message =3D> message >=20 > > +- ``session_id``: Session identifier. >=20 > Session =3D> session >=20 > > +- ``length``: Payload length. >=20 > Payload =3D> payload >=20 > > +- ``proto_id``: PPP Protocol identifier. >=20 > Protocol =3D> protocol >=20 > > +- Default ``mask`` matches session_id,proto_id. >=20 > Missing space between session_id and proto_id. >=20 > > + > > Item: ``ESP`` > > ^^^^^^^^^^^^^ > > > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > index 313e0707e..0da36d5f1 100644 > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > @@ -3904,6 +3904,16 @@ This section lists supported pattern items and > their attributes, if any. > > > > - ``teid {unsigned}``: tunnel endpoint identifier. > > > > +- ``gtp_psc``: match GTPv1 entension header (type is 0x85). > > + > > + - ``pdu_type {unsigned}``: PDU type (0 or 1). > > + - ``qfi {unsigned}``: QoS flow identifier. > > + > > +- ``pppoes``, ``pppoed``: match PPPOE header. >=20 > PPPOE =3D> PPPoE >=20 > [...] > > diff --git a/lib/librte_ethdev/rte_flow.h > > b/lib/librte_ethdev/rte_flow.h index b66bf1495..ad5e46190 100644 > > --- a/lib/librte_ethdev/rte_flow.h > > +++ b/lib/librte_ethdev/rte_flow.h > > @@ -328,6 +328,34 @@ enum rte_flow_item_type { > > */ > > RTE_FLOW_ITEM_TYPE_GTPU, > > > > + /** > > + * Matches a GTP PDU extension header (type is 0x85: > > + * PDU Session Container). >=20 > Session Container =3D> session container >=20 > > + * > > + * Configure flow for GTP packets with extension header type 0x85. > > + * > > + * See struct rte_flow_item_gtp_psc. > > + */ > > + RTE_FLOW_ITEM_TYPE_GTP_PSC, > > + > > + /** > > + * Matches a PPPOE header. > > + * > > + * Configure flow for PPPoE Session packets. >=20 > Session =3D> session >=20 > > + * > > + * See struct rte_flow_item_pppoe. > > + */ > > + RTE_FLOW_ITEM_TYPE_PPPOES, > > + > > + /** > > + * Matches a PPPOE header. > > + * > > + * Configure flow for PPPoE Discovery stage packets. >=20 > Discovery =3D> discovery >=20 > > + * > > + * See struct rte_flow_item_pppoe. > > + */ > > + RTE_FLOW_ITEM_TYPE_PPPOED, > > + > > /** > > * Matches a ESP header. > > * > > @@ -922,6 +950,49 @@ static const struct rte_flow_item_gtp > > rte_flow_item_gtp_mask =3D { }; #endif > > > > +/** > > + * RTE_FLOW_ITEM_TYPE_GTP_PSC. > > + * > > + * Matches a GTP-extension header > > + * (type is 0x85: PDU Session Container). >=20 > Session Container =3D> session container >=20 > (crusade against superfluous caps!) >=20 > [...] > > +/** > > + * RTE_FLOW_ITEM_TYPE_PPPOE. > > + * > > + * Matches a PPPOE header. > > + */ > > +struct rte_flow_item_pppoe { > > + /** > > + * Version (4b), type (4b). > > + */ > > + uint8_t v_t_flags; >=20 > v_t_flags =3D> version_type >=20 > > + uint8_t code; /**< Message type. */ > > + rte_be16_t session_id; /**< Session identifier. */ > > + rte_be16_t length; /**< Payload length. */ > > + rte_be16_t proto_id; /**< PPP Protocol identifier. */ >=20 > As discussed, I suggest dropping proto_id to make this a generic match fo= r > PPPoE. >=20 > > +}; > > + > > +/** Default mask for RTE_FLOW_ITEM_TYPE_PPPOE. */ #ifndef __cplusplus > > +static const struct rte_flow_item_pppoe rte_flow_item_pppoe_mask =3D { > > + .session_id =3D RTE_BE16(0xffff), > > + .proto_id =3D RTE_BE16(0xffff), > > +}; >=20 > I think the default for PPPoE should be an empty mask that simply says "m= atch > PPPoE" since session_id only becomes known after negotiation and proto_id > shouldn't be part of this object. >=20 > -- > Adrien Mazarguil > 6WIND