diff mbox series

[3/4] ethdev: support PPPoL2TPv2oUDP RSS Hash

Message ID 20210924151705.287571-4-jie1x.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series support PPPoL2TPv2oUDP RSS Hash |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Jie Wang Sept. 24, 2021, 3:17 p.m. UTC 
  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(+)

Comments

Ori Kam Sept. 30, 2021, 2:38 p.m. UTC | #1 
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
Ferruh Yigit Oct. 5, 2021, 2:42 p.m. UTC | #2 
On 9/30/2021 3:38 PM, Ori Kam wrote:
> 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?
> 

Hi Ori,

As far as I remember the decision was to define protocol headers separately
and use them as first element in the flow_item struct, this enables casting
the flow_item to protocol. And Andrew did some cleanups in the past related
to this.
Can't we use the same logic for this case?
Ori Kam Oct. 5, 2021, 3:06 p.m. UTC | #3 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, October 5, 2021 5:43 PM
> To: Ori Kam <orika@nvidia.com>; Jie Wang <jie1x.wang@intel.com>;
> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> xiaoyun.li@intel.com; jingjing.wu@intel.com; beilei.xing@intel.com;
> wenjun1.wu@intel.com; stevex.yang@intel.com
> Subject: Re: [dpdk-dev] [PATCH 3/4] ethdev: support PPPoL2TPv2oUDP RSS Hash
> 
> On 9/30/2021 3:38 PM, Ori Kam wrote:
> > 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?
> >
> 
> Hi Ori,
> 
> As far as I remember the decision was to define protocol headers separately
> and use them as first element in the flow_item struct, this enables casting the
> flow_item to protocol. And Andrew did some cleanups in the past related to
> this.
> Can't we use the same logic for this case?

This is also my thinking but I'm not familiar with this protocol and it looks like it has many different
options.

Best,
Ori
diff mbox series

Patch

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,
+
 	/**
 	 * [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) */
+			rte_be32_t offset;  /**< offset size(16) + offset padding(16) */
+		} 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;
+	};
+};
+
+/** 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,
+};
+#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) */
+	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.
  *