[dpdk-dev] [PATCH] doc: add GRO API requirement in prog_guide

[dpdk-dev] [PATCH] doc: add GRO API requirement in prog_guide

[Jiayu Hu]

The GRO library requires that applications set correct values for
MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len/pkt_len/data_len.

This patch adds the requirement in prog_guide.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/generic_receive_offload_lib.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst b/doc/guides/prog_guide/generic_receive_offload_lib.rst
index 9c6a4d0..7e6944d 100644
--- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
+++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
@@ -34,6 +34,12 @@ Currently, the GRO library provides GRO supports for TCP/IPv4 packets and
 VxLAN packets which contain an outer IPv4 header and an inner TCP/IPv4
 packet.
 
+.. note::
+        To use the GRO library, applications must guarantee that
+        MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len and
+        MBUF->pkt_len/data_len of input packets are set to correct
+        values.
+
 Two Sets of API
 ---------------
 
-- 
2.7.4

[dpdk-dev] [PATCH v2] doc: add GRO API limitations in prog_guide

[Jiayu Hu]

This patch adds GRO API limitations in the programmer guide.

Fixes: 2c900d09055e ("doc: add GRO guide")
Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
changes in v2:
- add fix versions
- add more limitations

 doc/guides/prog_guide/generic_receive_offload_lib.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst b/doc/guides/prog_guide/generic_receive_offload_lib.rst
index 9c6a4d0..3873610 100644
--- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
+++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
@@ -191,3 +191,14 @@ Header fields deciding if packets are neighbors include:
         ignore IPv4 ID fields for the packets whose DF bit is 1.
         Additionally, packets which have different value of DF bit can't
         be merged.
+
+GRO Library Limitations
+-----------------------
+
+- GRO library directly uses the values of MBUF->l2_len/l3_len/l4_len/
+  outer_l2_len/outer_l3_len to parse packet headers. In addition,
+  it relies on MBUF->pkt_len/data_len to process packets. Therefore,
+  to get correct reassembly results, applications must set correct
+  values to those MBUF metadata fields.
+
+- GRO library doesn't support to process packets with IPv4 option fields.
-- 
2.7.4

[dpdk-dev] [PATCH v3] doc: add GRO API limitations in prog_guide

[Jiayu Hu]

This patch adds GRO API limitations in the programmer guide.

Fixes: 2c900d09055e ("doc: add GRO guide")
Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
changes in v3:
- add fix versions
- add more limitations
- add MBUF limitation

 doc/guides/prog_guide/generic_receive_offload_lib.rst | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst b/doc/guides/prog_guide/generic_receive_offload_lib.rst
index 9c6a4d0..c8f9882 100644
--- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
+++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
@@ -191,3 +191,18 @@ Header fields deciding if packets are neighbors include:
         ignore IPv4 ID fields for the packets whose DF bit is 1.
         Additionally, packets which have different value of DF bit can't
         be merged.
+
+GRO Library Limitations
+-----------------------
+
+- GRO library directly uses the values of MBUF->l2_len/l3_len/l4_len/
+  outer_l2_len/outer_l3_len to parse packet headers. In addition,
+  it relies on MBUF->pkt_len/data_len to process packets. Therefore,
+  to get correct reassembly results, applications must set correct
+  values to those MBUF metadata fields.
+
+- GRO library doesn't support to process packets with IPv4 option fields.
+
+- Currently, GRO library just supports to process the packet organized
+  in a single MBUF. If the input packet consists of multiple MBUFs,
+  GRO reassembly behaviors are unknown.
-- 
2.7.4

[dpdk-dev] [PATCH V4] doc: add GRO API limitations in prog_guide

[Jiayu Hu]

This patch adds GRO API limitations in the programmer guide.

Fixes: 2c900d09055e ("doc: add GRO guide")
Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
changes in v4:
- update MBUF->l2_len/... requirement
changes in v3:
- add MBUF limitation
changes in v2:
- add fix versions
- add more limitations

 doc/guides/prog_guide/generic_receive_offload_lib.rst | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst b/doc/guides/prog_guide/generic_receive_offload_lib.rst
index 9c6a4d0..53c9d5c 100644
--- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
+++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
@@ -191,3 +191,20 @@ Header fields deciding if packets are neighbors include:
         ignore IPv4 ID fields for the packets whose DF bit is 1.
         Additionally, packets which have different value of DF bit can't
         be merged.
