DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ethdev: update qfi definition
@ 2021-03-23 12:11 Raslan Darawsheh
  2021-03-26 13:12 ` Ferruh Yigit
                   ` (2 more replies)
  0 siblings, 3 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-03-23 12:11 UTC (permalink / raw)
  To: dev; +Cc: viacheslavo, shirik, ying.a.wang, stable

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: update qfi definition
  2021-03-23 12:11 [dpdk-dev] [PATCH] ethdev: update qfi definition Raslan Darawsheh
@ 2021-03-26 13:12 ` Ferruh Yigit
  2021-03-29  8:53   ` Ori Kam
  2021-03-30  7:50 ` [dpdk-dev] [PATCH v2 0/2] fix gtp psc qfi support Raslan Darawsheh
  2021-04-14 12:40 ` [dpdk-dev] [PATCH] " Ferruh Yigit
  2 siblings, 1 reply; 37+ messages in thread
From: Ferruh Yigit @ 2021-03-26 13:12 UTC (permalink / raw)
  To: Raslan Darawsheh, dev, Ori Kam, Andrew Rybchenko, Ivan Malov
  Cc: viacheslavo, shirik, ying.a.wang, stable, Thomas Monjalon, Olivier Matz

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: update qfi definition
  2021-03-26 13:12 ` Ferruh Yigit
@ 2021-03-29  8:53   ` Ori Kam
  2021-03-29  9:06     ` Raslan Darawsheh
  0 siblings, 1 reply; 37+ messages in thread
From: Ori Kam @ 2021-03-29  8:53 UTC (permalink / raw)
  To: Ferruh Yigit, Raslan Darawsheh, dev, Andrew Rybchenko, Ivan Malov
  Cc: Slava Ovsiienko, Shiri Kuzin, ying.a.wang, stable,
	NBU-Contact-Thomas Monjalon, Olivier Matz

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: update qfi definition
  2021-03-29  8:53   ` Ori Kam
@ 2021-03-29  9:06     ` Raslan Darawsheh
  2021-03-29 11:14       ` Ferruh Yigit
  0 siblings, 1 reply; 37+ messages in thread
From: Raslan Darawsheh @ 2021-03-29  9:06 UTC (permalink / raw)
  To: Ori Kam, Ferruh Yigit, dev, Andrew Rybchenko, Ivan Malov
  Cc: Slava Ovsiienko, Shiri Kuzin, ying.a.wang, stable,
	NBU-Contact-Thomas Monjalon, Olivier Matz

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: update qfi definition
  2021-03-29  9:06     ` Raslan Darawsheh
@ 2021-03-29 11:14       ` Ferruh Yigit
  0 siblings, 0 replies; 37+ messages in thread
From: Ferruh Yigit @ 2021-03-29 11:14 UTC (permalink / raw)
  To: Raslan Darawsheh, Ori Kam, dev, Andrew Rybchenko, Ivan Malov
  Cc: Slava Ovsiienko, Shiri Kuzin, ying.a.wang, stable,
	NBU-Contact-Thomas Monjalon, Olivier Matz

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
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v2 0/2] fix gtp psc qfi support
  2021-03-23 12:11 [dpdk-dev] [PATCH] ethdev: update qfi definition Raslan Darawsheh
  2021-03-26 13:12 ` Ferruh Yigit
@ 2021-03-30  7:50 ` Raslan Darawsheh
  2021-03-30  7:50   ` [dpdk-dev] [PATCH v2 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
  2021-03-30  7:50   ` [dpdk-dev] [PATCH v2 2/2] ethdev: update qfi definition Raslan Darawsheh
  2021-04-14 12:40 ` [dpdk-dev] [PATCH] " Ferruh Yigit
  2 siblings, 2 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-03-30  7:50 UTC (permalink / raw)
  To: dev; +Cc: viacheslavo, shirik

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v2 1/2] ethdev: add new ext hdr for gtp psc
  2021-03-30  7:50 ` [dpdk-dev] [PATCH v2 0/2] fix gtp psc qfi support Raslan Darawsheh
