[v3,1/8] app/testpmd: store VXLAN/NVGRE encap data globally
Checks
Commit Message
From: Jiawei Wang <jiaweiw@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.
Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
---
app/test-pmd/cmdline_flow.c | 76 ++++++++++++++++++++++++-------------
1 file changed, 49 insertions(+), 27 deletions(-)
Comments
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Salem Sol
> Sent: Wednesday, March 17, 2021 11:26
> To: dev@dpdk.org
> Cc: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@intel.com>
> Subject: [dpdk-dev] [PATCH v3 1/8] app/testpmd: store VXLAN/NVGRE
> encap data globally
>
> From: Jiawei Wang <jiaweiw@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.
>
> Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
On 3/17/2021 9:26 AM, Salem Sol wrote:
> From: Jiawei Wang <jiaweiw@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.
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.
Tough, OK to refactor and split the function as preparation to support the
sample action.
> Signed-off-by: Jiawei Wang <jiaweiw@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.
<...>
> +/** 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.
Hello Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, March 31, 2021 8:08 PM
> To: Salem Sol <salems@nvidia.com>; dev@dpdk.org
> Cc: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Xiaoyun Li <xiaoyun.li@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 <jiaweiw@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://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];
> 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.
> Tough, OK to refactor and split the function as preparation to support the
> sample action.
>
> > Signed-off-by: Jiawei Wang <jiaweiw@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.
On 4/1/2021 5:13 AM, Jiawei(Jonny) Wang wrote:
> Hello Ferruh,
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, March 31, 2021 8:08 PM
>> To: Salem Sol <salems@nvidia.com>; dev@dpdk.org
>> Cc: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Ori Kam <orika@nvidia.com>;
>> Xiaoyun Li <xiaoyun.li@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 <jiaweiw@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://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 <jiaweiw@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.
>
Hi Ferruh,
-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com>
Sent: Tuesday, April 6, 2021 5:44 PM
To: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Salem Sol <salems@nvidia.com>; dev@dpdk.org
Cc: Ori Kam <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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.yigit@intel.com>
>> Sent: Wednesday, March 31, 2021 8:08 PM
>> To: Salem Sol <salems@nvidia.com>; dev@dpdk.org
>> Cc: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Ori Kam
>> <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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 <jiaweiw@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-jiaweiw@nvidia.com/
With the "raw_encap"
>> Tough, OK to refactor and split the function as preparation to
>> support the sample action.
>>
>>> Signed-off-by: Jiawei Wang <jiaweiw@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.
>
On 4/7/2021 9:19 AM, Salem Sol wrote:
> Hi Ferruh,
>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, April 6, 2021 5:44 PM
> To: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Salem Sol <salems@nvidia.com>; dev@dpdk.org
> Cc: Ori Kam <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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.yigit@intel.com>
>>> Sent: Wednesday, March 31, 2021 8:08 PM
>>> To: Salem Sol <salems@nvidia.com>; dev@dpdk.org
>>> Cc: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Ori Kam
>>> <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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 <jiaweiw@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-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 <jiaweiw@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.
>>
>
-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com>
Sent: Wednesday, April 7, 2021 11:24 AM
To: Salem Sol <salems@nvidia.com>; Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; dev@dpdk.org
Cc: Ori Kam <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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/7/2021 9:19 AM, Salem Sol wrote:
> Hi Ferruh,
>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, April 6, 2021 5:44 PM
> To: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Salem Sol
> <salems@nvidia.com>; dev@dpdk.org
> Cc: Ori Kam <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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.yigit@intel.com>
>>> Sent: Wednesday, March 31, 2021 8:08 PM
>>> To: Salem Sol <salems@nvidia.com>; dev@dpdk.org
>>> Cc: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Ori Kam
>>> <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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 <jiaweiw@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%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."
>>> Tough, OK to refactor and split the function as preparation to
>>> support the sample action.
>>>
>>>> Signed-off-by: Jiawei Wang <jiaweiw@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.
>>
>
On 4/7/2021 9:35 AM, Salem Sol wrote:
>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, April 7, 2021 11:24 AM
> To: Salem Sol <salems@nvidia.com>; Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; dev@dpdk.org
> Cc: Ori Kam <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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/7/2021 9:19 AM, Salem Sol wrote:
>> Hi Ferruh,
>>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, April 6, 2021 5:44 PM
>> To: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Salem Sol
>> <salems@nvidia.com>; dev@dpdk.org
>> Cc: Ori Kam <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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.yigit@intel.com>
>>>> Sent: Wednesday, March 31, 2021 8:08 PM
>>>> To: Salem Sol <salems@nvidia.com>; dev@dpdk.org
>>>> Cc: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Ori Kam
>>>> <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@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 <jiaweiw@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%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 <jiaweiw@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.
>>>
>>
>
@@ -5244,31 +5244,14 @@ parse_vc_action_rss_queue(struct context *ctx, const struct token *token,
return len;
}
-/** 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)
{
- struct buffer *out = buf;
- struct rte_flow_action *action;
- struct action_vxlan_encap_data *action_vxlan_encap_data;
- int ret;
-
- ret = parse_vc(ctx, token, str, len, buf, size);
- if (ret < 0)
- return ret;
- /* Nothing else to do if there is no buffer. */
- if (!out)
- return ret;
- if (!out->args.vc.actions_n)
+ if (!action_vxlan_encap_data)
return -1;
- action = &out->args.vc.actions[out->args.vc.actions_n - 1];
- /* Point to selected object. */
- ctx->object = out->args.vc.data;
- ctx->objmask = NULL;
/* Set up default configuration. */
- action_vxlan_encap_data = ctx->object;
*action_vxlan_encap_data = (struct action_vxlan_encap_data){
.conf = (struct rte_flow_action_vxlan_encap){
.definition = action_vxlan_encap_data->items,
@@ -5372,19 +5355,18 @@ parse_vc_action_vxlan_encap(struct context *ctx, const struct token *token,
}
memcpy(action_vxlan_encap_data->item_vxlan.vni, vxlan_encap_conf.vni,
RTE_DIM(vxlan_encap_conf.vni));
- action->conf = &action_vxlan_encap_data->conf;
- return ret;
+ return 0;
}
-/** Parse NVGRE encap action. */
+/** Parse VXLAN encap action. */
static int
-parse_vc_action_nvgre_encap(struct context *ctx, const struct token *token,
+parse_vc_action_vxlan_encap(struct context *ctx, const struct token *token,
const char *str, unsigned int len,
void *buf, unsigned int size)
{
struct buffer *out = buf;
struct rte_flow_action *action;
- struct action_nvgre_encap_data *action_nvgre_encap_data;
+ struct action_vxlan_encap_data *action_vxlan_encap_data;
int ret;
ret = parse_vc(ctx, token, str, len, buf, size);
@@ -5399,8 +5381,20 @@ parse_vc_action_nvgre_encap(struct context *ctx, const struct token *token,
/* Point to selected object. */
ctx->object = out->args.vc.data;
ctx->objmask = NULL;
+ action_vxlan_encap_data = ctx->object;
+ parse_setup_vxlan_encap_data(action_vxlan_encap_data);
+ action->conf = &action_vxlan_encap_data->conf;
+ return ret;
+}
+
+/** 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;
/* Set up default configuration. */
- action_nvgre_encap_data = ctx->object;
*action_nvgre_encap_data = (struct action_nvgre_encap_data){
.conf = (struct rte_flow_action_nvgre_encap){
.definition = action_nvgre_encap_data->items,
@@ -5463,6 +5457,34 @@ parse_vc_action_nvgre_encap(struct context *ctx, const struct token *token,
RTE_FLOW_ITEM_TYPE_VOID;
memcpy(action_nvgre_encap_data->item_nvgre.tni, nvgre_encap_conf.tni,
RTE_DIM(nvgre_encap_conf.tni));
+ return 0;
+}
+
+/** Parse NVGRE encap action. */
+static int
+parse_vc_action_nvgre_encap(struct context *ctx, const struct token *token,
+ const char *str, unsigned int len,
+ void *buf, unsigned int size)
+{
+ struct buffer *out = buf;
+ struct rte_flow_action *action;
+ struct action_nvgre_encap_data *action_nvgre_encap_data;
+ int ret;
+
+ ret = parse_vc(ctx, token, str, len, buf, size);
+ if (ret < 0)
+ return ret;
+ /* Nothing else to do if there is no buffer. */
+ if (!out)
+ return ret;
+ if (!out->args.vc.actions_n)
+ return -1;
+ action = &out->args.vc.actions[out->args.vc.actions_n - 1];
+ /* Point to selected object. */
+ ctx->object = out->args.vc.data;
+ ctx->objmask = NULL;
+ action_nvgre_encap_data = ctx->object;
+ parse_setup_nvgre_encap_data(action_nvgre_encap_data);
action->conf = &action_nvgre_encap_data->conf;
return ret;
}