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 A62AEA0546; Tue, 6 Apr 2021 16:49:39 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1FA2B14118F; Tue, 6 Apr 2021 16:44:16 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id AEEDE14118C for ; Tue, 6 Apr 2021 16:44:12 +0200 (CEST) IronPort-SDR: FjCxtnHt9xXONJgqDJWhLlh5l9fcfLIX5KBdw7wx0u1Ir8bxjNcoCuIhvOatejL7qTdYpX7pvq tIBOqDk8Msqw== X-IronPort-AV: E=McAfee;i="6000,8403,9946"; a="193192423" X-IronPort-AV: E=Sophos;i="5.81,309,1610438400"; d="scan'208";a="193192423" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Apr 2021 07:44:11 -0700 IronPort-SDR: HgNOjeu6qfxbJNYZ42AdxB5eqSX+85qQgq7h0fE4bdNO1g1SHTydPva60VhlCKF5UnByy9evDe srhx2IQIT3TA== X-IronPort-AV: E=Sophos;i="5.81,309,1610438400"; d="scan'208";a="421242521" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.250.199]) ([10.213.250.199]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Apr 2021 07:44:09 -0700 To: "Jiawei(Jonny) Wang" , Salem Sol , "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: Tue, 6 Apr 2021 15:44:05 +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/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://patches.dpdk.org/project/dpdk/patch/20210317092610.71000-5-salems@nvidia.com/) > 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? >> 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. >> 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. >