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

[Marc Sune <marcdevel@gmail.com>]

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.



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


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

Marc


>
> >[...]
>
> Nélio
> --
> Nélio Laranjeiro
> 6WIND
>