Re: [dpdk-dev] [RFC] function to parse packet headers

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

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > On Fri, Jan 11, 2019 at 10:56:11AM +0300, Andrew Rybchenko wrote:
> > > On 1/11/19 3:11 AM, Stephen Hemminger wrote:
> > > > On Thu, 10 Jan 2019 03:03:24 +0200
> > > > Rami Rosen <roszenrami@gmail.com> wrote:
> > > >
> > > > > > And regarding avoiding code duplicity, I'm pursuing Olivier
> about merging
> > > > > packet header validation into rte_net_get_ptype() instead of
> writing a
> > > > > separate function.
> > > > > This seems also a good alternative.
> > > > > +1
> >
> > Thanks Morten for volunteering for this task. I also think that
> rte_net
> > is the proper place. rte_net_get_ptype() is indeed quite similar,
> except
> > that it won't return any length. So it may not be that easy to share
> the
> > code between rte_net_get_ptype().
> >

rte_net_get_ptype() does return the lengths, but in another struct; so I am leaning towards adding packet header validity checks to rte_net_get_ptype(), thus making my function a simple wrapper to rte_net_get_ptype().

And then perhaps only a new function is required for the bulk function, because rte_net_get_ptype() then becomes the single packet parser function, but we'll see about that later.

> > As an aside, the Rx and Tx offloads fields are distinct. In Rx we
> have the
> > packet type that does not contain the length information, while in Tx
> we only
> > have the lengths. I'm sure there will be some benefits to merge them,
> but it's
> > another topic.
> >
> > About the function name, I feel "parse()" is a bit vague. What about
> something
> > like rte_net_set_tx_offload(), rte_net_set_offload_lengths() or
> > rte_net_set_tx_ol_len()?
> >
> > For the bulk API, marking invalid packets seems good, but I can't
> find a
> > good place for the mark.
> > Maybe setting m->tx_offload to INVALID (0xffffffff), or adding a
> specific
> > flag. Any better idea is welcome ;)
> 
> We probably can reset ptype to UKNOWN in that case,
> but my prefrence would be just group inavlid/unknown packets in some
> way
> (put them beyond good ones, move into different array, etc.)
> 

I see your point of grouping valid/invalid packets... It would be silly to mark a packet invalid and then pass it on to e.g. GRO for processing, and then require GRO to check the validity mark of each packet before processing it. So let's agree to use the bulk function header with both input and an output array parameters.

BL mentioned in a previous mail that details about the malformed reason could be useful, so we should discuss that too:

Let's start by agreeing that if an application only cares about L2/L3 headers, it will tell rte_net_get_ptype() only to parse up to L3 headers; so if the L4 header is malformed, the packet should be considered good, because the application doesn't care.

A packet can be malformed in multiple ways, so a bitfield for the error types might be required.
Such a bitfield would probably be too large for the yet unused bits in the MBUF->tx_offload field.
So I think that we have to limit ourselves to only indicating which header levels are malformed.
I suggest doing this by changing the packet_type: E.g. if the packet is invalid at L4 level, then clear the L4 bits in packet_type, or alternatively set the L4 bits to 0xF (thus introducing a new RTE_PTYPE_L4_INVALID value).

> >
> > > > All drivers that don't have hardware support for getting l2/l3
> and ptype
> > > > information should be calling rte_net_get_ptype() already.
> > >
> > > Is it documented somewhere?
> >
> > I don't think it's mandatory. Each driver can announce its supported
> ptype
> > through rte_eth_dev_get_supported_ptypes().
> 
> + 1
> It shouldn't be mandatory for PMD to do that.
> Konstantin

+1
I had missed that point. Andrew argued similarly in a crossed mail.

Med venlig hilsen / kind regards
- Morten Brørup