On Thu, Jan 07, 2016 at 10:24:19AM +0000, Ananyev, Konstantin wrote:
>
>
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> > Sent: Wednesday, January 06, 2016 5:23 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 04:44:43PM +0000, Ananyev, Konstantin wrote:
> > >
> > >
> > > > -----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.
> >
> > Well, yes, besides I overlooked ptypes[] itself is 32 bit, working around
> > the type width of the mask wouldn't help much.
> >
> > > > 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?
> >
> > No, that was just to illustrate my point.
> >
> > > > > > 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.
> >
> > Since this is already a slow manner for retrieving types, 10 or 100 doesn't
> > make much difference. Such a function shouldn't be used in the data path
> > directly.
>
> Yes, it is not supposed to be called from data-path.
>
> > My point is, since we're dealing with a slow function, let's keep its API as
> > simple as possible.
>
> Well, API should be simple, but from other side it has to be flexible and
> convenient
> for the user.
> As I user, I would prefer to have an ability to select the layers here -
> that's
> why I suggested to add the mask parameter.
>
> >By having a mask to match, a large number of checks are
> > added in all PMDs while they could just fill the array without
> > bothering.
>
> That's a valid point.
> We could move filter point into rte_ethdev layer.
> So PMD would always return an array of all supported ptypes, and
> then rte_ethdev layer will filter it based on mask parameter.
> Does it sound reasonable to you?
Yes, I think it's fine that way, thanks.
> >The filtering logic is an application requirement that could be
> > useful in its own function as well (converting any random value to its
> > related layer or mask).
> >
> > > > 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)) {...}
> >
> > All right, let's not use a different method to filter packet types.
> >
> > > > 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.
> >
> > No, really, I guess we'll extend ptype to 64 bit when necessary. My point on
> > filtering separately still stands.
> >
> > > Konstantin
> > >
> >
> > --
> > Adrien Mazarguil
> > 6WIND
--
Adrien Mazarguil
6WIND
