Re: [dpdk-dev] [PATCH v2 2/3] app/testpmd: support PPPoL2TPv2oUDP RSS Hash

[Ori Kam <orika@nvidia.com>]

> -----Original Message-----
> From: Wang, Jie1X <jie1x.wang@intel.com>
> Sent: Wednesday, October 13, 2021 12:54 PM
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] app/testpmd: support PPPoL2TPv2oUDP RSS Hash
> 
> 
> 
> > -----Original Message-----
> > From: Ori Kam <orika@nvidia.com>
> > Sent: Wednesday, October 13, 2021 5:15 PM
> > To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; NBU-Contact-Thomas
> > Monjalon <thomas@monjalon.net>; andrew.rybchenko@oktetlabs.ru; Li,
> > Xiaoyun <xiaoyun.li@intel.com>; Yang, SteveX <stevex.yang@intel.com>;
> > Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 2/3] app/testpmd: support
> > PPPoL2TPv2oUDP RSS Hash
> >
> > Hi Wang,
> >
> > > -----Original Message-----
> > > From: Wang, Jie1X <jie1x.wang@intel.com>
> > > Sent: Wednesday, October 13, 2021 11:16 AM
> > >
> > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] app/testpmd: support
> > > PPPoL2TPv2oUDP RSS Hash
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ori Kam <orika@nvidia.com>
> > > > Sent: Tuesday, October 12, 2021 11:32 PM
> > > > To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
> > > > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; NBU-Contact-Thomas
> > > > Monjalon <thomas@monjalon.net>; andrew.rybchenko@oktetlabs.ru; Li,
> > > > Xiaoyun <xiaoyun.li@intel.com>; Yang, SteveX
> > > > <stevex.yang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> > > > Xing, Beilei <beilei.xing@intel.com>; Wu, Wenjun1
> > > > <wenjun1.wu@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 2/3] app/testpmd: support
> > > > PPPoL2TPv2oUDP RSS Hash
> > > >
> > > > Hi Jie,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Jie Wang
> > > > > Sent: Tuesday, October 12, 2021 1:25 PM
> > > > > Subject: [dpdk-dev] [PATCH v2 2/3] app/testpmd: support
> > > > > PPPoL2TPv2oUDP RSS Hash
> > > > >
> > > > > Add support for test-pmd to parse protocol pattern L2TPv2 and PPP.
> > > > >
> > > > > Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
> > > > > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> > > > > ---
> > > > >  app/test-pmd/cmdline_flow.c | 34
> > > > > ++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 34 insertions(+)
> > > > >
> > > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > > b/app/test-pmd/cmdline_flow.c index
> > > > > bb22294dd3..3c9bcabd97 100644
> > > > > --- a/app/test-pmd/cmdline_flow.c
> > > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > > @@ -299,6 +299,8 @@ enum index {
> > > > >  	ITEM_GENEVE_OPT_TYPE,
> > > > >  	ITEM_GENEVE_OPT_LENGTH,
> > > > >  	ITEM_GENEVE_OPT_DATA,
> > > > > +	ITEM_PPP,
> > > > > +	ITEM_L2TPV2,
> > > > >  	ITEM_INTEGRITY,
> > > > >  	ITEM_INTEGRITY_LEVEL,
> > > > >  	ITEM_INTEGRITY_VALUE,
> > > > > @@ -997,6 +999,8 @@ static const enum index next_item[] = {
> > > > >  	ITEM_AH,
> > > > >  	ITEM_PFCP,
> > > > >  	ITEM_ECPRI,
> > > > > +	ITEM_PPP,
> > > > > +	ITEM_L2TPV2,
> > > >
> > > > Why in the middle?
> > > >
> > >
> > > Ok, I will update it.
> > >
> > > > >  	ITEM_GENEVE_OPT,
> > > > >  	ITEM_INTEGRITY,
> > > > >  	ITEM_CONNTRACK,
> > > > > @@ -1368,6 +1372,16 @@ static const enum index item_integrity_lv[] = {
> > > > >  	ZERO,
> > > > >  };
> > > > >
> > > > > +static const enum index item_ppp[] = {
> > > > > +	ITEM_NEXT,
> > > > > +	ZERO,
> > > > > +};
> > > > > +
> > > > > +static const enum index item_l2tpv2[] = {
> > > > > +	ITEM_NEXT,
> > > > > +	ZERO,
> > > > > +};
> > > > > +
> > > > >  static const enum index next_action[] = {
> > > > >  	ACTION_END,
> > > > >  	ACTION_VOID,
> > > > > @@ -3579,6 +3593,20 @@ static const struct token token_list[] = {
> > > > >  				(sizeof(struct rte_flow_item_geneve_opt),
> > > > >  				ITEM_GENEVE_OPT_DATA_SIZE)),
> > > > >  	},
> > > > > +	[ITEM_PPP] = {
> > > > > +		.name = "ppp",
> > > > > +		.help = "match ppp header",
> > > > > +		.priv = PRIV_ITEM(PPP, sizeof(struct rte_flow_item_ppp)),
> > > > > +		.next = NEXT(item_ppp),
> > > > > +		.call = parse_vc,
> > > > > +	},
> > > > > +	[ITEM_L2TPV2] = {
> > > > > +		.name = "l2tpv2",
> > > > > +		.help = "match l2tpv2 header",
> > > > > +		.priv = PRIV_ITEM(L2TPV2, sizeof(struct rte_flow_item_l2tpv2)),
> > > > > +		.next = NEXT(item_l2tpv2),
> > > > > +		.call = parse_vc,
> > > > > +	},
> > > > >  	[ITEM_INTEGRITY] = {
> > > > >  		.name = "integrity",
> > > > >  		.help = "match packet integrity", @@ -8343,6 +8371,12 @@
> > > > > flow_item_default_mask(const struct
> > > > rte_flow_item *item)
> > > > >  	case RTE_FLOW_ITEM_TYPE_PFCP:
> > > > >  		mask = &rte_flow_item_pfcp_mask;
> > > > >  		break;
> > > > > +	case RTE_FLOW_ITEM_TYPE_L2TPV2:
> > > > > +		mask = &rte_flow_item_l2tpv2_mask;
> > > > > +		break;
> > > > > +	case RTE_FLOW_ITEM_TYPE_PPP:
> > > > > +		mask = &rte_flow_item_ppp_mask;
> > > > > +		break;
> > > > >  	default:
> > > > >  		break;
> > > > >  	}
> > > > > --
> > > > > 2.25.1
> > > >
> > > > Maybe I'm missing something but I don't see that you added the
> > > > ability to match on any of the header fields value.
> > > > You also didn't update the code of encap (from my understanding
> > > > this is a tunnel
> > > > header)
> > > >
> > > > Best,
> > > > Ori
> > >
> > > Hi Ori,
> > >
> > > This feature is only support for iavf enable PPPoL2TPv2oUDP rss. So
> > > it doesn't need to add the ability to match on any of the header
> > > fields value and
> > the code of encap.
> > >
> > > I'm not sure if it is necessary to add these.
> >
> > You added a lot of fields in the rte_flow and you don't give any way to test them.
> > also Iike I said in previous patch what is the relation between
> > matching items to RSS?
> > You didn't add it to the RSS possible support.
> >
> > Best,
> > Ori
> 
> The feature apply a RSS rule on L2TP data packet (include PPP over L2TP) with inner IP / port as input
> set.
> It doesn't need match any L2TP field.

Just to be clear when you say RSS on mean RSS on ipv4+udp for example not on the ppp header itself right?
If so then the title should just state that you are adding new items (also for other patches)
since I can use this item to jump to other group or count or any other action. This patch dosesn't have anything to do with hash.

I understand that from your point of view you don't need to match on any field, but you added such fields and no one
can test it and no PMD support this so this feature is not valid.

Best,
Ori