[v2,3/4] app/testpmd: support GTP PDU type
Checks
Commit Message
Add gtp pdu type configure in the cmdline.
Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v1:
no change
---
app/test-pmd/cmdline_flow.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
Comments
Hi Jeff,
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo
> Sent: Thursday, March 26, 2020 6:41 PM
> To: xiaolong.ye@intel.com; qi.z.zhang@intel.com
> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
> simei.su@intel.com; jia.guo@intel.com
> Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>
> Add gtp pdu type configure in the cmdline.
Why not use ITEM_GTP_PSC_PDU?
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v1:
> no change
> ---
> app/test-pmd/cmdline_flow.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index a78154502..c1bd02919 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -49,6 +49,7 @@ enum index {
> PORT_ID,
> GROUP_ID,
> PRIORITY_LEVEL,
> + GTP_PSC_PDU_T,
>
> /* Top-level command. */
> SET,
> @@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
> .call = parse_int,
> .comp = comp_none,
> },
> + [GTP_PSC_PDU_T] = {
> + .name = "{GTPU pdu type}",
> + .type = "INTEGER",
> + .help = "gtpu pdu uplink/downlink identifier",
> + .call = parse_int,
> + .comp = comp_none,
> + },
Why is this created at this level?
This looks like is should be written totally differently.
> /* Top-level command. */
> [FLOW] = {
> .name = "flow",
> @@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
> [ITEM_GTP_PSC_PDU_T] = {
> .name = "pdu_t",
> .help = "PDU type",
> - .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
> item_param),
> + .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
> + item_param),
> .args = ARGS(ARGS_ENTRY_HTON(struct
> rte_flow_item_gtp_psc,
> pdu_type)),
> },
> --
> 2.20.1
hi, orika
On 3/29/2020 4:44 PM, Ori Kam wrote:
> Hi Jeff,
>
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo
>> Sent: Thursday, March 26, 2020 6:41 PM
>> To: xiaolong.ye@intel.com; qi.z.zhang@intel.com
>> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
>> simei.su@intel.com; jia.guo@intel.com
>> Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>>
>> Add gtp pdu type configure in the cmdline.
> Why not use ITEM_GTP_PSC_PDU?
I guess you mean ITEM_GTP_PSC_PDU_T, rihgt? We know we have got
ITEM_GTP_PSC_QFI/ITEM_GTP_PSC_PDU_T but not define the
spec for them, so what i use is add the spec into the ITEM_GTP_PSC_PDU_T
to let the pdu type to be configured.
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v1:
>> no change
>> ---
>> app/test-pmd/cmdline_flow.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>> index a78154502..c1bd02919 100644
>> --- a/app/test-pmd/cmdline_flow.c
>> +++ b/app/test-pmd/cmdline_flow.c
>> @@ -49,6 +49,7 @@ enum index {
>> PORT_ID,
>> GROUP_ID,
>> PRIORITY_LEVEL,
>> + GTP_PSC_PDU_T,
>>
>> /* Top-level command. */
>> SET,
>> @@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
>> .call = parse_int,
>> .comp = comp_none,
>> },
>> + [GTP_PSC_PDU_T] = {
>> + .name = "{GTPU pdu type}",
>> + .type = "INTEGER",
>> + .help = "gtpu pdu uplink/downlink identifier",
>> + .call = parse_int,
>> + .comp = comp_none,
>> + },
> Why is this created at this level?
> This looks like is should be written totally differently.
As i said above, the item we got but spec or say next token still need
to be added, do you mean it should not in the group of Common tokens? If
so, let me think about that, and please explicit your proposal if you
already have one.
>> /* Top-level command. */
>> [FLOW] = {
>> .name = "flow",
>> @@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
>> [ITEM_GTP_PSC_PDU_T] = {
>> .name = "pdu_t",
>> .help = "PDU type",
>> - .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
>> item_param),
>> + .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
>> + item_param),
>> .args = ARGS(ARGS_ENTRY_HTON(struct
>> rte_flow_item_gtp_psc,
>> pdu_type)),
>> },
>> --
>> 2.20.1
Hi Jeff,
My name is Ori 😊
I'm not an expert in GTP so this is just my thinking and maybe I'm
missing something, this is why a good explanation helps 😊
> -----Original Message-----
> From: Jeff Guo <jia.guo@intel.com>
> Sent: Monday, March 30, 2020 11:30 AM
> To: Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com;
> qi.z.zhang@intel.com
> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
> simei.su@intel.com
> Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>
> hi, orika
>
>
> On 3/29/2020 4:44 PM, Ori Kam wrote:
> > Hi Jeff,
> >
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo
> >> Sent: Thursday, March 26, 2020 6:41 PM
> >> To: xiaolong.ye@intel.com; qi.z.zhang@intel.com
> >> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
> >> simei.su@intel.com; jia.guo@intel.com
> >> Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
> >>
> >> Add gtp pdu type configure in the cmdline.
> > Why not use ITEM_GTP_PSC_PDU?
>
>
> I guess you mean ITEM_GTP_PSC_PDU_T, rihgt? We know we have got
> ITEM_GTP_PSC_QFI/ITEM_GTP_PSC_PDU_T but not define the
>
> spec for them, so what i use is add the spec into the ITEM_GTP_PSC_PDU_T
> to let the pdu type to be configured.
>
Yes you are correct, from rte_flow we have the RTE_FLOW_ITEM_TYPE_GTP_PSC
Item that include pdu_type. This is the field you need right?
In testpmd we have the ITEM_GTP_PSC_PDU_T which should support adding
the pdu type.
Basically you just need to type the following cmd line:
flow create 0 ingress pattern gtp_psc pdu_t is xxx
if this command is not working we need to understand why.
>
> >> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> >> ---
> >> v1:
> >> no change
> >> ---
> >> app/test-pmd/cmdline_flow.c | 11 ++++++++++-
> >> 1 file changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> >> index a78154502..c1bd02919 100644
> >> --- a/app/test-pmd/cmdline_flow.c
> >> +++ b/app/test-pmd/cmdline_flow.c
> >> @@ -49,6 +49,7 @@ enum index {
> >> PORT_ID,
> >> GROUP_ID,
> >> PRIORITY_LEVEL,
> >> + GTP_PSC_PDU_T,
> >>
> >> /* Top-level command. */
> >> SET,
> >> @@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
> >> .call = parse_int,
> >> .comp = comp_none,
> >> },
> >> + [GTP_PSC_PDU_T] = {
> >> + .name = "{GTPU pdu type}",
> >> + .type = "INTEGER",
> >> + .help = "gtpu pdu uplink/downlink identifier",
> >> + .call = parse_int,
> >> + .comp = comp_none,
> >> + },
> > Why is this created at this level?
> > This looks like is should be written totally differently.
>
>
> As i said above, the item we got but spec or say next token still need
> to be added, do you mean it should not in the group of Common tokens? If
> so, let me think about that, and please explicit your proposal if you
> already have one.
>
Please see above response.
>
> >> /* Top-level command. */
> >> [FLOW] = {
> >> .name = "flow",
> >> @@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
> >> [ITEM_GTP_PSC_PDU_T] = {
> >> .name = "pdu_t",
> >> .help = "PDU type",
> >> - .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
> >> item_param),
> >> + .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
> >> + item_param),
> >> .args = ARGS(ARGS_ENTRY_HTON(struct
> >> rte_flow_item_gtp_psc,
> >> pdu_type)),
> >> },
> >> --
> >> 2.20.1
yes, Ori, please check the comment below.
On 3/30/2020 6:18 PM, Ori Kam wrote:
> Hi Jeff,
>
> My name is Ori 😊
>
> I'm not an expert in GTP so this is just my thinking and maybe I'm
> missing something, this is why a good explanation helps 😊
>
>> -----Original Message-----
>> From: Jeff Guo <jia.guo@intel.com>
>> Sent: Monday, March 30, 2020 11:30 AM
>> To: Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com;
>> qi.z.zhang@intel.com
>> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
>> simei.su@intel.com
>> Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>>
>> hi, orika
>>
>>
>> On 3/29/2020 4:44 PM, Ori Kam wrote:
>>> Hi Jeff,
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo
>>>> Sent: Thursday, March 26, 2020 6:41 PM
>>>> To: xiaolong.ye@intel.com; qi.z.zhang@intel.com
>>>> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
>>>> simei.su@intel.com; jia.guo@intel.com
>>>> Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>>>>
>>>> Add gtp pdu type configure in the cmdline.
>>> Why not use ITEM_GTP_PSC_PDU?
>>
>> I guess you mean ITEM_GTP_PSC_PDU_T, rihgt? We know we have got
>> ITEM_GTP_PSC_QFI/ITEM_GTP_PSC_PDU_T but not define the
>>
>> spec for them, so what i use is add the spec into the ITEM_GTP_PSC_PDU_T
>> to let the pdu type to be configured.
>>
> Yes you are correct, from rte_flow we have the RTE_FLOW_ITEM_TYPE_GTP_PSC
> Item that include pdu_type. This is the field you need right?
>
> In testpmd we have the ITEM_GTP_PSC_PDU_T which should support adding
> the pdu type.
> Basically you just need to type the following cmd line:
> flow create 0 ingress pattern gtp_psc pdu_t is xxx
> if this command is not working we need to understand why.
>
>
please check the part before this patch as below:
[ITEM_GTP_PSC_PDU_T] = {
.name = "pdu_t",
.help = "PDU type",
.next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
item_param),
sure, we got the ITEM_GTP_PSC_PDU_T at prior but the NEXT_ENTRY is
UNSIGNED, that means we still not implement
the spec to let the pdu type to be configurable, so what the patch do is
to fix this issue.
>>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>>> ---
>>>> v1:
>>>> no change
>>>> ---
>>>> app/test-pmd/cmdline_flow.c | 11 ++++++++++-
>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>>> index a78154502..c1bd02919 100644
>>>> --- a/app/test-pmd/cmdline_flow.c
>>>> +++ b/app/test-pmd/cmdline_flow.c
>>>> @@ -49,6 +49,7 @@ enum index {
>>>> PORT_ID,
>>>> GROUP_ID,
>>>> PRIORITY_LEVEL,
>>>> + GTP_PSC_PDU_T,
>>>>
>>>> /* Top-level command. */
>>>> SET,
>>>> @@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
>>>> .call = parse_int,
>>>> .comp = comp_none,
>>>> },
>>>> + [GTP_PSC_PDU_T] = {
>>>> + .name = "{GTPU pdu type}",
>>>> + .type = "INTEGER",
>>>> + .help = "gtpu pdu uplink/downlink identifier",
>>>> + .call = parse_int,
>>>> + .comp = comp_none,
>>>> + },
>>> Why is this created at this level?
>>> This looks like is should be written totally differently.
>>
>> As i said above, the item we got but spec or say next token still need
>> to be added, do you mean it should not in the group of Common tokens? If
>> so, let me think about that, and please explicit your proposal if you
>> already have one.
>>
> Please see above response.
>
>>>> /* Top-level command. */
>>>> [FLOW] = {
>>>> .name = "flow",
>>>> @@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
>>>> [ITEM_GTP_PSC_PDU_T] = {
>>>> .name = "pdu_t",
>>>> .help = "PDU type",
>>>> - .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
>>>> item_param),
>>>> + .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
>>>> + item_param),
>>>> .args = ARGS(ARGS_ENTRY_HTON(struct
>>>> rte_flow_item_gtp_psc,
>>>> pdu_type)),
>>>> },
>>>> --
>>>> 2.20.1
Hi Jeff,
> -----Original Message-----
> From: Jeff Guo <jia.guo@intel.com>
> Sent: Tuesday, March 31, 2020 11:50 AM
> To: Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com;
> qi.z.zhang@intel.com
> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
> simei.su@intel.com
> Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>
> yes, Ori, please check the comment below.
>
>
> On 3/30/2020 6:18 PM, Ori Kam wrote:
> > Hi Jeff,
> >
> > My name is Ori 😊
> >
> > I'm not an expert in GTP so this is just my thinking and maybe I'm
> > missing something, this is why a good explanation helps 😊
> >
> >> -----Original Message-----
> >> From: Jeff Guo <jia.guo@intel.com>
> >> Sent: Monday, March 30, 2020 11:30 AM
> >> To: Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com;
> >> qi.z.zhang@intel.com
> >> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
> >> simei.su@intel.com
> >> Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU
> type
> >>
> >> hi, orika
> >>
> >>
> >> On 3/29/2020 4:44 PM, Ori Kam wrote:
> >>> Hi Jeff,
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo
> >>>> Sent: Thursday, March 26, 2020 6:41 PM
> >>>> To: xiaolong.ye@intel.com; qi.z.zhang@intel.com
> >>>> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
> >>>> simei.su@intel.com; jia.guo@intel.com
> >>>> Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
> >>>>
> >>>> Add gtp pdu type configure in the cmdline.
> >>> Why not use ITEM_GTP_PSC_PDU?
> >>
> >> I guess you mean ITEM_GTP_PSC_PDU_T, rihgt? We know we have got
> >> ITEM_GTP_PSC_QFI/ITEM_GTP_PSC_PDU_T but not define the
> >>
> >> spec for them, so what i use is add the spec into the ITEM_GTP_PSC_PDU_T
> >> to let the pdu type to be configured.
> >>
> > Yes you are correct, from rte_flow we have the
> RTE_FLOW_ITEM_TYPE_GTP_PSC
> > Item that include pdu_type. This is the field you need right?
> >
> > In testpmd we have the ITEM_GTP_PSC_PDU_T which should support adding
> > the pdu type.
> > Basically you just need to type the following cmd line:
> > flow create 0 ingress pattern gtp_psc pdu_t is xxx
> > if this command is not working we need to understand why.
> >
> >
>
> please check the part before this patch as below:
>
> [ITEM_GTP_PSC_PDU_T] = {
> .name = "pdu_t",
> .help = "PDU type",
> .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
> item_param),
>
> sure, we got the ITEM_GTP_PSC_PDU_T at prior but the NEXT_ENTRY is
> UNSIGNED, that means we still not implement
>
Sorry I don't understand your comment, what do you mean it is not implemented?
Yes it means that the parameter is should be unsigned value.
> the spec to let the pdu type to be configurable, so what the patch do is
> to fix this issue.
What do you mean configurable?
Lets start at the beginning, maybe I'm just missing some key point.
What is the PDU type? What values can he hold?
How do you want the command to look like?
>
>
> >>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> >>>> ---
> >>>> v1:
> >>>> no change
> >>>> ---
> >>>> app/test-pmd/cmdline_flow.c | 11 ++++++++++-
> >>>> 1 file changed, 10 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> >>>> index a78154502..c1bd02919 100644
> >>>> --- a/app/test-pmd/cmdline_flow.c
> >>>> +++ b/app/test-pmd/cmdline_flow.c
> >>>> @@ -49,6 +49,7 @@ enum index {
> >>>> PORT_ID,
> >>>> GROUP_ID,
> >>>> PRIORITY_LEVEL,
> >>>> + GTP_PSC_PDU_T,
> >>>>
> >>>> /* Top-level command. */
> >>>> SET,
> >>>> @@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
> >>>> .call = parse_int,
> >>>> .comp = comp_none,
> >>>> },
> >>>> + [GTP_PSC_PDU_T] = {
> >>>> + .name = "{GTPU pdu type}",
> >>>> + .type = "INTEGER",
> >>>> + .help = "gtpu pdu uplink/downlink identifier",
> >>>> + .call = parse_int,
> >>>> + .comp = comp_none,
> >>>> + },
> >>> Why is this created at this level?
> >>> This looks like is should be written totally differently.
> >>
> >> As i said above, the item we got but spec or say next token still need
> >> to be added, do you mean it should not in the group of Common tokens? If
> >> so, let me think about that, and please explicit your proposal if you
> >> already have one.
> >>
> > Please see above response.
> >
> >>>> /* Top-level command. */
> >>>> [FLOW] = {
> >>>> .name = "flow",
> >>>> @@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
> >>>> [ITEM_GTP_PSC_PDU_T] = {
> >>>> .name = "pdu_t",
> >>>> .help = "PDU type",
> >>>> - .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
> >>>> item_param),
> >>>> + .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
> >>>> + item_param),
> >>>> .args = ARGS(ARGS_ENTRY_HTON(struct
> >>>> rte_flow_item_gtp_psc,
> >>>> pdu_type)),
> >>>> },
> >>>> --
> >>>> 2.20.1
hi, Ori
On 4/5/2020 11:56 PM, Ori Kam wrote:
> Hi Jeff,
>
>> -----Original Message-----
>> From: Jeff Guo <jia.guo@intel.com>
>> Sent: Tuesday, March 31, 2020 11:50 AM
>> To: Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com;
>> qi.z.zhang@intel.com
>> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
>> simei.su@intel.com
>> Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>>
>> yes, Ori, please check the comment below.
>>
>>
>> On 3/30/2020 6:18 PM, Ori Kam wrote:
>>> Hi Jeff,
>>>
>>> My name is Ori 😊
>>>
>>> I'm not an expert in GTP so this is just my thinking and maybe I'm
>>> missing something, this is why a good explanation helps 😊
>>>
>>>> -----Original Message-----
>>>> From: Jeff Guo <jia.guo@intel.com>
>>>> Sent: Monday, March 30, 2020 11:30 AM
>>>> To: Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com;
>>>> qi.z.zhang@intel.com
>>>> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
>>>> simei.su@intel.com
>>>> Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU
>> type
>>>> hi, orika
>>>>
>>>>
>>>> On 3/29/2020 4:44 PM, Ori Kam wrote:
>>>>> Hi Jeff,
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo
>>>>>> Sent: Thursday, March 26, 2020 6:41 PM
>>>>>> To: xiaolong.ye@intel.com; qi.z.zhang@intel.com
>>>>>> Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
>>>>>> simei.su@intel.com; jia.guo@intel.com
>>>>>> Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>>>>>>
>>>>>> Add gtp pdu type configure in the cmdline.
>>>>> Why not use ITEM_GTP_PSC_PDU?
>>>> I guess you mean ITEM_GTP_PSC_PDU_T, rihgt? We know we have got
>>>> ITEM_GTP_PSC_QFI/ITEM_GTP_PSC_PDU_T but not define the
>>>>
>>>> spec for them, so what i use is add the spec into the ITEM_GTP_PSC_PDU_T
>>>> to let the pdu type to be configured.
>>>>
>>> Yes you are correct, from rte_flow we have the
>> RTE_FLOW_ITEM_TYPE_GTP_PSC
>>> Item that include pdu_type. This is the field you need right?
>>>
>>> In testpmd we have the ITEM_GTP_PSC_PDU_T which should support adding
>>> the pdu type.
>>> Basically you just need to type the following cmd line:
>>> flow create 0 ingress pattern gtp_psc pdu_t is xxx
>>> if this command is not working we need to understand why.
>>>
>>>
>> please check the part before this patch as below:
>>
>> [ITEM_GTP_PSC_PDU_T] = {
>> .name = "pdu_t",
>> .help = "PDU type",
>> .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
>> item_param),
>>
>> sure, we got the ITEM_GTP_PSC_PDU_T at prior but the NEXT_ENTRY is
>> UNSIGNED, that means we still not implement
>>
> Sorry I don't understand your comment, what do you mean it is not implemented?
> Yes it means that the parameter is should be unsigned value.
I mean that if it is a unsigned value, user could not set the pdu_t to
be a 0 or 1, or any other we
define for that.
>> the spec to let the pdu type to be configurable, so what the patch do is
>> to fix this issue.
> What do you mean configurable?
>
> Lets start at the beginning, maybe I'm just missing some key point.
> What is the PDU type? What values can he hold?
> How do you want the command to look like?
the command should be like as below
flow create 0 ingress pattern eth / ipv4 / udp / gtpu / gtp_psc pdu_t is
0/ ipv4 / end actions rss types ipv4-udp l3-dst-only endkey_len 0queues
end / end
It is eventually the same as you described about the command before.
User could set pdu_t to be 0 or 1, so what is need is modify
NEXT_ENTRY(UNSIGNED) to be "SIGNED" and defined.
>>
>>>>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>>>>> ---
>>>>>> v1:
>>>>>> no change
>>>>>> ---
>>>>>> app/test-pmd/cmdline_flow.c | 11 ++++++++++-
>>>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>>>>> index a78154502..c1bd02919 100644
>>>>>> --- a/app/test-pmd/cmdline_flow.c
>>>>>> +++ b/app/test-pmd/cmdline_flow.c
>>>>>> @@ -49,6 +49,7 @@ enum index {
>>>>>> PORT_ID,
>>>>>> GROUP_ID,
>>>>>> PRIORITY_LEVEL,
>>>>>> + GTP_PSC_PDU_T,
>>>>>>
>>>>>> /* Top-level command. */
>>>>>> SET,
>>>>>> @@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
>>>>>> .call = parse_int,
>>>>>> .comp = comp_none,
>>>>>> },
>>>>>> + [GTP_PSC_PDU_T] = {
>>>>>> + .name = "{GTPU pdu type}",
>>>>>> + .type = "INTEGER",
>>>>>> + .help = "gtpu pdu uplink/downlink identifier",
>>>>>> + .call = parse_int,
>>>>>> + .comp = comp_none,
>>>>>> + },
>>>>> Why is this created at this level?
>>>>> This looks like is should be written totally differently.
>>>> As i said above, the item we got but spec or say next token still need
>>>> to be added, do you mean it should not in the group of Common tokens? If
>>>> so, let me think about that, and please explicit your proposal if you
>>>> already have one.
>>>>
>>> Please see above response.
>>>
>>>>>> /* Top-level command. */
>>>>>> [FLOW] = {
>>>>>> .name = "flow",
>>>>>> @@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
>>>>>> [ITEM_GTP_PSC_PDU_T] = {
>>>>>> .name = "pdu_t",
>>>>>> .help = "PDU type",
>>>>>> - .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
>>>>>> item_param),
>>>>>> + .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
>>>>>> + item_param),
>>>>>> .args = ARGS(ARGS_ENTRY_HTON(struct
>>>>>> rte_flow_item_gtp_psc,
>>>>>> pdu_type)),
>>>>>> },
>>>>>> --
>>>>>> 2.20.1
Hi Jeff,
For some reason I got the mail in HTML format
So please see my comments marked by [Ori]
Thanks,
Ori
From: Jeff Guo <jia.guo@intel.com>
Sent: Tuesday, April 7, 2020 8:37 AM
To: Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com; qi.z.zhang@intel.com
Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com; simei.su@intel.com
Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
hi, Ori
On 4/5/2020 11:56 PM, Ori Kam wrote:
Hi Jeff,
-----Original Message-----
From: Jeff Guo <jia.guo@intel.com><mailto:jia.guo@intel.com>
Sent: Tuesday, March 31, 2020 11:50 AM
To: Ori Kam <orika@mellanox.com><mailto:orika@mellanox.com>; xiaolong.ye@intel.com<mailto:xiaolong.ye@intel.com>;
qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>; yahui.cao@intel.com<mailto:yahui.cao@intel.com>;
simei.su@intel.com<mailto:simei.su@intel.com>
Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
yes, Ori, please check the comment below.
On 3/30/2020 6:18 PM, Ori Kam wrote:
Hi Jeff,
My name is Ori 😊
I'm not an expert in GTP so this is just my thinking and maybe I'm
missing something, this is why a good explanation helps 😊
-----Original Message-----
From: Jeff Guo <jia.guo@intel.com><mailto:jia.guo@intel.com>
Sent: Monday, March 30, 2020 11:30 AM
To: Ori Kam <orika@mellanox.com><mailto:orika@mellanox.com>; xiaolong.ye@intel.com<mailto:xiaolong.ye@intel.com>;
qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>; yahui.cao@intel.com<mailto:yahui.cao@intel.com>;
simei.su@intel.com<mailto:simei.su@intel.com>
Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU
type
hi, orika
On 3/29/2020 4:44 PM, Ori Kam wrote:
Hi Jeff,
-----Original Message-----
From: dev <dev-bounces@dpdk.org><mailto:dev-bounces@dpdk.org> On Behalf Of Jeff Guo
Sent: Thursday, March 26, 2020 6:41 PM
To: xiaolong.ye@intel.com<mailto:xiaolong.ye@intel.com>; qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>; yahui.cao@intel.com<mailto:yahui.cao@intel.com>;
simei.su@intel.com<mailto:simei.su@intel.com>; jia.guo@intel.com<mailto:jia.guo@intel.com>
Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
Add gtp pdu type configure in the cmdline.
Why not use ITEM_GTP_PSC_PDU?
I guess you mean ITEM_GTP_PSC_PDU_T, rihgt? We know we have got
ITEM_GTP_PSC_QFI/ITEM_GTP_PSC_PDU_T but not define the
spec for them, so what i use is add the spec into the ITEM_GTP_PSC_PDU_T
to let the pdu type to be configured.
Yes you are correct, from rte_flow we have the
RTE_FLOW_ITEM_TYPE_GTP_PSC
Item that include pdu_type. This is the field you need right?
In testpmd we have the ITEM_GTP_PSC_PDU_T which should support adding
the pdu type.
Basically you just need to type the following cmd line:
flow create 0 ingress pattern gtp_psc pdu_t is xxx
if this command is not working we need to understand why.
please check the part before this patch as below:
[ITEM_GTP_PSC_PDU_T] = {
.name = "pdu_t",
.help = "PDU type",
.next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
item_param),
sure, we got the ITEM_GTP_PSC_PDU_T at prior but the NEXT_ENTRY is
UNSIGNED, that means we still not implement
Sorry I don't understand your comment, what do you mean it is not implemented?
Yes it means that the parameter is should be unsigned value.
I mean that if it is a unsigned value, user could not set the pdu_t to be a 0 or 1, or any other we
define for that.
the spec to let the pdu type to be configurable, so what the patch do is
to fix this issue.
What do you mean configurable?
Lets start at the beginning, maybe I'm just missing some key point.
What is the PDU type? What values can he hold?
How do you want the command to look like?
the command should be like as below
flow create 0 ingress pattern eth / ipv4 / udp / gtpu / gtp_psc pdu_t is 0 / ipv4 / end actions rss types ipv4-udp l3-dst-only end key_len 0 queues end / end
It is eventually the same as you described about the command before. User could set pdu_t to be 0 or 1, so what is need is modify
NEXT_ENTRY(UNSIGNED) to be "SIGNED" and defined.
[Ori] I agree about the command look.
Can pdu_t can be only 0 or 1?
Also I don’t understand why you need signed? Even if you add the GTP_PSC_PDU_T as int? and not unsinged?
All other items are unsinged. I also don’t see any reason to create the new type unless, you want the help line.
In any case, I feel that this patch already waste a lot of time.
(for both of us)
Please consider again if the new class is necessary, and if it should be signed.
If so you have my ack.
Acked-by: Ori Kam orika@mellanox.com<mailto:orika@mellanox.com>
Best,
Ori
Signed-off-by: Jeff Guo <jia.guo@intel.com><mailto:jia.guo@intel.com>
---
v1:
no change
---
app/test-pmd/cmdline_flow.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index a78154502..c1bd02919 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -49,6 +49,7 @@ enum index {
PORT_ID,
GROUP_ID,
PRIORITY_LEVEL,
+ GTP_PSC_PDU_T,
/* Top-level command. */
SET,
@@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
.call = parse_int,
.comp = comp_none,
},
+ [GTP_PSC_PDU_T] = {
+ .name = "{GTPU pdu type}",
+ .type = "INTEGER",
+ .help = "gtpu pdu uplink/downlink identifier",
+ .call = parse_int,
+ .comp = comp_none,
+ },
Why is this created at this level?
This looks like is should be written totally differently.
As i said above, the item we got but spec or say next token still need
to be added, do you mean it should not in the group of Common tokens? If
so, let me think about that, and please explicit your proposal if you
already have one.
Please see above response.
/* Top-level command. */
[FLOW] = {
.name = "flow",
@@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
[ITEM_GTP_PSC_PDU_T] = {
.name = "pdu_t",
.help = "PDU type",
- .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
item_param),
+ .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
+ item_param),
.args = ARGS(ARGS_ENTRY_HTON(struct
rte_flow_item_gtp_psc,
pdu_type)),
},
--
2.20.1
hi, Ori
On 4/12/2020 5:58 PM, Ori Kam wrote:
>
> Hi Jeff,
>
> For some reason I got the mail in HTML format
>
> So please see my comments marked by [Ori]
>
> Thanks,
>
> Ori
>
> *From:* Jeff Guo <jia.guo@intel.com>
> *Sent:* Tuesday, April 7, 2020 8:37 AM
> *To:* Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com;
> qi.z.zhang@intel.com
> *Cc:* dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com;
> simei.su@intel.com
> *Subject:* Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP
> PDU type
>
> hi, Ori
>
> On 4/5/2020 11:56 PM, Ori Kam wrote:
>
> Hi Jeff,
>
> -----Original Message-----
>
> From: Jeff Guo<jia.guo@intel.com> <mailto:jia.guo@intel.com>
>
> Sent: Tuesday, March 31, 2020 11:50 AM
>
> To: Ori Kam<orika@mellanox.com> <mailto:orika@mellanox.com>;xiaolong.ye@intel.com <mailto:xiaolong.ye@intel.com>;
>
> qi.z.zhang@intel.com <mailto:qi.z.zhang@intel.com>
>
> Cc:dev@dpdk.org <mailto:dev@dpdk.org>;jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>;yahui.cao@intel.com <mailto:yahui.cao@intel.com>;
>
> simei.su@intel.com <mailto:simei.su@intel.com>
>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>
> yes, Ori, please check the comment below.
>
> On 3/30/2020 6:18 PM, Ori Kam wrote:
>
> Hi Jeff,
>
> My name is Ori😊
>
> I'm not an expert in GTP so this is just my thinking and maybe I'm
>
> missing something, this is why a good explanation helps😊
>
> -----Original Message-----
>
> From: Jeff Guo<jia.guo@intel.com> <mailto:jia.guo@intel.com>
>
> Sent: Monday, March 30, 2020 11:30 AM
>
> To: Ori Kam<orika@mellanox.com> <mailto:orika@mellanox.com>;xiaolong.ye@intel.com <mailto:xiaolong.ye@intel.com>;
>
> qi.z.zhang@intel.com <mailto:qi.z.zhang@intel.com>
>
> Cc:dev@dpdk.org <mailto:dev@dpdk.org>;jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>;yahui.cao@intel.com <mailto:yahui.cao@intel.com>;
>
> simei.su@intel.com <mailto:simei.su@intel.com>
>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU
>
> type
>
> hi, orika
>
> On 3/29/2020 4:44 PM, Ori Kam wrote:
>
> Hi Jeff,
>
> -----Original Message-----
>
> From: dev<dev-bounces@dpdk.org> <mailto:dev-bounces@dpdk.org> On Behalf Of Jeff Guo
>
> Sent: Thursday, March 26, 2020 6:41 PM
>
> To:xiaolong.ye@intel.com <mailto:xiaolong.ye@intel.com>;qi.z.zhang@intel.com <mailto:qi.z.zhang@intel.com>
>
> Cc:dev@dpdk.org <mailto:dev@dpdk.org>;jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>;yahui.cao@intel.com <mailto:yahui.cao@intel.com>;
>
> simei.su@intel.com <mailto:simei.su@intel.com>;jia.guo@intel.com <mailto:jia.guo@intel.com>
>
> Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
>
> Add gtp pdu type configure in the cmdline.
>
> Why not use ITEM_GTP_PSC_PDU?
>
> I guess you mean ITEM_GTP_PSC_PDU_T, rihgt? We know we have got
>
> ITEM_GTP_PSC_QFI/ITEM_GTP_PSC_PDU_T but not define the
>
> spec for them, so what i use is add the spec into the ITEM_GTP_PSC_PDU_T
>
> to let the pdu type to be configured.
>
> Yes you are correct, from rte_flow we have the
>
> RTE_FLOW_ITEM_TYPE_GTP_PSC
>
> Item that include pdu_type. This is the field you need right?
>
> In testpmd we have the ITEM_GTP_PSC_PDU_T which should support adding
>
> the pdu type.
>
> Basically you just need to type the following cmd line:
>
> flow create 0 ingress pattern gtp_psc pdu_t is xxx
>
> if this command is not working we need to understand why.
>
> please check the part before this patch as below:
>
> [ITEM_GTP_PSC_PDU_T] = {
>
> .name = "pdu_t",
>
> .help = "PDU type",
>
> .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
>
> item_param),
>
> sure, we got the ITEM_GTP_PSC_PDU_T at prior but the NEXT_ENTRY is
>
> UNSIGNED, that means we still not implement
>
> Sorry I don't understand your comment, what do you mean it is not implemented?
>
> Yes it means that the parameter is should be unsigned value.
>
> I mean that if it is a unsigned value, user could not set the pdu_t to
> be a 0 or 1, or any other we
>
> define for that.
>
> the spec to let the pdu type to be configurable, so what the patch do is
>
> to fix this issue.
>
> What do you mean configurable?
>
> Lets start at the beginning, maybe I'm just missing some key point.
>
> What is the PDU type? What values can he hold?
>
> How do you want the command to look like?
>
> the command should be like as below
>
> flow create 0 ingress pattern eth / ipv4 / udp / gtpu / gtp_psc pdu_t
> is 0/ ipv4 / end actions rss types ipv4-udp l3-dst-only end key_len
> 0queues end / end
>
> It is eventually the same as you described about the command before.
> User could set pdu_t to be 0 or 1, so what is need is modify
>
> NEXT_ENTRY(UNSIGNED) to be "SIGNED" and defined.
>
> [Ori] I agree about the command look.
>
> Can pdu_t can be only 0 or 1?
>
> Also I don’t understand why you need signed? Even if you add the
> GTP_PSC_PDU_T as int? and not unsinged?
>
> All other items are unsinged. I also don’t see any reason to create
> the new type unless, you want the help line.
>
> In any case, I feel that this patch already waste a lot of time.
> (for both of us)
>
> Please consider again if the new class is necessary, and if it should
> be signed.
>
> If so you have my ack.
>
> Acked-by: Ori Kam orika@mellanox.com <mailto:orika@mellanox.com>
>
> Best,
>
I guess i know your meaning now, and after check it again, what we
though is the same but the new class is absolutely not necessary,
you are totally right here Ori, thanks for your targeting it and good
review.
> Ori
>
> Signed-off-by: Jeff Guo<jia.guo@intel.com> <mailto:jia.guo@intel.com>
>
> ---
>
> v1:
>
> no change
>
> ---
>
> app/test-pmd/cmdline_flow.c | 11 ++++++++++-
>
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>
> index a78154502..c1bd02919 100644
>
> --- a/app/test-pmd/cmdline_flow.c
>
> +++ b/app/test-pmd/cmdline_flow.c
>
> @@ -49,6 +49,7 @@ enum index {
>
> PORT_ID,
>
> GROUP_ID,
>
> PRIORITY_LEVEL,
>
> + GTP_PSC_PDU_T,
>
> /* Top-level command. */
>
> SET,
>
> @@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
>
> .call = parse_int,
>
> .comp = comp_none,
>
> },
>
> + [GTP_PSC_PDU_T] = {
>
> + .name = "{GTPU pdu type}",
>
> + .type = "INTEGER",
>
> + .help = "gtpu pdu uplink/downlink identifier",
>
> + .call = parse_int,
>
> + .comp = comp_none,
>
> + },
>
> Why is this created at this level?
>
> This looks like is should be written totally differently.
>
> As i said above, the item we got but spec or say next token still need
>
> to be added, do you mean it should not in the group of Common tokens? If
>
> so, let me think about that, and please explicit your proposal if you
>
> already have one.
>
> Please see above response.
>
> /* Top-level command. */
>
> [FLOW] = {
>
> .name = "flow",
>
> @@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
>
> [ITEM_GTP_PSC_PDU_T] = {
>
> .name = "pdu_t",
>
> .help = "PDU type",
>
> - .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
>
> item_param),
>
> + .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
>
> + item_param),
>
> .args = ARGS(ARGS_ENTRY_HTON(struct
>
> rte_flow_item_gtp_psc,
>
> pdu_type)),
>
> },
>
> --
>
> 2.20.1
>
Hi Jeff,
No problem about the review. I will see v3 right?
From: Jeff Guo <jia.guo@intel.com>
Sent: Tuesday, April 14, 2020 6:06 AM
To: Ori Kam <orika@mellanox.com>; xiaolong.ye@intel.com; qi.z.zhang@intel.com
Cc: dev@dpdk.org; jingjing.wu@intel.com; yahui.cao@intel.com; simei.su@intel.com
Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
hi, Ori
On 4/12/2020 5:58 PM, Ori Kam wrote:
Hi Jeff,
For some reason I got the mail in HTML format
So please see my comments marked by [Ori]
Thanks,
Ori
From: Jeff Guo <jia.guo@intel.com><mailto:jia.guo@intel.com>
Sent: Tuesday, April 7, 2020 8:37 AM
To: Ori Kam <orika@mellanox.com><mailto:orika@mellanox.com>; xiaolong.ye@intel.com<mailto:xiaolong.ye@intel.com>; qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>; yahui.cao@intel.com<mailto:yahui.cao@intel.com>; simei.su@intel.com<mailto:simei.su@intel.com>
Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
hi, Ori
On 4/5/2020 11:56 PM, Ori Kam wrote:
Hi Jeff,
-----Original Message-----
From: Jeff Guo <jia.guo@intel.com><mailto:jia.guo@intel.com>
Sent: Tuesday, March 31, 2020 11:50 AM
To: Ori Kam <orika@mellanox.com><mailto:orika@mellanox.com>; xiaolong.ye@intel.com<mailto:xiaolong.ye@intel.com>;
qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>; yahui.cao@intel.com<mailto:yahui.cao@intel.com>;
simei.su@intel.com<mailto:simei.su@intel.com>
Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
yes, Ori, please check the comment below.
On 3/30/2020 6:18 PM, Ori Kam wrote:
Hi Jeff,
My name is Ori 😊
I'm not an expert in GTP so this is just my thinking and maybe I'm
missing something, this is why a good explanation helps 😊
-----Original Message-----
From: Jeff Guo <jia.guo@intel.com><mailto:jia.guo@intel.com>
Sent: Monday, March 30, 2020 11:30 AM
To: Ori Kam <orika@mellanox.com><mailto:orika@mellanox.com>; xiaolong.ye@intel.com<mailto:xiaolong.ye@intel.com>;
qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>; yahui.cao@intel.com<mailto:yahui.cao@intel.com>;
simei.su@intel.com<mailto:simei.su@intel.com>
Subject: Re: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU
type
hi, orika
On 3/29/2020 4:44 PM, Ori Kam wrote:
Hi Jeff,
-----Original Message-----
From: dev <dev-bounces@dpdk.org><mailto:dev-bounces@dpdk.org> On Behalf Of Jeff Guo
Sent: Thursday, March 26, 2020 6:41 PM
To: xiaolong.ye@intel.com<mailto:xiaolong.ye@intel.com>; qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>; yahui.cao@intel.com<mailto:yahui.cao@intel.com>;
simei.su@intel.com<mailto:simei.su@intel.com>; jia.guo@intel.com<mailto:jia.guo@intel.com>
Subject: [dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type
Add gtp pdu type configure in the cmdline.
Why not use ITEM_GTP_PSC_PDU?
I guess you mean ITEM_GTP_PSC_PDU_T, rihgt? We know we have got
ITEM_GTP_PSC_QFI/ITEM_GTP_PSC_PDU_T but not define the
spec for them, so what i use is add the spec into the ITEM_GTP_PSC_PDU_T
to let the pdu type to be configured.
Yes you are correct, from rte_flow we have the
RTE_FLOW_ITEM_TYPE_GTP_PSC
Item that include pdu_type. This is the field you need right?
In testpmd we have the ITEM_GTP_PSC_PDU_T which should support adding
the pdu type.
Basically you just need to type the following cmd line:
flow create 0 ingress pattern gtp_psc pdu_t is xxx
if this command is not working we need to understand why.
please check the part before this patch as below:
[ITEM_GTP_PSC_PDU_T] = {
.name = "pdu_t",
.help = "PDU type",
.next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
item_param),
sure, we got the ITEM_GTP_PSC_PDU_T at prior but the NEXT_ENTRY is
UNSIGNED, that means we still not implement
Sorry I don't understand your comment, what do you mean it is not implemented?
Yes it means that the parameter is should be unsigned value.
I mean that if it is a unsigned value, user could not set the pdu_t to be a 0 or 1, or any other we
define for that.
the spec to let the pdu type to be configurable, so what the patch do is
to fix this issue.
What do you mean configurable?
Lets start at the beginning, maybe I'm just missing some key point.
What is the PDU type? What values can he hold?
How do you want the command to look like?
the command should be like as below
flow create 0 ingress pattern eth / ipv4 / udp / gtpu / gtp_psc pdu_t is 0 / ipv4 / end actions rss types ipv4-udp l3-dst-only end key_len 0 queues end / end
It is eventually the same as you described about the command before. User could set pdu_t to be 0 or 1, so what is need is modify
NEXT_ENTRY(UNSIGNED) to be "SIGNED" and defined.
[Ori] I agree about the command look.
Can pdu_t can be only 0 or 1?
Also I don’t understand why you need signed? Even if you add the GTP_PSC_PDU_T as int? and not unsinged?
All other items are unsinged. I also don’t see any reason to create the new type unless, you want the help line.
In any case, I feel that this patch already waste a lot of time.
(for both of us)
Please consider again if the new class is necessary, and if it should be signed.
If so you have my ack.
Acked-by: Ori Kam orika@mellanox.com<mailto:orika@mellanox.com>
Best,
I guess i know your meaning now, and after check it again, what we though is the same but the new class is absolutely not necessary,
you are totally right here Ori, thanks for your targeting it and good review.
Ori
Signed-off-by: Jeff Guo <jia.guo@intel.com><mailto:jia.guo@intel.com>
---
v1:
no change
---
app/test-pmd/cmdline_flow.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index a78154502..c1bd02919 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -49,6 +49,7 @@ enum index {
PORT_ID,
GROUP_ID,
PRIORITY_LEVEL,
+ GTP_PSC_PDU_T,
/* Top-level command. */
SET,
@@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
.call = parse_int,
.comp = comp_none,
},
+ [GTP_PSC_PDU_T] = {
+ .name = "{GTPU pdu type}",
+ .type = "INTEGER",
+ .help = "gtpu pdu uplink/downlink identifier",
+ .call = parse_int,
+ .comp = comp_none,
+ },
Why is this created at this level?
This looks like is should be written totally differently.
As i said above, the item we got but spec or say next token still need
to be added, do you mean it should not in the group of Common tokens? If
so, let me think about that, and please explicit your proposal if you
already have one.
Please see above response.
/* Top-level command. */
[FLOW] = {
.name = "flow",
@@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
[ITEM_GTP_PSC_PDU_T] = {
.name = "pdu_t",
.help = "PDU type",
- .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED),
item_param),
+ .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
+ item_param),
.args = ARGS(ARGS_ENTRY_HTON(struct
rte_flow_item_gtp_psc,
pdu_type)),
},
--
2.20.1
@@ -49,6 +49,7 @@ enum index {
PORT_ID,
GROUP_ID,
PRIORITY_LEVEL,
+ GTP_PSC_PDU_T,
/* Top-level command. */
SET,
@@ -1626,6 +1627,13 @@ static const struct token token_list[] = {
.call = parse_int,
.comp = comp_none,
},
+ [GTP_PSC_PDU_T] = {
+ .name = "{GTPU pdu type}",
+ .type = "INTEGER",
+ .help = "gtpu pdu uplink/downlink identifier",
+ .call = parse_int,
+ .comp = comp_none,
+ },
/* Top-level command. */
[FLOW] = {
.name = "flow",
@@ -2615,7 +2623,8 @@ static const struct token token_list[] = {
[ITEM_GTP_PSC_PDU_T] = {
.name = "pdu_t",
.help = "PDU type",
- .next = NEXT(item_gtp_psc, NEXT_ENTRY(UNSIGNED), item_param),
+ .next = NEXT(item_gtp_psc, NEXT_ENTRY(GTP_PSC_PDU_T),
+ item_param),
.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gtp_psc,
pdu_type)),
},