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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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