+
+GRO Library Limitations
+-----------------------
+
+- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
+  outer_l2_len/outer_l3_len to get protocol headers for the
+  input packet, rather than parsing the packet header. Therefore,
+  before call GRO APIs to merge packets, user applications
+  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
+  to the same values as the protocol headers of the packet.
+
+- GRO library doesn't support to process packets with IPv4 Options.
+
+- GRO library just supports to process the packet organized
+  in a single MBUF. If the input packet consists of multiple
+  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
+  unknown.
-- 
2.7.4

Re: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in prog_guide

[Varghese, Vipin]

Hi Jiayu Hu,

A request to information, why there is document update patch with code update patch shared?

As per DPDK 19.02-rc1 both code and document should be one patch. Can you share the code patch reference if it was applied before 19.02-rc1?

Thanks
Vipin Varghese

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jiayu Hu
> Sent: Wednesday, January 9, 2019 8:30 AM
> To: dev@dpdk.org
> Cc: stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; mb@smartsharesystems.com; Hu, Jiayu
> <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in prog_guide
> 
> This patch adds GRO API limitations in the programmer guide.
> 
> Fixes: 2c900d09055e ("doc: add GRO guide")
> Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
> changes in v4:
> - update MBUF->l2_len/... requirement
> changes in v3:
> - add MBUF limitation
> changes in v2:
> - add fix versions
> - add more limitations
> 
>  doc/guides/prog_guide/generic_receive_offload_lib.rst | 17
> +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst
> b/doc/guides/prog_guide/generic_receive_offload_lib.rst
> index 9c6a4d0..53c9d5c 100644
> --- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
> +++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
> @@ -191,3 +191,20 @@ Header fields deciding if packets are neighbors
> include:
>          ignore IPv4 ID fields for the packets whose DF bit is 1.
>          Additionally, packets which have different value of DF bit can't
>          be merged.
> +
> +GRO Library Limitations
> +-----------------------
> +
> +- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
> +  outer_l2_len/outer_l3_len to get protocol headers for the
> +  input packet, rather than parsing the packet header. Therefore,
> +  before call GRO APIs to merge packets, user applications
> +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
> +  to the same values as the protocol headers of the packet.
> +
> +- GRO library doesn't support to process packets with IPv4 Options.
> +
> +- GRO library just supports to process the packet organized
> +  in a single MBUF. If the input packet consists of multiple
> +  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
> +  unknown.
> --
> 2.7.4

Re: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in prog_guide

[Varghese, Vipin]

Typo in previous email

why there is document update patch with no code update patch shared?

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Varghese, Vipin
> Sent: Wednesday, January 9, 2019 9:55 AM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; mb@smartsharesystems.com; Hu, Jiayu
> <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in prog_guide
> 
> Hi Jiayu Hu,
> 
> A request to information, why there is document update patch with code
> update patch shared?
> 
> As per DPDK 19.02-rc1 both code and document should be one patch. Can you
> share the code patch reference if it was applied before 19.02-rc1?
> 
> Thanks
> Vipin Varghese
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Jiayu Hu
> > Sent: Wednesday, January 9, 2019 8:30 AM
> > To: dev@dpdk.org
> > Cc: stephen@networkplumber.org; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; mb@smartsharesystems.com; Hu, Jiayu
> > <jiayu.hu@intel.com>; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in
> > prog_guide
> >
> > This patch adds GRO API limitations in the programmer guide.
> >
> > Fixes: 2c900d09055e ("doc: add GRO guide")
> > Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> > changes in v4:
> > - update MBUF->l2_len/... requirement
> > changes in v3:
> > - add MBUF limitation
> > changes in v2:
> > - add fix versions
> > - add more limitations
> >
> >  doc/guides/prog_guide/generic_receive_offload_lib.rst | 17
> > +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst
> > b/doc/guides/prog_guide/generic_receive_offload_lib.rst
> > index 9c6a4d0..53c9d5c 100644
> > --- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
> > +++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
> > @@ -191,3 +191,20 @@ Header fields deciding if packets are neighbors
> > include:
> >          ignore IPv4 ID fields for the packets whose DF bit is 1.
> >          Additionally, packets which have different value of DF bit can't
> >          be merged.
> > +
> > +GRO Library Limitations
> > +-----------------------
> > +
> > +- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
> > +  outer_l2_len/outer_l3_len to get protocol headers for the
> > +  input packet, rather than parsing the packet header. Therefore,
> > +  before call GRO APIs to merge packets, user applications
> > +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
> > +  to the same values as the protocol headers of the packet.
> > +
> > +- GRO library doesn't support to process packets with IPv4 Options.
> > +
> > +- GRO library just supports to process the packet organized
> > +  in a single MBUF. If the input packet consists of multiple
> > +  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
> > +  unknown.
> > --
> > 2.7.4

