On Thu, Jan 07, 2016 at 10:24:19AM +0000, Ananyev, Konstantin wrote:

quoted

-----Original Message-----
From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
Sent: Wednesday, January 06, 2016 5:23 PM
To: Ananyev, Konstantin
Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@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:

quoted

quoted

-----Original Message-----
From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
Sent: Wednesday, January 06, 2016 3:45 PM
To: Ananyev, Konstantin
Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@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:

quoted

quoted

-----Original Message-----
From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
Sent: Wednesday, January 06, 2016 10:01 AM
To: Ananyev, Konstantin
Cc: Nélio Laranjeiro; Tan, Jianfeng; dev@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:
[...]

quoted

quoted

-----Original Message-----
From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]

[...]

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

quoted

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.

quoted

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. 

quoted

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.

quoted

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

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

Konstantin

--
Adrien Mazarguil
6WIND

-- 
Adrien Mazarguil
6WIND