From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition :content-transfer-encoding:in-reply-to; bh=m0qd2cS6A8tpqjyDQpniiUFqFBLQ6/VnMVOC1Yt5g7Y=; b=MUlWhFCpt8g7xz2BM1VvK6HQL0Fu7kPI8i0Fy8R2Y1fqnfx64eoE9xwo3gQY4nuFTv 6AS85DzLQRAarTY2U/ZDv06fWwP4AjuH/uMYaAYawOhv7ldxeUII5dwSMDsaAvwBmXPm RDnhjPH4I8AYbZj50jC2iTMQTj0pkyv4gkySI2Nk8SZc2acJrloYAXqZTsym/un7knd7 zxdtdZdZK+3BopM0gbxUbMaeFhVesw8zLNc9Ss5xKau9FkYn3nGMNxMKdtUZcQgYLwRZ NXrg0OxPjj0dZX2Gc3YdCDWz1bSPtoV3phHVLNJzujXNzN9ShAv1z0PNFSsa0fr6aAqN 2C6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-type :content-disposition:content-transfer-encoding:in-reply-to; bh=m0qd2cS6A8tpqjyDQpniiUFqFBLQ6/VnMVOC1Yt5g7Y=; b=F++21J/ZeGVFPuExTV3my242n62SAdy32WXsfJ6E07QpCxNEkGsq+TFD25H5ccVXxK If+aBvv1Qk45OusV1AVy/P/y6t6Bn10vLU28sLt4QsJg2AmoJpdZiY5sr4RKlIZkRF+c 14mQXNWi0PQiFcPxgpr71uk46D6RfHB+TjeR5S3cLpD44dA0boQgeH65tzFuNq0VyYNH glbFOlzXuAfX3d175KqkMuASx3W50hxBkm5PV97ePNUdHM9LClcDgiDQmweqqeanPDaQ uGWMk95MTFT01xHflXOrs0hKnwsSyhkcSNGUn7pKeLlkDE9lIZmEN6GMc+nH+EY3LGP7 Qaaw== X-Gm-Message-State: ALoCoQkf39qT6AXsMspNRHzUv8XImvjY/0JuxrcaSt62yJ+MsWDGSzdFcjxZ5QqntFHyWA5X1Xt8WaAHVeBJ+MvpLRrXONoNJg== X-Received: by 10.28.141.10 with SMTP id p10mr10498503wmd.83.1452095097353; Wed, 06 Jan 2016 07:44:57 -0800 (PST) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id m16sm9295041wmb.13.2016.01.06.07.44.55 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 06 Jan 2016 07:44:56 -0800 (PST) Date: Wed, 6 Jan 2016 16:44:38 +0100 From: Adrien Mazarguil To: "Ananyev, Konstantin" Message-ID: <20160106154438.GP12095@6wind.com> Mail-Followup-To: "Ananyev, Konstantin" , =?utf-8?B?TsOpbGlv?= Laranjeiro , "Tan, Jianfeng" , "dev@dpdk.org" 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-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2601191342CEEE43887BDE71AB97725836AE1B46@irsmsx105.ger.corp.intel.com> Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jan 2016 15:44:57 -0000 On Wed, Jan 06, 2016 at 02:29:07PM +0000, Ananyev, Konstantin wrote: > > > > -----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: > > [...] > > > > -----Original Message----- > > > > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@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. 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. > > 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. 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. Perhaps some sort of tunneled packet types beyond inner L4 consuming the four remaining bits will be added? That could happen soon. > Konstantin > > > > > > > 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 the 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 parsing 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 the performance impact of SW parsing. > > > > > > Konstantin > > > > > > > > > > > I see it more like an offload request (as checksum, etc...) and if the > > > > NIC does not support it then the application does the necessary overload. > > > > > > > > Best regards, > > > > > > > > -- > > > > Nélio Laranjeiro > > > > 6WIND > > > > -- > > Adrien Mazarguil > > 6WIND -- Adrien Mazarguil 6WIND