Re: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in prog_guide

[Hu, Jiayu]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Wednesday, January 9, 2019 12:27 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; mb@smartsharesystems.com; Hu, Jiayu
> <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in
> prog_guide
> 
> Typo in previous email
> 
> why there is document update patch with no code update patch shared?

I think I need to remove the fix commit 9e0b9d2ec0f4 from this patch, since
the GRO programmer doc is introduced in 2c900d09055e.

Thanks,
Jiayu

> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Varghese, Vipin
> > Sent: Wednesday, January 9, 2019 9:55 AM
> > To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> > Cc: stephen@networkplumber.org; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; mb@smartsharesystems.com; Hu,
> Jiayu
> > <jiayu.hu@intel.com>; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in
> prog_guide
> >
> > Hi Jiayu Hu,
> >
> > A request to information, why there is document update patch with code
> > update patch shared?
> >
> > As per DPDK 19.02-rc1 both code and document should be one patch. Can
> you
> > share the code patch reference if it was applied before 19.02-rc1?
> >
> > Thanks
> > Vipin Varghese
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Jiayu Hu
> > > Sent: Wednesday, January 9, 2019 8:30 AM
> > > To: dev@dpdk.org
> > > Cc: stephen@networkplumber.org; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; mb@smartsharesystems.com; Hu,
> Jiayu
> > > <jiayu.hu@intel.com>; stable@dpdk.org
> > > Subject: [dpdk-dev] [PATCH V4] doc: add GRO API limitations in
> > > prog_guide
> > >
> > > This patch adds GRO API limitations in the programmer guide.
> > >
> > > Fixes: 2c900d09055e ("doc: add GRO guide")
> > > Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > > ---
> > > changes in v4:
> > > - update MBUF->l2_len/... requirement
> > > changes in v3:
> > > - add MBUF limitation
> > > changes in v2:
> > > - add fix versions
> > > - add more limitations
> > >
> > >  doc/guides/prog_guide/generic_receive_offload_lib.rst | 17
> > > +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > >
> > > diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst
> > > b/doc/guides/prog_guide/generic_receive_offload_lib.rst
> > > index 9c6a4d0..53c9d5c 100644
> > > --- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
> > > +++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
> > > @@ -191,3 +191,20 @@ Header fields deciding if packets are neighbors
> > > include:
> > >          ignore IPv4 ID fields for the packets whose DF bit is 1.
> > >          Additionally, packets which have different value of DF bit can't
> > >          be merged.
> > > +
> > > +GRO Library Limitations
> > > +-----------------------
> > > +
> > > +- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
> > > +  outer_l2_len/outer_l3_len to get protocol headers for the
> > > +  input packet, rather than parsing the packet header. Therefore,
> > > +  before call GRO APIs to merge packets, user applications
> > > +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
> > > +  to the same values as the protocol headers of the packet.
> > > +
> > > +- GRO library doesn't support to process packets with IPv4 Options.
> > > +
> > > +- GRO library just supports to process the packet organized
> > > +  in a single MBUF. If the input packet consists of multiple
> > > +  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
> > > +  unknown.
> > > --
> > > 2.7.4

[dpdk-dev] [PATCH v5] doc: add GRO API limitations in prog_guide

[Jiayu Hu]

This patch adds GRO API limitations in the programmer guide.

Fixes: 2c900d09055e ("doc: add GRO guide")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
changes in v5:
- remove fix commit 9e0b9d2ec0f4
changes in v4:
- update MBUF->l2_len/... requirement
changes in v3:
- add MBUF limitation
changes in v2:
- add fix commits
- add more limitations

 doc/guides/prog_guide/generic_receive_offload_lib.rst | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst b/doc/guides/prog_guide/generic_receive_offload_lib.rst
index 9c6a4d0..53c9d5c 100644
--- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
+++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
@@ -191,3 +191,20 @@ Header fields deciding if packets are neighbors include:
         ignore IPv4 ID fields for the packets whose DF bit is 1.
         Additionally, packets which have different value of DF bit can't
         be merged.
+
+GRO Library Limitations
+-----------------------
+
+- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
+  outer_l2_len/outer_l3_len to get protocol headers for the
+  input packet, rather than parsing the packet header. Therefore,
+  before call GRO APIs to merge packets, user applications
+  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
+  to the same values as the protocol headers of the packet.
+
+- GRO library doesn't support to process packets with IPv4 Options.
+
+- GRO library just supports to process the packet organized
+  in a single MBUF. If the input packet consists of multiple
+  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
+  unknown.
-- 
2.7.4

