[dpdk-dev] [dpdk-dev v2 3/4] app/testpmd: support GTP PDU type

[Jeff Guo]

hi, Ori

On 4/5/2020 11:56 PM, Ori Kam wrote:
> Hi Jeff,
>
>> -----Original Message-----
>> From: Jeff Guo <jia.guo at intel.com>
>> Sent: Tuesday, March 31, 2020 11:50 AM
>> To: Ori Kam <orika at mellanox.com>; xiaolong.ye at intel.com;
>> qi.z.zhang at intel.com
>> Cc: dev at dpdk.org; jingjing.wu at intel.com; yahui.cao at intel.com;
>> simei.su at 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 at intel.com>
>>>> Sent: Monday, March 30, 2020 11:30 AM
>>>> To: Ori Kam <orika at mellanox.com>; xiaolong.ye at intel.com;
>>>> qi.z.zhang at intel.com
>>>> Cc: dev at dpdk.org; jingjing.wu at intel.com; yahui.cao at intel.com;
>>>> simei.su at 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 at dpdk.org> On Behalf Of Jeff Guo
>>>>>> Sent: Thursday, March 26, 2020 6:41 PM
>>>>>> To: xiaolong.ye at intel.com; qi.z.zhang at intel.com
>>>>>> Cc: dev at dpdk.org; jingjing.wu at intel.com; yahui.cao at intel.com;
>>>>>> simei.su at intel.com; jia.guo at 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 at 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