On 4/7/2021 9:19 AM, Salem Sol wrote:
Hi Ferruh,
-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@intel.com>
Sent: Tuesday, April 6, 2021 5:44 PM
To: Jiawei(Jonny) Wang <jiaw...@nvidia.com>; Salem Sol <sal...@nvidia.com>;
dev@dpdk.org
Cc: Ori Kam <or...@nvidia.com>; Xiaoyun Li <xiaoyun...@intel.com>
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 <ferruh.yi...@intel.com>
Sent: Wednesday, March 31, 2021 8:08 PM
To: Salem Sol <sal...@nvidia.com>; dev@dpdk.org
Cc: Jiawei(Jonny) Wang <jiaw...@nvidia.com>; Ori Kam
<or...@nvidia.com>; Xiaoyun Li <xiaoyun...@intel.com>
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 <jiaw...@nvidia.com>
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-jiaw...@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 <jiaw...@nvidia.com>
<...>
-/** 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.
