From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-f45.google.com (mail-vk0-f45.google.com [209.85.213.45]) by dpdk.org (Postfix) with ESMTP id 1393D379E for ; Tue, 15 Sep 2015 23:20:05 +0200 (CEST) Received: by vkgd64 with SMTP id d64so89026047vkg.0 for ; Tue, 15 Sep 2015 14:20:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=lnH2AmjAelyqsBFyB4VYPXdazoHNtkcXf2AW9UjMPxs=; b=Z9Q+nLjIdrHLfXa1Vn7nG+P0f4zibF7+fpbXFxE3+moudyaM6AH/Re9TYXrKfQOz2m 425kemRHTAwQZwPg+qppyDXOdWdAf7zQvi9AwF2NRHRfqKG4tQMIb3YCMtEri1dEgo0C 8REBS1me7revKczBLXEP80ng0eGGHRDYitwOtFDHjBikN3v544LvlwZkjdg3Ck7GU4u5 NWBvqw85dHaQgdgyKlB5Sko28RIg1ZliBPJRAaCH9SmXeeWmq1B1LRXgRRFpUiJf2wLj +fFHzzMYLGe/yxD3RkTAAzOiJ2YLjmUpIwN86Gx3oyQrLE9iavkyUe6MqfwFSuEn1UxC Za3g== MIME-Version: 1.0 X-Received: by 10.31.3.133 with SMTP id f5mr23238445vki.91.1442352003431; Tue, 15 Sep 2015 14:20:03 -0700 (PDT) Received: by 10.31.218.5 with HTTP; Tue, 15 Sep 2015 14:20:03 -0700 (PDT) In-Reply-To: <20150915100443.GA4924@6wind.com> References: <20150909131037.GA25122@autoinstall.dev.6wind.com> <2699193.9riTyGPe1z@xps13> <2046894.c3eJ0QZGuc@xps13> <98CBD80474FA8B44BF855DF32C47DC358AF3B7@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC358AF3BD@smartserver.smartshare.dk> <20150915082544.GG25122@autoinstall.dev.6wind.com> <20150915100443.GA4924@6wind.com> Date: Tue, 15 Sep 2015 23:20:03 +0200 Message-ID: From: Marc Sune To: Marc Sune , =?UTF-8?Q?N=C3=A9lio_Laranjeiro?= , =?UTF-8?Q?Morten_Br=C3=B8rup?= , Thomas Monjalon , dev@dpdk.org, Olga Shern Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap 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: Tue, 15 Sep 2015 21:20:05 -0000 Adrien, 2015-09-15 12:04 GMT+02:00 Adrien Mazarguil : > Hi Marc, > > Adding my thoughts to the discussion, see below. > > On Tue, Sep 15, 2015 at 10:48:03AM +0200, Marc Sune wrote: > > I will answer Morten in another mail, because I got his point on the > > AUTONEG as a separate bit, and it _makes_ sense to me. > > > > But Neilo, > > > > 2015-09-15 10:25 GMT+02:00 N=C3=A9lio Laranjeiro >: > > > > > On Tue, Sep 15, 2015 at 12:50:11AM +0200, Morten Br=C3=B8rup wrote: > > > > Comments inline, marked MB>. > > > > > > > > Med venlig hilsen / kind regards > > > > - Morten Br=C3=B8rup > > > > > > > > Marc Sune on 14. september 2015 23:34 wrote: > > > > > > > > 2015-09-14 12:52 GMT+02:00 Morten Br=C3=B8rup : > > > > > It is important to consider that a multipath link (bonding etc.) = is > > > not a physical link, but a logical link (built on top of multiple > physical > > > links). Regardless whether it is a Layer2 link aggregate (IEEE 802.1a= d, > > > Ethernet bonding, EtherChannel, DSL pair bonding, etc.) or a Layer3 > > > multipath link (e.g. simultaneously using Wi-Fi and mobile networks). > So it > > > doesn't make sense trying to impose physical link properties on a > purely > > > logical link. Likewise, it doesn't make sense to impose logical link > > > properties on physical links. In other words: Don't consider bonding > or any > > > other logical link types when designing the PHY API. > > > > > > > > +1 > > > > > > +1. > > I agree with the fact that physical link properties do not make sense for > logical links, however in the case of the bonding PMD, the aggregated lin= k > speed can be actually useful for applications (assuming it is kept up to > date, I think it's the case). The current API certainly allows this. > > > > > > I think there is consensus that 1/ (PHY capabilities) and 2/ (PHY > > > advertisements) should use the same definitions, specifically a bitma= p > > > field. And when you disregard bonding, I don't see any reason to use > > > different definitions for 3/ (PHY negotiation result). This makes it > one > > > unified API for all three purposes. > > > > > > > > Agree. > > > > > > I don't agree with this one, some PMDs don't use the advertise of > > > autoneg result to get the speed or the duplex. You make a > > > generality from your case above all PMDs. > > > > > > > can you please explain how a particular PMD is recovering the actual li= nk > > speed and the duplex has to do with the design of the (general) API? > > It's not so much about the way PMDs recover link information, rather abou= t > the amount of changes required to switch to a bit-field API for the curre= nt > link speed with no clear advantage. See below; these are trivial changes. > All PMDs must be modified, the initial > set of patches isn't complete in this regard. > Thanks for pointing out this. There are a couple missing. > > > > Mellanox get the speed, duplex and status information from IOCTLs > > > which are not related to your bitmap. So at least for this PMD, ther= e > > > is already a conversion from 3 fields to a bitmap, knowing that it wi= ll > > > use the speed as an integer after. What is the benefit of your > solution? > > > > > > > I said already I don't have a strong preference for 3/. But steering th= e > > design of an API through a "minimum common denominator" principle is no= t > a > > good idea, specially since we are talking about a super simple mapping > > issue for this specific PMD. > > I think Nelio was using mlx4 as an example, all PMDs have their own > particular method to recover it and several must perform calculations to > get > the final value. Using integers for this task is certainly easier than > going > through bit-field conversions. > Drivers have their *own* way to extract the link speed from the HW, because the way is stored is anyway HW specific. That a driver encodes their link speed as a numeric value is just a coincidence, and the exception. Specifically, and putting an example of e1000 (which you are right it is buggy in v4, see below): dpdk/drivers/net/e1000/base/e1000_mac.c 1651 s32 e1000_get_speed_and_duplex_copper_generic(struct e1000_hw *hw, u16 *speed, 1652 u16 *duplex) 1653 { 1654 u32 status; 1655 1656 DEBUGFUNC("e1000_get_speed_and_duplex_copper_generic"); 1657 1658 status =3D E1000_READ_REG(hw, E1000_STATUS); 1659 if (status & E1000_STATUS_SPEED_1000) { 1660 *speed =3D SPEED_1000; 1661 DEBUGOUT("1000 Mbs, "); 1662 } else if (status & E1000_STATUS_SPEED_100) { 1663 *speed =3D SPEED_100; 1664 DEBUGOUT("100 Mbs, "); 1665 } else { 1666 *speed =3D SPEED_10; 1667 DEBUGOUT("10 Mbs, "); 1668 } 1669 1670 if (status & E1000_STATUS_FD) { 1671 *duplex =3D FULL_DUPLEX; 1672 DEBUGOUT("Full Duplex\n"); 1673 } else { 1674 *duplex =3D HALF_DUPLEX; 1675 DEBUGOUT("Half Duplex\n"); 1676 } 1677 1678 return E1000_SUCCESS; 1679 } Can you please tell me which exact extra conversions are needed on the PMD side? It only needs to be fixed: 1662 } else if (status & E1000_STATUS_SPEED_100) { 1663 *speed =3D ETH_LINK_SPEED_100; 1664 DEBUGOUT("100 Mbs, "); Other drivers, like i40 are already ooing it correctly: 1191 int 1192 i40e_dev_link_update(struct rte_eth_dev *dev, 1193 int wait_to_complete) 1194 { /* ... */ 1231 /* Parse the link status */ 1232 switch (link_status.link_speed) { 1233 case I40E_LINK_SPEED_100MB: 1234 link.link_speed =3D ETH_LINK_SPEED_100; 1235 break; 1236 case I40E_LINK_SPEED_1GB: 1237 link.link_speed =3D ETH_LINK_SPEED_1000; 1238 break; 1239 case I40E_LINK_SPEED_10GB: 1240 link.link_speed =3D ETH_LINK_SPEED_10G; 1241 break; 1242 case I40E_LINK_SPEED_20GB: 1243 link.link_speed =3D ETH_LINK_SPEED_20G; 1244 break; 1245 case I40E_LINK_SPEED_40GB: 1246 link.link_speed =3D ETH_LINK_SPEED_40G; 1247 break; 1248 default: 1249 link.link_speed =3D ETH_LINK_SPEED_100; 1250 break; 1251 } > > > > > Nelio suggested adding a support function to convert the bitmap > field > > > to a speed value as an integer. I strongly support this, because you > cannot > > > expect the bitmap to be ordered by speed. > > > > > > > > Agree with Nelio&you. This is useful. > > > > > > It was exactly the extreme opposite, a function which takes a > > > rte_eth_link to a bitmap i.e. speed_to_bm (rte_eth_link link) because= , > > > the speed is mostly used as an integer and not some kind of bitmap. > > > > > > > > This support function will be able to determine which speed is > higher > > > when exotic speeds are added to the bitmap. Please extend this > conversion > > > function to give three output parameters: speed, full/half duplex, au= to > > > negotiation/non-auto negotiation, or add two separate functions to ge= t > the > > > duplex and auto-negotiation. > > > > > > > > Since, Full/Half duplex is for legacy 10/100Mbps only (afaik), I > have my > > > doubts on using a bit for all speeds. I would suggest to define > (unroll) > > > 100M (or 100M_FD) and 100M_HD, and the same 10Mbps/1gbps, as Thomas w= as > > > suggesting some mails ago. > > > > > > > > This was done in v4 (implicitely 100M =3D=3D 100M_FD). See below. > > > > > > > > MB> I didn't intend two bits to be allocated in the bitmap for all > > > speeds to support full/half duplex, only for the relevant speeds. Sin= ce > > > full duplex is dominant, I agree with the previous decision (original= ly > > > suggested by Thomas, I think) to make full duplex implicit unless hal= f > > > duplex is explicitly specified. E.g. 10M_HD, 10M (alias 10M_FD), > 100M_HD, > > > 100M (alias 100M_FD), 1000M (or 1G), 2500M, 10G, 40G, 100G, etc. > > > > > > > > > > > > > I haven't read the suggested code, but there should be some means > in > > > 2/ (advertisements) to disable auto negotiation, e.g. a single bit in > the > > > bitmap to indicate if the speed/duplex-indicating bits in the bitmap > means > > > forced speed/duplex (in which case only a single speed/duplex-bit > should be > > > set) or auto negotiation advertised speed/duplex (in which case > multiple > > > speed/duplex-bits can be set). > > > > > > > > Agree. > > > > > > > > v3/4 of this patch adds the bitmap in the advertised, as per > discussed, > > > to select a group of speeds This is not implemented by drivers yet (!= ). > > > > > > > > So, as of v4 of this patch, there could be: a) autoneg any supporte= d > > > speed (=3D> bitmap =3D=3D 0) b) autoneg over group of speeds (=3D> mo= re than > one > > > bit set in the bitmap) c) forced speed (one and only one set in the > bitmap). > > > > > > > > I think this is precisely what you meant + b) as a bonus > > > > > > > > MB> This was not what I meant, but it wasn't very clearly written, = so > > > I'll try again: Add an additional single bit "NO_AUTONEG" (or whateve= r > you > > > want to name it) to the 2/ (advertisements) bitmap that explicitly > turns > > > off auto negotiation and tries to force the selected speed/duplex (i.= e. > > > only one other bit can be set in the bitmap when the NO_AUTONEG bit i= s > > > set). Your c) makes it impossible to use auto negotiation to advertis= e > a > > > specific speed/duplex, e.g. 100M_FD. My suggested NO_AUTONEG bit can > also > > > be used in 3/ (result) to indicate that the speed was a result of > Parallel > > > Detection, i.e. that auto negotiation failed or was disabled in eithe= r > end > > > of the link. > > > > > > > > MB> However, I like your suggestion a). > > > > > > > > > > > > > And some means in 3/ (result) and maybe 2/ (advertisements) to > select > > > and/or indicate physical interface in dual-personality ports (e.g. > ports > > > where the PHY has both an SFP and a RJ45 connector, but only one of > the two > > > can be used at any time). > > > > > > > > For rte_eth_link_get() I don't have such a strong opinion. You eith= er > > > > > > > > * encode the link speed and duplex as of now, separating duplex and > > > numeric speed. I would suggest to add the encoded speed+duplex bitmap > flag > > > for consistency (although redundant). > > > > * or you return a single value, the bitmap with a single flag set o= f > the > > > unrolled speeds, and then have the helpers int > rte_eth_speed_from_bm(int > > > val_bm) and bool rte_eth_duplex_from_bm(int val_bm). > > > > > > > > MB> I prefer the latter of the two, only because it makes 3/ (resul= t) > > > consistent with 1/ (capabilities) and 2/ (advertisements). > > > > > > So I agree for 1/ capabilities and 2/ advertisements. > > > > > > But, I don't agree to modify rte_eth_link_get API > > > (and rte_eth_link structure) thus 3/ result. > > > We don't need a "consistent" result, we need something usable. This = is > > > not the case of the bitmap and using some conversion functions. > > > Remember that the speed and duplex will not change until the next lin= k > > > down and there is a lot of code using speeds as integers. > > > Your solution will just increase the number of instruction to get the > > > same result, is that a benefit? > > > > > > > So do you think we should care about roughly 10cycles (at very most) > which > > is what a unique switch case mapping will take? We are not talking abou= t > > the dataplane here, Neilo. > > He wasn't arguing about the number of CPU cycles but the amount of code > required to go back and forth between actual link status speed and its > bit-field counterpart for no apparent benefit. > Already covered. > > > The benefit that Morten is arguing here is to have a unified API, > > consistent for the user. Once more, I don't have a preference, though I > see > > what he means. I am not sure if the bitmap for retrieving the link is > > really more usable than the current API, which is IMHO what should stee= r > > the discussion, not performance. > > Everyone agrees that a link speed bit-field is useful as an input value t= o > advertise, request and allow a set of speeds. We do not agree with its > usage > as the current link speed which is often the result of a computation. We > aren't talking about performance. > > A given link cannot be simultaneously at 10 Gbps and 1 Gbps right? Using = a > bit-field for the current link speed is confusing at best. Output values = do > not need to be included in the unified API, they are never converted back > into enum values. > Although I agree it is unlikely that this would happen, we shouldn't anticipate what users will do, so in either approach you would need utility functions to translate from numerical to bitmap and viceversa. > > I'm stressing again the fact that doing so would require a changes in all > applications that use the current speed and in PMDs for no good reason. Well any change in the API will. This patch (v1,v2) started as speed caps only, and now we are refactoring the link API. How much code has to be changed or how is easier for PMDs is irrelevant IMHO. What matters is if the API makes sense for the user. And for that you are probably right; it might be more comprehensible to have "a" speed value as a result of rte_eth_get_link(), provided that we give utility functions to go back and forth from numerical constants and bitmap constants (both have to be defined then in rte_eth.h). Marc > > Regards, > > -- > Adrien Mazarguil > 6WIND >