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

[Jeff Guo <jia.guo@intel.com>]

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