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

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

[Morten Brørup]

I'm volunteering to provide a function to set the mbuf's l2_len/l3_len/... (tx_offload) fields by parsing the packet headers, possibly assisted by the packet_type field, if set.

I'm seeking initial feedback before submitting my first version of the code.

The single packet parser function header is straightforward, and returns 0 if OK and -1 if the packet is malformed:
int rte_hdr_parse(struct rte_mbuf * const m);


The bulk parser function needs a well defined and simple way to mark malformed packets, e.g. if the IPv4 header length field value is less than 5. There are multiple ways of doing this.

The bulk function could take the mbuf array as input and simply mark malformed packets by setting MBUF->tx_offload = 0:
void rte_hdr_parse_bulk(struct rte_mbuf **pkts, uint_fast16_t nb_pkts);

Or the bulk function could take both input and output arrays, and return the number of non-malformed packets in the output array like this:
uint_fast16_t rte_hdr_parse_bulk(struct rte_mbuf **pkts_in, struct rte_mbuf **pkts_out, uint_fast16_t nb_pkts_in);

The first is obviously faster. Which do you prefer?


Suggested file location and name:
root/lib/librte_net/rte_hdr_parse.[ch]


PS: Inspired by the discussion about GRO (https://mails.dpdk.org/archives/dev/2019-January/122572.html), where Konstantin called for volunteers. I already have some of it lying around, so I will contribute if useful. But it needs some cleaning up first.


Med venlig hilsen / kind regards

Morten Brørup
CTO


SmartShare Systems A/S
Tonsbakken 16-18
DK-2740 Skovlunde
Denmark

Office      +45 70 20 00 93
Direct      +45 89 93 50 22
Mobile     +45 25 40 82 12

mb@smartsharesystems.com
www.smartsharesystems.com

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

[Rami Rosen]

Hi Morten,
A good idea, thanks for volunteering!
Several minor comments:
I would consider calling it rte_mbuf_hdr_parse(), to make it more related
to the rte_mbuf* methods, and also I would consider having it in
librte_mbuf and not in librte_net as you suggested.

Also regarding the bulk method. The first option is indeed faster and
better in terms of performance, which is important since it is intended
probably mostly to the datapath. I would consider having the bulk method
iterating over the rte_hdr_parse() method ,( or rte_mbuf_hdr_parse() if you
will agree to my suggestion), and adding a boolean parameter (mark_malform
or something like that) to this method + removing the const quailifier. The
bulk method will set that flag when calling the rte_hdr_parse(). Thus you
will avoid duplicity of the parsing code.

Regards,
Rami Rosen


>
>

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

[Morten Brørup]

From: Rami Rosen [mailto:roszenrami@gmail.com] 
>Hi Morten,
>A good idea, thanks for volunteering! 
>Several minor comments: 
>I would consider calling it rte_mbuf_hdr_parse(), to make it more related to the rte_mbuf* methods, and also I would consider having it in librte_mbuf and not in librte_net as you suggested.

I considered this, but since it looks inside the packet data, and not just processes metadata, I think it doesn't belong in librte_mbuf.

>
>Also regarding the bulk method. The first option is indeed faster and better in terms of performance, which is important since it is intended probably mostly to the datapath. I would consider having the bulk method iterating over the rte_hdr_parse() method ,( or rte_mbuf_hdr_parse() if you will agree to my suggestion), and adding a boolean parameter (mark_malform or something like that) to this method + removing the const quailifier. The bulk method will set that flag when calling the rte_hdr_parse(). Thus you will avoid duplicity of the parsing code.

Good points...

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.

>
>Regards,
>Rami Rosen

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

[Thomas Monjalon]

Adding few more Cc's

09/01/2019 16:53, Morten Brørup:
> From: Rami Rosen [mailto:roszenrami@gmail.com] 
> >Hi Morten,
> >A good idea, thanks for volunteering! 
> >Several minor comments: 
> >I would consider calling it rte_mbuf_hdr_parse(), to make it more related to the rte_mbuf* methods, and also I would consider having it in librte_mbuf and not in librte_net as you suggested.
> 
> I considered this, but since it looks inside the packet data, and not just processes metadata, I think it doesn't belong in librte_mbuf.
> 
> >
> >Also regarding the bulk method. The first option is indeed faster and better in terms of performance, which is important since it is intended probably mostly to the datapath. I would consider having the bulk method iterating over the rte_hdr_parse() method ,( or rte_mbuf_hdr_parse() if you will agree to my suggestion), and adding a boolean parameter (mark_malform or something like that) to this method + removing the const quailifier. The bulk method will set that flag when calling the rte_hdr_parse(). Thus you will avoid duplicity of the parsing code.
> 
> Good points...
> 
> 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.
> 
> >
> >Regards,
> >Rami Rosen

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

[Rami Rosen]

Hi, Morten,

> 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

Regards,
Rami Rosen



>
>
>

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

[Stephen Hemminger]

On Thu, 10 Jan 2019 03:03:24 +0200
Rami Rosen <roszenrami@gmail.com> wrote:

> Hi, Morten,
> 
> > 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
> 
> Regards,
> Rami Rosen
> 

+1
All drivers that don't have hardware support for getting l2/l3 and ptype
information should be calling rte_net_get_ptype() already.

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

[Andrew Rybchenko]

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:
>
>> Hi, Morten,
>>
>>> 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
>>
>> Regards,
>> Rami Rosen
>>
> +1
> 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?

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

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrew Rybchenko
> On 1/11/19 3:11 AM, Stephen Hemminger wrote:
> >
> > 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?
> 
The drivers need to parse the packet headers to set MBUF->packet_type, and without hardware support for it, the alternative to calling rte_net_get_ptype() is implementing duplicate code in the driver.

In other words, you can interpret Stephen's "should be" as "would be silly if not". :-)


Med venlig hilsen / kind regards
- Morten Brørup

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

[Andrew Rybchenko]

On 1/11/19 11:16 AM, Morten Brørup wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrew Rybchenko
>> On 1/11/19 3:11 AM, Stephen Hemminger wrote:
>>> 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?
>>
> The drivers need to parse the packet headers to set MBUF->packet_type, and without hardware support for it, the alternative to calling rte_net_get_ptype() is implementing duplicate code in the driver.

I'm sorry, but I see it a bit different. The driver provides 
rte_eth_dev_get_supported_ptypes().
If nothing is promised, nothing may be provided. If application needs 
more ptypes, it can
call the library function and obtain required information. If 
application does not need ptypes,
what the point to do ptype discovery in SW and through it away?

> In other words, you can interpret Stephen's "should be" as "would be silly if not". :-)
>
>
> Med venlig hilsen / kind regards
> - Morten Brørup
>
>
>

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

[Olivier Matz]

Hi,

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:
> > 
> > > Hi, Morten,
> > > 
> > > > 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().

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

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

Olivier

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

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Friday, January 11, 2019 8:36 AM
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Rami Rosen <roszenrami@gmail.com>; Thomas Monjalon
> <thomas@monjalon.net>; Morten Brørup <mb@smartsharesystems.com>; dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [RFC] function to parse packet headers
> 
> Hi,
> 
> 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:
> > >
> > > > Hi, Morten,
> > > >
> > > > > 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().
> 
> 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.)

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

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

