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