Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Marc,

(making this discussion public again)

On Wed, Sep 09, 2015 at 12:07:01PM +0200, Marc Sune wrote:
> Hi Nelio
> 
> 2015-09-09 11:08 GMT+02:00 Nélio Laranjeiro <nelio.laranjeiro@6wind.com>:
> 
>     Marc,
> 
>     On Tue, Sep 08, 2015 at 10:24:36PM +0200, Marc Sune wrote:
>     > Neilo,
>     >
>     > 2015-09-08 12:03 GMT+02:00 Nélio Laranjeiro <nelio.laranjeiro@6wind.com>:
>     >
>     >     On Mon, Sep 07, 2015 at 10:52:53PM +0200, Marc Sune wrote:
>     >     > 2015-08-29 2:16 GMT+02:00 Marc Sune <marcdevel@gmail.com>:
>     >     >
>     >     > > The current rte_eth_dev_info abstraction does not provide any
>     mechanism
>     >     to
>     >     > > get the supported speed(s) of an ethdev.
>     >     > >
>     >     > > For some drivers (e.g. ixgbe), an educated guess can be done
>     based on
>     >     the
>     >     > > driver's name (driver_name in rte_eth_dev_info), see:
>     >     > >
>     >     > > http://dpdk.org/ml/archives/dev/2013-August/000412.html
>     >     > >
>     >     > > However, i) doing string comparisons is annoying, and can
>     silently
>     >     > > break existing applications if PMDs change their names ii) it
>     does not
>     >     > > provide all the supported capabilities of the ethdev iii) for
>     some
>     >     drivers
>     >     > > it
>     >     > > is impossible determine correctly the (max) speed by the
>     application
>     >     > > (e.g. in i40, distinguish between XL710 and X710).
>     >     > >
>     >     > > This small patch adds speed_capa bitmap in rte_eth_dev_info,
>     which is
>     >     > > filled
>     >     > > by the PMDs according to the physical device capabilities.
>     >     > >
>     >     > > v2: rebase, converted speed_capa into 32 bits bitmap, fixed
>     alignment
>     >     > > (checkpatch).
>     >     > >
>     >     > > v3: rebase to v2.1. unified ETH_LINK_SPEED and ETH_SPEED_CAP into
>     >     > > ETH_SPEED.
>     >     > >     Converted field speed in struct rte_eth_conf to speeds, to
>     allow a
>     >     > > bitmap
>     >     > >     for defining the announced speeds, as suggested by M. Brorup.
>     Fixed
>     >     > >     spelling issues.
>     >     > >
>     >     > > v4: fixed errata in the documentation of field speeds of
>     rte_eth_conf,
>     >     and
>     >     > >     commit 1/2 message. rebased to v2.1.0. v3 was incorrectly
>     based on
>     >     > >     ~2.1.0-rc1.
>     >     > >
>     >     >
>     >     > Thomas,
>     >     >
>     >     > Since mostly you were commenting for v1 and v2; any opinion on this
>     one?
>     >     >
>     >     > Regards
>     >     > marc
>     >
>     >     Hi Marc,
>     >
>     >     I have read your patches, and there are a few mistakes, for instance
>     mlx4
>     >     (ConnectX-3 devices) does not support 100Gbps.
>     >
>     >
>     > When I circulated v1 and v2 I was kindly asking maintainers and reviewers
>     of
>     > the drivers to fix any mistakes in SPEED capabilities, since I was 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.
> 
>     From Mellanox website:
>      - ConnectX-3 EN: 10/40/56Gb/s
>      - ConnectX-3 Pro EN 10GBASE-T: 10G/s
>      - ConnectX-3 Pro: EN 10/40/56GbE
>      - ConnectX-3 Pro Programmable: 10/40Gb/s
> 
>     This PMD works with any of the ConnectX-3 adapters, so the announce speed
>     should be 10/40/56Gb/s.
>    
> 
> 
> I will change this
>  
> 
>     >     In addition, it seems your new bitmap does not support all kind of
>     >     speeds, take a look at the header of Ethtool, in the Linux kernel
>     >     (include/uapi/linux/ethtool.h) which already consumes 30bits without
>     even
>     >     managing speeds above 56Gbps.
>     >
>     >
>     > The bitmaps you are referring is SUPPORTED_ and ADVERTISED_. These
>     bitmaps not
>     > only contain the speeds but PHY properties (e.g. BASE for ETH).
>     >
>     > The intention of this patch was to expose speed capabilities, similar to
>     the
>     > bitmap SPEED_ in include/uapi/linux/ethtool.h, which as you see maps
>     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 here, so
>     open to
>     > hear opinions.
> 
>     I understand the need to have capability fields, but I don't understand
>     why you want to mix speeds and duplex mode in something which was
>     previously only handling speeds.
> 
> 
> 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 changing
> current APIs. There seems to be a consensus on that, so please reply back
> directly to Thomas for this.

Sorry, I did not follow the thread from the beginning, I only read from
V4 patch.
 
>     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?
> 
> 
> 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.
  
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.

>     >     It would be nice to keep the field to represent the real speed of the
>     >     link, in case it is not represented by the bitmap, it could be also
>     >     useful for aggregated links (bonding for instance).  The current API
>     >     already works this way, it just needs to be extended from 16 to 32
>     bit
>     >     to manage speed above 64Gbps.
>     >
>     >
>     > This patch does not remove rte_eth_link_get() API. It just changes the
>     encoding
>     > of speed in struct rte_eth_link, to have an homogeneous set of constants
>     with
>     > the speed capabilities bitmap, as discussed previously in the thread (see
>     > Thomas comments). IOW, it returns now a single SPEED_ value in the struct
>     > rte_eth_link's link_speed field.
> 
>     You change the coding of the speed field, but applications still expect
>     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 places
>     as well in PMDs (bn2x, bond, ...).
> 
> 
> Agree. This has been overlooked, thanks.
>  
>     This patch currently expects that everything uses a bitmap but it is not
>     the case.
> 
>     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:
>       - capability
>       - speed and duplex negotiated (or not).
> 
>     I suggest to drop the part of the patch which changes the behavior of
>     link_speed in struct rte_eth_link.
> 
> 
> 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 macros.

Thomas what is your opinion?

-- 
Nélio Laranjeiro
6WIND