From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 559E9A0A02; Wed, 7 Apr 2021 13:30:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D9A3F4069F; Wed, 7 Apr 2021 13:30:43 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mails.dpdk.org (Postfix) with ESMTP id 1696E4013F for ; Wed, 7 Apr 2021 13:30:41 +0200 (CEST) IronPort-SDR: hagrBsKGzJxoxVaAZNZMDjubzdPodMMkKXs0ipLrEFXy/DwI72ZHplFXW+A7mXaQAftUQgsVVx 3bB3VdvFpQ7g== X-IronPort-AV: E=McAfee;i="6000,8403,9946"; a="257272075" X-IronPort-AV: E=Sophos;i="5.82,203,1613462400"; d="scan'208";a="257272075" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Apr 2021 04:30:40 -0700 IronPort-SDR: qWjwuO4b+yxggFG0mMTr8lJ/lpdIGW5QU5iQ7Dw0ftNCcGjIHwSybM2Vm5NFDOPeQXAhFV5rqz FnJJWP38wYnA== X-IronPort-AV: E=Sophos;i="5.82,203,1613462400"; d="scan'208";a="379790874" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.202.196]) ([10.213.202.196]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Apr 2021 04:30:39 -0700 To: Salem Sol , "Jiawei(Jonny) Wang" , "dev@dpdk.org" Cc: Ori Kam , Xiaoyun Li References: <20210317092610.71000-1-salems@nvidia.com> <20210317092610.71000-2-salems@nvidia.com> <5eff804f-baf3-7dce-7cb8-6189454f0665@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <7f6afb17-13d8-7566-3202-dabfdb0784e1@intel.com> Date: Wed, 7 Apr 2021 12:30:35 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 1/8] app/testpmd: store VXLAN/NVGRE encap data globally X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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" On 4/7/2021 9:35 AM, Salem Sol wrote: > > -----Original Message----- > From: Ferruh Yigit > Sent: Wednesday, April 7, 2021 11:24 AM > To: Salem Sol ; Jiawei(Jonny) Wang ; dev@dpdk.org > Cc: Ori Kam ; Xiaoyun Li > Subject: Re: [dpdk-dev] [PATCH v3 1/8] app/testpmd: store VXLAN/NVGRE encap data globally > > External email: Use caution opening links or attachments > > > On 4/7/2021 9:19 AM, Salem Sol wrote: >> Hi Ferruh, >> >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Tuesday, April 6, 2021 5:44 PM >> To: Jiawei(Jonny) Wang ; Salem Sol >> ; dev@dpdk.org >> Cc: Ori Kam ; Xiaoyun Li >> Subject: Re: [dpdk-dev] [PATCH v3 1/8] app/testpmd: store VXLAN/NVGRE >> encap data globally >> >> External email: Use caution opening links or attachments >> >> >> On 4/1/2021 5:13 AM, Jiawei(Jonny) Wang wrote: >>> Hello Ferruh, >>> >>>> -----Original Message----- >>>> From: Ferruh Yigit >>>> Sent: Wednesday, March 31, 2021 8:08 PM >>>> To: Salem Sol ; dev@dpdk.org >>>> Cc: Jiawei(Jonny) Wang ; Ori Kam >>>> ; Xiaoyun Li >>>> Subject: Re: [dpdk-dev] [PATCH v3 1/8] app/testpmd: store >>>> VXLAN/NVGRE encap data globally >>>> >>>> On 3/17/2021 9:26 AM, Salem Sol wrote: >>>>> From: Jiawei Wang >>>>> >>>>> With the current code the VXLAN/NVGRE parsing routine stored the >>>>> configuration of the header on stack, this might lead to >>>>> overwriting the data on the stack. >>>>> >>>>> This patch stores the external data of vxlan and nvgre encap into >>>>> global data as a pre-step to supporting vxlan and nvgre encap as a >>>>> sample actions. >>>>> >>>> >>>> I didn't get what was on stack and moved in to the global data, can >>>> you please elaborate. >>>> >>> >>> This patch split the function and saved input data into input parameter: >>> So it mentioned here "pre-step" for next store the data of vxlan/nvgre into global. >>> >>> The global var for sample action is defined in: >>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa >>> t >>> ches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20210317092610.71000-5-salem >>> s >>> %40nvidia.com%2F&data=04%7C01%7Csalems%40nvidia.com%7Cd9baadbc48f >>> c >>> 44caf4fc08d8f90a7553%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637 >>> 5 >>> 33170601193210%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2 >>> l >>> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=olwLyRnHnM7TyKDF >>> I >>> xVH3Dj6KhWtUzdXgAyGgON4M9M%3D&reserved=0) >>> struct action_vxlan_encap_data >>> sample_vxlan_encap[RAW_SAMPLE_CONFS_MAX_NUM]; >>> >> >> Commit log says: >> >> " >> This patch stores the external data of vxlan and nvgre encap into global data as a pre-step to supporting vxlan and nvgre encap as a sample actions. >> " >> >> It says this patch does storing into the global data, but as far as I can see from code and above description, this patch is just preparation for it. >> I can see there is a new version which has same commit log, can you please update it in new version? >> >> I will update in the next series. >> >>>> For example for nvgre, 'action_nvgre_encap_data' is pointer in stack >>>> but the actual object is 'ctx->object', so it is not really in the stack. >>>> >>> >>> After call 'set sample 0 nvgre .. ', the data be stored into >>> 'ctx->object', the 'ctx->object' will be reused for the following CLI >>> command, After that, while we try to use previous 'sample action' to fetch nvgre data, these data may be lost. >>> >>> For sample action, use global data can save the previous nvgre configurations data. >>> >> >> Got it, the target is to use "set sample_actions ..." testpmd command to store vxlan/nvgre encap sample actions. >> For record, can you please document what was the way to the same before this support, can you please document the old testpmd command. >> >> Can you please elaborate regarding where did you want this documentation? >> Prior to this support it is already documented, in >> http://patches.dpdk.org/project/dpdk/patch/1617244796-358287-1-git-sen >> d-email-jiaweiw@nvidia.com/ >> With the "raw_encap" >> > >> I was just thinking putting it in the commit log, for reference. To record how it was previously done. > > Does this seem acceptable? > " app/testpmd: prepare storing VXLAN/NVGRE encap data globally > > With the current code the VXLAN/NVGRE parsing routine > stored the configuration of the header on stack, this > might lead to overwriting the data on the stack. > > Currently having VXLAN/NVGRE encap as sample actions > is done using RAW_ENCAP, for example: > 1. set raw_encap 1 eth src.../ vxlan vni.../ ipv4.../ ... > set sample_actions 0 raw_encap / port_id id 0 / end > flow create 0 ... pattern eth / end actions > sample ration 1 index 0 / jump group 1 / end > > The goal is to utilize the rte_flow_action_vxlan_encap > and rte_flow_action_nvgre_encap for sample actions. > > This patch prepares storing the external data of vxlan and > nvgre encap into global data as a pre-step to supporting > vxlan and nvgre encap as a sample actions." > Sounds good, thank you. >>>> Tough, OK to refactor and split the function as preparation to >>>> support the sample action. >>>> >>>>> Signed-off-by: Jiawei Wang >>>> >>>> <...> >>>> >>>>> -/** Parse VXLAN encap action. */ >>>>> +/** Setup VXLAN encap configuration. */ >>>>> static int >>>>> -parse_vc_action_vxlan_encap(struct context *ctx, const struct >>>>> token >>>> *token, >>>>> - const char *str, unsigned int len, >>>>> - void *buf, unsigned int size) >>>>> +parse_setup_vxlan_encap_data >>>>> + (struct action_vxlan_encap_data >>>>> +*action_vxlan_encap_data) >>>> >>>> Can you please fix the syntax, I guess this is done to keep within >>>> in >>>> 80 column limit, but from readability perspective I think better to >>>> go over the 80 column a little instead of breaking the line like this. >>>> >>> >>> Ok, will change into one line. >>> >>>> <...> >>>> >>>>> +/** Setup NVGRE encap configuration. */ static int >>>>> +parse_setup_nvgre_encap_data >>>>> + (struct action_nvgre_encap_data >>>>> +*action_nvgre_encap_data) >>>> { >>>>> + if (!action_nvgre_encap_data) >>>>> + return -1; >>>> >>>> This is a static function, and the input of it is in our control, so >>>> not sure if we should verify the input here. >>>> But if we need to, can you please check the return value of this >>>> function where it is called. >>> >>> I agree with you that can remove this checking inside, since we make sure it's valid before call. >>> >>> Thanks. >>> >> >