From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Subject: Re: [PATCH v4 0/2] ethdev: add port speed capability bitmap Date: Wed, 9 Sep 2015 15:10:37 +0200 Message-ID: <20150909131037.GA25122@autoinstall.dev.6wind.com> References: <20150908100309.GE10297@autoinstall.dev.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org To: Marc Sune , Thomas Monjalon Return-path: Received: from mail-wi0-f171.google.com (mail-wi0-f171.google.com [209.85.212.171]) by dpdk.org (Postfix) with ESMTP id 92E2B5683 for ; Wed, 9 Sep 2015 15:11:02 +0200 (CEST) Received: by wicgb1 with SMTP id gb1so115938754wic.1 for ; Wed, 09 Sep 2015 06:11:02 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20150908100309.GE10297@autoinstall.dev.6wind.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" Marc, (making this discussion public again) On Wed, Sep 09, 2015 at 12:07:01PM +0200, Marc Sune wrote: > Hi Nelio >=20 > 2015-09-09 11:08 GMT+02:00 N=E9lio Laranjeiro : >=20 > Marc, >=20 > On Tue, Sep 08, 2015 at 10:24:36PM +0200, Marc Sune wrote: > > Neilo, > > > > 2015-09-08 12:03 GMT+02:00 N=E9lio Laranjeiro : > > > >=A0 =A0 =A0On Mon, Sep 07, 2015 at 10:52:53PM +0200, Marc Sune wro= te: > >=A0 =A0 =A0> 2015-08-29 2:16 GMT+02:00 Marc Sune : > >=A0 =A0 =A0> > >=A0 =A0 =A0> > The current rte_eth_dev_info abstraction does not p= rovide any > mechanism > >=A0 =A0 =A0to > >=A0 =A0 =A0> > get the supported speed(s) of an ethdev. > >=A0 =A0 =A0> > > >=A0 =A0 =A0> > For some drivers (e.g. ixgbe), an educated guess ca= n be done > based on > >=A0 =A0 =A0the > >=A0 =A0 =A0> > driver's name (driver_name in rte_eth_dev_info), se= e: > >=A0 =A0 =A0> > > >=A0 =A0 =A0> > http://dpdk.org/ml/archives/dev/2013-August/000412.= html > >=A0 =A0 =A0> > > >=A0 =A0 =A0> > However, i) doing string comparisons is annoying, a= nd can > silently > >=A0 =A0 =A0> > break existing applications if PMDs change their na= mes ii) it > does not > >=A0 =A0 =A0> > provide all the supported capabilities of the ethde= v iii) for > some > >=A0 =A0 =A0drivers > >=A0 =A0 =A0> > it > >=A0 =A0 =A0> > is impossible determine correctly the (max) speed b= y the > application > >=A0 =A0 =A0> > (e.g. in i40, distinguish between XL710 and X710). > >=A0 =A0 =A0> > > >=A0 =A0 =A0> > This small patch adds speed_capa bitmap in rte_eth_= dev_info, > which is > >=A0 =A0 =A0> > filled > >=A0 =A0 =A0> > by the PMDs according to the physical device capabi= lities. > >=A0 =A0 =A0> > > >=A0 =A0 =A0> > v2: rebase, converted speed_capa into 32 bits bitma= p, fixed > alignment > >=A0 =A0 =A0> > (checkpatch). > >=A0 =A0 =A0> > > >=A0 =A0 =A0> > v3: rebase to v2.1. unified ETH_LINK_SPEED and ETH_= SPEED_CAP into > >=A0 =A0 =A0> > ETH_SPEED. > >=A0 =A0 =A0> >=A0 =A0 =A0Converted field speed in struct rte_eth_c= onf to speeds, to > allow a > >=A0 =A0 =A0> > bitmap > >=A0 =A0 =A0> >=A0 =A0 =A0for defining the announced speeds, as sug= gested by M. Brorup. > Fixed > >=A0 =A0 =A0> >=A0 =A0 =A0spelling issues. > >=A0 =A0 =A0> > > >=A0 =A0 =A0> > v4: fixed errata in the documentation of field spee= ds of > rte_eth_conf, > >=A0 =A0 =A0and > >=A0 =A0 =A0> >=A0 =A0 =A0commit 1/2 message. rebased to v2.1.0. v3= was incorrectly > based on > >=A0 =A0 =A0> >=A0 =A0 =A0~2.1.0-rc1. > >=A0 =A0 =A0> > > >=A0 =A0 =A0> > >=A0 =A0 =A0> Thomas, > >=A0 =A0 =A0> > >=A0 =A0 =A0> Since mostly you were commenting for v1 and v2; any o= pinion on this > one? > >=A0 =A0 =A0> > >=A0 =A0 =A0> Regards > >=A0 =A0 =A0> marc > > > >=A0 =A0 =A0Hi Marc, > > > >=A0 =A0 =A0I have read your patches, and there are a few mistakes,= for instance > mlx4 > >=A0 =A0 =A0(ConnectX-3 devices) does not support 100Gbps. > > > > > > When I circulated v1 and v2 I was kindly asking maintainers and r= eviewers > of > > the drivers to fix any mistakes in SPEED capabilities, since I wa= s taking > the > > speeds from the online websites&catalogues. Some were fixed, but > apparently > > some were still missing. I will remove 100Gbps. Please circulate = any > other > > error you have spotted. >=20 > From Mellanox website: > =A0- ConnectX-3 EN: 10/40/56Gb/s > =A0- ConnectX-3 Pro EN 10GBASE-T: 10G/s > =A0- ConnectX-3 Pro: EN 10/40/56GbE > =A0- ConnectX-3 Pro Programmable: 10/40Gb/s >=20 > This PMD works with any of the ConnectX-3 adapters, so the announce= speed > should be 10/40/56Gb/s. > =20 >=20 >=20 > I will change this > =A0 >=20 > >=A0 =A0 =A0In addition, it seems your new bitmap does not support = all kind of > >=A0 =A0 =A0speeds, take a look at the header of Ethtool, in the Li= nux kernel > >=A0 =A0 =A0(include/uapi/linux/ethtool.h) which already consumes 3= 0bits without > even > >=A0 =A0 =A0managing speeds above 56Gbps. > > > > > > The bitmaps you are referring is SUPPORTED_ and ADVERTISED_. Thes= e > bitmaps not > > only contain the speeds but PHY properties (e.g. BASE for ETH). > > > > The intention of this patch was to expose speed capabilities, sim= ilar to > the > > bitmap SPEED_ in include/uapi/linux/ethtool.h, which as you see m= aps > closely to > > ETH_SPEED_ proposed in this patch. > > > > I think the encoding of other things, like the exact model of the > interface and > > its PHY details should go somewhere else. But I might be wrong he= re, so > open to > > hear opinions. >=20 > I understand the need to have capability fields, but I don't unders= tand > why you want to mix speeds and duplex mode in something which was > previously only handling speeds. >=20 >=20 > Please refer to the comments from Thomas. He was arguing the duplicity = in > speeds between link and capabilities was not necessary, hence patch v3 = and 4 > are unifying. The reason why there is only 100 and 100_HD is because of= that > and the "solution" Thomas was proposing. > > I was originally doing as you suggested, separating them and not changi= ng > current APIs. There seems to be a consensus on that, so please reply ba= ck > directly to Thomas for this. Sorry, I did not follow the thread from the beginning, I only read from V4 patch. =20 > We now have redundant information in struct rte_eth_conf, whereas > that structure has a speed field which embeds the duplex mode and > a duplex field which does the same, which one should be used? >=20 >=20 > There is only the RTE_SPEED_ now, so speeds are also setting this (the = value > RTE_SPEED_XX). Old constants have been removed. It is not what I mean, inside the structure rte_eth_conf, there are two fields: * link_speeds, * link_duplex. =20 The link_speeds contains a bitmap of ETH_SPEED_XXX, ETH_SPEED_ already contains an information on the duplex mode, so the field "link_duplex" becomes redundant. > >=A0 =A0 =A0It would be nice to keep the field to represent the rea= l speed of the > >=A0 =A0 =A0link, in case it is not represented by the bitmap, it c= ould be also > >=A0 =A0 =A0useful for aggregated links (bonding for instance).=A0 = The current API > >=A0 =A0 =A0already works this way, it just needs to be extended fr= om 16 to 32 > bit > >=A0 =A0 =A0to manage speed above 64Gbps. > > > > > > This patch does not remove rte_eth_link_get() API. It just change= s the > encoding > > of speed in struct rte_eth_link, to have an homogeneous set of co= nstants > with > > the speed capabilities bitmap, as discussed previously in the thr= ead (see > > Thomas comments). IOW, it returns now a single SPEED_ value in th= e struct > > rte_eth_link's link_speed field. >=20 > You change the coding of the speed field, but applications still ex= pect > an integer, see port_infos_display function in app/test-pmd/config.= c which > directly uses printf on rte_eth_link.speed field, there are other p= laces > as well in PMDs (bn2x, bond, ...). >=20 >=20 > Agree. This has been overlooked, thanks. > =A0 > This patch currently expects that everything uses a bitmap but it i= s not > the case. >=20 > I don't understand the need to change the rte_eth_link.speed field > behavior to have the informations about the capability of the PHY, = for > this are two distinct things: > =A0 - capability > =A0 - speed and duplex negotiated (or not). >=20 > I suggest to drop the part of the patch which changes the behavior = of > link_speed in struct rte_eth_link. >=20 >=20 > So going back to v2? I finally took a look at your V2 patch and the discussion which follows between you and Thomas. Take a look at the function bandwidth_left in drivers/net/bonding/rte_eth_bond_pmd.c, it computes a bandwidth in bits/s from the link_speed for each port present in the bond, furthermore, it is computed each second in TLB or ALB mode. Same situation if we need to display the aggregated speed to the user, it is easier to sum the speed of each slave in the bond, instead of computing a value from a bitmap to sum it in an integer. I think V2 is better, maybe you can add a function to convert a single bitmap value to the equivalent integer and get rid of ETH_SPEED_XXX macro= s. Thomas what is your opinion? --=20 N=E9lio Laranjeiro 6WIND