[PATCH RFC 0/2] ethtool: Add actual port speed reporting

[PATCH RFC 0/2] ethtool: Add actual port speed reporting

[Gal Pressman]

Sending RFC to get feedback for the following ethtool proposal:

In some cases such as virtual machines and multi functions (SR-IOV), the actual
bandwidth exposed for each machine is not accurately shown in ethtool.
Currently ethtool shows only physical port link speed.
In our case we would like to show the virtual port operational link speed
which in some cases is less than the physical port speed.

This will give users better visibility for the actual speed running on their device.

$ ethtool ens6
...
Speed: 50000Mb/s
Actual speed: 25000Mb/s

Gal Pressman (2):
  ethtool: Add get actual port speed support
  net/mlx5e: Add support for ethtool get actual speed callback

 drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c |  7 +++++++
 include/linux/ethtool.h                              |  1 +
 include/uapi/linux/ethtool.h                         |  2 ++
 net/core/ethtool.c                                   | 20 ++++++++++++++++++++
 4 files changed, 30 insertions(+)

-- 
2.7.4

[PATCH RFC 1/2] ethtool: Add get actual port speed

[Gal Pressman]

Add an additional actual speed field when using ethtool DEVNAME.
Actual speed will show the actual bandwidth exposed for the machine,
which may be different from the HCA operating speed.

