From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id A7C5795C6 for ; Thu, 7 Jan 2016 14:32:47 +0100 (CET) Received: by mail-wm0-f50.google.com with SMTP id b14so123471654wmb.1 for ; Thu, 07 Jan 2016 05:32:47 -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=uqPY5raxMTXsD8FRP00PMpDPXL9EaTe4bLYcyl62Ov0=; b=KsQxsJuwGiNCrS7zPKPTAZex3nJgdjQ3r+GAP/FZq9zJtxitrsFeWp6KNOoBsAxXRe EvVM1eA2MJIx6kAJlYZk13gkcOJnoN3yGuDsRKhSU+FDuIsBe9tkqnQ683xgmJYeBOoS 4Z9EBPVzh6w9hwR7NZIc/HeHJvWndnkN+GM25l+o3TeljPg3J8yZrXwP2suE/cldTISD uPh34oQHITIw+2+eSV0eUALfeDzwNBpNJJ67rQ03zWyjI6BiMd0zoJ0/xQoBGPeom+8i 5mZHjps0EwtpXO9fuKX735qsuDGtQ1iCcB/kbpnDeOYWiPizp1J2M74qeOjlu6rliYp7 33pg== 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=uqPY5raxMTXsD8FRP00PMpDPXL9EaTe4bLYcyl62Ov0=; b=E6npVZbzsDFjQA+SjW+hxQymsbp55QowPOLaFEPGwCSkwH5iJNccyE6qDTskeTLjcQ y/dPayNrOo/jDaRUnbe87VyXWOTooVPDTA2XkleRjM+z+SRZrgK0v7eiQQ3MypLuVu5p xq4wZAOyd+l8gW+9atfsX9ShkxWmL1teMkj8VG7qgAF1AzoSPFsZ3Vk62jfynBA4FeRa qEhJ3Lju5qTgILm7QGG63J+zgEQuF3ZgudWzB7h9OwXpMYSjDc3nyw2HDZpv8iq7NZSj q0C65as1WTf8Eh7QDcrljwEZKuRbpmZnNDwsU5mJzwcw71dLhHwJdKlCbAZ9cQoZcA8i 4TzA== X-Gm-Message-State: ALoCoQn4Xo2qey4jih/95/ICvu2EgRip8fAkNC47C/EfleXHQLrvZVMfMfBbA75IIOwgDVZpxxWz3iz7SxNQFz/uu+vwA4VdpA== X-Received: by 10.28.180.10 with SMTP id d10mr17704725wmf.14.1452173567499; Thu, 07 Jan 2016 05:32:47 -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 g187sm13676031wmf.8.2016.01.07.05.32.45 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 07 Jan 2016 05:32:46 -0800 (PST) Date: Thu, 7 Jan 2016 14:32:27 +0100 From: Adrien Mazarguil To: "Ananyev, Konstantin" Message-ID: <20160107133227.GU12095@6wind.com> Mail-Followup-To: "Ananyev, Konstantin" , =?utf-8?B?TsOpbGlv?= Laranjeiro , "Tan, Jianfeng" , "dev@dpdk.org" References: <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> <20160106154438.GP12095@6wind.com> <2601191342CEEE43887BDE71AB97725836AE2DBC@irsmsx105.ger.corp.intel.com> <20160106172248.GT12095@6wind.com> <2601191342CEEE43887BDE71AB97725836AE2F5B@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: <2601191342CEEE43887BDE71AB97725836AE2F5B@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: Thu, 07 Jan 2016 13:32:47 -0000 On Thu, Jan 07, 2016 at 10:24:19AM +0000, Ananyev, Konstantin wrote: > > > > -----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: > > > > > > > > > > -----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: > > > > > > > > > > > > > > > > -----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. > > > > > > 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