[Morten Brørup]

> 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

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

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> longtb5@viettel.com.vn
> 
> Also for the bulk API, another option would be passing in a `uint64_t
> malformed_mask` and let the bulk function set the correspond bit (1 <<
> pos) of that mask if the packet at position pos is malformed.
> 
> void rte_hdr_parse_bulk(struct rte_mbuf **pkts, uint64_t
> *malformed_mask, uint_fast16_t nb_pkts);
> 
> The packet mask idea is used extensively in the packet framework
> (rte_pipeline, rte_port, rte_table).
> 

A key property of the bulk function is to make it easy passing on the resulting array of non-malformed packets to the next bulk function, e.g. GRO. A malformed_mask would require an intermediate step of processing before the resulting array can be passed on.

For functions like GRO, either the MBUF must contain a valid/malformed indication, or the array provided to them must contain only valid packets.

Med venlig hilsen / kind regards
- Morten Brørup

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

[longtb5]

Also for the bulk API, another option would be passing in a `uint64_t malformed_mask` and let the bulk function set the correspond bit (1 << pos) of that mask if the packet at position pos is malformed.

void rte_hdr_parse_bulk(struct rte_mbuf **pkts, uint64_t *malformed_mask, uint_fast16_t nb_pkts);

The packet mask idea is used extensively in the packet framework (rte_pipeline, rte_port, rte_table).

