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

[Ori Kam <orika@nvidia.com>]

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