[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set

Ananyev, Konstantin konstantin.ananyev at intel.com
Wed Jan 6 17:44:43 CET 2016



> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Wednesday, January 06, 2016 3:45 PM
> To: Ananyev, Konstantin
> Cc: Nélio Laranjeiro; Tan, Jianfeng; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> 
> On Wed, Jan 06, 2016 at 02:29:07PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> > > Sent: Wednesday, January 06, 2016 10:01 AM
> > > To: Ananyev, Konstantin
> > > Cc: Nélio Laranjeiro; Tan, Jianfeng; dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > >
> > > On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote:
> > > [...]
> > > > > -----Original Message-----
> > > > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com]
> > > [...]
> > > > > I think we miss a comment here in how those 2/6/4 values are chosen
> > > > > because, according to the mask, I expect 16 possibilities but I get
> > > > > less.  It will help a lot anyone who needs to add a new type.
> > > > >
> > > > > Extending the snprintf behavior above, it is best to remove the mask
> > > > > argument altogether and have rte_eth_dev_get_ptype_info() return the
> > > > > entire list every time.  Applications need to iterate on the result in
> > > > > any case.
> > > >
> > > > I think we'd better keep mask argument.
> > > > In many cases upper layer only interested in some particular  subset of
> > > > all packet types that HW can recognise.
> > > > Let say l3fwd only cares about  RTE_PTYPE_L3_MASK, it is not interested in L4,
> > > > tunnelling packet types, etc.
> > > > If caller needs to know all recognised ptypes, he can set mask==-1,
> > > > In that case all supported packet types will be returned.
> > >
> > > There are other drawbacks to the mask argument in my opinion. The API will
> > > have to be updated again as soon as 32 bits aren't enough to represent all
> > > possible masks. We can't predict it will be large enough forever but on the
> > > other hand, using uint64_t seems overkill at this point.
> >
> > Inside rte_mbuf packet_type itself is a 32 bit value.
> > These 32 bits are divided into several fields to mark packet types,
> > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc.
> > As long as packet_type itself is 32bits, 32bit mask is sufficient.
> > If we'll ever run out of 32 bits in packet_type itself, it will be ABI change anyway.
> 
> Sure, however why not do it now this issue has been raised so this function
> doesn't need updating the day it breaks? I know there's a million other
> places with a similar problem but I'm all for making new code future proof.

If rte_mbuf packet_type will have to be increased to 64bit long, then
this function will have to change anyway (with or without mask parameter).
It will have to become:

rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...)

So I think we don't have to worry about mask parameter itself.

> 
> Perhaps in this particular case there is no way to hit the limit (although
> there are only four unused bits left to extend RTE_PTYPE masks) but we've
> seen this happen too many times with subsequent ABI breakage.

When ptype was introduced we tried to reserve some free space for each layer (L2/L3/L4/...),
so it wouldn't be overrun immediately.
But of course if there would be a new HW that can recognise dozen new packet types - it is possible.
Do you have any particular use-case in mind? 

> 
> > > I think this use for masks should be avoided when performance does not
> > > matter much, as in this case, user application cannot know the number of
> > > entries in advance and must rely on the returned value to iterate.
> >
> > User doesn't know numbers of entries in advance anyway (with and without the mask).
> > That's why this function was introduced at first place.
> >
> > With mask it just a bit more handy, in case user cares only about particular subset of supported
> > packet types (only L2 let say).
> 
> OK, so we definitely need something to let applications know the layer a
> given packet type belongs to, I'm sure it can be done in a convenient way
> that won't be limited to the underlying type of the mask.
> 
> > > A helper function can be added to convert a RTE_PTYPE_* value to the layer
> > > it belongs to (using enum to define possible values).
> >
> > Not sure what for?
> 
> This is assuming rte_eth_dev_get_ptype_info() doesn't filter anything (no
> "mask" argument). In that case a separate function must be added to convert
> RTE_PTYPE_* values to a layer, so applications can look for interesting
> packet types while parsing plist[] on their own.

Honestly, I don't see why do you need that.
You already do know that  let say RTE_PTYPE_L3_IPV4 belongs to L3.
Why do you need some extra enum here?
From my thought - the only purpose of mask parameter was to limit number of elements in the ptypes[] at return.
So let say user would need to iterate over 10 elements, instead of 100 to find
the ones he is interested in.

> 
> This layer information could be defined as an enum, i.e.:
> 
>  enum rte_ptype_info {
>      RTE_PTYPE_UNKNOWN,
>      RTE_PTYPE_L2,
>      RTE_PTYPE_L3,
>     ...
>  };
> 
> Or even an int value (2 standing for for "layer 2" etc. Tunnel encapsulation
> wouldn't be described easily that way though). It's just an idea.
> 
> > > If we absolutely want a mean to filter returned values, I suggest we use
> > > this enum instead of the mask argument.
> > > Since it won't be a mask, it won't
> > > have to be updated every time a new protocol requires extending one.
> >
> > Number of bits PTYPE_L2/L3/L4,... layers are already defined.
> > So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype -
> > there are few reserved values right now.
> > if one day we'll run out bits in let say RTE_PTYPE_L2_MASK  and will have to increase its size -
> > it would mean change of the packet_type layout and possible ABI breakage anyway.
> 
> I'm aware of this, only pointing out we tend to rely on masks and type
> boundaries a bit too much when there are other methods that are as (if not
> more) convenient.

Yes, we do rely on masks in ptype.
That's how ptype was defined.
Let say to check that incoming packet is Ether/IPv4(no extentions)/UDP,
you probably would do:

if (mbuf->packet_type & (RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK | RTE_PTYPE_L4_MASK) ==
(RTE_PTYPE_L2_ETHER  | RTE_PTYPE_L3_IPV4 |  RTE_PTYPE_L4_UDP)) {...}

> 
> Perhaps some sort of tunneled packet types beyond inner L4 consuming the
> four remaining bits will be added? That could happen soon.

As I said above: do you have particular scenario in mind when 32bits for packet_type
might be not enough?
If yes, then it is probably a good idea to submit an RFC for extending it to 64 bit,
or introduce packet_type2, or whatever would be your preferred way to deal with it.

Konstantin



More information about the dev mailing list