Re: [dpdk-dev] [PATCH v5] doc: add GRO API limitations in prog_guide

[Stephen Hemminger]

O        be merged.
> +
> +GRO Library Limitations
> +-----------------------
> +
> +- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
> +  outer_l2_len/outer_l3_len to get protocol headers for the
> +  input packet, rather than parsing the packet header. Therefore,
> +  before call GRO APIs to merge packets, user applications
> +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
> +  to the same values as the protocol headers of the packet.
> +

Since these length values are critical to other functionality
why not require all poll mode drivers to set them.

Many poll mode drivers call rte_net_get_ptype() on the received
mbuf and it already handles setting this.

One could argue that GRO should just log and die if it
gets malformed data.

Re: [dpdk-dev] [PATCH v5] doc: add GRO API limitations in prog_guide

[Ananyev, Konstantin]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, January 9, 2019 6:20 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; mb@smartsharesystems.com; thomas@monjalon.net;
> Varghese, Vipin <vipin.varghese@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v5] doc: add GRO API limitations in prog_guide
> 
> O        be merged.
> > +
> > +GRO Library Limitations
> > +-----------------------
> > +
> > +- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
> > +  outer_l2_len/outer_l3_len to get protocol headers for the
> > +  input packet, rather than parsing the packet header. Therefore,
> > +  before call GRO APIs to merge packets, user applications
> > +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
> > +  to the same values as the protocol headers of the packet.
> > +
> 
> Since these length values are critical to other functionality
> why not require all poll mode drivers to set them.

Most of current HW doesn't provide that functionality,
so RX function would need to parse (touch) packet data.
>From other side not every rx_burst() consumer does use GRO library.

> 
> Many poll mode drivers call rte_net_get_ptype() on the received
> mbuf and it already handles setting this.
> 
> One could argue that GRO should just log and die if it
> gets malformed data.

Re: [dpdk-dev] [PATCH v5] doc: add GRO API limitations in prog_guide

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> >
> > O        be merged.
> > > +
> > > +GRO Library Limitations
> > > +-----------------------
> > > +
> > > +- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
> > > +  outer_l2_len/outer_l3_len to get protocol headers for the
> > > +  input packet, rather than parsing the packet header. Therefore,
> > > +  before call GRO APIs to merge packets, user applications
> > > +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
> > > +  to the same values as the protocol headers of the packet.
> > > +
> >
> > Since these length values are critical to other functionality
> > why not require all poll mode drivers to set them.
> 
> Most of current HW doesn't provide that functionality,
> so RX function would need to parse (touch) packet data.

True... In an application where the first process step is to receive the packet from the PMD and put it directly into the appropriate queue (or drops it) solely based on information provided in the "RX part" of the MBUF, two cache misses can be avoided - reading the header in the packet data and writing the "TX part" of the mbuf.

This is a key selling point for NIC hardware with the ability to perform classification, such as the Flow Director feature, so it can efficiently identify and discard packets related to DDOS attacks, as an example.

> From other side not every rx_burst() consumer does use GRO library.
> 

Nonetheless, many applications need to touch the packet header on ingress for classification purposes - either to identify a flow or to identify the attributes used for routing and QoS classification.

I expect that validating packet headers (i.e. identifying malformed packets) somewhere early on the ingress fast path is a very common use case, which is why I on another thread suggested extending rte_net_get_ptype() to check packet validity and building a bulk function on top of that to set the MBUF->l2_len/l3_len... fields, so they are ready for GRO, Fragment Reassembly and other ingress path libraries requiring this information: https://mails.dpdk.org/archives/dev/2019-January/122701.html

> >
> > Many poll mode drivers call rte_net_get_ptype() on the received
> > mbuf and it already handles setting this.
> >
> > One could argue that GRO should just log and die if it
> > gets malformed data.

This would be a good principle! If preconditions are not met, it is a bug and should be treated as such. As I mentioned before, this specific function is not taking foreign input; the application is in full control of passing garbage or not to this function.


Med venlig hilsen / kind regards
- Morten Brørup

Re: [dpdk-dev] [PATCH v5] doc: add GRO API limitations in prog_guide

[Hu, Jiayu]

> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Thursday, January 10, 2019 5:32 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Stephen
> Hemminger <stephen@networkplumber.org>; Hu, Jiayu
> <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; Varghese, Vipin
> <vipin.varghese@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5] doc: add GRO API limitations in
> prog_guide
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > >
> > > O        be merged.
> > > > +
> > > > +GRO Library Limitations
> > > > +-----------------------
> > > > +
> > > > +- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
> > > > +  outer_l2_len/outer_l3_len to get protocol headers for the
> > > > +  input packet, rather than parsing the packet header. Therefore,
> > > > +  before call GRO APIs to merge packets, user applications
> > > > +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
> > > > +  to the same values as the protocol headers of the packet.
> > > > +
> > >
> > > Since these length values are critical to other functionality
> > > why not require all poll mode drivers to set them.
> >
> > Most of current HW doesn't provide that functionality,
> > so RX function would need to parse (touch) packet data.
> 
> True... In an application where the first process step is to receive the packet
> from the PMD and put it directly into the appropriate queue (or drops it)
> solely based on information provided in the "RX part" of the MBUF, two
> cache misses can be avoided - reading the header in the packet data and
> writing the "TX part" of the mbuf.
> 
> This is a key selling point for NIC hardware with the ability to perform
> classification, such as the Flow Director feature, so it can efficiently identify
> and discard packets related to DDOS attacks, as an example.
> 
> > From other side not every rx_burst() consumer does use GRO library.
> >
> 
> Nonetheless, many applications need to touch the packet header on ingress
> for classification purposes - either to identify a flow or to identify the
> attributes used for routing and QoS classification.
> 
> I expect that validating packet headers (i.e. identifying malformed packets)
> somewhere early on the ingress fast path is a very common use case, which
> is why I on another thread suggested extending rte_net_get_ptype() to
> check packet validity and building a bulk function on top of that to set the
> MBUF->l2_len/l3_len... fields, so they are ready for GRO, Fragment
> Reassembly and other ingress path libraries requiring this information:
> https://mails.dpdk.org/archives/dev/2019-January/122701.html
> 
> > >
> > > Many poll mode drivers call rte_net_get_ptype() on the received
> > > mbuf and it already handles setting this.
> > >
> > > One could argue that GRO should just log and die if it
> > > gets malformed data.
> 
> This would be a good principle! If preconditions are not met, it is a bug and
> should be treated as such. As I mentioned before, this specific function is
> not taking foreign input; the application is in full control of passing garbage
> or not to this function.

Since we already have lots of discussions around GRO, I want to make a summary
just in case that we go far away from the purpose of the two GRO patches.

1. To accelerate header processing, GRO is designed to use MBUF->l2_len/... to
get protocol headers for input packets; user applications must set MBUF->l2_len/...
to the same values as packet headers. As we discussed in the previous mails, in
the real-world scenarios, many applications know the header information, and they
can set MBUF->l2_len/... to the corresponding values. So I think the design of GRO
makes sense. What we lack is to well explain it in the document, and this patch is to
add the missing information.

One thing to notice is that GRO shouldn't check if the values of MBUF->l2_len/...
are the same as protocol headers in the packet. This is because that checking the
values requires GRO to re-parse packet headers, which makes the design that uses
MBUF->l2_len/... to get protocol headers meaningless. If you don't think using
MBUF->l2_len/... is a reasonable design and you have better ideas, we can discuss
in a new thread or in a RFC. Because it's a feature change rather than a bug fix.

