From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 01/12] ethdev: add API to query what/if packet type is set Date: Wed, 6 Jan 2016 16:44:38 +0100 Message-ID: <20160106154438.GP12095@6wind.com> References: <1451544799-70776-1-git-send-email-jianfeng.tan@intel.com> <1451544799-70776-2-git-send-email-jianfeng.tan@intel.com> <20160104113814.GT3806@6wind.com> <2601191342CEEE43887BDE71AB97725836AE1002@irsmsx105.ger.corp.intel.com> <20160105161423.GE4712@autoinstall.dev.6wind.com> <2601191342CEEE43887BDE71AB97725836AE18E3@irsmsx105.ger.corp.intel.com> <20160106100053.GJ12095@6wind.com> <2601191342CEEE43887BDE71AB97725836AE1B46@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: "Ananyev, Konstantin" Return-path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 8956195BA for ; Wed, 6 Jan 2016 16:44:57 +0100 (CET) Received: by mail-wm0-f46.google.com with SMTP id b14so81289295wmb.1 for ; Wed, 06 Jan 2016 07:44:57 -0800 (PST) Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB97725836AE1B46@irsmsx105.ger.corp.intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jan 06, 2016 at 02:29:07PM +0000, Ananyev, Konstantin wrote: >=20 >=20 > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Wednesday, January 06, 2016 10:01 AM > > To: Ananyev, Konstantin > > Cc: N=C3=A9lio Laranjeiro; Tan, Jianfeng; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/i= f packet type is set > >=20 > > On Tue, Jan 05, 2016 at 04:50:31PM +0000, Ananyev, Konstantin wrote: > > [...] > > > > -----Original Message----- > > > > From: N=C3=A9lio Laranjeiro [mailto:nelio.laranjeiro@6wind.com] > > [...] > > > > I think we miss a comment here in how those 2/6/4 values are chos= en > > > > because, according to the mask, I expect 16 possibilities but I g= et > > > > 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 m= ask > > > > argument altogether and have rte_eth_dev_get_ptype_info() return = the > > > > entire list every time. Applications need to iterate on the resu= lt in > > > > any case. > > > > > > I think we'd better keep mask argument. > > > In many cases upper layer only interested in some particular subse= t of > > > all packet types that HW can recognise. > > > Let say l3fwd only cares about RTE_PTYPE_L3_MASK, it is not intere= sted in L4, > > > tunnelling packet types, etc. > > > If caller needs to know all recognised ptypes, he can set mask=3D=3D= -1, > > > In that case all supported packet types will be returned. > >=20 > > 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 represen= t 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. >=20 > 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.=20 > 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 functi= on 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 proo= f. Perhaps in this particular case there is no way to hit the limit (althoug= h 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. > > I think this use for masks should be avoided when performance does no= t > > 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. >=20 > User doesn't know numbers of entries in advance anyway (with and withou= t the mask). > That's why this function was introduced at first place. >=20 > With mask it just a bit more handy, in case user cares only about parti= cular subset of supported > packet types (only L2 let say).=20 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). >=20 > 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 conve= rt RTE_PTYPE_* values to a layer, so applications can look for interesting packet types while parsing plist[] on their own. 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 encapsulat= ion 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. >=20 > 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 ptyp= e - > there are few reserved values right now. =20 > if one day we'll run out bits in let say RTE_PTYPE_L2_MASK and will ha= ve to increase its size - > it would mean change of the packet_type layout and possible ABI breakag= e anyway.=20 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 no= t more) convenient. Perhaps some sort of tunneled packet types beyond inner L4 consuming the four remaining bits will be added? That could happen soon. > Konstantin >=20 > >=20 > > > > rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[], > > > > size_t max_entries) > > > > > > > > > > > > > > > > Another point, I have read the example patch (l3fwd) but I don't > > > > understand why the PMD is not responsible for filling the packet = type in > > > > the MBUF (packet parsing is done by parse_packet_type()). Why th= e extra > > > > computation? > > > > > > As I understand there are 3 possibilities now: > > > 1. HW supports ptype recognition and SW ptype parsing is never done > > > (--parse-ptype is not specified). > > > 2. HW supports ptype recognition, but and SW ptype parsing is done = anyway > > > (--parse-ptype is specified). > > > 3. HW doesn't support and ptype recognition, and and SW ptype parsi= ng is done > > > (--parse-ptype is specified). > > > > > > I suppose the question is what for introduce '--parse-ptype' at all= ? > > > My thought because of #2, so people can easily check what will be t= he performance impact of SW parsing. > > > > > > Konstantin > > > > > > > > > > > I see it more like an offload request (as checksum, etc...) and i= f the > > > > NIC does not support it then the application does the necessary o= verload. > > > > > > > > Best regards, > > > > > > > > -- > > > > N=C3=A9lio Laranjeiro > > > > 6WIND > >=20 > > -- > > Adrien Mazarguil > > 6WIND --=20 Adrien Mazarguil 6WIND