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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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