@ 2021-03-30  7:50   ` Raslan Darawsheh
  2021-03-30  8:00     ` [dpdk-dev] [PATCH v3 0/2] fix gtp psc qfi support Raslan Darawsheh
  2021-04-04  7:45     ` [dpdk-dev] [PATCH v4 0/2] fix gtp psc qfi support Raslan Darawsheh
  2021-03-30  7:50   ` [dpdk-dev] [PATCH v2 2/2] ethdev: update qfi definition Raslan Darawsheh
  1 sibling, 2 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-03-30  7:50 UTC (permalink / raw)
  To: dev; +Cc: viacheslavo, shirik

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v2 2/2] ethdev: update qfi definition
  2021-03-30  7:50 ` [dpdk-dev] [PATCH v2 0/2] fix gtp psc qfi support Raslan Darawsheh
  2021-03-30  7:50   ` [dpdk-dev] [PATCH v2 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
@ 2021-03-30  7:50   ` Raslan Darawsheh
  1 sibling, 0 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-03-30  7:50 UTC (permalink / raw)
  To: dev; +Cc: viacheslavo, shirik, ying.a.wang, stable

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v3 0/2] fix gtp psc qfi support
  2021-03-30  7:50   ` [dpdk-dev] [PATCH v2 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
@ 2021-03-30  8:00     ` Raslan Darawsheh
  2021-03-30  8:00       ` [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
  2021-03-30  8:00       ` [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition Raslan Darawsheh
  2021-04-04  7:45     ` [dpdk-dev] [PATCH v4 0/2] fix gtp psc qfi support Raslan Darawsheh
  1 sibling, 2 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-03-30  8:00 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, orika, andrew.rybchenko, ivan.malov, ying.a.wang,
	olivier.matz, viacheslavo, shirik

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc
  2021-03-30  8:00     ` [dpdk-dev] [PATCH v3 0/2] fix gtp psc qfi support Raslan Darawsheh
@ 2021-03-30  8:00       ` Raslan Darawsheh
  2021-04-01 16:51         ` Ferruh Yigit
  2021-03-30  8:00       ` [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition Raslan Darawsheh
  1 sibling, 1 reply; 37+ messages in thread
From: Raslan Darawsheh @ 2021-03-30  8:00 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, orika, andrew.rybchenko, ivan.malov, ying.a.wang,
	olivier.matz, viacheslavo, shirik

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition
  2021-03-30  8:00     ` [dpdk-dev] [PATCH v3 0/2] fix gtp psc qfi support Raslan Darawsheh
  2021-03-30  8:00       ` [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
@ 2021-03-30  8:00       ` Raslan Darawsheh
  2021-04-01 16:54         ` Ferruh Yigit
  1 sibling, 1 reply; 37+ messages in thread
From: Raslan Darawsheh @ 2021-03-30  8:00 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, orika, andrew.rybchenko, ivan.malov, ying.a.wang,
	olivier.matz, viacheslavo, shirik, stable

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc
  2021-03-30  8:00       ` [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
@ 2021-04-01 16:51         ` Ferruh Yigit
  2021-04-04  7:17           ` Raslan Darawsheh
  0 siblings, 1 reply; 37+ messages in thread
From: Ferruh Yigit @ 2021-04-01 16:51 UTC (permalink / raw)
  To: Raslan Darawsheh, dev
  Cc: orika, andrew.rybchenko, ivan.malov, ying.a.wang, olivier.matz,
	viacheslavo, shirik

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))
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition
  2021-03-30  8:00       ` [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition Raslan Darawsheh
@ 2021-04-01 16:54         ` Ferruh Yigit
  2021-04-04  7:18           ` Raslan Darawsheh
  0 siblings, 1 reply; 37+ messages in thread
From: Ferruh Yigit @ 2021-04-01 16:54 UTC (permalink / raw)
  To: Raslan Darawsheh, dev
  Cc: orika, andrew.rybchenko, ivan.malov, ying.a.wang, olivier.matz,
	viacheslavo, shirik, stable

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
>   
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-01 16:51         ` Ferruh Yigit
@ 2021-04-04  7:17           ` Raslan Darawsheh
  0 siblings, 0 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-04  7:17 UTC (permalink / raw)
  To: Ferruh Yigit, dev
  Cc: Ori Kam, andrew.rybchenko, ivan.malov, ying.a.wang, olivier.matz,
	Slava Ovsiienko, Shiri Kuzin


> -----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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition
  2021-04-01 16:54         ` Ferruh Yigit
@ 2021-04-04  7:18           ` Raslan Darawsheh
  0 siblings, 0 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-04  7:18 UTC (permalink / raw)
  To: Ferruh Yigit, dev
  Cc: Ori Kam, andrew.rybchenko, ivan.malov, ying.a.wang, olivier.matz,
	Slava Ovsiienko, Shiri Kuzin, stable

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
> >
> >


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v4 0/2] fix gtp psc qfi support
  2021-03-30  7:50   ` [dpdk-dev] [PATCH v2 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
  2021-03-30  8:00     ` [dpdk-dev] [PATCH v3 0/2] fix gtp psc qfi support Raslan Darawsheh
@ 2021-04-04  7:45     ` Raslan Darawsheh
  2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
                         ` (2 more replies)
  1 sibling, 3 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-04  7:45 UTC (permalink / raw)
  To: dev, ferruh.yigit
  Cc: orika, andrew.rybchenko, ivan.malov, ying.a.wang, olivier.matz,
	viacheslavo, shirik

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-04  7:45     ` [dpdk-dev] [PATCH v4 0/2] fix gtp psc qfi support Raslan Darawsheh
@ 2021-04-04  7:45       ` Raslan Darawsheh
  2021-04-08 12:29         ` Olivier Matz
  2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition Raslan Darawsheh
  2021-04-29  8:10       ` [dpdk-dev] [PATCH v5 0/1] add new hdr for gtp qfi Raslan Darawsheh
  2 siblings, 1 reply; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-04  7:45 UTC (permalink / raw)
  To: dev, ferruh.yigit
  Cc: orika, andrew.rybchenko, ivan.malov, ying.a.wang, olivier.matz,
	viacheslavo, shirik

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition
  2021-04-04  7:45     ` [dpdk-dev] [PATCH v4 0/2] fix gtp psc qfi support Raslan Darawsheh
  2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
@ 2021-04-04  7:45       ` Raslan Darawsheh
  2021-04-06 16:09         ` Ferruh Yigit
  2021-04-29  8:10       ` [dpdk-dev] [PATCH v5 0/1] add new hdr for gtp qfi Raslan Darawsheh
  2 siblings, 1 reply; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-04  7:45 UTC (permalink / raw)
  To: dev, ferruh.yigit
  Cc: orika, andrew.rybchenko, ivan.malov, ying.a.wang, olivier.matz,
	viacheslavo, shirik, stable

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition
  2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition Raslan Darawsheh
@ 2021-04-06 16:09         ` Ferruh Yigit
  2021-04-13  8:14           ` Raslan Darawsheh
  0 siblings, 1 reply; 37+ messages in thread
From: Ferruh Yigit @ 2021-04-06 16:09 UTC (permalink / raw)
  To: Raslan Darawsheh, dev, orika, andrew.rybchenko
  Cc: ivan.malov, ying.a.wang, olivier.matz, viacheslavo, shirik,
	stable, David Marchand, Thomas Monjalon

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
>   
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
@ 2021-04-08 12:29         ` Olivier Matz
  2021-04-08 12:37           ` Raslan Darawsheh
  2021-04-29 16:29           ` Tyler Retzlaff
  0 siblings, 2 replies; 37+ messages in thread
From: Olivier Matz @ 2021-04-08 12:29 UTC (permalink / raw)
  To: Raslan Darawsheh
  Cc: dev, ferruh.yigit, orika, andrew.rybchenko, ivan.malov,
	ying.a.wang, viacheslavo, shirik

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-08 12:29         ` Olivier Matz
@ 2021-04-08 12:37           ` Raslan Darawsheh
  2021-04-08 14:10             ` Ferruh Yigit
  2021-04-08 14:10             ` Olivier Matz
  2021-04-29 16:29           ` Tyler Retzlaff
  1 sibling, 2 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-08 12:37 UTC (permalink / raw)
  To: Olivier Matz
  Cc: dev, ferruh.yigit, Ori Kam, andrew.rybchenko, ivan.malov,
	ying.a.wang, Slava Ovsiienko, Shiri Kuzin

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-08 12:37           ` Raslan Darawsheh
@ 2021-04-08 14:10             ` Ferruh Yigit
  2021-04-08 14:10             ` Olivier Matz
  1 sibling, 0 replies; 37+ messages in thread
From: Ferruh Yigit @ 2021-04-08 14:10 UTC (permalink / raw)
  To: Raslan Darawsheh, Olivier Matz
  Cc: dev, Ori Kam, andrew.rybchenko, ivan.malov, ying.a.wang,
	Slava Ovsiienko, Shiri Kuzin

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
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-08 12:37           ` Raslan Darawsheh
  2021-04-08 14:10             ` Ferruh Yigit
@ 2021-04-08 14:10             ` Olivier Matz
  2021-04-13  7:45               ` Raslan Darawsheh
  1 sibling, 1 reply; 37+ messages in thread
From: Olivier Matz @ 2021-04-08 14:10 UTC (permalink / raw)
  To: Raslan Darawsheh
  Cc: dev, ferruh.yigit, Ori Kam, andrew.rybchenko, ivan.malov,
	ying.a.wang, Slava Ovsiienko, Shiri Kuzin

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-08 14:10             ` Olivier Matz
@ 2021-04-13  7:45               ` Raslan Darawsheh
  0 siblings, 0 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-13  7:45 UTC (permalink / raw)
  To: Olivier Matz
  Cc: dev, ferruh.yigit, Ori Kam, andrew.rybchenko, ivan.malov,
	ying.a.wang, Slava Ovsiienko, Shiri Kuzin

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition
  2021-04-06 16:09         ` Ferruh Yigit
@ 2021-04-13  8:14           ` Raslan Darawsheh
  2021-04-13  9:24             ` Ori Kam
  0 siblings, 1 reply; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-13  8:14 UTC (permalink / raw)
  To: Ferruh Yigit, dev, Ori Kam, andrew.rybchenko
  Cc: ivan.malov, ying.a.wang, olivier.matz, Slava Ovsiienko,
	Shiri Kuzin, stable, David Marchand, NBU-Contact-Thomas Monjalon

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
> >
> >


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition
  2021-04-13  8:14           ` Raslan Darawsheh
@ 2021-04-13  9:24             ` Ori Kam
  2021-04-14 12:16               ` Ferruh Yigit
  0 siblings, 1 reply; 37+ messages in thread
From: Ori Kam @ 2021-04-13  9:24 UTC (permalink / raw)
  To: Raslan Darawsheh, Ferruh Yigit, dev, andrew.rybchenko
  Cc: ivan.malov, ying.a.wang, olivier.matz, Slava Ovsiienko,
	Shiri Kuzin, stable, David Marchand, NBU-Contact-Thomas Monjalon

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
> > >
> > >


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition
  2021-04-13  9:24             ` Ori Kam
@ 2021-04-14 12:16               ` Ferruh Yigit
  2021-04-15  6:33                 ` Raslan Darawsheh
  0 siblings, 1 reply; 37+ messages in thread
From: Ferruh Yigit @ 2021-04-14 12:16 UTC (permalink / raw)
  To: Ori Kam, Raslan Darawsheh, dev, andrew.rybchenko
  Cc: ivan.malov, ying.a.wang, olivier.matz, Slava Ovsiienko,
	Shiri Kuzin, stable, David Marchand, NBU-Contact-Thomas Monjalon

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
>>>>
>>>>
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: update qfi definition
  2021-03-23 12:11 [dpdk-dev] [PATCH] ethdev: update qfi definition Raslan Darawsheh
  2021-03-26 13:12 ` Ferruh Yigit
  2021-03-30  7:50 ` [dpdk-dev] [PATCH v2 0/2] fix gtp psc qfi support Raslan Darawsheh
@ 2021-04-14 12:40 ` Ferruh Yigit
  2 siblings, 0 replies; 37+ messages in thread
From: Ferruh Yigit @ 2021-04-14 12:40 UTC (permalink / raw)
  To: Raslan Darawsheh, dev; +Cc: viacheslavo, shirik, ying.a.wang, stable

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.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition
  2021-04-14 12:16               ` Ferruh Yigit
@ 2021-04-15  6:33                 ` Raslan Darawsheh
  0 siblings, 0 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-15  6:33 UTC (permalink / raw)
  To: Ferruh Yigit, Ori Kam, dev, andrew.rybchenko
  Cc: ivan.malov, ying.a.wang, olivier.matz, Slava Ovsiienko,
	Shiri Kuzin, stable, David Marchand, NBU-Contact-Thomas Monjalon



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
> >>>>
> >>>>
> >


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v5 0/1] add new hdr for gtp qfi
  2021-04-04  7:45     ` [dpdk-dev] [PATCH v4 0/2] fix gtp psc qfi support Raslan Darawsheh
  2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
  2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition Raslan Darawsheh
@ 2021-04-29  8:10       ` Raslan Darawsheh
  2021-04-29  8:10         ` [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
  2 siblings, 1 reply; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-29  8:10 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, orika, andrew.rybchenko, ivan.malov, ying.a.wang,
	olivier.matz, viacheslavo, shirik

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc
  2021-04-29  8:10       ` [dpdk-dev] [PATCH v5 0/1] add new hdr for gtp qfi Raslan Darawsheh
@ 2021-04-29  8:10         ` Raslan Darawsheh
  2021-05-11 11:46           ` Ferruh Yigit
  2021-06-08 12:17           ` Andrew Rybchenko
  0 siblings, 2 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-04-29  8:10 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, orika, andrew.rybchenko, ivan.malov, ying.a.wang,
	olivier.matz, viacheslavo, shirik

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-08 12:29         ` Olivier Matz
  2021-04-08 12:37           ` Raslan Darawsheh
@ 2021-04-29 16:29           ` Tyler Retzlaff
  2021-06-08 12:13             ` Andrew Rybchenko
  1 sibling, 1 reply; 37+ messages in thread
From: Tyler Retzlaff @ 2021-04-29 16:29 UTC (permalink / raw)
  To: Olivier Matz
  Cc: Raslan Darawsheh, dev, ferruh.yigit, orika, andrew.rybchenko,
	ivan.malov, ying.a.wang, viacheslavo, shirik

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?


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc
  2021-04-29  8:10         ` [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
@ 2021-05-11 11:46           ` Ferruh Yigit
  2021-06-08 12:17           ` Andrew Rybchenko
  1 sibling, 0 replies; 37+ messages in thread
From: Ferruh Yigit @ 2021-05-11 11:46 UTC (permalink / raw)
  To: Raslan Darawsheh, dev
  Cc: orika, andrew.rybchenko, ivan.malov, ying.a.wang, olivier.matz,
	viacheslavo, shirik

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.


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc
  2021-04-29 16:29           ` Tyler Retzlaff
@ 2021-06-08 12:13             ` Andrew Rybchenko
  0 siblings, 0 replies; 37+ messages in thread
From: Andrew Rybchenko @ 2021-06-08 12:13 UTC (permalink / raw)
  To: Tyler Retzlaff, Olivier Matz
  Cc: Raslan Darawsheh, dev, ferruh.yigit, orika, ivan.malov,
	ying.a.wang, viacheslavo, shirik

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.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc
  2021-04-29  8:10         ` [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
  2021-05-11 11:46           ` Ferruh Yigit
@ 2021-06-08 12:17           ` Andrew Rybchenko
  2021-06-08 12:18             ` Andrew Rybchenko
  1 sibling, 1 reply; 37+ messages in thread
From: Andrew Rybchenko @ 2021-06-08 12:17 UTC (permalink / raw)
  To: Raslan Darawsheh, dev
  Cc: ferruh.yigit, orika, ivan.malov, ying.a.wang, olivier.matz,
	viacheslavo, shirik

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.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc
  2021-06-08 12:17           ` Andrew Rybchenko
@ 2021-06-08 12:18             ` Andrew Rybchenko
  2021-06-08 14:07               ` Raslan Darawsheh
  0 siblings, 1 reply; 37+ messages in thread
From: Andrew Rybchenko @ 2021-06-08 12:18 UTC (permalink / raw)
  To: Raslan Darawsheh, dev
  Cc: ferruh.yigit, orika, ivan.malov, ying.a.wang, olivier.matz,
	viacheslavo, shirik

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc
  2021-06-08 12:18             ` Andrew Rybchenko
@ 2021-06-08 14:07               ` Raslan Darawsheh
  0 siblings, 0 replies; 37+ messages in thread
From: Raslan Darawsheh @ 2021-06-08 14:07 UTC (permalink / raw)
  To: Andrew Rybchenko, dev
  Cc: ferruh.yigit, Ori Kam, ivan.malov, ying.a.wang, olivier.matz,
	Slava Ovsiienko, Shiri Kuzin

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, other threads:[~2021-06-08 14:07 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 12:11 [dpdk-dev] [PATCH] ethdev: update qfi definition Raslan Darawsheh
2021-03-26 13:12 ` Ferruh Yigit
2021-03-29  8:53   ` Ori Kam
2021-03-29  9:06     ` Raslan Darawsheh
2021-03-29 11:14       ` Ferruh Yigit
2021-03-30  7:50 ` [dpdk-dev] [PATCH v2 0/2] fix gtp psc qfi support Raslan Darawsheh
2021-03-30  7:50   ` [dpdk-dev] [PATCH v2 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
2021-03-30  8:00     ` [dpdk-dev] [PATCH v3 0/2] fix gtp psc qfi support Raslan Darawsheh
2021-03-30  8:00       ` [dpdk-dev] [PATCH v3 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
2021-04-01 16:51         ` Ferruh Yigit
2021-04-04  7:17           ` Raslan Darawsheh
2021-03-30  8:00       ` [dpdk-dev] [PATCH v3 2/2] ethdev: update qfi definition Raslan Darawsheh
2021-04-01 16:54         ` Ferruh Yigit
2021-04-04  7:18           ` Raslan Darawsheh
2021-04-04  7:45     ` [dpdk-dev] [PATCH v4 0/2] fix gtp psc qfi support Raslan Darawsheh
2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 1/2] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
2021-04-08 12:29         ` Olivier Matz
2021-04-08 12:37           ` Raslan Darawsheh
2021-04-08 14:10             ` Ferruh Yigit
2021-04-08 14:10             ` Olivier Matz
2021-04-13  7:45               ` Raslan Darawsheh
2021-04-29 16:29           ` Tyler Retzlaff
2021-06-08 12:13             ` Andrew Rybchenko
2021-04-04  7:45       ` [dpdk-dev] [PATCH v4 2/2] ethdev: update qfi definition Raslan Darawsheh
2021-04-06 16:09         ` Ferruh Yigit
2021-04-13  8:14           ` Raslan Darawsheh
2021-04-13  9:24             ` Ori Kam
2021-04-14 12:16               ` Ferruh Yigit
2021-04-15  6:33                 ` Raslan Darawsheh
2021-04-29  8:10       ` [dpdk-dev] [PATCH v5 0/1] add new hdr for gtp qfi Raslan Darawsheh
2021-04-29  8:10         ` [dpdk-dev] [PATCH v5 1/1] ethdev: add new ext hdr for gtp psc Raslan Darawsheh
2021-05-11 11:46           ` Ferruh Yigit
2021-06-08 12:17           ` Andrew Rybchenko
2021-06-08 12:18             ` Andrew Rybchenko
2021-06-08 14:07               ` Raslan Darawsheh
2021-03-30  7:50   ` [dpdk-dev] [PATCH v2 2/2] ethdev: update qfi definition Raslan Darawsheh
2021-04-14 12:40 ` [dpdk-dev] [PATCH] " Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git