Signed-off-by: Gal Pressman <galp@mellanox.com>
---
 include/linux/ethtool.h      |  1 +
 include/uapi/linux/ethtool.h |  2 ++
 net/core/ethtool.c           | 20 ++++++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9ded8c6..215baa1 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -311,6 +311,7 @@ struct ethtool_ops {
 	void	(*set_msglevel)(struct net_device *, u32);
 	int	(*nway_reset)(struct net_device *);
 	u32	(*get_link)(struct net_device *);
+	int	(*get_actual_speed)(struct net_device *);
 	int	(*get_eeprom_len)(struct net_device *);
 	int	(*get_eeprom)(struct net_device *,
 			      struct ethtool_eeprom *, u8 *);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 099a420..63057a2 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1315,6 +1315,8 @@ struct ethtool_per_queue_op {
 #define ETHTOOL_GLINKSETTINGS	0x0000004c /* Get ethtool_link_settings */
 #define ETHTOOL_SLINKSETTINGS	0x0000004d /* Set ethtool_link_settings */
 
+#define ETHTOOL_GASPD		0x0000004e /* Get port actual speed */
+
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 9774898..a1921fd 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1516,6 +1516,22 @@ static int ethtool_get_link(struct net_device *dev, char __user *useraddr)
 	return 0;
 }
 
+static int ethtool_get_actual_speed(struct net_device *dev,
+				    char __user *useraddr)
+{
+	struct ethtool_value edata = { .cmd = ETHTOOL_GASPD };
+
+	if (!dev->ethtool_ops->get_actual_speed)
+		return -EOPNOTSUPP;
+
+	edata.data = dev->ethtool_ops->get_actual_speed(dev);
+
+	if (copy_to_user(useraddr, &edata, sizeof(edata)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int ethtool_get_any_eeprom(struct net_device *dev, void __user *useraddr,
 				  int (*getter)(struct net_device *,
 						struct ethtool_eeprom *, u8 *),
@@ -2450,6 +2466,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GDRVINFO:
 	case ETHTOOL_GMSGLVL:
 	case ETHTOOL_GLINK:
+	case ETHTOOL_GASPD:
 	case ETHTOOL_GCOALESCE:
 	case ETHTOOL_GRINGPARAM:
 	case ETHTOOL_GPAUSEPARAM:
@@ -2531,6 +2548,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GLINK:
 		rc = ethtool_get_link(dev, useraddr);
 		break;
+	case ETHTOOL_GASPD:
+		rc = ethtool_get_actual_speed(dev, useraddr);
+		break;
 	case ETHTOOL_GEEPROM:
 		rc = ethtool_get_eeprom(dev, useraddr);
 		break;
-- 
2.7.4

[PATCH RFC 2/2] net/mlx5e: Add support for ethtool get actual speed callback

[Gal Pressman]

ethtool DEVNAME will now show actual port speed in addition
to physical port speed.

Signed-off-by: Gal Pressman <galp@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 27ff401..933b687 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1504,9 +1504,16 @@ static int mlx5e_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 	return err;
 }
 
+static int mlx5e_get_actual_speed(struct net_device *netdev)
+{
+	/* TODO: FW command to query the actual speed */
+	return SPEED_25000;
+}
+
 const struct ethtool_ops mlx5e_ethtool_ops = {
 	.get_drvinfo       = mlx5e_get_drvinfo,
 	.get_link          = ethtool_op_get_link,
+	.get_actual_speed  = mlx5e_get_actual_speed,
 	.get_strings       = mlx5e_get_strings,
 	.get_sset_count    = mlx5e_get_sset_count,
 	.get_ethtool_stats = mlx5e_get_ethtool_stats,
-- 
2.7.4

RE: [PATCH RFC 0/2] ethtool: Add actual port speed reporting

[Mintz, Yuval]

> Sending RFC to get feedback for the following ethtool proposal:
> 
> In some cases such as virtual machines and multi functions (SR-IOV), the actual
> bandwidth exposed for each machine is not accurately shown in ethtool.
> Currently ethtool shows only physical port link speed.
> In our case we would like to show the virtual port operational link speed which
> in some cases is less than the physical port speed.
> 
> This will give users better visibility for the actual speed running on their device.
> 
> $ ethtool ens6
> ...
> Speed: 50000Mb/s
> Actual speed: 25000Mb/s

Not saying this is a bad thing, but where exactly is it listed that ethtool has
to show the physical port speed?
E.g., bnx2x shows the logical speed instead, and has been doing that for years.
[Perhaps that's a past wrongness, but that's how it goes].

And besides, one can argue that in the SR-IOV scenario the VF has no business
knowing the physical port speed.

Re: [PATCH RFC 0/2] ethtool: Add actual port speed reporting

[Or Gerlitz]

On Wed, Nov 2, 2016 at 5:35 PM, Gal Pressman <galp@mellanox.com> wrote:

> In some cases such as virtual machines and multi functions (SR-IOV), the actual
> bandwidth exposed for each machine is not accurately shown in ethtool.

You mean that if you rate-limit a VF from the host they will be able
to actually query that
and report it to their OS?

Re: [PATCH RFC 0/2] ethtool: Add actual port speed reporting

[Gal Pressman]

On 02/11/2016 17:50, Mintz, Yuval wrote:
>> Sending RFC to get feedback for the following ethtool proposal:
>>
>> In some cases such as virtual machines and multi functions (SR-IOV), the actual
>> bandwidth exposed for each machine is not accurately shown in ethtool.
>> Currently ethtool shows only physical port link speed.
>> In our case we would like to show the virtual port operational link speed which
>> in some cases is less than the physical port speed.
>>
>> This will give users better visibility for the actual speed running on their device.
>>
>> $ ethtool ens6
>> ...
>> Speed: 50000Mb/s
>> Actual speed: 25000Mb/s
> 
> Not saying this is a bad thing, but where exactly is it listed that ethtool has
> to show the physical port speed?
> E.g., bnx2x shows the logical speed instead, and has been doing that for years.
> [Perhaps that's a past wrongness, but that's how it goes].
> 
> And besides, one can argue that in the SR-IOV scenario the VF has no business
> knowing the physical port speed.
> 

Good point, but there are more use-cases we should consider.
For example, when using Multi-Host/Flex-10/Multi-PF each PF should
be able to query both physical port speed and actual speed.

Re: [PATCH RFC 0/2] ethtool: Add actual port speed reporting

[Rick Jones]

>> And besides, one can argue that in the SR-IOV scenario the VF has no business
>> knowing the physical port speed.
>>
>
> Good point, but there are more use-cases we should consider.
> For example, when using Multi-Host/Flex-10/Multi-PF each PF should
> be able to query both physical port speed and actual speed.

Despite my email address, I'm not fully versed on VC/Flex, but I have 
always been under the impression that the flexnics created were, 
conceptually, "distinct" NICs considered independently of the physical 
port over which they operated.  Tossing another worm or three into the 
can, while "back in the day" (when some of the first ethtool changes to 
report speeds other than the "normal" ones went in) the speed of a 
flexnic was fixed, today, it can actually operate in a range.  From a 
minimum guarantee to an "if there is bandwidth available" cap.

rick jones

Re: [PATCH RFC 0/2] ethtool: Add actual port speed reporting

[Saeed Mahameed]

On Wed, Nov 2, 2016 at 5:50 PM, Mintz, Yuval <Yuval.Mintz@cavium.com> wrote:
>> Sending RFC to get feedback for the following ethtool proposal:
>>
>> In some cases such as virtual machines and multi functions (SR-IOV), the actual
>> bandwidth exposed for each machine is not accurately shown in ethtool.
>> Currently ethtool shows only physical port link speed.
>> In our case we would like to show the virtual port operational link speed which
>> in some cases is less than the physical port speed.
>>
>> This will give users better visibility for the actual speed running on their device.
>>
>> $ ethtool ens6
>> ...
>> Speed: 50000Mb/s
>> Actual speed: 25000Mb/s
>
> Not saying this is a bad thing, but where exactly is it listed that ethtool has
> to show the physical port speed?

Well, looking at the ethtool fields you can clearly see those fields
refer only to physical properties of port connector module.
from this you can conclude that the speed field refers to the physical
port speed.

Settings for ens1f0:
Supported ports: [ FIBRE Backplane ]
Supported link modes:   1000baseKX/Full
                       10000baseKR/Full
                       40000baseKR4/Full
                       40000baseCR4/Full
                       40000baseSR4/Full
                       40000baseLR4/Full
                       25000baseCR/Full
                       25000baseKR/Full
                       25000baseSR/Full
                       50000baseCR2/Full
                       50000baseKR2/Full
                       100000baseKR4/Full
                       100000baseSR4/Full
                       100000baseCR4/Full
                       100000baseLR4_ER4/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Advertised link modes:  1000baseKX/Full
                       10000baseKR/Full
                       40000baseKR4/Full
                       40000baseCR4/Full
                       40000baseSR4/Full
                       40000baseLR4/Full
                       25000baseCR/Full
                       25000baseKR/Full
                       25000baseSR/Full
                       50000baseCR2/Full
                       50000baseKR2/Full
                       100000baseKR4/Full
                       100000baseSR4/Full
                       100000baseCR4/Full
                       100000baseLR4_ER4/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Speed: 100000Mb/s
Duplex: Full
Port: Direct Attach Copper
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
Supports Wake-on: d
Wake-on: d
Link detected: yes

> E.g., bnx2x shows the logical speed instead, and has been doing that for years.
> [Perhaps that's a past wrongness, but that's how it goes].
>
> And besides, one can argue that in the SR-IOV scenario the VF has no business
> knowing the physical port speed.

Yes for SR-IOV VFs one field (logical) is sufficient.
But in some cases on a native system (no SR-IOV nor virtualization)
there will be a need for both physical and logical speed reporting.
logical speed can be limited for several reasons (NIC Low power mode,
pci (width,gen), Internal HCA rate limiters, etc ... ).

Such information will be more than useful for system administrators
and will not be available if we decide to show only one field.

-Saeed.