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

Marc Sune marcdevel at gmail.com
Wed Sep 9 14:45:21 CEST 2015


Answering

2015-09-09 11:29 GMT+02:00 Nélio Laranjeiro <nelio.laranjeiro at 6wind.com>:

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


>
> >     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 discussion in this thread for patches v1 and v2,
specially the comments by Thomas. He was arguing the duplicity in speeds
between link and capabilities was not necessary. Patches v3 and v4 try to
unify it. The reason why there is only 100 and 100_HD is because of the use
on both link and capabilities.

I was originally doing as you suggested, separating them and not changing
current APIs. There seemed to be a consensus on that, so let's just go back
to that discussion if needed.


>
> 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 ETH_SPEED_ now, so speeds are also setting this (the
value ETH_SPEED_XX). Old constants have been removed.


>
> >     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 rollback to v2?


Marc


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


More information about the dev mailing list