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

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

[Nélio Laranjeiro]

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

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

[Marc Sune]

Answering

2015-09-09 11:29 GMT+02:00 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.
>

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
>