From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id A5A21A0562; Mon, 30 Mar 2020 10:49:37 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6F3281C065; Mon, 30 Mar 2020 10:49:37 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 0A9741C032; Mon, 30 Mar 2020 10:49:34 +0200 (CEST) IronPort-SDR: GND5HncGbhtTaryRJUDMAYzp1Bpjx3QhAGoGCVrQ6F0GK5jN6MlXoAru3funPwrUrpdF9htNpM TG8CKOz+snQQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Mar 2020 01:49:34 -0700 IronPort-SDR: 3KLCa4EuYd5WgCNZ5wCBNAsHr+C7CzTyBreRGU8PJMUhfcyxI5YiRxioxcmFA8m9KcXk8L0rZ0 7XRZ/7Gw/Yrg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,323,1580803200"; d="scan'208";a="283527599" Received: from pgsmsx114.gar.corp.intel.com ([10.108.55.203]) by fmsmga002.fm.intel.com with ESMTP; 30 Mar 2020 01:49:32 -0700 Received: from pgsmsx103.gar.corp.intel.com ([169.254.2.161]) by pgsmsx114.gar.corp.intel.com ([169.254.4.204]) with mapi id 14.03.0439.000; Mon, 30 Mar 2020 16:49:31 +0800 From: "Zhao1, Wei" To: Ori Kam , "Zhang, Xiao" , "dev@dpdk.org" CC: "Wang, Ying A" , "Zhang, Qi Z" , "stable@dpdk.org" Thread-Topic: app/testpmd: fix PPPOES flow API Thread-Index: AQHWBBGjr1afimNo8Ei5D1Va5hwRdahemB+AgAAsNoCAABQ7AIAAHG8AgAAMp4CAAOBoAIAAXUyAgACXgWA= Date: Mon, 30 Mar 2020 08:49:31 +0000 Message-ID: References: <20200327081926.6154-1-xiao.zhang@intel.com> <2966f158164c411e897b3ab741787eea@intel.com> <2723defc86e04f0aaeb42a14183b4b5f@intel.com> <17e9c85bd1ee4ce190fbd4d1be26105e@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDgyYWMyYjYtNThmMS00ZjBjLTgyMzAtN2MyZTI0MmI4OGE0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiRXlUU3FNR2h4M0FJRjVQY1poVXZlMjY5Z3JHYVA0MDVTRUJMQWFDQW5YUGFzampQcFRTZ0FuNWFKcUdrQ1p3RiJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [172.30.20.205] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] app/testpmd: fix PPPOES flow API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Ori > -----Original Message----- > From: Ori Kam > Sent: Monday, March 30, 2020 3:43 PM > To: Zhang, Xiao ; dev@dpdk.org > Cc: Wang, Ying A ; Zhang, Qi Z > ; Zhao1, Wei ; > stable@dpdk.org > Subject: RE: app/testpmd: fix PPPOES flow API >=20 > Hi Xiao >=20 > > -----Original Message----- > > From: Zhang, Xiao > > Sent: Monday, March 30, 2020 5:09 AM > > To: Ori Kam ; dev@dpdk.org > > Cc: Wang, Ying A ; Zhang, Qi Z > > ; Zhao1, Wei ; > > stable@dpdk.org > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > Hi Ori, > > > > > -----Original Message----- > > > From: Ori Kam > > > Sent: Sunday, March 29, 2020 8:46 PM > > > To: Zhang, Xiao ; dev@dpdk.org > > > Cc: Wang, Ying A ; Zhang, Qi Z > > > ; Zhao1, Wei ; > > stable@dpdk.org > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > Hi Xiao, > > > > > > > -----Original Message----- > > > > From: Zhang, Xiao > > > > Sent: Sunday, March 29, 2020 3:00 PM > > > > To: Ori Kam ; dev@dpdk.org > > > > Cc: Wang, Ying A ; Zhang, Qi Z > > > > ; Zhao1, Wei ; > > > > stable@dpdk.org > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > Hi Ori, > > > > > > > > > -----Original Message----- > > > > > From: Ori Kam > > > > > Sent: Sunday, March 29, 2020 6:19 PM > > > > > To: Zhang, Xiao ; dev@dpdk.org > > > > > Cc: Wang, Ying A ; Zhang, Qi Z > > > > > ; Zhao1, Wei ; > > > > stable@dpdk.org > > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > > Hi Xiao, > > > > > > > > > > > -----Original Message----- > > > > > > From: Zhang, Xiao > > > > > > Sent: Sunday, March 29, 2020 12:06 PM > > > > > > To: Ori Kam ; dev@dpdk.org > > > > > > Cc: Wang, Ying A ; Zhang, Qi Z > > > > > > ; Zhao1, Wei ; > > > > > > stable@dpdk.org > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > > > > Hi Ori, > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Ori Kam > > > > > > > Sent: Sunday, March 29, 2020 2:28 PM > > > > > > > To: Zhang, Xiao ; dev@dpdk.org > > > > > > > Cc: Wang, Ying A ; Zhang, Qi Z > > > > > > > ; Zhao1, Wei ; > > > > > > stable@dpdk.org > > > > > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > > > > > > Hi Xiao, > > > > > > > > > > > > > > Is the proto_id part of the basic header or not? > > > > > > > > > > > > Proto_id is part of PPPOE session header, > > > > > > > > > > > > > > > > Where is the porto_id located? Inside the payload? > > > > > > > > Yes, my previous explanation was not clear. The protocol ID is in > > > > the beginning of the payload in PPP Session Stage according to RFC2= 516. > > > > > > > > 1 > 2 3 > > > > 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 > > > > 8 9 > > > > 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-= +-+ > > > > | VER | TYPE | CODE | > SESSION_ID | > > > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > > > | LENGTH | > payload ~ > > > > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > > > > > > > > > Yes this is what I thought, does proto_id must be the first part of t= he > payload? > > > > It must be the first part of the payload for PPP Session Stage, not > > all PPPOE packets. > > > > > > > > > > > > > > > > > > > > > > > > From the spec it looks like a different header. > > > > > > > > > > > > > > If it is part of the original header then all documentations > > > > > > > and rte_structs > > > > > > should > > > > > > > be changed, to reflect this. > > > > > > > > > > > > > > It will be very helpful if the patch message would explain > > > > > > > the bug and why it > > > > > > was > > > > > > > changed. > > > > > > > > > > > > Okay, will add more message. The next value of the > > > > ITEM_PPPOE_PROTO_ID > > > > > > should be unsigned value but not item list. > > > > > > > > > > > > > > > > > > > > Also please see inline other comment. > > > > > > > > > > > > > > Best, > > > > > > > Ori > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Xiao Zhang > > > > > > > > Sent: Friday, March 27, 2020 11:19 AM > > > > > > > > To: dev@dpdk.org > > > > > > > > Cc: Ori Kam ; ying.a.wang@intel.com; > > > > > > > > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang > > > > > > > > ; stable@dpdk.org > > > > > > > > Subject: app/testpmd: fix PPPOES flow API > > > > > > > > > > > > > > > > The command line to create RTE flow for specific proto_id > > > > > > > > of PPPOES is not correct. This patch is to fix this issue. > > > > > > > > > > > > > > > > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API") > > > > > > > > Cc: stable@dpdk.org > > > > > > > > > > > > > > > > Signed-off-by: Xiao Zhang > > > > > > > > --- > > > > > > > > app/test-pmd/cmdline_flow.c | 13 +++---------- > > > > > > > > 1 file changed, 3 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > > > diff --git a/app/test-pmd/cmdline_flow.c > > > > > > > > b/app/test-pmd/cmdline_flow.c index a78154502..c25a2598d > > > > > > > > 100644 > > > > > > > > --- a/app/test-pmd/cmdline_flow.c > > > > > > > > +++ b/app/test-pmd/cmdline_flow.c > > > > > > > > @@ -768,7 +768,6 @@ static const enum index next_item[] =3D= { > > > > > > > > ITEM_GTP_PSC, > > > > > > > > ITEM_PPPOES, > > > > > > > > ITEM_PPPOED, > > > > > > > > - ITEM_PPPOE_PROTO_ID, > > > > > > > > ITEM_HIGIG2, > > > > > > > > ITEM_TAG, > > > > > > > > ITEM_L2TPV3OIP, > > > > > > > > @@ -1030,11 +1029,6 @@ static const enum index > > > > > > > > item_pppoed[] =3D { > > > > > > > > > > > > > > > > static const enum index item_pppoes[] =3D { > > > > > > > > ITEM_PPPOE_SEID, > > > > > > > > - ITEM_NEXT, > > > > > > > > - ZERO, > > > > > > > > -}; > > > > > > > > - > > > > > > > > -static const enum index item_pppoe_proto_id[] =3D { > > > > > > > > ITEM_PPPOE_PROTO_ID, > > > > > > > > ITEM_NEXT, > > > > > > > > ZERO, > > > > > > > > @@ -2643,10 +2637,9 @@ static const struct token token_list= [] =3D > { > > > > > > > > [ITEM_PPPOE_PROTO_ID] =3D { > > > > > > > > .name =3D "proto_id", > > > > > > > > .help =3D "match PPPoE session protocol identifier", > > > > > > > > - .priv =3D PRIV_ITEM(PPPOE_PROTO_ID, > > > > > > > > - sizeof(struct > > > > rte_flow_item_pppoe_proto_id)), > > > > > > > > - .next =3D NEXT(item_pppoe_proto_id), > > > > > > > > - .call =3D parse_vc, > > > > > > > > + .next =3D NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED), > > > > > > > > item_param), > > > > > > > > + .args =3D ARGS(ARGS_ENTRY_HTON > > > > > > > > + (struct rte_flow_item_pppoe_proto_id, > > > > proto_id)), > > > > > > > > > > > > > > Where is the memory for this proto_id is defined? > > > > > > > > > > > > Do you mean this? > > > > > > lib/librte_ethdev/rte_flow.h > > > > > > 1360 struct rte_flow_item_pppoe_proto_id { > > > > > > 1361 rte_be16_t proto_id; /**< PPP protocol identifier.= */ > > > > > > 1362 }; > > > > > > > > > > > > > > > > Yes. Why don't you use this one? > > > > > > > > I think I was using this, am I using it incorrectly? > > > > > > > > + .args =3D ARGS(ARGS_ENTRY_HTON > > > > + (struct rte_flow_item_pppoe_proto_id, proto_id)), > > > > > > > > > > Yes but there is no space to save this data since you deleted the pri= v. > > > I think you are trying to implement something like > > > RTE_FLOW_ITEM_TYPE_IPV6_EXT. > > > > > > And I don't understand what was the problem with the previous > > implementation. > > > > I deleted the priv because it changed to a subcommand in pppoes, the > > command line will be like this: > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / > > testpmd> pppoes > > proto_id is 21 > > >=20 > The issue is that the pppoe struct doesn't have definition to the proto_i= d. > If you wish a possible solution will be to add it to the pppoe struct, I'= m not sure > if this is the correct approach since this field is not a must. >=20 > Like I said there are examples on how to work with extended headers, whic= h I > think are more correct, buy may be the problem is that the pppoe struct i= s not > aligned and this result in an issue when adding the last bytes. There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do you mean mak= e use of that? That is the reason for use extended header for this? But that seems as you say, has some bug. >=20 > > The previous implementation would be infinite loop for proto_id > > command and can not specific the value for proto_id. > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / > > testpmd> proto_id > > proto_id [TOKEN]: match PPPoE session protocol identifier / [TOKEN]: > > specify next pattern item > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / > > testpmd> proto_id > > proto_id > > proto_id [TOKEN]: match PPPoE session protocol identifier / [TOKEN]: > > specify next pattern item > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / > > testpmd> proto_id > > proto_id proto_id > > proto_id [TOKEN]: match PPPoE session protocol identifier / [TOKEN]: > > specify next pattern item > > testpmd> flow create 0 ingress pattern eth dst is 00:11:22:33:44:55 / > > testpmd> proto_id > > proto_id proto_id > > > > > > > > > > > > > > > > > > > > > > > > > }, > > > > > > > > [ITEM_HIGIG2] =3D { > > > > > > > > .name =3D "higig2", > > > > > > > > -- > > > > > > > > 2.17.1