ethdev: add port speed capability

PATCH Nélio Laranjeiro 2 participants 2 messages Sep 9, 2015

Share & tools

Nélio Laranjeiro

author

2015-09-09 09:29 UTC #0 lore raw

bitmap
Reply-To:

Shern <[email protected]>, Adrien
Mazarguil <[email protected]>
Bcc:
Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap Reply-To:
In-Reply-To: <[email protected]>

Marc,

Quoted 58 lines · Marc Sune

Neilo,
2015-09-08 12:03 GMT+02:00 Nélio Laranjeiro <[email protected]>:
On Mon, Sep 07, 2015 at 10:52:53PM +0200, Marc Sune wrote:
> 2015-08-29 2:16 GMT+02:00 Marc Sune <[email protected]>:
>
> > 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.

Quoted 7 lines

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.

Quoted 9 lines

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

Quoted 7 lines

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?

Quoted 7 lines

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.

Quoted 5 lines

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.

Signature · 2 lines

Nélio Laranjeiro
6WIND

Marc Sune

2015-09-09 12:45 UTC #1 lore raw

Answering

2015-09-09 11:29 GMT+02:00 Nélio Laranjeiro <[email protected]>:

Quoted 97 lines

bitmap
Reply-To:
Shern <[email protected]>, Adrien
Mazarguil <[email protected]>
Bcc:
Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap
Reply-To:
In-Reply-To: <[email protected]>
Marc,
Quoted 9 lines · Marc Sune
Neilo,
2015-09-08 12:03 GMT+02:00 Nélio Laranjeiro <[email protected] :
    On Mon, Sep 07, 2015 at 10:52:53PM +0200, Marc Sune wrote:
    > 2015-08-29 2:16 GMT+02:00 Marc Sune <[email protected]>:
    >
    > > The current rte_eth_dev_info abstraction does not provide any
mechanism
Quoted 4 lines
to
> > get the supported speed(s) of an ethdev.
> >
> > For some drivers (e.g. ixgbe), an educated guess can be done
based on
Quoted 6 lines
    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
Quoted 1 line
Quoted 1 line
Quoted 1 line
break existing applications if PMDs change their names ii) it
does not
Quoted 1 line
Quoted 1 line
Quoted 1 line
provide all the supported capabilities of the ethdev iii) for
some
Quoted 3 lines
drivers
> > it
> > is impossible determine correctly the (max) speed by the
application
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(e.g. in i40, distinguish between XL710 and X710).
This small patch adds speed_capa bitmap in rte_eth_dev_info,
which is
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
filled
by the PMDs according to the physical device capabilities.
v2: rebase, converted speed_capa into 32 bits bitmap, fixed
alignment
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
(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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
bitmap
for defining the announced speeds, as suggested by M.
Brorup. Fixed
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
spelling issues.
v4: fixed errata in the documentation of field speeds of
rte_eth_conf,
Quoted 2 lines
and
> > commit 1/2 message. rebased to v2.1.0. v3 was incorrectly
based on
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
    ~2.1.0-rc1.
Thomas,
Since mostly you were commenting for v1 and v2; any opinion on
this one?
Quoted 7 lines
Quoted 3 lines
Regards
marc
Hi Marc,
I have read your patches, and there are a few mistakes, for instance
mlx4
Quoted 4 lines
(ConnectX-3 devices) does not support 100Gbps.
When I circulated v1 and v2 I was kindly asking maintainers and
reviewers of
Quoted 1 line
the drivers to fix any mistakes in SPEED capabilities, since I was
taking the
Quoted 1 line
speeds from the online websites&catalogues. Some were fixed, but
apparently
Quoted 1 line
some were still missing. I will remove 100Gbps. Please circulate any
other
Quoted 1 line
error you have spotted.

Quoted 9 lines

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.

Quoted 19 lines

Quoted 4 lines
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
Quoted 4 lines
managing speeds above 56Gbps.
The bitmaps you are referring is SUPPORTED_ and ADVERTISED_. These
bitmaps not
Quoted 3 lines
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
Quoted 1 line
bitmap SPEED_ in include/uapi/linux/ethtool.h, which as you see maps
closely to
Quoted 3 lines
ETH_SPEED_ proposed in this patch.
I think the encoding of other things, like the exact model of the

Quoted 9 lines

interface and
Quoted 1 line
its PHY details should go somewhere else. But I might be wrong here, so
open to
Quoted 1 line
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.

Quoted 5 lines

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.

Quoted 13 lines

Quoted 5 lines
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
Quoted 4 lines
to manage speed above 64Gbps.
This patch does not remove rte_eth_link_get() API. It just changes the
encoding
Quoted 1 line
of speed in struct rte_eth_link, to have an homogeneous set of constants
with

Quoted 9 lines

Quoted 3 lines
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.

Quoted 4 lines

This patch currently expects that everything uses a bitmap but it is not the case.

Quoted 9 lines

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

Quoted 7 lines

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