Re: [dpdk-dev] [PATCH v2] ethdev: add more protocol support in flow API

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, August 14, 2019 5:08 PM
> To: Wang, Ying A <ying.a.wang@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: add more protocol support in flow
> API
> 
> 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 <ying.a.wang@intel.com>
> 
> 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).

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.
> 
> 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