Re: [PATCH v4 0/2] ethdev: add port speed capability

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

bitmap
Reply-To: 

Shern <olgas@mellanox.com>, Adrien
        Mazarguil <adrien.mazarguil@6wind.com>
Bcc: 
Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap
Reply-To: 
In-Reply-To: <20150909090855.GC17463@autoinstall.dev.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.

>     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.

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? 

>     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, ...).

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.


PS: Sorry I did not sent the email as reply all.
-- 
Nélio Laranjeiro
6WIND