Re: [dpdk-dev] [PATCH 3/4] ethdev: support PPPoL2TPv2oUDP RSS Hash

[Ori Kam <orika@nvidia.com>]

Hi Jie,

You are missing updating the rte_flow.rst and release notes:

Please see more comments below.

Thanks,
Ori

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jie Wang
> Sent: Friday, September 24, 2021 6:17 PM
> Subject: [dpdk-dev] [PATCH 3/4] ethdev: support PPPoL2TPv2oUDP RSS
> Hash
> 
> Add flow pattern items, RSS offload types and header formats of L2TPv2 and
> PPP.
> 
> Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> ---
>  lib/ethdev/rte_flow.c |  2 +
>  lib/ethdev/rte_flow.h | 99
> +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
> 
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> 8cb7a069c8..307fbc3abe 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -98,6 +98,8 @@ static const struct rte_flow_desc_data
> rte_flow_desc_item[] = {
>  	MK_FLOW_ITEM(PFCP, sizeof(struct rte_flow_item_pfcp)),
>  	MK_FLOW_ITEM(ECPRI, sizeof(struct rte_flow_item_ecpri)),
>  	MK_FLOW_ITEM(GENEVE_OPT, sizeof(struct
> rte_flow_item_geneve_opt)),
> +	MK_FLOW_ITEM(L2TPV2, sizeof(struct rte_flow_item_l2tpv2)),
> +	MK_FLOW_ITEM(PPP, sizeof(struct rte_flow_item_ppp)),
>  	MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
>  	MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),  }; diff --git
> a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> 70f455d47d..93205b7d1e 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -554,6 +554,20 @@ enum rte_flow_item_type {
>  	 */
>  	RTE_FLOW_ITEM_TYPE_GENEVE_OPT,
> 
> +	/**
> +	 * Matches L2TPV2 Header.
> +	 *
> +	 * See struct rte_flow_item_l2tpv2.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_L2TPV2,
> +
> +	/**
> +	 * Matches PPP Header.
> +	 *
> +	 * See struct rte_flow_item_ppp.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_PPP,
> +

Why did you push the new items here and not in the end?

>  	/**
>  	 * [META]
>  	 *
> @@ -1799,6 +1813,91 @@ static const struct rte_flow_item_conntrack
> rte_flow_item_conntrack_mask = {  };  #endif
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + * RTE_FLOW_ITEM_TYPE_L2TPV2
> + *
> + * Matches L2TPv2 Header
> + */
> +RTE_STD_C11
> +struct rte_flow_item_l2tpv2 {
> +	rte_be16_t flags_version;   /**< flag(12)  version(2). version must be
> 2 */
> +	union{
> +		struct{
> +			rte_be16_t length;  /**< length(16) */
> +			rte_be16_t tunnel_id;   /**< tunnel id(16) */
> +			rte_be16_t session_id;  /**< session id(16) */
> +			rte_be32_t ns_nr;   /**< Ns(16) + Nr(16) */

Why not split those fields? 

> +			rte_be32_t offset;  /**< offset size(16) + offset
> padding(16) */

Why not split those fields?

> +		} type1;
> +		struct{
> +			rte_be16_t tunnel_id;   /**< tunnel id(16) */
> +			rte_be16_t session_id;  /**< session id(16) */
> +			rte_be32_t ns_nr;   /**< Ns(16) + Nr(16) */
> +			rte_be32_t offset;  /**< offset size(16) + offset
> padding(16) */
> +		} type2;
> +		struct{
> +			rte_be16_t length;  /**< length(16) */
> +			rte_be16_t tunnel_id;   /**< tunnel id(16) */
> +			rte_be16_t session_id;  /**< session id(16) */
> +			rte_be32_t offset;  /**< offset size(16) + offset
> padding(16) */
> +		} type3;
> +		struct{
> +			rte_be16_t length;  /**< length(16) */
> +			rte_be16_t tunnel_id;   /**< tunnel id(16) */
> +			rte_be16_t session_id;  /**< session id(16) */
> +			rte_be32_t ns_nr;   /**< Ns(16) + Nr(16) */
> +		} type4;
> +		struct{
> +			rte_be16_t tunnel_id;   /**< tunnel id(16) */
> +			rte_be16_t session_id;  /**< session id(16) */
> +			rte_be32_t offset;  /**< offset size(16) + offset
> padding(16) */
> +		} type5;
> +		struct{
> +			rte_be16_t tunnel_id;   /**< tunnel id(16) */
> +			rte_be16_t session_id;  /**< session id(16) */
> +			rte_be32_t ns_nr;   /**< Ns(16) + Nr(16) */
> +		} type6;
> +		struct{
> +			rte_be16_t length;  /**< length(16) */
> +			rte_be16_t tunnel_id;   /**< tunnel id(16) */
> +			rte_be16_t session_id;  /**< session id(16) */
> +		} type7;
> +		struct{
> +			rte_be16_t tunnel_id;   /**< tunnel id(16) */
> +			rte_be16_t session_id;  /**< session id(16) */
> +		} type8;
> +	};
> +};
> +

The size of each of the types is different so using this struct as is will be hard
if someone wish to use it for encap or just as header.
Why not creating different structs for each type?

> +/** Default mask for RTE_FLOW_ITEM_TYPE_L2TPV2. */ #ifndef
> __cplusplus
> +static const struct rte_flow_item_l2tpv2 rte_flow_item_l2tpv2_mask = {
> +	.flags_version = 0xffff,

Should the default be match on all the flags? And the version?

> +};
> +#endif
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + * RTE_FLOW_ITEM_TYPE_PPP
> + *
> + * Matches PPP Header
> + */
> +struct rte_flow_item_ppp {
> +	rte_be16_t pppaddr_ctrl; /**< ppp address(8) + control(8) */

Why not split?

> +	rte_be16_t pppproto_id; /**< ppp protocol id(16) */ };
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_PPP. */ #ifndef __cplusplus
> +static const struct rte_flow_item_ppp rte_flow_item_ppp_mask = {
> +	.pppaddr_ctrl = 0xffff,
> +	.pppproto_id = 0xffff,
> +};
> +#endif
> +
>  /**
>   * Matching pattern item definition.
>   *
> --
> 2.25.1