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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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?