[dpdk-dev] [PATCH] ethdev: update qfi definition

[dpdk-dev] [PATCH] ethdev: update qfi definition

[Raslan Darawsheh]

qfi field is 8 bits which represent single bit for
PPP (paging Policy Presence) single bit for RQI
(Reflective QoS Indicator) and 6 bits for qfi
(QoS Flow Identifier)

This update the doxygen format and the mask for qfi
to properly identify the full 8 bits of the field.

note: changing the default mask would cause different
patterns generated by testpmd.

Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
Cc: ying.a.wang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++-
 lib/librte_ethdev/rte_flow.h                | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f59eb8a27d..dd39c4c3c2 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any.
 - ``gtp_psc``: match GTP PDU extension header with type 0x85.
 
   - ``pdu_type {unsigned}``: PDU type.
-  - ``qfi {unsigned}``: QoS flow identifier.
+
+  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
 
 - ``pppoes``, ``pppoed``: match PPPoE header.
 
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 669e677e91..79106e0246 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = {
  */
 struct rte_flow_item_gtp_psc {
 	uint8_t pdu_type; /**< PDU type. */
-	uint8_t qfi; /**< QoS flow identifier. */
+	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
 };
 
 /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
 #ifndef __cplusplus
 static const struct rte_flow_item_gtp_psc
 rte_flow_item_gtp_psc_mask = {
-	.qfi = 0x3f,
+	.qfi = 0xff,
 };
 #endif
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH] ethdev: update qfi definition

[Ferruh Yigit]

On 3/23/2021 12:11 PM, Raslan Darawsheh wrote:
> qfi field is 8 bits which represent single bit for
> PPP (paging Policy Presence) single bit for RQI
> (Reflective QoS Indicator) and 6 bits for qfi
> (QoS Flow Identifier)

Can you please put a reference for above information?

