From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <marcdevel@gmail.com> Received: from mail-vk0-f44.google.com (mail-vk0-f44.google.com [209.85.213.44]) by dpdk.org (Postfix) with ESMTP id A156F919D for <dev@dpdk.org>; Tue, 15 Sep 2015 10:48:04 +0200 (CEST) Received: by vkgd64 with SMTP id d64so75176549vkg.0 for <dev@dpdk.org>; Tue, 15 Sep 2015 01:48:04 -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 :cc:content-type; bh=2qWjsJD+xMnxispJsYqNWKOQGiYoYMSc3/XmDLaT8r0=; b=IKdq6NhmIMriCMnoGAOfn6aX8Hf+cgUO5cP0Ps7Ocj4GkQPm4UKPqS0QAjbtbHFtYj gd9Mx2PxjlnVFhal/uFZEU43DnN70WYhb1V0e19/HXbrEpDdE803WLP87RpR2AA0Q3Sj AnuO7j4uMXS76UobECOoI1HxgaXoz8hDWW7w6/gMYjwQpiYJMqj2ND8ofB6nz3nEuApi xNXKhYVbLD7+SkOK/YCpeprdUyGzLFBygzi6j6N5mR1HPl8qCK6SqOePPgbwawBJgo4o ttcaUizWglbiQ25o6eJ/I+kzEfHzy6HrBsJPKZSBN8oTOKyamKejOcjUOS+sbEJFSLND 7Rpg== MIME-Version: 1.0 X-Received: by 10.31.3.133 with SMTP id f5mr18202935vki.91.1442306884038; Tue, 15 Sep 2015 01:48:04 -0700 (PDT) Received: by 10.31.218.5 with HTTP; Tue, 15 Sep 2015 01:48:03 -0700 (PDT) In-Reply-To: <20150915082544.GG25122@autoinstall.dev.6wind.com> References: <20150909131037.GA25122@autoinstall.dev.6wind.com> <2699193.9riTyGPe1z@xps13> <CA+3n-TorPf4Kp3YsA3TpDpJ2pyqwQ_x3OVjA_2jm9_-q_8+uFA@mail.gmail.com> <2046894.c3eJ0QZGuc@xps13> <98CBD80474FA8B44BF855DF32C47DC358AF3B7@smartserver.smartshare.dk> <CA+3n-TqTraUpWpRXN1RfZjijwDXJR0ovXkH8ZiH8PWOAzuYbEQ@mail.gmail.com> <98CBD80474FA8B44BF855DF32C47DC358AF3BD@smartserver.smartshare.dk> <20150915082544.GG25122@autoinstall.dev.6wind.com> Date: Tue, 15 Sep 2015 10:48:03 +0200 Message-ID: <CA+3n-TqmNBEF4FZVtJZA474iSyGKpDx3bqjuZJvqJrD4NpNM5g@mail.gmail.com> From: Marc Sune <marcdevel@gmail.com> To: =?UTF-8?Q?N=C3=A9lio_Laranjeiro?= <nelio.laranjeiro@6wind.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: dev@dpdk.org, =?UTF-8?Q?Morten_Br=C3=B8rup?= <mb@smartsharesystems.com> 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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Tue, 15 Sep 2015 08:48:05 -0000 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 <nelio.laranjeiro@6wind.co= m>: > 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 <marcdevel@gmail.com> on 14. september 2015 23:34 wrote: > > > > 2015-09-14 12:52 GMT+02:00 Morten Br=C3=B8rup <mb@smartsharesystems.com= >: > > > 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 physica= l > links). Regardless whether it is a Layer2 link aggregate (IEEE 802.1ad, > 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 a= ny > other logical link types when designing the PHY API. > > > > +1 > > +1. > > > > > > > > I think there is consensus that 1/ (PHY capabilities) and 2/ (PHY > advertisements) should use the same definitions, specifically a bitmap > 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 link speed and the duplex has to do with the design of the (general) API? > > Mellanox get the speed, duplex and status information from IOCTLs > which are not related to your bitmap. So at least for this PMD, there > is already a conversion from 3 fields to a bitmap, knowing that it will > 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 the design of an API through a "minimum common denominator" principle is not a good idea, specially since we are talking about a super simple mapping issue for this specific PMD. > > > > Nelio suggested adding a support function to convert the bitmap field > to a speed value as an integer. I strongly support this, because you cann= ot > 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, auto > negotiation/non-auto negotiation, or add two separate functions to get th= e > duplex and auto-negotiation. > > > > Since, Full/Half duplex is for legacy 10/100Mbps only (afaik), I have m= y > 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 was > 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. Since > full duplex is dominant, I agree with the previous decision (originally > suggested by Thomas, I think) to make full duplex implicit unless half > 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 mean= s > 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 supported > speed (=3D> bitmap =3D=3D 0) b) autoneg over group of speeds (=3D> more t= han one > bit set in the bitmap) c) forced speed (one and only one set in the bitma= p). > > > > 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 whatever yo= u > 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 is > set). Your c) makes it impossible to use auto negotiation to advertise 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 Paralle= l > Detection, i.e. that auto negotiation failed or was disabled in either en= d > 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 t= wo > can be used at any time). > > > > For rte_eth_link_get() I don't have such a strong opinion. You either > > > > * encode the link speed and duplex as of now, separating duplex and > numeric speed. I would suggest to add the encoded speed+duplex bitmap fla= g > for consistency (although redundant). > > * or you return a single value, the bitmap with a single flag set of th= e > 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/ (result) > 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 link > 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 about the dataplane here, Neilo. 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 steer the discussion, not performance. marc > > In addition, some PMDs need the speed to make some stuff with it, > so this structure will be replicated all over DPDK. > > -- > N=C3=A9lio Laranjeiro > 6WIND >