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 ADB20A0546; Wed, 7 Apr 2021 10:24:03 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 498E7141096; Wed, 7 Apr 2021 10:24:03 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id E39CF141088 for ; Wed, 7 Apr 2021 10:24:01 +0200 (CEST) IronPort-SDR: Sow76vKIw4Xki4GeJx+v6mZm3A0FiXWOM0eMBVThb/23hN8IP4YJbM4/yBAS6HVS836sGoWE86 /EAac2P/LcoA== X-IronPort-AV: E=McAfee;i="6000,8403,9946"; a="213646588" X-IronPort-AV: E=Sophos;i="5.82,203,1613462400"; d="scan'208";a="213646588" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Apr 2021 01:24:00 -0700 IronPort-SDR: 86u6s3tRNIsWGqg97EUAWbUmPc1+SjlLjlPogELwzow8m/6WmMHEZRGkL4G/nOY9Y/hHDwZyha pDLsC8x0EL8w== X-IronPort-AV: E=Sophos;i="5.82,203,1613462400"; d="scan'208";a="379738596" 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 01:23:59 -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: Date: Wed, 7 Apr 2021 09:23:56 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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: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%2Fpat >> ches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20210317092610.71000-5-salems >> %40nvidia.com%2F&data=04%7C01%7Csalems%40nvidia.com%7Cd9baadbc48fc >> 44caf4fc08d8f90a7553%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C6375 >> 33170601193210%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l >> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=olwLyRnHnM7TyKDFI >> 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-send-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. >>> 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. >> >