2. The second patch is to forbid GRO to process invalid input packets
(http://patches.dpdk.org/patch/49491/). Even if the application sets
MBUF->l2_len.. to the same values as packet headers, the input packets
of GRO may still be invalid. E.g., TCP header is less than 20 bytes. In current
implementation, GRO will still process these invalid packets. In previous mails,
you suggested to terminate applications. But it's too extreme. As a reassembly
library, I think a better way is to add necessary checks to find invalid packets
and return them to applications.

Thanks,
Jiayu
> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup

Re: [dpdk-dev] [PATCH v5] doc: add GRO API limitations in prog_guide

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hu, Jiayu
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > Konstantin
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > >
> > > > O        be merged.
> > > > > +
> > > > > +GRO Library Limitations
> > > > > +-----------------------
> > > > > +
> > > > > +- GRO library uses the values of MBUF->l2_len/l3_len/l4_len/
> > > > > +  outer_l2_len/outer_l3_len to get protocol headers for the
> > > > > +  input packet, rather than parsing the packet header.
> Therefore,
> > > > > +  before call GRO APIs to merge packets, user applications
> > > > > +  must set MBUF-
> >l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len
> > > > > +  to the same values as the protocol headers of the packet.
> > > > > +
> > > >
> > > > Since these length values are critical to other functionality
> > > > why not require all poll mode drivers to set them.
> > >
> > > Most of current HW doesn't provide that functionality,
> > > so RX function would need to parse (touch) packet data.
> >
> > True... In an application where the first process step is to receive
> the packet
> > from the PMD and put it directly into the appropriate queue (or drops
> it)
> > solely based on information provided in the "RX part" of the MBUF,
> two
> > cache misses can be avoided - reading the header in the packet data
> and
> > writing the "TX part" of the mbuf.
> >
> > This is a key selling point for NIC hardware with the ability to
> perform
> > classification, such as the Flow Director feature, so it can
> efficiently identify
> > and discard packets related to DDOS attacks, as an example.
> >
> > > From other side not every rx_burst() consumer does use GRO library.
> > >
> >
> > Nonetheless, many applications need to touch the packet header on
> ingress
> > for classification purposes - either to identify a flow or to
> identify the
> > attributes used for routing and QoS classification.
> >
> > I expect that validating packet headers (i.e. identifying malformed
> packets)
> > somewhere early on the ingress fast path is a very common use case,
> which
> > is why I on another thread suggested extending rte_net_get_ptype() to
> > check packet validity and building a bulk function on top of that to
> set the
> > MBUF->l2_len/l3_len... fields, so they are ready for GRO, Fragment
> > Reassembly and other ingress path libraries requiring this
> information:
> > https://mails.dpdk.org/archives/dev/2019-January/122701.html
> >
> > > >
> > > > Many poll mode drivers call rte_net_get_ptype() on the received
> > > > mbuf and it already handles setting this.
> > > >
> > > > One could argue that GRO should just log and die if it
> > > > gets malformed data.
> >
> > This would be a good principle! If preconditions are not met, it is a
> bug and
> > should be treated as such. As I mentioned before, this specific
> function is
> > not taking foreign input; the application is in full control of
> passing garbage
> > or not to this function.
> 
> Since we already have lots of discussions around GRO, I want to make a
> summary
> just in case that we go far away from the purpose of the two GRO
> patches.
> 
> 1. To accelerate header processing, GRO is designed to use MBUF-
> >l2_len/... to
> get protocol headers for input packets; user applications must set
> MBUF->l2_len/...
> to the same values as packet headers. As we discussed in the previous
> mails, in
> the real-world scenarios, many applications know the header
> information, and they
> can set MBUF->l2_len/... to the corresponding values. So I think the
> design of GRO
> makes sense. What we lack is to well explain it in the document, and
> this patch is to
> add the missing information.
> 
> One thing to notice is that GRO shouldn't check if the values of MBUF-
> >l2_len/...
> are the same as protocol headers in the packet. This is because that
> checking the
> values requires GRO to re-parse packet headers, which makes the design
> that uses
> MBUF->l2_len/... to get protocol headers meaningless. If you don't
> think using
> MBUF->l2_len/... is a reasonable design and you have better ideas, we
> can discuss
> in a new thread or in a RFC. Because it's a feature change rather than
> a bug fix.
> 
> 2. The second patch is to forbid GRO to process invalid input packets
> (http://patches.dpdk.org/patch/49491/). Even if the application sets
> MBUF->l2_len.. to the same values as packet headers, the input packets
> of GRO may still be invalid. E.g., TCP header is less than 20 bytes. In
> current
> implementation, GRO will still process these invalid packets. In
> previous mails,
> you suggested to terminate applications. But it's too extreme. As a
> reassembly
> library, I think a better way is to add necessary checks to find
> invalid packets
> and return them to applications.
> 
> Thanks,
> Jiayu

+1

Let's close the GRO API sidetrack here, and continue the RFC discussion in the other thread:
https://mails.dpdk.org/archives/dev/2019-January/122774.html

> >
> >
> > Med venlig hilsen / kind regards
> > - Morten Brørup

[dpdk-dev] [PATCH v6] doc: add GRO limitations in prog_guide

[Jiayu Hu]

This patch adds GRO limitations in the programmer guide.

Fixes: 2c900d09055e ("doc: add GRO guide")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
changes in v6:
- add VLAN limitation
changes in v5:
- remove fix commit 9e0b9d2ec0f4
changes in v4:
- update MBUF->l2_len/... requirement
changes in v3:
- add MBUF limitation
changes in v2:
- add fix commits
- add more limitations

 doc/guides/prog_guide/generic_receive_offload_lib.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst b/doc/guides/prog_guide/generic_receive_offload_lib.rst
index 9c6a4d0..5b3fb91 100644
--- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
+++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
@@ -191,3 +191,22 @@ Header fields deciding if packets are neighbors include:
         ignore IPv4 ID fields for the packets whose DF bit is 1.
         Additionally, packets which have different value of DF bit can't
         be merged.
+
+GRO Library Limitations
+-----------------------
+
+- GRO library uses MBUF->l2_len/l3_len/l4_len/outer_l2_len/
+  outer_l3_len/packet_type to get protocol headers for the
+  input packet, rather than parsing the packet header. Therefore,
+  before call GRO APIs to merge packets, user applications
+  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len/
+  packet_type to the same values as the protocol headers of the
+  packet.
+
+- GRO library doesn't support to process the packets with IPv4
+  Options or VLAN tagged.
+
+- GRO library just supports to process the packet organized
+  in a single MBUF. If the input packet consists of multiple
+  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
+  unknown.
-- 
2.7.4

Re: [dpdk-dev] [PATCH v6] doc: add GRO limitations in prog_guide

[Ananyev, Konstantin]

> -----Original Message-----
> From: Hu, Jiayu
> Sent: Wednesday, January 16, 2019 2:14 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net; Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: [PATCH v6] doc: add GRO limitations in prog_guide
> 
> This patch adds GRO limitations in the programmer guide.
> 
> Fixes: 2c900d09055e ("doc: add GRO guide")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
> changes in v6:
> - add VLAN limitation
> changes in v5:
> - remove fix commit 9e0b9d2ec0f4
> changes in v4:
> - update MBUF->l2_len/... requirement
> changes in v3:
> - add MBUF limitation
> changes in v2:
> - add fix commits
> - add more limitations
> 
>  doc/guides/prog_guide/generic_receive_offload_lib.rst | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/generic_receive_offload_lib.rst b/doc/guides/prog_guide/generic_receive_offload_lib.rst
> index 9c6a4d0..5b3fb91 100644
> --- a/doc/guides/prog_guide/generic_receive_offload_lib.rst
> +++ b/doc/guides/prog_guide/generic_receive_offload_lib.rst
> @@ -191,3 +191,22 @@ Header fields deciding if packets are neighbors include:
>          ignore IPv4 ID fields for the packets whose DF bit is 1.
>          Additionally, packets which have different value of DF bit can't
>          be merged.
> +
> +GRO Library Limitations
> +-----------------------
> +
> +- GRO library uses MBUF->l2_len/l3_len/l4_len/outer_l2_len/
> +  outer_l3_len/packet_type to get protocol headers for the
> +  input packet, rather than parsing the packet header. Therefore,
> +  before call GRO APIs to merge packets, user applications
> +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len/
> +  packet_type to the same values as the protocol headers of the
> +  packet.
> +
> +- GRO library doesn't support to process the packets with IPv4
> +  Options or VLAN tagged.
> +
> +- GRO library just supports to process the packet organized
> +  in a single MBUF. If the input packet consists of multiple
> +  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
> +  unknown.
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.7.4

Re: [dpdk-dev] [PATCH v6] doc: add GRO limitations in prog_guide

[Thomas Monjalon]

16/01/2019 10:50, Ananyev, Konstantin:
> From: Hu, Jiayu
> > 
> > This patch adds GRO limitations in the programmer guide.
> > 
> > Fixes: 2c900d09055e ("doc: add GRO guide")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> > changes in v6:
> > - add VLAN limitation
> > changes in v5:
> > - remove fix commit 9e0b9d2ec0f4
> > changes in v4:
> > - update MBUF->l2_len/... requirement
> > changes in v3:
> > - add MBUF limitation
> > changes in v2:
> > - add fix commits
> > - add more limitations
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks

Re: [dpdk-dev] [PATCH v6] doc: add GRO limitations in prog_guide

[Kovacevic, Marko]

> +GRO Library Limitations
> +-----------------------
> +
> +- GRO library uses MBUF->l2_len/l3_len/l4_len/outer_l2_len/
> +  outer_l3_len/packet_type to get protocol headers for the
> +  input packet, rather than parsing the packet header. Therefore,
> +  before call GRO APIs to merge packets, user applications
> +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len/
> +  packet_type to the same values as the protocol headers of the
> +  packet.
> +
> +- GRO library doesn't support to process the packets with IPv4
> +  Options or VLAN tagged.
> +
> +- GRO library just supports to process the packet organized
> +  in a single MBUF. If the input packet consists of multiple
> +  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
> +  unknown.
> --

Would it be better said like this ??

- GRO library uses different MBUF->packet_types for example
  ``l2_len, l3_len, l4_len, outer_l2_len, outer_l3_len`` to get protocol
  headers for the input packet, rather than parsing the packet header.
  Therefore, before calling GRO APIs to merge packets, user applications
  must set MBUF->packet_type to the same values as the protocol headers of
  the packet.

Re: [dpdk-dev] [dpdk-stable] [PATCH v6] doc: add GRO limitations in prog_guide

[Thomas Monjalon]

17/01/2019 14:55, Kovacevic, Marko:
> > +GRO Library Limitations
> > +-----------------------
> > +
> > +- GRO library uses MBUF->l2_len/l3_len/l4_len/outer_l2_len/
> > +  outer_l3_len/packet_type to get protocol headers for the
> > +  input packet, rather than parsing the packet header. Therefore,
> > +  before call GRO APIs to merge packets, user applications
> > +  must set MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len/
> > +  packet_type to the same values as the protocol headers of the
> > +  packet.
> > +
> > +- GRO library doesn't support to process the packets with IPv4
> > +  Options or VLAN tagged.
> > +
> > +- GRO library just supports to process the packet organized
> > +  in a single MBUF. If the input packet consists of multiple
> > +  MBUFs (i.e. chained MBUFs), GRO reassembly behaviors are
> > +  unknown.
> > --
> 
> Would it be better said like this ??
> 
> - GRO library uses different MBUF->packet_types for example
>   ``l2_len, l3_len, l4_len, outer_l2_len, outer_l3_len`` to get protocol
>   headers for the input packet, rather than parsing the packet header.
>   Therefore, before calling GRO APIs to merge packets, user applications
>   must set MBUF->packet_type to the same values as the protocol headers of
>   the packet.

packet_type is really a field in mbuf.
I think the wording from Jiayu is more correct.

Re: [dpdk-dev] [PATCH] doc: add GRO API requirement in prog_guide

[Stephen Hemminger]

On Tue, 18 Dec 2018 12:25:19 +0800
Jiayu Hu <jiayu.hu@intel.com> wrote:

> +.. note::
> +        To use the GRO library, applications must guarantee that
> +        MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len and
> +        MBUF->pkt_len/data_len of input packets are set to correct
> +        values.
> +

This statement seems confusing to me since
this is Generic Receive Offload, and receive buffers come from
the poll mode driver.

Aren't you saying that the PMD must set the correct values.
If this is correct, then any PMD not doing so is buggy.
And should be reported and fixed. If it is really buggy PMD's
then the statement in the programmers guide is not the place to handle
this kind of issue.  Better to just shame the buggy drivers in the
release notes for that NIC.

Re: [dpdk-dev] [PATCH] doc: add GRO API requirement in prog_guide

[Hu, Jiayu]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, January 5, 2019 3:36 AM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] doc: add GRO API requirement in
> prog_guide
> 
> On Tue, 18 Dec 2018 12:25:19 +0800
> Jiayu Hu <jiayu.hu@intel.com> wrote:
> 
> > +.. note::
> > +        To use the GRO library, applications must guarantee that
> > +        MBUF->l2_len/l3_len/l4_len/outer_l2_len/outer_l3_len and
> > +        MBUF->pkt_len/data_len of input packets are set to correct
> > +        values.
> > +
> 
> This statement seems confusing to me since
> this is Generic Receive Offload, and receive buffers come from
> the poll mode driver.
> 
> Aren't you saying that the PMD must set the correct values.
> If this is correct, then any PMD not doing so is buggy.
> And should be reported and fixed. If it is really buggy PMD's
> then the statement in the programmers guide is not the place to handle
> this kind of issue.  Better to just shame the buggy drivers in the
> release notes for that NIC.

IMO, since GRO is a library, all input packets are from user applications.
So I want to tell them that GRO relies on MBUF->l2_len/... to
parse packets. If they want correct reassembly results, all those fields of MBUF
should be set correctly. If the packets are received by buggy PMDs and user
applications directly pass them to GRO, GRO results are unknown. If the
packets are received from correct PMDs, but user applications modify MBUF
and make MBUF->l2_len/... incorrect, GRO results are still unknown.

This statement is more like a programmer note. If you think it is confusing,
how do you think this one:
GRO library uses MBUF->l2_len/... to parse packet headers. If the input packets
have incorrect values of MBUF->l2_len/.., GRO reassembly results are unknown.

Thanks,
Jiayu