[v5] doc: add GRO API limitations in prog_guide

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

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.
  

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

> 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
  

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

> 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