From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5ECB6A0471 for ; Wed, 14 Aug 2019 11:08:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2F723378B; Wed, 14 Aug 2019 11:08:13 +0200 (CEST) Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id 7B495375B for ; Wed, 14 Aug 2019 11:08:12 +0200 (CEST) Received: by mail-wr1-f66.google.com with SMTP id b16so13648718wrq.9 for ; Wed, 14 Aug 2019 02:08:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=yqGnQYVPnf6J7G76LzbfwGO3Jy7odBXUQNich9HRYzI=; b=XFnZrqsDQRbKLJC8QCcEVRapp/tJABp9BNxbEvmRWfsuT7QMqzvlZApgIHm/m9u84H YrPVnAM3wAEdmbAzZnQNfVV+L+FQPMB+nepfeUAW6wYoTNsfp2we1RUfK/Dg/LosgPBq senmjAUtub9KmJx1P0h1Ul+GhX4LDOPNrsDVijLYc0sJPHLgsjTq6dIBVctCGUy8RIAU FPYtqp+qZn4oYw+qb09XpJZ20PNmeQSzqBnankDJY5cwkMUISvXNwiZqSRlS2sK7iTZr aPWEhUs0uSUPrK/qlmsL8KrWkrLQnYM3BFtnsKNyM1ORXUdUzkXzzyMGBIZ9APQ659z5 gh7A== 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=yqGnQYVPnf6J7G76LzbfwGO3Jy7odBXUQNich9HRYzI=; b=ZpkXLG8UwfrFuiH8gzob4lMlwB4XgaevlCSd0OojKW6rGxZPacM8z12XA5+vyyOFdR SNvaS9EMZh/g6k1ALCvVYbcyi+49jQaR1HU+hS4RnAq4vo7yq2y7mvj2zWQ2ctkDYcPn GwBphdzKhGDfk2jdZSScz9wKo8pEeRsnPD3Cv0N8YMdr/8102rMSCMVYrESm3Y4Uv5x3 UabPhPT2Kn+HA96KXUspQBhOCma+U2VaEBgs8w0JWXVTnsoFWnC6ZmR1rZtENBpie0Tn hNmgWyt8xMQDW9+wRPWjsSyhf+P+y+nOjAUDwPGk3tGkiEsKl41sMyb+pJio6YlUyL2b Oqlw== X-Gm-Message-State: APjAAAVjAuf16ag/8UcCtMQz2thqMC9BtJqI39AEQ31BjPr4gdbFVgrQ 7PU1i7Z4Z3Zg6ETb+JWnJy5VtA== X-Google-Smtp-Source: APXvYqxVO4vsN6MrI8rOIJYDQkjOYy73YbwWud3Tp+TAsG4t566mJOYhwF3oqhtclrjau1i70QNaHw== X-Received: by 2002:adf:e50b:: with SMTP id j11mr14487870wrm.65.1565773692095; Wed, 14 Aug 2019 02:08:12 -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 t63sm4902022wmt.6.2019.08.14.02.08.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 14 Aug 2019 02:08:11 -0700 (PDT) Date: Wed, 14 Aug 2019 11:08:10 +0200 From: Adrien Mazarguil To: Wang Ying A Cc: qi.z.zhang@intel.com, xiaolong.ye@intel.com, qiming.yang@intel.com, dev@dpdk.org Message-ID: <20190814090810.GM4512@6wind.com> References: <20190814030018.373628-1-ying.a.wang@intel.com> <20190814032430.404190-1-ying.a.wang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190814032430.404190-1-ying.a.wang@intel.com> 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" Hi Wang Ying, 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 OK, please split this patch, one for GTP and the other for PPPoE to make title less vague than "add more protocol support". For PPPoE, the distinction between session and discovery is not a bad idea but since discovery packets typically have a different format (tags in place of protocol), I'm not sure they should share a common structure. How about a single "PPPOE" item without proto_id to cover both session and 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). Such an approach is already used for IPV6 and IPV6_EXT. Another benefit is that applications could match PPPoE regardless of session or discovery when they do not care, while PPPOED/PPPOES make that distinction mandatory. Also by inserting new entries in the middle of the pattern items list, this 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. More nits below. > --- > --- > 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] = { > + .name = "pppoes", > + .help = "match PPPoE Session header", Session => session > + .priv = PRIV_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)), > + .next = NEXT(item_pppoe), > + .call = parse_vc, > + }, > + [ITEM_PPPOED] = { > + .name = "pppoed", > + .help = "match PPPoE Discovery stage header", Discovery => discovery > + .priv = PRIV_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)), > + .next = NEXT(item_pppoe), > + .call = parse_vc, > + }, > + [ITEM_PPPOE_SEID] = { > + .name = "seid", > + .help = "Session identifier", Session => session [...] > 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`` > +^^^^^^^^^^^^^^^^^^^^^^^ Too many "^^^"'s. [...] > +Item: ``PPPOES``, ``PPPOED`` > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Matches a PPPOE header. PPPOE => PPPoE > + > +Note: PPPOES and PPPOED use the same structure. PPPOES and PPPOED item item => items (or better, "pattern items") > +are defined for a user-friendly API when creating PPPOE-Session and > +PPPOE-Discovery flow rules. Super nit: use "PPPoE" when mentioning the protocol itself and "PPPOE" when mentioning the pattern item. > + > +- ``v_t_flags``: version (4b), type (4b). Why "flags"? I don't see any so you could name it "version_type" (same in documentation). > +- ``code``: Message type. Message => message > +- ``session_id``: Session identifier. Session => session > +- ``length``: Payload length. Payload => payload > +- ``proto_id``: PPP Protocol identifier. Protocol => protocol > +- Default ``mask`` matches session_id,proto_id. Missing space between session_id and proto_id. > + > 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. PPPOE => PPPoE [...] > 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). Session Container => session container > + * > + * 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. Session => session > + * > + * See struct rte_flow_item_pppoe. > + */ > + RTE_FLOW_ITEM_TYPE_PPPOES, > + > + /** > + * Matches a PPPOE header. > + * > + * Configure flow for PPPoE Discovery stage packets. Discovery => discovery > + * > + * 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 = { > }; > #endif > > +/** > + * RTE_FLOW_ITEM_TYPE_GTP_PSC. > + * > + * Matches a GTP-extension header > + * (type is 0x85: PDU Session Container). Session Container => session container (crusade against superfluous caps!) [...] > +/** > + * RTE_FLOW_ITEM_TYPE_PPPOE. > + * > + * Matches a PPPOE header. > + */ > +struct rte_flow_item_pppoe { > + /** > + * Version (4b), type (4b). > + */ > + uint8_t v_t_flags; v_t_flags => version_type > + 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. */ As discussed, I suggest dropping proto_id to make this a generic match for PPPoE. > +}; > + > +/** Default mask for RTE_FLOW_ITEM_TYPE_PPPOE. */ > +#ifndef __cplusplus > +static const struct rte_flow_item_pppoe rte_flow_item_pppoe_mask = { > + .session_id = RTE_BE16(0xffff), > + .proto_id = RTE_BE16(0xffff), > +}; I think the default for PPPoE should be an empty mask that simply says "match PPPoE" since session_id only becomes known after negotiation and proto_id shouldn't be part of this object. -- Adrien Mazarguil 6WIND