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 294C2A0562 for ; Tue, 31 Mar 2020 15:04:39 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EA2581BFC6; Tue, 31 Mar 2020 15:04:38 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 4F734FFA; Tue, 31 Mar 2020 15:04:35 +0200 (CEST) IronPort-SDR: FU6Y6sMFh3vFLP50EA6NcYX1NhYQkIXPb2NYoQoX0t5HJ9iQAkiaBWH3r+ypKK01c2gGbq6y9c mYY9hFRtimpg== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Mar 2020 06:04:34 -0700 IronPort-SDR: 7nATKDhn68cx4f1xyM0QyDCDwLyCkrIwsJi3rkrRNrvqV9n9x5lfOyCmmm5E55qTjn8uRb3uOm nuco2uMtWlvg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,327,1580803200"; d="scan'208";a="252235583" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga006.jf.intel.com with ESMTP; 31 Mar 2020 06:04:33 -0700 Received: from shsmsx606.ccr.corp.intel.com (10.109.6.216) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 31 Mar 2020 06:04:33 -0700 Received: from shsmsx602.ccr.corp.intel.com (10.109.6.142) by SHSMSX606.ccr.corp.intel.com (10.109.6.216) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 31 Mar 2020 21:04:32 +0800 Received: from shsmsx602.ccr.corp.intel.com ([10.109.6.142]) by SHSMSX602.ccr.corp.intel.com ([10.109.6.142]) with mapi id 15.01.1713.004; Tue, 31 Mar 2020 21:04:32 +0800 From: "Zhang, Xiao" To: Ori Kam , "Zhao1, Wei" , "dev@dpdk.org" CC: "Wang, Ying A" , "Zhang, Qi Z" , "stable@dpdk.org" Thread-Topic: app/testpmd: fix PPPOES flow API Thread-Index: AQHWBBGlpU3Uumzm102lBgy06K+oaKhemB+AgACm4VD//5mQAIAAnsbg//+KUICAAWNXcP//2l2AAAJU/4AAAEXjAAAbkMVgABJ3qgAAEofNkP//kDAA//84zZA= Date: Tue, 31 Mar 2020 13:04:32 +0000 Message-ID: References: <20200327081926.6154-1-xiao.zhang@intel.com> <2966f158164c411e897b3ab741787eea@intel.com> <2723defc86e04f0aaeb42a14183b4b5f@intel.com> <17e9c85bd1ee4ce190fbd4d1be26105e@intel.com> <704bf77207994d6182d5c4df625004e9@intel.com> <99365860d10049a6ad751b1a286933ac@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-version: 11.2.0.6 dlp-reaction: no-action dlp-product: dlpe-windows x-originating-ip: [10.239.127.36] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-stable] app/testpmd: fix PPPOES flow API X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" > -----Original Message----- > From: Ori Kam > Sent: Tuesday, March 31, 2020 5:06 PM > To: Zhang, Xiao ; Zhao1, Wei ; > dev@dpdk.org > Cc: Wang, Ying A ; Zhang, Qi Z > ; stable@dpdk.org > Subject: RE: app/testpmd: fix PPPOES flow API >=20 >=20 >=20 > > -----Original Message----- > > From: Zhang, Xiao > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > -----Original Message----- > > > From: Ori Kam > > > Subject: RE: app/testpmd: fix PPPOES flow API > > > > > > > > > > > > > -----Original Message----- > > > > From: Zhang, Xiao > > > > > > > > Hi Ori, > > > > > > > > > -----Original Message----- > > > > > From: Ori Kam > > > > > > > > > > Hi Xiao, > > > > > > > > > > > -----Original Message----- > > > > > > From: Zhao1, Wei > > > > > > > > > > > > Hi, Ori > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Ori Kam > > > > > > > > > > > > > > Hi Xiao > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Zhang, Xiao > > > > > > > > > > > > > > > > Hi Ori, > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > From: Ori Kam > > > > > > > > > > > > > > > > > > Hi Xiao, > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > > From: Zhang, Xiao > > > > > > > > > > > > > > > > > > > > Hi Ori, > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > > > From: Ori Kam > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > RFC2516. > > > > > > > > > > > > > > > > > > > > 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 the > > > > > > > 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 > > priv. > > > > > > > > > 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 > > > > > > > > testpmd> 00:11:22:33:44:55 / pppoes > > > > > > > > proto_id is 21 > > > > > > > > > > > > > > > > > > > > > > The issue is that the pppoe struct doesn't have definition > > > > > > > to the > > proto_id. > > > > > > > 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 mus= t. > > > > > > > > > > > > > > Like I said there are examples on how to work with extended > > > > > > > headers, which I think are more correct, buy may be the > > > > > > > problem is that the pppoe struct is > > > > > > not > > > > > > > aligned and this result in an issue when adding the last byte= s. > > > > > > > > > > > > > > > > > > There is a defination of RTE_FLOW_ITEM_TYPE_PPPOE_PROTO_ID, do > > you > > > > > > mean make use of that? > > > > > > That is the reason for use extended header for this? > > > > > > But that seems as you say, has some bug. > > > > > > > > > > > > > > > > I understand there is a bug, the question how to solve it. > > > > > I suggested two approaches. Add the proto_id to the pppoe > > > > > struct, but this means that we will add a new member that is not > > > > > part of the original > > > > definition. > > > > > Maybe the issue is in the PMD and it needs to understand that > > > > > the proto_id should be located in a different offset. > > > > > In any case it doesn't look like the current fix the right one. > > > > > > > > From my understanding, you mean there are two approaches. One is > > > > adding proto_id to pppoe struct. But you don't prefer this one > > > > since proto_id is not a must. I am not clear about the other one. > > > > > > > > > > The solution should be just like the pdu_type which is part of the gt= p_psc. > > > You can find also my comments on this, in the ML. > > > I think it is exactly the same case. > > > Example line for pdu type: flow create 0 ingress pattern gtp_psc > > > pdu_t is xxx > > The > > > thread > > https://eur03.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fpat= c > > hes.d > > > pdk.org%2Fpatch%2F67198%2F&data=3D02%7C01%7Corika%40mellanox.co > > > m%7Cb393253da4f84b2e909908d7d54b817b%7Ca652971c7d2e4d9ba6a4d149 > > > 256f461b%7C0%7C0%7C637212392573440621&sdata=3DQBGaw8RypOoikbb > > veKhbgv4PxZbOJ7p7pNESV6D%2FBT0%3D&reserved=3D0 > > > > Yes, so the line for should be: > > flow create 0 ingress pattern pppoes proto_id is xxx . > > > > But since we already have pppoes line for command pppoes seid is xxx, > > we need use another word instead of pppoes for proto_id, right? If > > yes, do you have any suggestion? > > >=20 > I think it should be something like this: > Flow create 0 ingress pattern pppoe_proto_id proto_id is xxx >=20 > Since the pppoe_proto_id has only one field maybe we can go with the > following approach: > Flow create 0 ingress pattern pppoe_proto_id is xxx Yes, "flow create 0 ingress pattern pppoe_proto_id is xxx" will be better, = will change to it. >=20 >=20 > > > > > > > > > > And also how do you suggest the command line be for proto_id? > > > > "proto_id is 0x0021" or "pppoes proto_id is 0x0021"? If the former > > > > just like what it was, I think it maybe a little confused. If the > > > > latter (as proto_id is part of pppoes), do we still need to put > > > > proto_id in > > > rte_flow_item_pppoe? > > > > > > > > Thanks, > > > > Xiao > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The previous implementation would be infinite loop for > > > > > > > > proto_id command and can not specific the value for proto_i= d. > > > > > > > > testpmd> flow create 0 ingress pattern eth dst is > > > > > > > > testpmd> 00:11:22:33:44:55 / proto_id > > > > > > > > proto_id [TOKEN]: match PPPoE session protocol identifier > > > > > > > > / > > [TOKEN]: > > > > > > > > specify next pattern item > > > > > > > > testpmd> flow create 0 ingress pattern eth dst is > > > > > > > > testpmd> 00:11:22:33:44:55 / 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 > > > > > > > > testpmd> 00:11:22:33:44:55 / 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 > > > > > > > > testpmd> 00:11:22:33:44:55 / proto_id > > > > > > > > proto_id proto_id > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > [ITEM_HIGIG2] =3D { > > > > > > > > > > > > > > .name =3D "higig2", > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > 2.17.1