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

["Hu, Jiayu" <jiayu.hu@intel.com>]

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