> -----Original Message-----
> From: mb@smartsharesystems.com [mailto:mb@smartsharesystems.com]
> Sent: Wednesday, January 9, 2019 10:39 PM
> To: longtb5@viettel.com.vn; roszenrami@gmail.com; Olivier Matz
> <olivier.matz@6wind.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [RFC] function to parse packet headers
> 
> Cutting in Olivier, requesting input as the maintainer of rte_net.
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> > longtb5@viettel.com.vn
> >
> > Hi Morten,
> >
> > What is the difference compare to rte_net_get_ptype(), which also
> > parses packet types and reports on header length.
> >
> > In my application I have also done something similar about malformed
> > packets. IMO it's very useful to have return value indicate different
> > types of malformed packets, not just -1, e.g. invalid IP options, IP
> > loopback, etc.
> 
> They are very similar. The main differences are:
> - The header length fields are set in the MBUF, not returned in a separate
> structure. This would only be relevant for a bulk function.
> - In theory, it would be possible to set the length fields without accessing the
> packet data, but just by looking at MBUF->packet_type for some packets;
> e.g. RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_UDP is
> quite common due to Google's QUIC protocol (and will be with the coming
> HTTP/3 protocol).
> - Testing for malformed packets, e.g. a length field suggesting that the packet
> is longer than it actually is, or a header length field suggesting that the header
> is shorter than the header's structure. (This obviously requires accessing the
> packet data, which makes the above point about not accessing the packet
> data irrelevant.)
> 
> It might be better to extend rte_net_get_ptype() by adding checks for
> malformed packets, rather than introducing an almost similar function.
> 
> And then the bulk function could use rte_net_get_ptype().
> 
> >
> > Regards,
> > BL

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

[Morten Brørup]

Cutting in Olivier, requesting input as the maintainer of rte_net.

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> longtb5@viettel.com.vn
> 
> Hi Morten,
> 
> What is the difference compare to rte_net_get_ptype(), which also
> parses
> packet types and reports on header length.
> 
> In my application I have also done something similar about malformed
> packets. IMO it's very useful to have return value indicate different
> types
> of malformed packets, not just -1, e.g. invalid IP options, IP
> loopback,
> etc.

They are very similar. The main differences are:
- The header length fields are set in the MBUF, not returned in a separate structure. This would only be relevant for a bulk function.
- In theory, it would be possible to set the length fields without accessing the packet data, but just by looking at MBUF->packet_type for some packets; e.g. RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_UDP is quite common due to Google's QUIC protocol (and will be with the coming HTTP/3 protocol).
- Testing for malformed packets, e.g. a length field suggesting that the packet is longer than it actually is, or a header length field suggesting that the header is shorter than the header's structure. (This obviously requires accessing the packet data, which makes the above point about not accessing the packet data irrelevant.)

It might be better to extend rte_net_get_ptype() by adding checks for malformed packets, rather than introducing an almost similar function.

And then the bulk function could use rte_net_get_ptype().

> 
> Regards,
> BL

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

[longtb5]

Hi Morten,

What is the difference compare to rte_net_get_ptype(), which also parses
packet types and reports on header length.

In my application I have also done something similar about malformed
packets. IMO it's very useful to have return value indicate different types
of malformed packets, not just -1, e.g. invalid IP options, IP loopback,
etc. 

Regards,
BL