> 
> This update the doxygen format and the mask for qfi
> to properly identify the full 8 bits of the field.
> 
> note: changing the default mask would cause different
> patterns generated by testpmd.
> 
> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> Cc: ying.a.wang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++-
>   lib/librte_ethdev/rte_flow.h                | 4 ++--
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index f59eb8a27d..dd39c4c3c2 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any.
>   - ``gtp_psc``: match GTP PDU extension header with type 0x85.
>   
>     - ``pdu_type {unsigned}``: PDU type.
> -  - ``qfi {unsigned}``: QoS flow identifier.
> +
> +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
>   
>   - ``pppoes``, ``pppoed``: match PPPoE header.
>   
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 669e677e91..79106e0246 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = {
>    */
>   struct rte_flow_item_gtp_psc {
>   	uint8_t pdu_type; /**< PDU type. */
> -	uint8_t qfi; /**< QoS flow identifier. */
> +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
>   };

By design requirement, rte_flow_item_* should start with the relevant protocol 
header.

There is already a deprecation notice for using protocol header directly in the 
rte_flow_item* [1] and Adrew/Ivan already fixed a few of them [2].

Your patch is not directly related on this, but since you are touching to the 
flow_item, would you mind create a protocol struct for it first, and use it in 
the "struct rte_flow_item_gtp_psc"?
So the protocol related update can be done in the protocol header, instead of 
rte_flow struct.


[1]
https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v21.02#n99

[2]
https://git.dpdk.org/next/dpdk-next-net/commit/?id=4a061a7bd70bfa023de23e8e654e

Re: [dpdk-dev] [PATCH] ethdev: update qfi definition

[Ori Kam]

Hi

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> Subject: Re: [dpdk-dev] [PATCH] ethdev: update qfi definition
> 
> On 3/23/2021 12:11 PM, Raslan Darawsheh wrote:
> > qfi field is 8 bits which represent single bit for
> > PPP (paging Policy Presence) single bit for RQI
> > (Reflective QoS Indicator) and 6 bits for qfi
> > (QoS Flow Identifier)
> 
> Can you please put a reference for above information?
> >
> > This update the doxygen format and the mask for qfi
> > to properly identify the full 8 bits of the field.
> >
> > note: changing the default mask would cause different
> > patterns generated by testpmd.
> >
> > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> > Cc: ying.a.wang@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > ---
> >   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++-
> >   lib/librte_ethdev/rte_flow.h                | 4 ++--
> >   2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index f59eb8a27d..dd39c4c3c2 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their
> attributes, if any.
> >   - ``gtp_psc``: match GTP PDU extension header with type 0x85.
> >
> >     - ``pdu_type {unsigned}``: PDU type.
> > -  - ``qfi {unsigned}``: QoS flow identifier.
> > +
> > +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
> >
> >   - ``pppoes``, ``pppoed``: match PPPoE header.
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 669e677e91..79106e0246 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta
> rte_flow_item_meta_mask = {
> >    */
> >   struct rte_flow_item_gtp_psc {
> >   	uint8_t pdu_type; /**< PDU type. */
> > -	uint8_t qfi; /**< QoS flow identifier. */
> > +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> >   };
> 
> By design requirement, rte_flow_item_* should start with the relevant protocol
> header.
> 
> There is already a deprecation notice for using protocol header directly in the
> rte_flow_item* [1] and Adrew/Ivan already fixed a few of them [2].
> 
> Your patch is not directly related on this, but since you are touching to the
> flow_item, would you mind create a protocol struct for it first, and use it in
> the "struct rte_flow_item_gtp_psc"?
> So the protocol related update can be done in the protocol header, instead of
> rte_flow struct.
> 
+1

> 
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.dpdk.
> org%2Fdpdk%2Ftree%2Fdoc%2Fguides%2Frel_notes%2Fdeprecation.rst%3Fh%3
> Dv21.02%23n99&amp;data=04%7C01%7Corika%40nvidia.com%7C6391a4c0640
> f4592b70b08d8f058e322%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0
> %7C637523611870497932%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdat
> a=vU%2F5oO47zb9erTnZL1pl9j0nHCKzea3NJgOeo1FTW0Q%3D&amp;reserved=
> 0
> 
> [2]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.dpdk.
> org%2Fnext%2Fdpdk-next-
> net%2Fcommit%2F%3Fid%3D4a061a7bd70bfa023de23e8e654e&amp;data=04%
> 7C01%7Corika%40nvidia.com%7C6391a4c0640f4592b70b08d8f058e322%7C43
> 083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637523611870497932%7CU
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik
> 1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WurlQ5VSLqFGVthfRj363xZsC
> No3xJuvxNQCFVcxdkk%3D&amp;reserved=0

Re: [dpdk-dev] [PATCH] ethdev: update qfi definition

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Monday, March 29, 2021 11:53 AM
> To: Ferruh Yigit <ferruh.yigit@intel.com>; Raslan Darawsheh
> <rasland@nvidia.com>; dev@dpdk.org; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ivan Malov <ivan.malov@oktetlabs.ru>
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin
> <shirik@nvidia.com>; ying.a.wang@intel.com; stable@dpdk.org; NBU-
> Contact-Thomas Monjalon <thomas@monjalon.net>; Olivier Matz
> <olivier.matz@6wind.com>
> Subject: RE: [dpdk-dev] [PATCH] ethdev: update qfi definition
> 
> Hi
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: update qfi definition
> >
> > On 3/23/2021 12:11 PM, Raslan Darawsheh wrote:
> > > qfi field is 8 bits which represent single bit for
> > > PPP (paging Policy Presence) single bit for RQI
> > > (Reflective QoS Indicator) and 6 bits for qfi
> > > (QoS Flow Identifier)
> >
> > Can you please put a reference for above information?
> > >
Sure, will send in V2,

> > > This update the doxygen format and the mask for qfi
> > > to properly identify the full 8 bits of the field.
> > >
> > > note: changing the default mask would cause different
> > > patterns generated by testpmd.
> > >
> > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> > > Cc: ying.a.wang@intel.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > > ---
> > >   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++-
> > >   lib/librte_ethdev/rte_flow.h                | 4 ++--
> > >   2 files changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > index f59eb8a27d..dd39c4c3c2 100644
> > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and
> their
> > attributes, if any.
> > >   - ``gtp_psc``: match GTP PDU extension header with type 0x85.
> > >
> > >     - ``pdu_type {unsigned}``: PDU type.
> > > -  - ``qfi {unsigned}``: QoS flow identifier.
> > > +
> > > +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
> > >
> > >   - ``pppoes``, ``pppoed``: match PPPoE header.
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > index 669e677e91..79106e0246 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta
> > rte_flow_item_meta_mask = {
> > >    */
> > >   struct rte_flow_item_gtp_psc {
> > >   	uint8_t pdu_type; /**< PDU type. */
> > > -	uint8_t qfi; /**< QoS flow identifier. */
> > > +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> > >   };
> >
> > By design requirement, rte_flow_item_* should start with the relevant
> protocol
> > header.
> >
> > There is already a deprecation notice for using protocol header directly in
> the
> > rte_flow_item* [1] and Adrew/Ivan already fixed a few of them [2].
> >
> > Your patch is not directly related on this, but since you are touching to the
> > flow_item, would you mind create a protocol struct for it first, and use it in
> > the "struct rte_flow_item_gtp_psc"?
> > So the protocol related update can be done in the protocol header, instead
> of
> > rte_flow struct.
> >
> +1
Sure, I can create the new protocol and use it , and will send in V2. But, wouldn't it cause any ABI breakage ?
> 
> >
> > [1]
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.d
> pdk.
> >
> org%2Fdpdk%2Ftree%2Fdoc%2Fguides%2Frel_notes%2Fdeprecation.rst%3F
> h%3
> >
> Dv21.02%23n99&amp;data=04%7C01%7Corika%40nvidia.com%7C6391a4c064
> 0
> >
> f4592b70b08d8f058e322%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0
> >
> %7C637523611870497932%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> w
> >
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sda
> t
> >
> a=vU%2F5oO47zb9erTnZL1pl9j0nHCKzea3NJgOeo1FTW0Q%3D&amp;reserve
> d=
> > 0
> >
> > [2]
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.d
> pdk.
> > org%2Fnext%2Fdpdk-next-
> >
> net%2Fcommit%2F%3Fid%3D4a061a7bd70bfa023de23e8e654e&amp;data=0
> 4%
> >
> 7C01%7Corika%40nvidia.com%7C6391a4c0640f4592b70b08d8f058e322%7C43
> >
> 083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637523611870497932%7CU
> >
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> 6Ik
> >
> 1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WurlQ5VSLqFGVthfRj363xZs
> C
> > No3xJuvxNQCFVcxdkk%3D&amp;reserved=0

Re: [dpdk-dev] [PATCH] ethdev: update qfi definition

[Ferruh Yigit]

On 3/29/2021 10:06 AM, Raslan Darawsheh wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Ori Kam <orika@nvidia.com>
>> Sent: Monday, March 29, 2021 11:53 AM
>> To: Ferruh Yigit <ferruh.yigit@intel.com>; Raslan Darawsheh
>> <rasland@nvidia.com>; dev@dpdk.org; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Ivan Malov <ivan.malov@oktetlabs.ru>
>> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin
>> <shirik@nvidia.com>; ying.a.wang@intel.com; stable@dpdk.org; NBU-
>> Contact-Thomas Monjalon <thomas@monjalon.net>; Olivier Matz
>> <olivier.matz@6wind.com>
>> Subject: RE: [dpdk-dev] [PATCH] ethdev: update qfi definition
>>
>> Hi
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: update qfi definition
>>>
>>> On 3/23/2021 12:11 PM, Raslan Darawsheh wrote:
>>>> qfi field is 8 bits which represent single bit for
>>>> PPP (paging Policy Presence) single bit for RQI
>>>> (Reflective QoS Indicator) and 6 bits for qfi
>>>> (QoS Flow Identifier)
>>>
>>> Can you please put a reference for above information?
>>>>
> Sure, will send in V2,
> 
>>>> This update the doxygen format and the mask for qfi
>>>> to properly identify the full 8 bits of the field.
>>>>
>>>> note: changing the default mask would cause different
>>>> patterns generated by testpmd.
>>>>
>>>> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
>>>> Cc: ying.a.wang@intel.com
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>>>> ---
>>>>    doc/guides/testpmd_app_ug/testpmd_funcs.rst | 3 ++-
>>>>    lib/librte_ethdev/rte_flow.h                | 4 ++--
>>>>    2 files changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> index f59eb8a27d..dd39c4c3c2 100644
>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and
>> their
>>> attributes, if any.
>>>>    - ``gtp_psc``: match GTP PDU extension header with type 0x85.
>>>>
>>>>      - ``pdu_type {unsigned}``: PDU type.
>>>> -  - ``qfi {unsigned}``: QoS flow identifier.
>>>> +
>>>> +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
>>>>
>>>>    - ``pppoes``, ``pppoed``: match PPPoE header.
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
>>>> index 669e677e91..79106e0246 100644
>>>> --- a/lib/librte_ethdev/rte_flow.h
>>>> +++ b/lib/librte_ethdev/rte_flow.h
>>>> @@ -1392,14 +1392,14 @@ static const struct rte_flow_item_meta
>>> rte_flow_item_meta_mask = {
>>>>     */
>>>>    struct rte_flow_item_gtp_psc {
>>>>    	uint8_t pdu_type; /**< PDU type. */
>>>> -	uint8_t qfi; /**< QoS flow identifier. */
>>>> +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
>>>>    };
>>>
>>> By design requirement, rte_flow_item_* should start with the relevant
>> protocol
>>> header.
>>>
>>> There is already a deprecation notice for using protocol header directly in
>> the
>>> rte_flow_item* [1] and Adrew/Ivan already fixed a few of them [2].
>>>
>>> Your patch is not directly related on this, but since you are touching to the
>>> flow_item, would you mind create a protocol struct for it first, and use it in
>>> the "struct rte_flow_item_gtp_psc"?
>>> So the protocol related update can be done in the protocol header, instead
>> of
>>> rte_flow struct.
>>>
>> +1
> Sure, I can create the new protocol and use it , and will send in V2. But, wouldn't it cause any ABI breakage ?

For the protocol header, it won't be problem since it is a new struct.

For the flow item struct, it may be, that is why need to use a union for now, 
please check what Adrew/Ivan did.

>>
>>>
>>> [1]
>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.d
>> pdk.
>>>
>> org%2Fdpdk%2Ftree%2Fdoc%2Fguides%2Frel_notes%2Fdeprecation.rst%3F
>> h%3
>>>
>> Dv21.02%23n99&amp;data=04%7C01%7Corika%40nvidia.com%7C6391a4c064
>> 0
>>>
>> f4592b70b08d8f058e322%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0
>>>
>> %7C637523611870497932%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>> w
>>>
>> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sda
>> t
>>>
>> a=vU%2F5oO47zb9erTnZL1pl9j0nHCKzea3NJgOeo1FTW0Q%3D&amp;reserve
>> d=
>>> 0
>>>
>>> [2]
>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.d
>> pdk.
>>> org%2Fnext%2Fdpdk-next-
>>>
>> net%2Fcommit%2F%3Fid%3D4a061a7bd70bfa023de23e8e654e&amp;data=0
>> 4%
>>>
>> 7C01%7Corika%40nvidia.com%7C6391a4c0640f4592b70b08d8f058e322%7C43
>>>
>> 083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637523611870497932%7CU
>>>
>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>> 6Ik
>>>
>> 1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WurlQ5VSLqFGVthfRj363xZs
>> C
>>> No3xJuvxNQCFVcxdkk%3D&amp;reserved=0
> 

[dpdk-dev] [PATCH v2 0/2] fix gtp psc qfi support

[Raslan Darawsheh]

This is fixing the gtp psc support to match the RFC 38415-g30

v2: introduce new header definition for gtp psc
    update commit msg for rte flow item change.

Raslan Darawsheh (2):
  ethdev: add new ext hdr for gtp psc
  ethdev: update qfi definition

 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 +-
 lib/librte_ethdev/rte_flow.h                | 18 +++++++++--
 lib/librte_net/rte_gtp.h                    | 34 +++++++++++++++++++++
 3 files changed, 51 insertions(+), 4 deletions(-)

-- 
2.29.0

[dpdk-dev] [PATCH v2 1/2] ethdev: add new ext hdr for gtp psc

[Raslan Darawsheh]

Define new rte header for gtp PDU session container
based on RFC 38415-g30

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
index 6a6f9b238d..8af48ea1ec 100644
--- a/lib/librte_net/rte_gtp.h
+++ b/lib/librte_net/rte_gtp.h
@@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
 	uint8_t next_ext;     /**< Next Extension Header Type. */
 }  __rte_packed;
 
+/**
+ * Optional extention for GTP with next_ext set to 0x85
+ * defined based on RFC 38415-g30.
+ */
+__extension__
+struct rte_gtp_psc {
+	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
+	uint8_t type:4; /**< PDU type */
+	uint8_t qmp:1; /**< Qos Monitoring Packet */
+	union {
+		struct {
+			uint8_t snp:1; /**< Sequence number presence */
+			uint8_t spare_dl1:2; /**< spare down link bits */
+		};
+		struct {
+			uint8_t dl_delay_ind:1; /**< dl delay result presence */
+			uint8_t ul_delay_ind:1; /**< ul delay result presence */
+			uint8_t snp_ul1:1; /**< Sequence number presence ul */
+		};
+	};
+	union {
+		struct {
+			uint8_t ppp:1; /**< Paging policy presence */
+			uint8_t rqi:1; /**< Reflective Qos Indicator */
+		};
+		struct {
+			uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
+			uint8_t spare_ul2:1; /**< spare up link bits */
+		};
+	};
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+	uint8_t data[0]; /**< data feilds */
+} __rte_packed;
+
 /** GTP header length */
 #define RTE_ETHER_GTP_HLEN \
 	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
-- 
2.29.0

[dpdk-dev] [PATCH v2 2/2] ethdev: update qfi definition

[Raslan Darawsheh]

qfi field is 8 bits which represent single bit for
PPP (paging Policy Presence) single bit for RQI
(Reflective QoS Indicator) and 6 bits for qfi
(QoS Flow Identifier) based on RFC 38415-g30

This update the doxygen format and the mask for qfi
to properly identify the full 8 bits of the field.

note: changing the default mask would cause different
patterns generated by testpmd.

Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
Cc: ying.a.wang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
 lib/librte_ethdev/rte_flow.h                | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f59eb8a27d..dd39c4c3c2 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any.
 - ``gtp_psc``: match GTP PDU extension header with type 0x85.
 
   - ``pdu_type {unsigned}``: PDU type.
-  - ``qfi {unsigned}``: QoS flow identifier.
+
+  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
 
 - ``pppoes``, ``pppoed``: match PPPoE header.
 
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 6cc57136ac..1eb9711707 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -20,6 +20,7 @@
 #include <rte_arp.h>
 #include <rte_common.h>
 #include <rte_ether.h>
+#include <rte_gtp.h>
 #include <rte_icmp.h>
 #include <rte_ip.h>
 #include <rte_sctp.h>
@@ -1421,16 +1422,27 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = {
  *
  * Matches a GTP PDU extension header with type 0x85.
  */
+RTE_STD_C11
 struct rte_flow_item_gtp_psc {
-	uint8_t pdu_type; /**< PDU type. */
-	uint8_t qfi; /**< QoS flow identifier. */
+	union {
+		struct {
+			/*
+			 * These fields are retained for compatibility.
+			 * Please switch to the new header field below.
+			 */
+			uint8_t pdu_type; /**< PDU type. */
+			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
+
+		};
+		struct rte_gtp_psc gtp_psc;
+	};
 };
 
 /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
 #ifndef __cplusplus
 static const struct rte_flow_item_gtp_psc
 rte_flow_item_gtp_psc_mask = {
-	.qfi = 0x3f,
+	.qfi = 0xff,
 };
 #endif
 
-- 
2.29.0

[dpdk-dev] [PATCH v3 0/2] fix gtp psc qfi support

[Raslan Darawsheh]

This is fixing the gtp psc support to match the RFC 38415-g30

v2: introduce new header definition for gtp psc
    update commit msg for rte flow item change.

v3: fixed typo in comment
    Cc relevant people.
 
Raslan Darawsheh (2):
  ethdev: add new ext hdr for gtp psc
  ethdev: update qfi definition

 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 +-
 lib/librte_ethdev/rte_flow.h                | 18 +++++++++--
 lib/librte_net/rte_gtp.h                    | 34 +++++++++++++++++++++
 3 files changed, 51 insertions(+), 4 deletions(-)

-- 
2.29.0

[dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc

[Raslan Darawsheh]

Define new rte header for gtp PDU session container
based on RFC 38415-g30

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
index 6a6f9b238d..bfaae26535 100644
--- a/lib/librte_net/rte_gtp.h
+++ b/lib/librte_net/rte_gtp.h
@@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
 	uint8_t next_ext;     /**< Next Extension Header Type. */
 }  __rte_packed;
 
+/**
+ * Optional extension for GTP with next_ext set to 0x85
+ * defined based on RFC 38415-g30.
+ */
+__extension__
+struct rte_gtp_psc {
+	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
+	uint8_t type:4; /**< PDU type */
+	uint8_t qmp:1; /**< Qos Monitoring Packet */
+	union {
+		struct {
+			uint8_t snp:1; /**< Sequence number presence */
+			uint8_t spare_dl1:2; /**< spare down link bits */
+		};
+		struct {
+			uint8_t dl_delay_ind:1; /**< dl delay result presence */
+			uint8_t ul_delay_ind:1; /**< ul delay result presence */
+			uint8_t snp_ul1:1; /**< Sequence number presence ul */
+		};
+	};
+	union {
+		struct {
+			uint8_t ppp:1; /**< Paging policy presence */
+			uint8_t rqi:1; /**< Reflective Qos Indicator */
+		};
+		struct {
+			uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
+			uint8_t spare_ul2:1; /**< spare up link bits */
+		};
+	};
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+	uint8_t data[0]; /**< data feilds */
+} __rte_packed;
+
 /** GTP header length */
 #define RTE_ETHER_GTP_HLEN \
 	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
-- 
2.29.0

[dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition

[Raslan Darawsheh]

qfi field is 8 bits which represent single bit for
PPP (paging Policy Presence) single bit for RQI
(Reflective QoS Indicator) and 6 bits for qfi
(QoS Flow Identifier) based on RFC 38415-g30

This update the doxygen format and the mask for qfi
to properly identify the full 8 bits of the field.

note: changing the default mask would cause different
patterns generated by testpmd.

Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
Cc: ying.a.wang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
 lib/librte_ethdev/rte_flow.h                | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f59eb8a27d..dd39c4c3c2 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any.
 - ``gtp_psc``: match GTP PDU extension header with type 0x85.
 
   - ``pdu_type {unsigned}``: PDU type.
-  - ``qfi {unsigned}``: QoS flow identifier.
+
+  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
 
 - ``pppoes``, ``pppoed``: match PPPoE header.
 
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 6cc57136ac..1eb9711707 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -20,6 +20,7 @@
 #include <rte_arp.h>
 #include <rte_common.h>
 #include <rte_ether.h>
+#include <rte_gtp.h>
 #include <rte_icmp.h>
 #include <rte_ip.h>
 #include <rte_sctp.h>
@@ -1421,16 +1422,27 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = {
  *
  * Matches a GTP PDU extension header with type 0x85.
  */
+RTE_STD_C11
 struct rte_flow_item_gtp_psc {
-	uint8_t pdu_type; /**< PDU type. */
-	uint8_t qfi; /**< QoS flow identifier. */
+	union {
+		struct {
+			/*
+			 * These fields are retained for compatibility.
+			 * Please switch to the new header field below.
+			 */
+			uint8_t pdu_type; /**< PDU type. */
+			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
+
+		};
+		struct rte_gtp_psc gtp_psc;
+	};
 };
 
 /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
 #ifndef __cplusplus
 static const struct rte_flow_item_gtp_psc
 rte_flow_item_gtp_psc_mask = {
-	.qfi = 0x3f,
+	.qfi = 0xff,
 };
 #endif
 
-- 
2.29.0

Re: [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc

[Ferruh Yigit]

On 3/30/2021 9:00 AM, Raslan Darawsheh wrote:
> Define new rte header for gtp PDU session container
> based on RFC 38415-g30
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>   lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
> index 6a6f9b238d..bfaae26535 100644
> --- a/lib/librte_net/rte_gtp.h
> +++ b/lib/librte_net/rte_gtp.h
> @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
>   	uint8_t next_ext;     /**< Next Extension Header Type. */
>   }  __rte_packed;
>   
> +/**
> + * Optional extension for GTP with next_ext set to 0x85
> + * defined based on RFC 38415-g30.
> + */
> +__extension__
> +struct rte_gtp_psc {

general consensus in other protocols seems having '_hdr' suffix in the struct 
name, I can see this is extension but do you think does it make to add suffix?

> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> +	uint8_t type:4; /**< PDU type */
> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> +	union {
> +		struct {
> +			uint8_t snp:1; /**< Sequence number presence */
> +			uint8_t spare_dl1:2; /**< spare down link bits */
> +		};
> +		struct {
> +			uint8_t dl_delay_ind:1; /**< dl delay result presence */
> +			uint8_t ul_delay_ind:1; /**< ul delay result presence */
> +			uint8_t snp_ul1:1; /**< Sequence number presence ul */
> +		};
> +	};
> +	union {
> +		struct {
> +			uint8_t ppp:1; /**< Paging policy presence */
> +			uint8_t rqi:1; /**< Reflective Qos Indicator */
> +		};
> +		struct {
> +			uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> +			uint8_t spare_ul2:1; /**< spare up link bits */
> +		};
> +	};
> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> +	uint8_t data[0]; /**< data feilds */
> +} __rte_packed;
> +
>   /** GTP header length */
>   #define RTE_ETHER_GTP_HLEN \
>   	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
> 

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition

[Ferruh Yigit]

On 3/30/2021 9:00 AM, Raslan Darawsheh wrote:
> qfi field is 8 bits which represent single bit for
> PPP (paging Policy Presence) single bit for RQI
> (Reflective QoS Indicator) and 6 bits for qfi
> (QoS Flow Identifier) based on RFC 38415-g30
> 
> This update the doxygen format and the mask for qfi
> to properly identify the full 8 bits of the field.
> 
> note: changing the default mask would cause different
> patterns generated by testpmd.
> 
> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> Cc: ying.a.wang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
>   lib/librte_ethdev/rte_flow.h                | 18 +++++++++++++++---
>   2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index f59eb8a27d..dd39c4c3c2 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any.
>   - ``gtp_psc``: match GTP PDU extension header with type 0x85.
>   
>     - ``pdu_type {unsigned}``: PDU type.
> -  - ``qfi {unsigned}``: QoS flow identifier.
> +
> +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
>   
>   - ``pppoes``, ``pppoed``: match PPPoE header.
>   
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 6cc57136ac..1eb9711707 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -20,6 +20,7 @@
>   #include <rte_arp.h>
>   #include <rte_common.h>
>   #include <rte_ether.h>
> +#include <rte_gtp.h>
>   #include <rte_icmp.h>
>   #include <rte_ip.h>
>   #include <rte_sctp.h>
> @@ -1421,16 +1422,27 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = {
>    *
>    * Matches a GTP PDU extension header with type 0x85.
>    */
> +RTE_STD_C11
>   struct rte_flow_item_gtp_psc {
> -	uint8_t pdu_type; /**< PDU type. */
> -	uint8_t qfi; /**< QoS flow identifier. */
> +	union {
> +		struct {
> +			/*
> +			 * These fields are retained for compatibility.
> +			 * Please switch to the new header field below.
> +			 */
> +			uint8_t pdu_type; /**< PDU type. */
> +			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> +
> +		};
> +		struct rte_gtp_psc gtp_psc;

Again for consistency, what do you think to rename the variable as 'hdr'?

> +	};
>   };
>   
>   /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
>   #ifndef __cplusplus
>   static const struct rte_flow_item_gtp_psc
>   rte_flow_item_gtp_psc_mask = {
> -	.qfi = 0x3f,
> +	.qfi = 0xff,

Since the protocol header is the preferred way, (individual fields may be 
deprecated in the future), can you please switch to new field, like:
  .gtp_psc.qfi = 0xff,

>   };
>   #endif
>   
> 

Re: [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc

[Raslan Darawsheh]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, April 1, 2021 7:51 PM
> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org
> Cc: Ori Kam <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru;
> ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; olivier.matz@6wind.com;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin <shirik@nvidia.com>
> Subject: Re: [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc
> 
> On 3/30/2021 9:00 AM, Raslan Darawsheh wrote:
> > Define new rte header for gtp PDU session container
> > based on RFC 38415-g30
> >
> > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > ---
> >   lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 34 insertions(+)
> >
> > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
> > index 6a6f9b238d..bfaae26535 100644
> > --- a/lib/librte_net/rte_gtp.h
> > +++ b/lib/librte_net/rte_gtp.h
> > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
> >   	uint8_t next_ext;     /**< Next Extension Header Type. */
> >   }  __rte_packed;
> >
> > +/**
> > + * Optional extension for GTP with next_ext set to 0x85
> > + * defined based on RFC 38415-g30.
> > + */
> > +__extension__
> > +struct rte_gtp_psc {
> 
> general consensus in other protocols seems having '_hdr' suffix in the struct
> name, I can see this is extension but do you think does it make to add suffix?
Yes sure, will send in a new version.
> 
> > +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> > +	uint8_t type:4; /**< PDU type */
> > +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> > +	union {
> > +		struct {
> > +			uint8_t snp:1; /**< Sequence number presence */
> > +			uint8_t spare_dl1:2; /**< spare down link bits */
> > +		};
> > +		struct {
> > +			uint8_t dl_delay_ind:1; /**< dl delay result presence
> */
> > +			uint8_t ul_delay_ind:1; /**< ul delay result presence
> */
> > +			uint8_t snp_ul1:1; /**< Sequence number presence
> ul */
> > +		};
> > +	};
> > +	union {
> > +		struct {
> > +			uint8_t ppp:1; /**< Paging policy presence */
> > +			uint8_t rqi:1; /**< Reflective Qos Indicator */
> > +		};
> > +		struct {
> > +			uint8_t n_delay_ind:1; /**< N3/N9 delay result
> presence */
> > +			uint8_t spare_ul2:1; /**< spare up link bits */
> > +		};
> > +	};
> > +	uint8_t qfi:6; /**< Qos Flow Identifier */
> > +	uint8_t data[0]; /**< data feilds */
> > +} __rte_packed;
> > +
> >   /** GTP header length */
> >   #define RTE_ETHER_GTP_HLEN \
> >   	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
> >

Kindest regards
Raslan Darawsheh

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition

[Raslan Darawsheh]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, April 1, 2021 7:54 PM
> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org
> Cc: Ori Kam <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru;
> ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; olivier.matz@6wind.com;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin <shirik@nvidia.com>;
> stable@dpdk.org
> Subject: Re: [PATCH v3 2/2] ethdev: update qfi definition
> 
> On 3/30/2021 9:00 AM, Raslan Darawsheh wrote:
> > qfi field is 8 bits which represent single bit for
> > PPP (paging Policy Presence) single bit for RQI
> > (Reflective QoS Indicator) and 6 bits for qfi
> > (QoS Flow Identifier) based on RFC 38415-g30
> >
> > This update the doxygen format and the mask for qfi
> > to properly identify the full 8 bits of the field.
> >
> > note: changing the default mask would cause different
> > patterns generated by testpmd.
> >
> > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> > Cc: ying.a.wang@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > ---
> >   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
> >   lib/librte_ethdev/rte_flow.h                | 18 +++++++++++++++---
> >   2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index f59eb8a27d..dd39c4c3c2 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and
> their attributes, if any.
> >   - ``gtp_psc``: match GTP PDU extension header with type 0x85.
> >
> >     - ``pdu_type {unsigned}``: PDU type.
> > -  - ``qfi {unsigned}``: QoS flow identifier.
> > +
> > +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
> >
> >   - ``pppoes``, ``pppoed``: match PPPoE header.
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 6cc57136ac..1eb9711707 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -20,6 +20,7 @@
> >   #include <rte_arp.h>
> >   #include <rte_common.h>
> >   #include <rte_ether.h>
> > +#include <rte_gtp.h>
> >   #include <rte_icmp.h>
> >   #include <rte_ip.h>
> >   #include <rte_sctp.h>
> > @@ -1421,16 +1422,27 @@ static const struct rte_flow_item_meta
> rte_flow_item_meta_mask = {
> >    *
> >    * Matches a GTP PDU extension header with type 0x85.
> >    */
> > +RTE_STD_C11
> >   struct rte_flow_item_gtp_psc {
> > -	uint8_t pdu_type; /**< PDU type. */
> > -	uint8_t qfi; /**< QoS flow identifier. */
> > +	union {
> > +		struct {
> > +			/*
> > +			 * These fields are retained for compatibility.
> > +			 * Please switch to the new header field below.
> > +			 */
> > +			uint8_t pdu_type; /**< PDU type. */
> > +			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> > +
> > +		};
> > +		struct rte_gtp_psc gtp_psc;
> 
> Again for consistency, what do you think to rename the variable as 'hdr'?
Will do,
> 
> > +	};
> >   };
> >
> >   /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
> >   #ifndef __cplusplus
> >   static const struct rte_flow_item_gtp_psc
> >   rte_flow_item_gtp_psc_mask = {
> > -	.qfi = 0x3f,
> > +	.qfi = 0xff,
> 
> Since the protocol header is the preferred way, (individual fields may be
> deprecated in the future), can you please switch to new field, like:
Yes, will do it accordingly
>   .gtp_psc.qfi = 0xff,
> 
> >   };
> >   #endif
> >
> >

[dpdk-dev] [PATCH v4 0/2] fix gtp psc qfi support

[Raslan Darawsheh]

This is fixing the gtp psc support to match the RFC 38415-g30

v2: introduce new header definition for gtp psc
    update commit msg for rte flow item change.

v3: fixed typo in comment
    Cc relevant people.

v4: update hdr definition to have hdr suffix.
    update variable name to be hdr in the gtp_psc item.
    update default max to use the new added hdr.

Raslan Darawsheh (2):
  ethdev: add new ext hdr for gtp psc
  ethdev: update qfi definition

 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 +-
 lib/librte_ethdev/rte_flow.h                | 20 ++++++++++--
 lib/librte_net/rte_gtp.h                    | 34 +++++++++++++++++++++
 3 files changed, 53 insertions(+), 4 deletions(-)

-- 
2.29.0

[dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc

[Raslan Darawsheh]

Define new rte header for gtp PDU session container
based on RFC 38415-g30

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
index 6a6f9b238d..088b0b5a53 100644
--- a/lib/librte_net/rte_gtp.h
+++ b/lib/librte_net/rte_gtp.h
@@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
 	uint8_t next_ext;     /**< Next Extension Header Type. */
 }  __rte_packed;
 
+/**
+ * Optional extension for GTP with next_ext set to 0x85
+ * defined based on RFC 38415-g30.
+ */
+__extension__
+struct rte_gtp_psc_hdr {
+	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
+	uint8_t type:4; /**< PDU type */
+	uint8_t qmp:1; /**< Qos Monitoring Packet */
+	union {
+		struct {
+			uint8_t snp:1; /**< Sequence number presence */
+			uint8_t spare_dl1:2; /**< spare down link bits */
+		};
+		struct {
+			uint8_t dl_delay_ind:1; /**< dl delay result presence */
+			uint8_t ul_delay_ind:1; /**< ul delay result presence */
+			uint8_t snp_ul1:1; /**< Sequence number presence ul */
+		};
+	};
+	union {
+		struct {
+			uint8_t ppp:1; /**< Paging policy presence */
+			uint8_t rqi:1; /**< Reflective Qos Indicator */
+		};
+		struct {
+			uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
+			uint8_t spare_ul2:1; /**< spare up link bits */
+		};
+	};
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+	uint8_t data[0]; /**< data feilds */
+} __rte_packed;
+
 /** GTP header length */
 #define RTE_ETHER_GTP_HLEN \
 	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
-- 
2.29.0

[dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition

[Raslan Darawsheh]

qfi field is 8 bits which represent single bit for
PPP (paging Policy Presence) single bit for RQI
(Reflective QoS Indicator) and 6 bits for qfi
(QoS Flow Identifier) based on RFC 38415-g30

This update the doxygen format and the mask for qfi
to properly identify the full 8 bits of the field.

note: changing the default mask would cause different
patterns generated by testpmd.

Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
Cc: ying.a.wang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
 lib/librte_ethdev/rte_flow.h                | 20 +++++++++++++++++---
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f59eb8a27d..dd39c4c3c2 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any.
 - ``gtp_psc``: match GTP PDU extension header with type 0x85.
 
   - ``pdu_type {unsigned}``: PDU type.
-  - ``qfi {unsigned}``: QoS flow identifier.
+
+  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
 
 - ``pppoes``, ``pppoed``: match PPPoE header.
 
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 6cc57136ac..20b4389429 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -20,6 +20,7 @@
 #include <rte_arp.h>
 #include <rte_common.h>
 #include <rte_ether.h>
+#include <rte_gtp.h>
 #include <rte_icmp.h>
 #include <rte_ip.h>
 #include <rte_sctp.h>
@@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = {
  *
  * Matches a GTP PDU extension header with type 0x85.
  */
+RTE_STD_C11
 struct rte_flow_item_gtp_psc {
-	uint8_t pdu_type; /**< PDU type. */
-	uint8_t qfi; /**< QoS flow identifier. */
+	union {
+		struct {
+			/*
+			 * These fields are retained for compatibility.
+			 * Please switch to the new header field below.
+			 */
+			uint8_t pdu_type; /**< PDU type. */
+			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
+
+		};
+		struct rte_gtp_psc_hdr hdr;
+	};
 };
 
 /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
 #ifndef __cplusplus
 static const struct rte_flow_item_gtp_psc
 rte_flow_item_gtp_psc_mask = {
-	.qfi = 0x3f,
+	.hdr.ppp = 1,
+	.hdr.rqi = 1,
+	.hdr.qfi = 0x3f,
 };
 #endif
 
-- 
2.29.0

Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition

[Ferruh Yigit]

On 4/4/2021 8:45 AM, Raslan Darawsheh wrote:
> qfi field is 8 bits which represent single bit for
> PPP (paging Policy Presence) single bit for RQI
> (Reflective QoS Indicator) and 6 bits for qfi
> (QoS Flow Identifier) based on RFC 38415-g30
> 
> This update the doxygen format and the mask for qfi
> to properly identify the full 8 bits of the field.
> 
> note: changing the default mask would cause different
> patterns generated by testpmd.
> 
> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> Cc: ying.a.wang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
>   lib/librte_ethdev/rte_flow.h                | 20 +++++++++++++++++---
>   2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index f59eb8a27d..dd39c4c3c2 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and their attributes, if any.
>   - ``gtp_psc``: match GTP PDU extension header with type 0x85.
>   
>     - ``pdu_type {unsigned}``: PDU type.
> -  - ``qfi {unsigned}``: QoS flow identifier.
> +
> +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
>   
>   - ``pppoes``, ``pppoed``: match PPPoE header.
>   
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 6cc57136ac..20b4389429 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -20,6 +20,7 @@
>   #include <rte_arp.h>
>   #include <rte_common.h>
>   #include <rte_ether.h>
> +#include <rte_gtp.h>
>   #include <rte_icmp.h>
>   #include <rte_ip.h>
>   #include <rte_sctp.h>
> @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta rte_flow_item_meta_mask = {
>    *
>    * Matches a GTP PDU extension header with type 0x85.
>    */
> +RTE_STD_C11
>   struct rte_flow_item_gtp_psc {
> -	uint8_t pdu_type; /**< PDU type. */
> -	uint8_t qfi; /**< QoS flow identifier. */
> +	union {
> +		struct {
> +			/*
> +			 * These fields are retained for compatibility.
> +			 * Please switch to the new header field below.
> +			 */
> +			uint8_t pdu_type; /**< PDU type. */
> +			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> +
> +		};
> +		struct rte_gtp_psc_hdr hdr;
> +	};
>   };

This will change the struct size even with union, since old version is missing 
all fields from protocol header. Struct size will go from 2 --> 5 bytes [1].

Since this is public struct, we can't change its size.

@Ori, Andrew,

Do you have a suggestion for next step?

- We can still add the "struct rte_gtp_psc_hdr", and add a deprecation notice 
for "struct rte_flow_item_gtp_psc" to say it will use  "struct rte_gtp_psc_hdr" 
on 21.11.

- And for this release use the Raslan's first version:
   -	uint8_t qfi; /**< QoS flow identifier. */
   +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */


Does it make sense? Any comments?



[1]
struct rte_gtp_psc_hdr {
         uint8_t                    ext_hdr_len;          /*     0     1 */
         uint8_t                    type:4;               /*     1: 0  1 */
         uint8_t                    qmp:1;                /*     1: 4  1 */

         /* XXX 3 bits hole, try to pack */

         union {
                 struct {
                         uint8_t    snp:1;                /*     2: 0  1 */
                         uint8_t    spare_dl1:2;          /*     2: 1  1 */
                 };                                       /*     2     1 */
                 struct {
                         uint8_t    dl_delay_ind:1;       /*     2: 0  1 */
                         uint8_t    ul_delay_ind:1;       /*     2: 1  1 */
                         uint8_t    snp_ul1:1;            /*     2: 2  1 */
                 };                                       /*     2     1 */
         };                                               /*     2     1 */
         union {
                 struct {
                         uint8_t    ppp:1;                /*     3: 0  1 */
                         uint8_t    rqi:1;                /*     3: 1  1 */
                 };                                       /*     3     1 */
                 struct {
                         uint8_t    n_delay_ind:1;        /*     3: 0  1 */
                         uint8_t    spare_ul2:1;          /*     3: 1  1 */
                 };                                       /*     3     1 */
         };                                               /*     3     1 */
         uint8_t                    qfi:6;                /*     4: 0  1 */

         /* XXX 2 bits hole, try to pack */

         uint8_t                    data[];               /*     5     0 */

         /* size: 5, cachelines: 1, members: 7 */
         /* sum members: 3 */
         /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits */
         /* last cacheline: 5 bytes */
};

>   
>   /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
>   #ifndef __cplusplus
>   static const struct rte_flow_item_gtp_psc
>   rte_flow_item_gtp_psc_mask = {
> -	.qfi = 0x3f,
> +	.hdr.ppp = 1,
> +	.hdr.rqi = 1,
> +	.hdr.qfi = 0x3f,
>   };
>   #endif
>   
> 

Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc

[Olivier Matz]

Hi Raslan,

On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote:
> Define new rte header for gtp PDU session container
> based on RFC 38415-g30

Do you have a link to this RFC?

> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>  lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
> index 6a6f9b238d..088b0b5a53 100644
> --- a/lib/librte_net/rte_gtp.h
> +++ b/lib/librte_net/rte_gtp.h
> @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
>  	uint8_t next_ext;     /**< Next Extension Header Type. */
>  }  __rte_packed;
>  
> +/**
> + * Optional extension for GTP with next_ext set to 0x85
> + * defined based on RFC 38415-g30.
> + */
> +__extension__
> +struct rte_gtp_psc_hdr {
> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> +	uint8_t type:4; /**< PDU type */
> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> +	union {
> +		struct {
> +			uint8_t snp:1; /**< Sequence number presence */
> +			uint8_t spare_dl1:2; /**< spare down link bits */
> +		};
> +		struct {
> +			uint8_t dl_delay_ind:1; /**< dl delay result presence */
> +			uint8_t ul_delay_ind:1; /**< ul delay result presence */
> +			uint8_t snp_ul1:1; /**< Sequence number presence ul */
> +		};
> +	};
> +	union {
> +		struct {
> +			uint8_t ppp:1; /**< Paging policy presence */
> +			uint8_t rqi:1; /**< Reflective Qos Indicator */
> +		};
> +		struct {
> +			uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> +			uint8_t spare_ul2:1; /**< spare up link bits */
> +		};
> +	};
> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> +	uint8_t data[0]; /**< data feilds */
> +} __rte_packed;

With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected?

It would help to see the specification to have a better idea of how to
split, but a possible solution is to do something like this:

struct rte_gtp_psc_generic_hdr {
	uint8_t ext_hdr_len;
	uint8_t type:4
	uint8_t qmp:1;
	uint8_t pad:3;
};

struct rte_gtp_psc_<name1>_hdr {
	uint8_t ext_hdr_len;
	uint8_t type:4
	uint8_t qmp:1;
	uint8_t uint8_t snp:1;
	uint8_t spare_dl1:2;
	...
};

...

struct rte_gtp_psc_hdr {
	union {
		struct rte_gtp_psc_generic_hdr generic;
		struct rte_gtp_psc_<name1>_hdr <name1>;
		struct rte_gtp_psc_<name2>_hdr <name2>;
	};
};

Also, you need to take care about endianness.


Regards,
Olivier

Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc

[Raslan Darawsheh]

Hi Olivier,

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, April 8, 2021 3:30 PM
> To: Raslan Darawsheh <rasland@nvidia.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>;
> andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru;
> ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
> Kuzin <shirik@nvidia.com>
> Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
> 
> Hi Raslan,
> 
> On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote:
> > Define new rte header for gtp PDU session container
> > based on RFC 38415-g30
> 
> Do you have a link to this RFC?
Yes sure,
https://www.3gpp.org/ftp/Specs/archive/38_series/38.415/38415-g30.zip

> 
> > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > ---
> >  lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
> > index 6a6f9b238d..088b0b5a53 100644
> > --- a/lib/librte_net/rte_gtp.h
> > +++ b/lib/librte_net/rte_gtp.h
> > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
> >  	uint8_t next_ext;     /**< Next Extension Header Type. */
> >  }  __rte_packed;
> >
> > +/**
> > + * Optional extension for GTP with next_ext set to 0x85
> > + * defined based on RFC 38415-g30.
> > + */
> > +__extension__
> > +struct rte_gtp_psc_hdr {
> > +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> > +	uint8_t type:4; /**< PDU type */
> > +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> > +	union {
> > +		struct {
> > +			uint8_t snp:1; /**< Sequence number presence */
> > +			uint8_t spare_dl1:2; /**< spare down link bits */
> > +		};
> > +		struct {
> > +			uint8_t dl_delay_ind:1; /**< dl delay result presence
> */
> > +			uint8_t ul_delay_ind:1; /**< ul delay result presence
> */
> > +			uint8_t snp_ul1:1; /**< Sequence number presence
> ul */
> > +		};
> > +	};
> > +	union {
> > +		struct {
> > +			uint8_t ppp:1; /**< Paging policy presence */
> > +			uint8_t rqi:1; /**< Reflective Qos Indicator */
> > +		};
> > +		struct {
> > +			uint8_t n_delay_ind:1; /**< N3/N9 delay result
> presence */
> > +			uint8_t spare_ul2:1; /**< spare up link bits */
> > +		};
> > +	};
> > +	uint8_t qfi:6; /**< Qos Flow Identifier */
> > +	uint8_t data[0]; /**< data feilds */
> > +} __rte_packed;
> 
> With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected?
The data[0] is variable length data, I guess I should send another version to mention that in the comment maybe.
The header size according to the spec should be 4 octets aligned in general.
> 
> It would help to see the specification to have a better idea of how to
Sure, I've just posted the link above, please let me know of any suggestion that you have, and I'll be glad to do accordingly.

> split, but a possible solution is to do something like this:
> 
> struct rte_gtp_psc_generic_hdr {
> 	uint8_t ext_hdr_len;
> 	uint8_t type:4
> 	uint8_t qmp:1;
> 	uint8_t pad:3;
> };
> 
> struct rte_gtp_psc_<name1>_hdr {
> 	uint8_t ext_hdr_len;
> 	uint8_t type:4
> 	uint8_t qmp:1;
> 	uint8_t uint8_t snp:1;
> 	uint8_t spare_dl1:2;
> 	...
> };
> 
> ...
> 
> struct rte_gtp_psc_hdr {
> 	union {
> 		struct rte_gtp_psc_generic_hdr generic;
> 		struct rte_gtp_psc_<name1>_hdr <name1>;
> 		struct rte_gtp_psc_<name2>_hdr <name2>;
> 	};
> };
> 
> Also, you need to take care about endianness.
> 
> 
> Regards,
> Olivier
Kindest regards
Raslan Darawsheh

Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc

[Ferruh Yigit]

On 4/8/2021 1:37 PM, Raslan Darawsheh wrote:
> Hi Olivier,
> 
>> -----Original Message-----
>> From: Olivier Matz <olivier.matz@6wind.com>
>> Sent: Thursday, April 8, 2021 3:30 PM
>> To: Raslan Darawsheh <rasland@nvidia.com>
>> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>;
>> andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru;
>> ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
>> Kuzin <shirik@nvidia.com>
>> Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
>>
>> Hi Raslan,
>>
>> On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote:
>>> Define new rte header for gtp PDU session container
>>> based on RFC 38415-g30
>>
>> Do you have a link to this RFC?
> Yes sure,
> https://www.3gpp.org/ftp/Specs/archive/38_series/38.415/38415-g30.zip
> 
>>
>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>>> ---
>>>   lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 34 insertions(+)
>>>
>>> diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
>>> index 6a6f9b238d..088b0b5a53 100644
>>> --- a/lib/librte_net/rte_gtp.h
>>> +++ b/lib/librte_net/rte_gtp.h
>>> @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
>>>   	uint8_t next_ext;     /**< Next Extension Header Type. */
>>>   }  __rte_packed;
>>>
>>> +/**
>>> + * Optional extension for GTP with next_ext set to 0x85
>>> + * defined based on RFC 38415-g30.
>>> + */
>>> +__extension__
>>> +struct rte_gtp_psc_hdr {
>>> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
>>> +	uint8_t type:4; /**< PDU type */
>>> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
>>> +	union {
>>> +		struct {
>>> +			uint8_t snp:1; /**< Sequence number presence */
>>> +			uint8_t spare_dl1:2; /**< spare down link bits */
>>> +		};
>>> +		struct {
>>> +			uint8_t dl_delay_ind:1; /**< dl delay result presence
>> */
>>> +			uint8_t ul_delay_ind:1; /**< ul delay result presence
>> */
>>> +			uint8_t snp_ul1:1; /**< Sequence number presence
>> ul */
>>> +		};
>>> +	};
>>> +	union {
>>> +		struct {
>>> +			uint8_t ppp:1; /**< Paging policy presence */
>>> +			uint8_t rqi:1; /**< Reflective Qos Indicator */
>>> +		};
>>> +		struct {
>>> +			uint8_t n_delay_ind:1; /**< N3/N9 delay result
>> presence */
>>> +			uint8_t spare_ul2:1; /**< spare up link bits */
>>> +		};
>>> +	};
>>> +	uint8_t qfi:6; /**< Qos Flow Identifier */
>>> +	uint8_t data[0]; /**< data feilds */
>>> +} __rte_packed;
>>
>> With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected?
> The data[0] is variable length data, I guess I should send another version to mention that in the comment maybe.
> The header size according to the spec should be 4 octets aligned in general.

The struct is 5 btyes, this is not related to data[0], please check the pahole 
output:
http://inbox.dpdk.org/dev/536631b9-c634-ddac-c154-91978ffc29a5@intel.com/

>>
>> It would help to see the specification to have a better idea of how to
> Sure, I've just posted the link above, please let me know of any suggestion that you have, and I'll be glad to do accordingly.
> 
>> split, but a possible solution is to do something like this:
>>
>> struct rte_gtp_psc_generic_hdr {
>> 	uint8_t ext_hdr_len;
>> 	uint8_t type:4
>> 	uint8_t qmp:1;
>> 	uint8_t pad:3;
>> };
>>
>> struct rte_gtp_psc_<name1>_hdr {
>> 	uint8_t ext_hdr_len;
>> 	uint8_t type:4
>> 	uint8_t qmp:1;
>> 	uint8_t uint8_t snp:1;
>> 	uint8_t spare_dl1:2;
>> 	...
>> };
>>
>> ...
>>
>> struct rte_gtp_psc_hdr {
>> 	union {
>> 		struct rte_gtp_psc_generic_hdr generic;
>> 		struct rte_gtp_psc_<name1>_hdr <name1>;
>> 		struct rte_gtp_psc_<name2>_hdr <name2>;
>> 	};
>> };
>>
>> Also, you need to take care about endianness.
>>
>>
>> Regards,
>> Olivier
> Kindest regards
> Raslan Darawsheh
> 

Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc

[Olivier Matz]

On Thu, Apr 08, 2021 at 12:37:27PM +0000, Raslan Darawsheh wrote:
> Hi Olivier,
> 
> > -----Original Message-----
> > From: Olivier Matz <olivier.matz@6wind.com>
> > Sent: Thursday, April 8, 2021 3:30 PM
> > To: Raslan Darawsheh <rasland@nvidia.com>
> > Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>;
> > andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru;
> > ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
> > Kuzin <shirik@nvidia.com>
> > Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
> > 
> > Hi Raslan,
> > 
> > On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote:
> > > Define new rte header for gtp PDU session container
> > > based on RFC 38415-g30
> > 
> > Do you have a link to this RFC?
> Yes sure,
> https://www.3gpp.org/ftp/Specs/archive/38_series/38.415/38415-g30.zip
> 
> > 
> > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > > ---
> > >  lib/librte_net/rte_gtp.h | 34 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
> > > index 6a6f9b238d..088b0b5a53 100644
> > > --- a/lib/librte_net/rte_gtp.h
> > > +++ b/lib/librte_net/rte_gtp.h
> > > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
> > >  	uint8_t next_ext;     /**< Next Extension Header Type. */
> > >  }  __rte_packed;
> > >
> > > +/**
> > > + * Optional extension for GTP with next_ext set to 0x85
> > > + * defined based on RFC 38415-g30.
> > > + */
> > > +__extension__
> > > +struct rte_gtp_psc_hdr {
> > > +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> > > +	uint8_t type:4; /**< PDU type */
> > > +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> > > +	union {
> > > +		struct {
> > > +			uint8_t snp:1; /**< Sequence number presence */
> > > +			uint8_t spare_dl1:2; /**< spare down link bits */
> > > +		};
> > > +		struct {
> > > +			uint8_t dl_delay_ind:1; /**< dl delay result presence
> > */
> > > +			uint8_t ul_delay_ind:1; /**< ul delay result presence
> > */
> > > +			uint8_t snp_ul1:1; /**< Sequence number presence
> > ul */
> > > +		};
> > > +	};
> > > +	union {
> > > +		struct {
> > > +			uint8_t ppp:1; /**< Paging policy presence */
> > > +			uint8_t rqi:1; /**< Reflective Qos Indicator */
> > > +		};
> > > +		struct {
> > > +			uint8_t n_delay_ind:1; /**< N3/N9 delay result
> > presence */
> > > +			uint8_t spare_ul2:1; /**< spare up link bits */
> > > +		};
> > > +	};
> > > +	uint8_t qfi:6; /**< Qos Flow Identifier */
> > > +	uint8_t data[0]; /**< data feilds */
> > > +} __rte_packed;
> > 
> > With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected?
> The data[0] is variable length data, I guess I should send another version to mention that in the comment maybe.
> The header size according to the spec should be 4 octets aligned in general.

What I wanted to highlight is that using union of structs containing
bitfields does not work as you expect: each union is at least 1 byte.
This results in a structure that does not match the expected header.

> > 
> > It would help to see the specification to have a better idea of how to
> Sure, I've just posted the link above, please let me know of any suggestion that you have, and I'll be glad to do accordingly.
> 
> > split, but a possible solution is to do something like this:
> > 
> > struct rte_gtp_psc_generic_hdr {
> > 	uint8_t ext_hdr_len;
> > 	uint8_t type:4
> > 	uint8_t qmp:1;
> > 	uint8_t pad:3;
> > };
> > 
> > struct rte_gtp_psc_<name1>_hdr {
> > 	uint8_t ext_hdr_len;
> > 	uint8_t type:4
> > 	uint8_t qmp:1;
> > 	uint8_t uint8_t snp:1;
> > 	uint8_t spare_dl1:2;
> > 	...
> > };
> > 
> > ...
> > 
> > struct rte_gtp_psc_hdr {
> > 	union {
> > 		struct rte_gtp_psc_generic_hdr generic;
> > 		struct rte_gtp_psc_<name1>_hdr <name1>;
> > 		struct rte_gtp_psc_<name2>_hdr <name2>;
> > 	};
> > };

From what I see in the documation, I think this approach should
work. From afar, I suggest:

struct rte_gtp_psc_generic_hdr {
#if big endian
	uint8_t type:4
	uint8_t qmp:1;
	uint8_t pad:3;
#else
	uint8_t pad:3;
	uint8_t qmp:1;
	uint8_t type:4
#endif
};

struct rte_gtp_psc_type0_hdr {
#if big endian
	uint8_t type:4
	uint8_t qmp:1;
	uint8_t snp:1;
	uint8_t spare:2;

	uint8_t ppp:1;
	...
#else
	uint8_t pad:3;
	uint8_t qmp:1;
	uint8_t type:4
	uint8_t spare:2;
	uint8_t snp:1;

	...
#endif
	uint8_t data[0]; /* for variable fields */
};

struct rte_gtp_psc_type1_hdr {
	... same for fixed fields of type1


	uint8_t data[0]; /* for variable fields */
};

I don't see in the spec where is the reference to ext_hdr_len.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc

[Raslan Darawsheh]

Hi and sorry for late response, 


> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, April 8, 2021 5:11 PM
> To: Raslan Darawsheh <rasland@nvidia.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>;
> andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru;
> ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
> Kuzin <shirik@nvidia.com>
> Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
> 
> On Thu, Apr 08, 2021 at 12:37:27PM +0000, Raslan Darawsheh wrote:
> > Hi Olivier,
> >
> > > -----Original Message-----
> > > From: Olivier Matz <olivier.matz@6wind.com>
> > > Sent: Thursday, April 8, 2021 3:30 PM
> > > To: Raslan Darawsheh <rasland@nvidia.com>
> > > Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>;
> > > andrew.rybchenko@oktetlabs.ru; ivan.malov@oktetlabs.ru;
> > > ying.a.wang@intel.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
> > > Kuzin <shirik@nvidia.com>
> > > Subject: Re: [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
> > >
> > > Hi Raslan,
> > >
> > > On Sun, Apr 04, 2021 at 10:45:51AM +0300, Raslan Darawsheh wrote:
> > > > Define new rte header for gtp PDU session container
> > > > based on RFC 38415-g30
> > >
> > > Do you have a link to this RFC?
> > Yes sure,
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> w.3gpp.org%2Fftp%2FSpecs%2Farchive%2F38_series%2F38.415%2F38415-
> g30.zip&amp;data=04%7C01%7Crasland%40nvidia.com%7C5279dde172024bc
> d1f6b08d8fa981668%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C6
> 37534878435191995%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=
> LuD7%2FvJgqJbZ78ncaM7UrpiLNPUt7zbPPfSMemyb2Y8%3D&amp;reserved
> =0
> >
> > >
> > > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > > > ---
> > > >  lib/librte_net/rte_gtp.h | 34
> ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/lib/librte_net/rte_gtp.h b/lib/librte_net/rte_gtp.h
> > > > index 6a6f9b238d..088b0b5a53 100644
> > > > --- a/lib/librte_net/rte_gtp.h
> > > > +++ b/lib/librte_net/rte_gtp.h
> > > > @@ -61,6 +61,40 @@ struct rte_gtp_hdr_ext_word {
> > > >  	uint8_t next_ext;     /**< Next Extension Header Type. */
> > > >  }  __rte_packed;
> > > >
> > > > +/**
> > > > + * Optional extension for GTP with next_ext set to 0x85
> > > > + * defined based on RFC 38415-g30.
> > > > + */
> > > > +__extension__
> > > > +struct rte_gtp_psc_hdr {
> > > > +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> > > > +	uint8_t type:4; /**< PDU type */
> > > > +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> > > > +	union {
> > > > +		struct {
> > > > +			uint8_t snp:1; /**< Sequence number presence */
> > > > +			uint8_t spare_dl1:2; /**< spare down link bits */
> > > > +		};
> > > > +		struct {
> > > > +			uint8_t dl_delay_ind:1; /**< dl delay result presence
> > > */
> > > > +			uint8_t ul_delay_ind:1; /**< ul delay result presence
> > > */
> > > > +			uint8_t snp_ul1:1; /**< Sequence number presence
> > > ul */
> > > > +		};
> > > > +	};
> > > > +	union {
> > > > +		struct {
> > > > +			uint8_t ppp:1; /**< Paging policy presence */
> > > > +			uint8_t rqi:1; /**< Reflective Qos Indicator */
> > > > +		};
> > > > +		struct {
> > > > +			uint8_t n_delay_ind:1; /**< N3/N9 delay result
> > > presence */
> > > > +			uint8_t spare_ul2:1; /**< spare up link bits */
> > > > +		};
> > > > +	};
> > > > +	uint8_t qfi:6; /**< Qos Flow Identifier */
> > > > +	uint8_t data[0]; /**< data feilds */
> > > > +} __rte_packed;
> > >
> > > With this header, sizeof(rte_gtp_psc_hdr) = 5, is it really expected?
> > The data[0] is variable length data, I guess I should send another version to
> mention that in the comment maybe.
> > The header size according to the spec should be 4 octets aligned in general.
> 
> What I wanted to highlight is that using union of structs containing
> bitfields does not work as you expect: each union is at least 1 byte.
> This results in a structure that does not match the expected header.
I see thanks for explaining.
> 
> > >
> > > It would help to see the specification to have a better idea of how to
> > Sure, I've just posted the link above, please let me know of any suggestion
> that you have, and I'll be glad to do accordingly.
> >
> > > split, but a possible solution is to do something like this:
> > >
> > > struct rte_gtp_psc_generic_hdr {
> > > 	uint8_t ext_hdr_len;
> > > 	uint8_t type:4
> > > 	uint8_t qmp:1;
> > > 	uint8_t pad:3;
> > > };
> > >
> > > struct rte_gtp_psc_<name1>_hdr {
> > > 	uint8_t ext_hdr_len;
> > > 	uint8_t type:4
> > > 	uint8_t qmp:1;
> > > 	uint8_t uint8_t snp:1;
> > > 	uint8_t spare_dl1:2;
> > > 	...
> > > };
> > >
> > > ...
> > >
> > > struct rte_gtp_psc_hdr {
> > > 	union {
> > > 		struct rte_gtp_psc_generic_hdr generic;
> > > 		struct rte_gtp_psc_<name1>_hdr <name1>;
> > > 		struct rte_gtp_psc_<name2>_hdr <name2>;
> > > 	};
> > > };
> 
> From what I see in the documation, I think this approach should
> work. From afar, I suggest:
> 
> struct rte_gtp_psc_generic_hdr {
> #if big endian
> 	uint8_t type:4
> 	uint8_t qmp:1;
> 	uint8_t pad:3;
> #else
> 	uint8_t pad:3;
> 	uint8_t qmp:1;
> 	uint8_t type:4
> #endif
> };
> 
> struct rte_gtp_psc_type0_hdr {
> #if big endian
> 	uint8_t type:4
> 	uint8_t qmp:1;
> 	uint8_t snp:1;
> 	uint8_t spare:2;
> 
> 	uint8_t ppp:1;
> 	...
> #else
> 	uint8_t pad:3;
> 	uint8_t qmp:1;
> 	uint8_t type:4
> 	uint8_t spare:2;
> 	uint8_t snp:1;
> 
> 	...
> #endif
> 	uint8_t data[0]; /* for variable fields */
> };
> 
> struct rte_gtp_psc_type1_hdr {
> 	... same for fixed fields of type1
> 
> 
> 	uint8_t data[0]; /* for variable fields */
> };
> 
Sure, will consider and do it this way in the next version.

> I don't see in the spec where is the reference to ext_hdr_len.
> 
The ext_hdr_len is part of the definition for all ext_hdrs see this link for more details:
https://en.wikipedia.org/wiki/GPRS_Tunnelling_Protocol
So, it's not mentioned in the doc.

> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, April 6, 2021 7:10 PM
> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ori Kam
> <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru
> Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com;
> olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
> Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand
> <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>
> Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition
> 
> On 4/4/2021 8:45 AM, Raslan Darawsheh wrote:
> > qfi field is 8 bits which represent single bit for
> > PPP (paging Policy Presence) single bit for RQI
> > (Reflective QoS Indicator) and 6 bits for qfi
> > (QoS Flow Identifier) based on RFC 38415-g30
> >
> > This update the doxygen format and the mask for qfi
> > to properly identify the full 8 bits of the field.
> >
> > note: changing the default mask would cause different
> > patterns generated by testpmd.
> >
> > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> > Cc: ying.a.wang@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > ---
> >   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
> >   lib/librte_ethdev/rte_flow.h                | 20 +++++++++++++++++---
> >   2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index f59eb8a27d..dd39c4c3c2 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and
> their attributes, if any.
> >   - ``gtp_psc``: match GTP PDU extension header with type 0x85.
> >
> >     - ``pdu_type {unsigned}``: PDU type.
> > -  - ``qfi {unsigned}``: QoS flow identifier.
> > +
> > +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
> >
> >   - ``pppoes``, ``pppoed``: match PPPoE header.
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 6cc57136ac..20b4389429 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -20,6 +20,7 @@
> >   #include <rte_arp.h>
> >   #include <rte_common.h>
> >   #include <rte_ether.h>
> > +#include <rte_gtp.h>
> >   #include <rte_icmp.h>
> >   #include <rte_ip.h>
> >   #include <rte_sctp.h>
> > @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta
> rte_flow_item_meta_mask = {
> >    *
> >    * Matches a GTP PDU extension header with type 0x85.
> >    */
> > +RTE_STD_C11
> >   struct rte_flow_item_gtp_psc {
> > -	uint8_t pdu_type; /**< PDU type. */
> > -	uint8_t qfi; /**< QoS flow identifier. */
> > +	union {
> > +		struct {
> > +			/*
> > +			 * These fields are retained for compatibility.
> > +			 * Please switch to the new header field below.
> > +			 */
> > +			uint8_t pdu_type; /**< PDU type. */
> > +			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> > +
> > +		};
> > +		struct rte_gtp_psc_hdr hdr;
> > +	};
> >   };
> 
> This will change the struct size even with union, since old version is missing
> all fields from protocol header. Struct size will go from 2 --> 5 bytes [1].
> 
> Since this is public struct, we can't change its size.
> 
> @Ori, Andrew,
> 
> Do you have a suggestion for next step?
> 
> - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation notice
> for "struct rte_flow_item_gtp_psc" to say it will use  "struct
> rte_gtp_psc_hdr"
> on 21.11.
> 
> - And for this release use the Raslan's first version:
>    -	uint8_t qfi; /**< QoS flow identifier. */
>    +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> 
> 
> Does it make sense? Any comments?
> 
@Ori Kam, @andrew.rybchenko@oktetlabs.ru, 
This is a kind remainder of this patch, 

> 
> 
> [1]
> struct rte_gtp_psc_hdr {
>          uint8_t                    ext_hdr_len;          /*     0     1 */
>          uint8_t                    type:4;               /*     1: 0  1 */
>          uint8_t                    qmp:1;                /*     1: 4  1 */
> 
>          /* XXX 3 bits hole, try to pack */
> 
>          union {
>                  struct {
>                          uint8_t    snp:1;                /*     2: 0  1 */
>                          uint8_t    spare_dl1:2;          /*     2: 1  1 */
>                  };                                       /*     2     1 */
>                  struct {
>                          uint8_t    dl_delay_ind:1;       /*     2: 0  1 */
>                          uint8_t    ul_delay_ind:1;       /*     2: 1  1 */
>                          uint8_t    snp_ul1:1;            /*     2: 2  1 */
>                  };                                       /*     2     1 */
>          };                                               /*     2     1 */
>          union {
>                  struct {
>                          uint8_t    ppp:1;                /*     3: 0  1 */
>                          uint8_t    rqi:1;                /*     3: 1  1 */
>                  };                                       /*     3     1 */
>                  struct {
>                          uint8_t    n_delay_ind:1;        /*     3: 0  1 */
>                          uint8_t    spare_ul2:1;          /*     3: 1  1 */
>                  };                                       /*     3     1 */
>          };                                               /*     3     1 */
>          uint8_t                    qfi:6;                /*     4: 0  1 */
> 
>          /* XXX 2 bits hole, try to pack */
> 
>          uint8_t                    data[];               /*     5     0 */
> 
>          /* size: 5, cachelines: 1, members: 7 */
>          /* sum members: 3 */
>          /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits */
>          /* last cacheline: 5 bytes */
> };
> 
> >
> >   /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
> >   #ifndef __cplusplus
> >   static const struct rte_flow_item_gtp_psc
> >   rte_flow_item_gtp_psc_mask = {
> > -	.qfi = 0x3f,
> > +	.hdr.ppp = 1,
> > +	.hdr.rqi = 1,
> > +	.hdr.qfi = 0x3f,
> >   };
> >   #endif
> >
> >

Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition

[Ori Kam]

Hi Raslan,

> -----Original Message-----
> From: Raslan Darawsheh <rasland@nvidia.com>
> Subject: RE: [PATCH v4 2/2] ethdev: update qfi definition
> 
> Hi,
> 
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > Sent: Tuesday, April 6, 2021 7:10 PM
> > To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ori Kam
> > <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru
> > Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com;
> > olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
> > Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand
> > <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon
> > <thomas@monjalon.net>
> > Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition
> >
> > On 4/4/2021 8:45 AM, Raslan Darawsheh wrote:
> > > qfi field is 8 bits which represent single bit for
> > > PPP (paging Policy Presence) single bit for RQI
> > > (Reflective QoS Indicator) and 6 bits for qfi
> > > (QoS Flow Identifier) based on RFC 38415-g30
> > >
> > > This update the doxygen format and the mask for qfi
> > > to properly identify the full 8 bits of the field.
> > >
> > > note: changing the default mask would cause different
> > > patterns generated by testpmd.
> > >
> > > Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> > > Cc: ying.a.wang@intel.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> > > ---
> > >   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
> > >   lib/librte_ethdev/rte_flow.h                | 20 +++++++++++++++++---
> > >   2 files changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > index f59eb8a27d..dd39c4c3c2 100644
> > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > @@ -3742,7 +3742,8 @@ This section lists supported pattern items and
> > their attributes, if any.
> > >   - ``gtp_psc``: match GTP PDU extension header with type 0x85.
> > >
> > >     - ``pdu_type {unsigned}``: PDU type.
> > > -  - ``qfi {unsigned}``: QoS flow identifier.
> > > +
> > > +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
> > >
> > >   - ``pppoes``, ``pppoed``: match PPPoE header.
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > index 6cc57136ac..20b4389429 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -20,6 +20,7 @@
> > >   #include <rte_arp.h>
> > >   #include <rte_common.h>
> > >   #include <rte_ether.h>
> > > +#include <rte_gtp.h>
> > >   #include <rte_icmp.h>
> > >   #include <rte_ip.h>
> > >   #include <rte_sctp.h>
> > > @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta
> > rte_flow_item_meta_mask = {
> > >    *
> > >    * Matches a GTP PDU extension header with type 0x85.
> > >    */
> > > +RTE_STD_C11
> > >   struct rte_flow_item_gtp_psc {
> > > -	uint8_t pdu_type; /**< PDU type. */
> > > -	uint8_t qfi; /**< QoS flow identifier. */
> > > +	union {
> > > +		struct {
> > > +			/*
> > > +			 * These fields are retained for compatibility.
> > > +			 * Please switch to the new header field below.
> > > +			 */
> > > +			uint8_t pdu_type; /**< PDU type. */
> > > +			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> > > +
> > > +		};
> > > +		struct rte_gtp_psc_hdr hdr;
> > > +	};
> > >   };
> >
> > This will change the struct size even with union, since old version is missing
> > all fields from protocol header. Struct size will go from 2 --> 5 bytes [1].
> >
> > Since this is public struct, we can't change its size.
> >
> > @Ori, Andrew,
> >
> > Do you have a suggestion for next step?
> >

I think Ferruh is right, and I think that we should at this point just update the documentation.
Sorry for the detour 
Just small comment about the original  patch.
I don’t think you should change the default mask since it may break existing apps.

> > - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation notice
> > for "struct rte_flow_item_gtp_psc" to say it will use  "struct
> > rte_gtp_psc_hdr"
> > on 21.11.
> >
> > - And for this release use the Raslan's first version:
> >    -	uint8_t qfi; /**< QoS flow identifier. */
> >    +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> >
> >
> > Does it make sense? Any comments?
> >
> @Ori Kam, @andrew.rybchenko@oktetlabs.ru,
> This is a kind remainder of this patch,
> 
> >
> >
> > [1]
> > struct rte_gtp_psc_hdr {
> >          uint8_t                    ext_hdr_len;          /*     0     1 */
> >          uint8_t                    type:4;               /*     1: 0  1 */
> >          uint8_t                    qmp:1;                /*     1: 4  1 */
> >
> >          /* XXX 3 bits hole, try to pack */
> >
> >          union {
> >                  struct {
> >                          uint8_t    snp:1;                /*     2: 0  1 */
> >                          uint8_t    spare_dl1:2;          /*     2: 1  1 */
> >                  };                                       /*     2     1 */
> >                  struct {
> >                          uint8_t    dl_delay_ind:1;       /*     2: 0  1 */
> >                          uint8_t    ul_delay_ind:1;       /*     2: 1  1 */
> >                          uint8_t    snp_ul1:1;            /*     2: 2  1 */
> >                  };                                       /*     2     1 */
> >          };                                               /*     2     1 */
> >          union {
> >                  struct {
> >                          uint8_t    ppp:1;                /*     3: 0  1 */
> >                          uint8_t    rqi:1;                /*     3: 1  1 */
> >                  };                                       /*     3     1 */
> >                  struct {
> >                          uint8_t    n_delay_ind:1;        /*     3: 0  1 */
> >                          uint8_t    spare_ul2:1;          /*     3: 1  1 */
> >                  };                                       /*     3     1 */
> >          };                                               /*     3     1 */
> >          uint8_t                    qfi:6;                /*     4: 0  1 */
> >
> >          /* XXX 2 bits hole, try to pack */
> >
> >          uint8_t                    data[];               /*     5     0 */
> >
> >          /* size: 5, cachelines: 1, members: 7 */
> >          /* sum members: 3 */
> >          /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits */
> >          /* last cacheline: 5 bytes */
> > };
> >
> > >
> > >   /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
> > >   #ifndef __cplusplus
> > >   static const struct rte_flow_item_gtp_psc
> > >   rte_flow_item_gtp_psc_mask = {
> > > -	.qfi = 0x3f,
> > > +	.hdr.ppp = 1,
> > > +	.hdr.rqi = 1,
> > > +	.hdr.qfi = 0x3f,
> > >   };
> > >   #endif
> > >
> > >

Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition

[Ferruh Yigit]

On 4/13/2021 10:24 AM, Ori Kam wrote:
> Hi Raslan,
> 
>> -----Original Message-----
>> From: Raslan Darawsheh <rasland@nvidia.com>
>> Subject: RE: [PATCH v4 2/2] ethdev: update qfi definition
>>
>> Hi,
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Sent: Tuesday, April 6, 2021 7:10 PM
>>> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ori Kam
>>> <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru
>>> Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com;
>>> olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
>>> Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand
>>> <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon
>>> <thomas@monjalon.net>
>>> Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition
>>>
>>> On 4/4/2021 8:45 AM, Raslan Darawsheh wrote:
>>>> qfi field is 8 bits which represent single bit for
>>>> PPP (paging Policy Presence) single bit for RQI
>>>> (Reflective QoS Indicator) and 6 bits for qfi
>>>> (QoS Flow Identifier) based on RFC 38415-g30
>>>>
>>>> This update the doxygen format and the mask for qfi
>>>> to properly identify the full 8 bits of the field.
>>>>
>>>> note: changing the default mask would cause different
>>>> patterns generated by testpmd.
>>>>
>>>> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
>>>> Cc: ying.a.wang@intel.com
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>>>> ---
>>>>    doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
>>>>    lib/librte_ethdev/rte_flow.h                | 20 +++++++++++++++++---
>>>>    2 files changed, 19 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> index f59eb8a27d..dd39c4c3c2 100644
>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and
>>> their attributes, if any.
>>>>    - ``gtp_psc``: match GTP PDU extension header with type 0x85.
>>>>
>>>>      - ``pdu_type {unsigned}``: PDU type.
>>>> -  - ``qfi {unsigned}``: QoS flow identifier.
>>>> +
>>>> +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
>>>>
>>>>    - ``pppoes``, ``pppoed``: match PPPoE header.
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
>>>> index 6cc57136ac..20b4389429 100644
>>>> --- a/lib/librte_ethdev/rte_flow.h
>>>> +++ b/lib/librte_ethdev/rte_flow.h
>>>> @@ -20,6 +20,7 @@
>>>>    #include <rte_arp.h>
>>>>    #include <rte_common.h>
>>>>    #include <rte_ether.h>
>>>> +#include <rte_gtp.h>
>>>>    #include <rte_icmp.h>
>>>>    #include <rte_ip.h>
>>>>    #include <rte_sctp.h>
>>>> @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta
>>> rte_flow_item_meta_mask = {
>>>>     *
>>>>     * Matches a GTP PDU extension header with type 0x85.
>>>>     */
>>>> +RTE_STD_C11
>>>>    struct rte_flow_item_gtp_psc {
>>>> -	uint8_t pdu_type; /**< PDU type. */
>>>> -	uint8_t qfi; /**< QoS flow identifier. */
>>>> +	union {
>>>> +		struct {
>>>> +			/*
>>>> +			 * These fields are retained for compatibility.
>>>> +			 * Please switch to the new header field below.
>>>> +			 */
>>>> +			uint8_t pdu_type; /**< PDU type. */
>>>> +			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
>>>> +
>>>> +		};
>>>> +		struct rte_gtp_psc_hdr hdr;
>>>> +	};
>>>>    };
>>>
>>> This will change the struct size even with union, since old version is missing
>>> all fields from protocol header. Struct size will go from 2 --> 5 bytes [1].
>>>
>>> Since this is public struct, we can't change its size.
>>>
>>> @Ori, Andrew,
>>>
>>> Do you have a suggestion for next step?
>>>
> 
> I think Ferruh is right, and I think that we should at this point just update the documentation.
> Sorry for the detour
> Just small comment about the original  patch.
> I don’t think you should change the default mask since it may break existing apps.
> 

I will continue with first patch [1], and will update the protocol reference in 
the commit log.

Meanwhile adding 'struct rte_gtp_psc_hdr' (patch v4 1/2) work can continue 
separately.
And when it is added we can send a deprecation notice to update 'struct 
rte_flow_item_gtp_psc' and finally update it on v21.11 to have 'struct 
rte_gtp_psc_hdr' in it. Makes sense?

[1]
https://patches.dpdk.org/project/dpdk/patch/20210323121134.19113-1-rasland@nvidia.com/

>>> - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation notice
>>> for "struct rte_flow_item_gtp_psc" to say it will use  "struct
>>> rte_gtp_psc_hdr"
>>> on 21.11.
>>>
>>> - And for this release use the Raslan's first version:
>>>     -	uint8_t qfi; /**< QoS flow identifier. */
>>>     +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
>>>
>>>
>>> Does it make sense? Any comments?
>>>
>> @Ori Kam, @andrew.rybchenko@oktetlabs.ru,
>> This is a kind remainder of this patch,
>>
>>>
>>>
>>> [1]
>>> struct rte_gtp_psc_hdr {
>>>           uint8_t                    ext_hdr_len;          /*     0     1 */
>>>           uint8_t                    type:4;               /*     1: 0  1 */
>>>           uint8_t                    qmp:1;                /*     1: 4  1 */
>>>
>>>           /* XXX 3 bits hole, try to pack */
>>>
>>>           union {
>>>                   struct {
>>>                           uint8_t    snp:1;                /*     2: 0  1 */
>>>                           uint8_t    spare_dl1:2;          /*     2: 1  1 */
>>>                   };                                       /*     2     1 */
>>>                   struct {
>>>                           uint8_t    dl_delay_ind:1;       /*     2: 0  1 */
>>>                           uint8_t    ul_delay_ind:1;       /*     2: 1  1 */
>>>                           uint8_t    snp_ul1:1;            /*     2: 2  1 */
>>>                   };                                       /*     2     1 */
>>>           };                                               /*     2     1 */
>>>           union {
>>>                   struct {
>>>                           uint8_t    ppp:1;                /*     3: 0  1 */
>>>                           uint8_t    rqi:1;                /*     3: 1  1 */
>>>                   };                                       /*     3     1 */
>>>                   struct {
>>>                           uint8_t    n_delay_ind:1;        /*     3: 0  1 */
>>>                           uint8_t    spare_ul2:1;          /*     3: 1  1 */
>>>                   };                                       /*     3     1 */
>>>           };                                               /*     3     1 */
>>>           uint8_t                    qfi:6;                /*     4: 0  1 */
>>>
>>>           /* XXX 2 bits hole, try to pack */
>>>
>>>           uint8_t                    data[];               /*     5     0 */
>>>
>>>           /* size: 5, cachelines: 1, members: 7 */
>>>           /* sum members: 3 */
>>>           /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits */
>>>           /* last cacheline: 5 bytes */
>>> };
>>>
>>>>
>>>>    /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
>>>>    #ifndef __cplusplus
>>>>    static const struct rte_flow_item_gtp_psc
>>>>    rte_flow_item_gtp_psc_mask = {
>>>> -	.qfi = 0x3f,
>>>> +	.hdr.ppp = 1,
>>>> +	.hdr.rqi = 1,
>>>> +	.hdr.qfi = 0x3f,
>>>>    };
>>>>    #endif
>>>>
>>>>
> 

Re: [dpdk-dev] [PATCH] ethdev: update qfi definition

[Ferruh Yigit]

On 3/23/2021 12:11 PM, Raslan Darawsheh wrote:
> qfi field is 8 bits which represent single bit for
> PPP (paging Policy Presence) single bit for RQI
> (Reflective QoS Indicator) and 6 bits for qfi
> (QoS Flow Identifier)
> 
> This update the doxygen format and the mask for qfi
> to properly identify the full 8 bits of the field.
> 
> note: changing the default mask would cause different
> patterns generated by testpmd.
> 
> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> Cc: ying.a.wang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.


Sorry for going through multiple versions, hopefully proper solution can be 
merged after deprecation notice on v21.11.

Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition

[Raslan Darawsheh]

Kindest regards,
Raslan Darawsheh

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, April 14, 2021 3:17 PM
> To: Ori Kam <orika@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru
> Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com;
> olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri
> Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand
> <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>
> Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition
> 
> On 4/13/2021 10:24 AM, Ori Kam wrote:
> > Hi Raslan,
> >
> >> -----Original Message-----
> >> From: Raslan Darawsheh <rasland@nvidia.com>
> >> Subject: RE: [PATCH v4 2/2] ethdev: update qfi definition
> >>
> >> Hi,
> >>
> >>> -----Original Message-----
> >>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>> Sent: Tuesday, April 6, 2021 7:10 PM
> >>> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ori Kam
> >>> <orika@nvidia.com>; andrew.rybchenko@oktetlabs.ru
> >>> Cc: ivan.malov@oktetlabs.ru; ying.a.wang@intel.com;
> >>> olivier.matz@6wind.com; Slava Ovsiienko <viacheslavo@nvidia.com>;
> Shiri
> >>> Kuzin <shirik@nvidia.com>; stable@dpdk.org; David Marchand
> >>> <david.marchand@redhat.com>; NBU-Contact-Thomas Monjalon
> >>> <thomas@monjalon.net>
> >>> Subject: Re: [PATCH v4 2/2] ethdev: update qfi definition
> >>>
> >>> On 4/4/2021 8:45 AM, Raslan Darawsheh wrote:
> >>>> qfi field is 8 bits which represent single bit for
> >>>> PPP (paging Policy Presence) single bit for RQI
> >>>> (Reflective QoS Indicator) and 6 bits for qfi
> >>>> (QoS Flow Identifier) based on RFC 38415-g30
> >>>>
> >>>> This update the doxygen format and the mask for qfi
> >>>> to properly identify the full 8 bits of the field.
> >>>>
> >>>> note: changing the default mask would cause different
> >>>> patterns generated by testpmd.
> >>>>
> >>>> Fixes: 346553db5bd1 ("ethdev: add GTP extension header to flow API")
> >>>> Cc: ying.a.wang@intel.com
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> >>>> ---
> >>>>    doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
> >>>>    lib/librte_ethdev/rte_flow.h                | 20 +++++++++++++++++---
> >>>>    2 files changed, 19 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> index f59eb8a27d..dd39c4c3c2 100644
> >>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> @@ -3742,7 +3742,8 @@ This section lists supported pattern items and
> >>> their attributes, if any.
> >>>>    - ``gtp_psc``: match GTP PDU extension header with type 0x85.
> >>>>
> >>>>      - ``pdu_type {unsigned}``: PDU type.
> >>>> -  - ``qfi {unsigned}``: QoS flow identifier.
> >>>> +
> >>>> +  - ``qfi {unsigned}``: PPP, RQI and QoS flow identifier.
> >>>>
> >>>>    - ``pppoes``, ``pppoed``: match PPPoE header.
> >>>>
> >>>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> >>>> index 6cc57136ac..20b4389429 100644
> >>>> --- a/lib/librte_ethdev/rte_flow.h
> >>>> +++ b/lib/librte_ethdev/rte_flow.h
> >>>> @@ -20,6 +20,7 @@
> >>>>    #include <rte_arp.h>
> >>>>    #include <rte_common.h>
> >>>>    #include <rte_ether.h>
> >>>> +#include <rte_gtp.h>
> >>>>    #include <rte_icmp.h>
> >>>>    #include <rte_ip.h>
> >>>>    #include <rte_sctp.h>
> >>>> @@ -1421,16 +1422,29 @@ static const struct rte_flow_item_meta
> >>> rte_flow_item_meta_mask = {
> >>>>     *
> >>>>     * Matches a GTP PDU extension header with type 0x85.
> >>>>     */
> >>>> +RTE_STD_C11
> >>>>    struct rte_flow_item_gtp_psc {
> >>>> -	uint8_t pdu_type; /**< PDU type. */
> >>>> -	uint8_t qfi; /**< QoS flow identifier. */
> >>>> +	union {
> >>>> +		struct {
> >>>> +			/*
> >>>> +			 * These fields are retained for compatibility.
> >>>> +			 * Please switch to the new header field below.
> >>>> +			 */
> >>>> +			uint8_t pdu_type; /**< PDU type. */
> >>>> +			uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> >>>> +
> >>>> +		};
> >>>> +		struct rte_gtp_psc_hdr hdr;
> >>>> +	};
> >>>>    };
> >>>
> >>> This will change the struct size even with union, since old version is
> missing
> >>> all fields from protocol header. Struct size will go from 2 --> 5 bytes [1].
> >>>
> >>> Since this is public struct, we can't change its size.
> >>>
> >>> @Ori, Andrew,
> >>>
> >>> Do you have a suggestion for next step?
> >>>
> >
> > I think Ferruh is right, and I think that we should at this point just update
> the documentation.
> > Sorry for the detour
> > Just small comment about the original  patch.
> > I don't think you should change the default mask since it may break existing
> apps.
> >
> 
> I will continue with first patch [1], and will update the protocol reference in
> the commit log.
> 
> Meanwhile adding 'struct rte_gtp_psc_hdr' (patch v4 1/2) work can continue
> separately.
> And when it is added we can send a deprecation notice to update 'struct
> rte_flow_item_gtp_psc' and finally update it on v21.11 to have 'struct
> rte_gtp_psc_hdr' in it. Makes sense?
> 
Yes, makes sense to me, 
I'll work in the meanwhile on providing the new hdrs definition .

> [1]
> https://patches.dpdk.org/project/dpdk/patch/20210323121134.19113-1-
> rasland@nvidia.com/
> 
> >>> - We can still add the "struct rte_gtp_psc_hdr", and add a deprecation
> notice
> >>> for "struct rte_flow_item_gtp_psc" to say it will use  "struct
> >>> rte_gtp_psc_hdr"
> >>> on 21.11.
> >>>
> >>> - And for this release use the Raslan's first version:
> >>>     -	uint8_t qfi; /**< QoS flow identifier. */
> >>>     +	uint8_t qfi; /**< PPP, RQI, QoS flow identifier. */
> >>>
> >>>
> >>> Does it make sense? Any comments?
> >>>
> >> @Ori Kam, @andrew.rybchenko@oktetlabs.ru,
> >> This is a kind remainder of this patch,
> >>
> >>>
> >>>
> >>> [1]
> >>> struct rte_gtp_psc_hdr {
> >>>           uint8_t                    ext_hdr_len;          /*     0     1 */
> >>>           uint8_t                    type:4;               /*     1: 0  1 */
> >>>           uint8_t                    qmp:1;                /*     1: 4  1 */
> >>>
> >>>           /* XXX 3 bits hole, try to pack */
> >>>
> >>>           union {
> >>>                   struct {
> >>>                           uint8_t    snp:1;                /*     2: 0  1 */
> >>>                           uint8_t    spare_dl1:2;          /*     2: 1  1 */
> >>>                   };                                       /*     2     1 */
> >>>                   struct {
> >>>                           uint8_t    dl_delay_ind:1;       /*     2: 0  1 */
> >>>                           uint8_t    ul_delay_ind:1;       /*     2: 1  1 */
> >>>                           uint8_t    snp_ul1:1;            /*     2: 2  1 */
> >>>                   };                                       /*     2     1 */
> >>>           };                                               /*     2     1 */
> >>>           union {
> >>>                   struct {
> >>>                           uint8_t    ppp:1;                /*     3: 0  1 */
> >>>                           uint8_t    rqi:1;                /*     3: 1  1 */
> >>>                   };                                       /*     3     1 */
> >>>                   struct {
> >>>                           uint8_t    n_delay_ind:1;        /*     3: 0  1 */
> >>>                           uint8_t    spare_ul2:1;          /*     3: 1  1 */
> >>>                   };                                       /*     3     1 */
> >>>           };                                               /*     3     1 */
> >>>           uint8_t                    qfi:6;                /*     4: 0  1 */
> >>>
> >>>           /* XXX 2 bits hole, try to pack */
> >>>
> >>>           uint8_t                    data[];               /*     5     0 */
> >>>
> >>>           /* size: 5, cachelines: 1, members: 7 */
> >>>           /* sum members: 3 */
> >>>           /* sum bitfield members: 11 bits, bit holes: 2, sum bit holes: 5 bits
> */
> >>>           /* last cacheline: 5 bytes */
> >>> };
> >>>
> >>>>
> >>>>    /** Default mask for RTE_FLOW_ITEM_TYPE_GTP_PSC. */
> >>>>    #ifndef __cplusplus
> >>>>    static const struct rte_flow_item_gtp_psc
> >>>>    rte_flow_item_gtp_psc_mask = {
> >>>> -	.qfi = 0x3f,
> >>>> +	.hdr.ppp = 1,
> >>>> +	.hdr.rqi = 1,
> >>>> +	.hdr.qfi = 0x3f,
> >>>>    };
> >>>>    #endif
> >>>>
> >>>>
> >

[dpdk-dev] [PATCH v5 0/1] add new hdr for gtp qfi

[Raslan Darawsheh]

This is introducin a new hdr definition of gtp psc
support to match the RFC 38415-g30

v2: introduce new header definition for gtp psc
    update commit msg for rte flow item change.

v3: fixed typo in comment
    Cc relevant people.

v4: update hdr definition to have hdr suffix.
    update variable name to be hdr in the gtp_psc item.
    update default max to use the new added hdr.

v5: updated the hdr definition after code review.
    dropped the change for rte_flow item psc to avoid ABI breakage
    added hdr definition for type0 and type1 psc's

Raslan Darawsheh (1):
  ethdev: add new ext hdr for gtp psc

 lib/net/rte_gtp.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

-- 
2.25.1

[dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc

[Raslan Darawsheh]

Define new rte header for gtp PDU session container
based on RFC 38415-g30

Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 lib/net/rte_gtp.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/lib/net/rte_gtp.h b/lib/net/rte_gtp.h
index 6a6f9b238d..5a850a26e4 100644
--- a/lib/net/rte_gtp.h
+++ b/lib/net/rte_gtp.h
@@ -61,6 +61,84 @@ struct rte_gtp_hdr_ext_word {
 	uint8_t next_ext;     /**< Next Extension Header Type. */
 }  __rte_packed;
 
+/**
+ * Optional extension for GTP with next_ext set to 0x85
+ * defined based on RFC 38415-g30.
+ */
+__extension__
+struct rte_gtp_psc_generic_hdr {
+	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
+#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+	uint8_t type:4; /**< PDU type */
+	uint8_t qmp:1; /**< Qos Monitoring Packet */
+	uint8_t pad:3; /**< type specfic pad bits */
+	uint8_t spare:2; /**< type specific spare bits */
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+#else
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+	uint8_t spare:2; /**< type specific spare bits */
+	uint8_t pad:3; /**< type specfic pad bits */
+	uint8_t qmp:1; /**< Qos Monitoring Packet */
+	uint8_t type:4; /**< PDU type */
+#endif
+	uint8_t data[0]; /**< variable length data feilds */
+} __rte_packed;
+
+/**
+ * Optional extension for GTP with next_ext set to 0x85
+ * type0 defined based on RFC 38415-g30
+ */
+__extension__
+struct rte_gtp_psc_type0_hdr {
+	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
+#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+	uint8_t type:4; /**< PDU type */
+	uint8_t qmp:1; /**< Qos Monitoring Packet */
+	uint8_t snp:1; /**< Sequence number presence */
+	uint8_t spare_dl1:2; /**< spare down link bits */
+	uint8_t ppp:1; /**< Paging policy presence */
+	uint8_t rqi:1; /**< Reflective Qos Indicator */
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+#else
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+	uint8_t rqi:1; /**< Reflective Qos Indicator */
+	uint8_t ppp:1; /**< Paging policy presence */
+	uint8_t spare_dl1:2; /**< spare down link bits */
+	uint8_t snp:1; /**< Sequence number presence */
+	uint8_t type:4; /**< PDU type */
+#endif
+	uint8_t data[0]; /**< variable length data feilds */
+} __rte_packed;
+
+/**
+ * Optional extension for GTP with next_ext set to 0x85
+ * type1 defined based on RFC 38415-g30
+ */
+__extension__
+struct rte_gtp_psc_type1_hdr {
+	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
+#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+	uint8_t type:4; /**< PDU type */
+	uint8_t qmp:1; /**< Qos Monitoring Packet */
+	uint8_t dl_delay_ind:1; /**< dl delay result presence */
+	uint8_t ul_delay_ind:1; /**< ul delay result presence */
+	uint8_t snp:1; /**< Sequence number presence ul */
+	uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
+	uint8_t spare_ul2:1; /**< spare up link bits */
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+#else
+	uint8_t qfi:6; /**< Qos Flow Identifier */
+	uint8_t spare_ul2:1; /**< spare up link bits */
+	uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
+	uint8_t snp:1; /**< Sequence number presence ul */
+	uint8_t ul_delay_ind:1; /**< ul delay result presence */
+	uint8_t dl_delay_ind:1; /**< dl delay result presence */
+	uint8_t qmp:1; /**< Qos Monitoring Packet */
+	uint8_t type:4; /**< PDU type */
+#endif
+	uint8_t data[0]; /**< variable length data feilds */
+} __rte_packed;
+
 /** GTP header length */
 #define RTE_ETHER_GTP_HLEN \
 	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
-- 
2.25.1

Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc

[Tyler Retzlaff]

On Thu, Apr 08, 2021 at 02:29:56PM +0200, Olivier Matz wrote:
> Hi Raslan,
> 
> > +/**
> > + * Optional extension for GTP with next_ext set to 0x85
> > + * defined based on RFC 38415-g30.
> > + */
> > +__extension__
> > +struct rte_gtp_psc_hdr {
> > +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> > +	uint8_t type:4; /**< PDU type */
> > +	uint8_t qmp:1; /**< Qos Monitoring Packet */

would it be a lot ot ask to have the structure defined using standard C
instead of using compiler specific extensions?

Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc

[Ferruh Yigit]

On 4/29/2021 9:10 AM, Raslan Darawsheh wrote:
> Define new rte header for gtp PDU session container
> based on RFC 38415-g30
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>

This patch has not urgent for this release, postponing it to next release.

Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc

[Andrew Rybchenko]

On 4/29/21 7:29 PM, Tyler Retzlaff wrote:
> On Thu, Apr 08, 2021 at 02:29:56PM +0200, Olivier Matz wrote:
>> Hi Raslan,
>>
>>> +/**
>>> + * Optional extension for GTP with next_ext set to 0x85
>>> + * defined based on RFC 38415-g30.
>>> + */
>>> +__extension__
>>> +struct rte_gtp_psc_hdr {
>>> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
>>> +	uint8_t type:4; /**< PDU type */
>>> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> 
> would it be a lot ot ask to have the structure defined using standard C
> instead of using compiler specific extensions?
> 

It is a valid request, but I'm afraid bit fields are used
in many-many core DPDK libraries. So, change of the approach
or at least direction for a new code should be discussed and
approved by techboard.

Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc

[Andrew Rybchenko]

On 4/29/21 11:10 AM, Raslan Darawsheh wrote:
> Define new rte header for gtp PDU session container
> based on RFC 38415-g30
> 
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>  lib/net/rte_gtp.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/lib/net/rte_gtp.h b/lib/net/rte_gtp.h
> index 6a6f9b238d..5a850a26e4 100644
> --- a/lib/net/rte_gtp.h
> +++ b/lib/net/rte_gtp.h
> @@ -61,6 +61,84 @@ struct rte_gtp_hdr_ext_word {
>  	uint8_t next_ext;     /**< Next Extension Header Type. */
>  }  __rte_packed;
>  
> +/**
> + * Optional extension for GTP with next_ext set to 0x85
> + * defined based on RFC 38415-g30.
> + */
> +__extension__
> +struct rte_gtp_psc_generic_hdr {
> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +	uint8_t type:4; /**< PDU type */
> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> +	uint8_t pad:3; /**< type specfic pad bits */
> +	uint8_t spare:2; /**< type specific spare bits */
> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> +#else
> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> +	uint8_t spare:2; /**< type specific spare bits */
> +	uint8_t pad:3; /**< type specfic pad bits */
> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> +	uint8_t type:4; /**< PDU type */
> +#endif
> +	uint8_t data[0]; /**< variable length data feilds */
> +} __rte_packed;
> +
> +/**
> + * Optional extension for GTP with next_ext set to 0x85
> + * type0 defined based on RFC 38415-g30
> + */
> +__extension__
> +struct rte_gtp_psc_type0_hdr {
> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +	uint8_t type:4; /**< PDU type */
> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> +	uint8_t snp:1; /**< Sequence number presence */
> +	uint8_t spare_dl1:2; /**< spare down link bits */
> +	uint8_t ppp:1; /**< Paging policy presence */
> +	uint8_t rqi:1; /**< Reflective Qos Indicator */
> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> +#else
> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> +	uint8_t rqi:1; /**< Reflective Qos Indicator */
> +	uint8_t ppp:1; /**< Paging policy presence */
> +	uint8_t spare_dl1:2; /**< spare down link bits */
> +	uint8_t snp:1; /**< Sequence number presence */
> +	uint8_t type:4; /**< PDU type */
> +#endif
> +	uint8_t data[0]; /**< variable length data feilds */
> +} __rte_packed;
> +
> +/**
> + * Optional extension for GTP with next_ext set to 0x85
> + * type1 defined based on RFC 38415-g30
> + */
> +__extension__
> +struct rte_gtp_psc_type1_hdr {
> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +	uint8_t type:4; /**< PDU type */
> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> +	uint8_t dl_delay_ind:1; /**< dl delay result presence */
> +	uint8_t ul_delay_ind:1; /**< ul delay result presence */
> +	uint8_t snp:1; /**< Sequence number presence ul */
> +	uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> +	uint8_t spare_ul2:1; /**< spare up link bits */
> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> +#else
> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> +	uint8_t spare_ul2:1; /**< spare up link bits */
> +	uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> +	uint8_t snp:1; /**< Sequence number presence ul */
> +	uint8_t ul_delay_ind:1; /**< ul delay result presence */
> +	uint8_t dl_delay_ind:1; /**< dl delay result presence */
> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> +	uint8_t type:4; /**< PDU type */
> +#endif
> +	uint8_t data[0]; /**< variable length data feilds */
> +} __rte_packed;
> +
>  /** GTP header length */
>  #define RTE_ETHER_GTP_HLEN \
>  	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
> 

This way the structure is very hard to read.
May I ask to indent field comments as it is done in
other rte_net structures (e.g. rte_ipv4_hdr, rte_udp_hdr,
rte_geneve_hdr). Personally I don't care if it will be
indent by TABs or spaces, however, it looks like TABs are
used in more places.

Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc

[Andrew Rybchenko]

On 6/8/21 3:17 PM, Andrew Rybchenko wrote:
> On 4/29/21 11:10 AM, Raslan Darawsheh wrote:
>> Define new rte header for gtp PDU session container
>> based on RFC 38415-g30
>>
>> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
>> ---
>>  lib/net/rte_gtp.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 78 insertions(+)
>>
>> diff --git a/lib/net/rte_gtp.h b/lib/net/rte_gtp.h
>> index 6a6f9b238d..5a850a26e4 100644
>> --- a/lib/net/rte_gtp.h
>> +++ b/lib/net/rte_gtp.h
>> @@ -61,6 +61,84 @@ struct rte_gtp_hdr_ext_word {
>>  	uint8_t next_ext;     /**< Next Extension Header Type. */
>>  }  __rte_packed;
>>  
>> +/**
>> + * Optional extension for GTP with next_ext set to 0x85
>> + * defined based on RFC 38415-g30.
>> + */
>> +__extension__
>> +struct rte_gtp_psc_generic_hdr {
>> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>> +	uint8_t type:4; /**< PDU type */
>> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
>> +	uint8_t pad:3; /**< type specfic pad bits */
>> +	uint8_t spare:2; /**< type specific spare bits */
>> +	uint8_t qfi:6; /**< Qos Flow Identifier */
>> +#else
>> +	uint8_t qfi:6; /**< Qos Flow Identifier */
>> +	uint8_t spare:2; /**< type specific spare bits */
>> +	uint8_t pad:3; /**< type specfic pad bits */
>> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
>> +	uint8_t type:4; /**< PDU type */
>> +#endif
>> +	uint8_t data[0]; /**< variable length data feilds */
>> +} __rte_packed;
>> +
>> +/**
>> + * Optional extension for GTP with next_ext set to 0x85
>> + * type0 defined based on RFC 38415-g30
>> + */
>> +__extension__
>> +struct rte_gtp_psc_type0_hdr {
>> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>> +	uint8_t type:4; /**< PDU type */
>> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
>> +	uint8_t snp:1; /**< Sequence number presence */
>> +	uint8_t spare_dl1:2; /**< spare down link bits */
>> +	uint8_t ppp:1; /**< Paging policy presence */
>> +	uint8_t rqi:1; /**< Reflective Qos Indicator */
>> +	uint8_t qfi:6; /**< Qos Flow Identifier */
>> +#else
>> +	uint8_t qfi:6; /**< Qos Flow Identifier */
>> +	uint8_t rqi:1; /**< Reflective Qos Indicator */
>> +	uint8_t ppp:1; /**< Paging policy presence */
>> +	uint8_t spare_dl1:2; /**< spare down link bits */
>> +	uint8_t snp:1; /**< Sequence number presence */
>> +	uint8_t type:4; /**< PDU type */
>> +#endif
>> +	uint8_t data[0]; /**< variable length data feilds */
>> +} __rte_packed;
>> +
>> +/**
>> + * Optional extension for GTP with next_ext set to 0x85
>> + * type1 defined based on RFC 38415-g30
>> + */
>> +__extension__
>> +struct rte_gtp_psc_type1_hdr {
>> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes */
>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>> +	uint8_t type:4; /**< PDU type */
>> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
>> +	uint8_t dl_delay_ind:1; /**< dl delay result presence */
>> +	uint8_t ul_delay_ind:1; /**< ul delay result presence */
>> +	uint8_t snp:1; /**< Sequence number presence ul */
>> +	uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
>> +	uint8_t spare_ul2:1; /**< spare up link bits */
>> +	uint8_t qfi:6; /**< Qos Flow Identifier */
>> +#else
>> +	uint8_t qfi:6; /**< Qos Flow Identifier */
>> +	uint8_t spare_ul2:1; /**< spare up link bits */
>> +	uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
>> +	uint8_t snp:1; /**< Sequence number presence ul */
>> +	uint8_t ul_delay_ind:1; /**< ul delay result presence */
>> +	uint8_t dl_delay_ind:1; /**< dl delay result presence */
>> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
>> +	uint8_t type:4; /**< PDU type */
>> +#endif
>> +	uint8_t data[0]; /**< variable length data feilds */
>> +} __rte_packed;
>> +
>>  /** GTP header length */
>>  #define RTE_ETHER_GTP_HLEN \
>>  	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
>>
> 
> This way the structure is very hard to read.
> May I ask to indent field comments as it is done in
> other rte_net structures (e.g. rte_ipv4_hdr, rte_udp_hdr,
> rte_geneve_hdr). Personally I don't care if it will be
> indent by TABs or spaces, however, it looks like TABs are
> used in more places.
> 

Also there are spelling bugs [1].

[1] http://mails.dpdk.org/archives/test-report/2021-April/191649.html

Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc

[Raslan Darawsheh]

Hi Andrew,

Thanks for your comments and review, I'll attempt to handle and fix in the next version.

Kindest regards,
Raslan Darawsheh

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, June 8, 2021 3:19 PM
> To: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org
> Cc: ferruh.yigit@intel.com; Ori Kam <orika@nvidia.com>;
> ivan.malov@oktetlabs.ru; ying.a.wang@intel.com; olivier.matz@6wind.com;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Shiri Kuzin <shirik@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc
> 
> On 6/8/21 3:17 PM, Andrew Rybchenko wrote:
> > On 4/29/21 11:10 AM, Raslan Darawsheh wrote:
> >> Define new rte header for gtp PDU session container based on RFC
> >> 38415-g30
> >>
> >> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> >> ---
> >>  lib/net/rte_gtp.h | 78
> >> +++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 78 insertions(+)
> >>
> >> diff --git a/lib/net/rte_gtp.h b/lib/net/rte_gtp.h index
> >> 6a6f9b238d..5a850a26e4 100644
> >> --- a/lib/net/rte_gtp.h
> >> +++ b/lib/net/rte_gtp.h
> >> @@ -61,6 +61,84 @@ struct rte_gtp_hdr_ext_word {
> >>  	uint8_t next_ext;     /**< Next Extension Header Type. */
> >>  }  __rte_packed;
> >>
> >> +/**
> >> + * Optional extension for GTP with next_ext set to 0x85
> >> + * defined based on RFC 38415-g30.
> >> + */
> >> +__extension__
> >> +struct rte_gtp_psc_generic_hdr {
> >> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes
> >> +*/ #if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >> +	uint8_t type:4; /**< PDU type */
> >> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> +	uint8_t pad:3; /**< type specfic pad bits */
> >> +	uint8_t spare:2; /**< type specific spare bits */
> >> +	uint8_t qfi:6; /**< Qos Flow Identifier */ #else
> >> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> >> +	uint8_t spare:2; /**< type specific spare bits */
> >> +	uint8_t pad:3; /**< type specfic pad bits */
> >> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> +	uint8_t type:4; /**< PDU type */
> >> +#endif
> >> +	uint8_t data[0]; /**< variable length data feilds */ }
> >> +__rte_packed;
> >> +
> >> +/**
> >> + * Optional extension for GTP with next_ext set to 0x85
> >> + * type0 defined based on RFC 38415-g30  */ __extension__ struct
> >> +rte_gtp_psc_type0_hdr {
> >> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes
> >> +*/ #if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >> +	uint8_t type:4; /**< PDU type */
> >> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> +	uint8_t snp:1; /**< Sequence number presence */
> >> +	uint8_t spare_dl1:2; /**< spare down link bits */
> >> +	uint8_t ppp:1; /**< Paging policy presence */
> >> +	uint8_t rqi:1; /**< Reflective Qos Indicator */
> >> +	uint8_t qfi:6; /**< Qos Flow Identifier */ #else
> >> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> >> +	uint8_t rqi:1; /**< Reflective Qos Indicator */
> >> +	uint8_t ppp:1; /**< Paging policy presence */
> >> +	uint8_t spare_dl1:2; /**< spare down link bits */
> >> +	uint8_t snp:1; /**< Sequence number presence */
> >> +	uint8_t type:4; /**< PDU type */
> >> +#endif
> >> +	uint8_t data[0]; /**< variable length data feilds */ }
> >> +__rte_packed;
> >> +
> >> +/**
> >> + * Optional extension for GTP with next_ext set to 0x85
> >> + * type1 defined based on RFC 38415-g30  */ __extension__ struct
> >> +rte_gtp_psc_type1_hdr {
> >> +	uint8_t ext_hdr_len; /**< PDU ext hdr len in multiples of 4 bytes
> >> +*/ #if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >> +	uint8_t type:4; /**< PDU type */
> >> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> +	uint8_t dl_delay_ind:1; /**< dl delay result presence */
> >> +	uint8_t ul_delay_ind:1; /**< ul delay result presence */
> >> +	uint8_t snp:1; /**< Sequence number presence ul */
> >> +	uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> >> +	uint8_t spare_ul2:1; /**< spare up link bits */
> >> +	uint8_t qfi:6; /**< Qos Flow Identifier */ #else
> >> +	uint8_t qfi:6; /**< Qos Flow Identifier */
> >> +	uint8_t spare_ul2:1; /**< spare up link bits */
> >> +	uint8_t n_delay_ind:1; /**< N3/N9 delay result presence */
> >> +	uint8_t snp:1; /**< Sequence number presence ul */
> >> +	uint8_t ul_delay_ind:1; /**< ul delay result presence */
> >> +	uint8_t dl_delay_ind:1; /**< dl delay result presence */
> >> +	uint8_t qmp:1; /**< Qos Monitoring Packet */
> >> +	uint8_t type:4; /**< PDU type */
> >> +#endif
> >> +	uint8_t data[0]; /**< variable length data feilds */ }
> >> +__rte_packed;
> >> +
> >>  /** GTP header length */
> >>  #define RTE_ETHER_GTP_HLEN \
> >>  	(sizeof(struct rte_udp_hdr) + sizeof(struct rte_gtp_hdr))
> >>
> >
> > This way the structure is very hard to read.
> > May I ask to indent field comments as it is done in other rte_net
> > structures (e.g. rte_ipv4_hdr, rte_udp_hdr, rte_geneve_hdr).
> > Personally I don't care if it will be indent by TABs or spaces,
> > however, it looks like TABs are used in more places.
> >
> 
> Also there are spelling bugs [1].
> 
> [1] http://mails.dpdk.org/archives/test-report/2021-April/191649.html