Xen devel - Nov 2011 - [PATCH] xen-netfront: report link speed to ethtool

Add .get_settings function, return fake data so that ethtool can get
enough information. For some application like VCS, this is useful,
otherwise some of application logic will get panic.
The reported data refers to VMWare vmxnet.

Signed-off-by: Xin Wei Hu <xwhu@suse.com>
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Signed-off-by: Olaf Hering <olaf@aepfle.de>

---
 drivers/net/xen-netfront.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Index: linux-3.2-rc2/drivers/net/xen-netfront.c
==================================================================---
linux-3.2-rc2.orig/drivers/net/xen-netfront.c
+++ linux-3.2-rc2/drivers/net/xen-netfront.c
@@ -1727,6 +1727,17 @@ static void netback_changed(struct xenbu
 	}
 }
 
+static int xennet_get_settings(struct net_device *netdev, struct ethtool_cmd
*ecmd)
+{
+	ecmd->supported = SUPPORTED_1000baseT_Full | SUPPORTED_TP;
+	ecmd->advertising = ADVERTISED_TP;
+	ecmd->port = PORT_TP;
+	ecmd->transceiver = XCVR_INTERNAL;
+	ecmd->speed = SPEED_1000;
+	ecmd->duplex = DUPLEX_FULL;
+	return 0;
+}
+
 static const struct xennet_stat {
 	char name[ETH_GSTRING_LEN];
 	u16 offset;
@@ -1774,6 +1785,7 @@ static const struct ethtool_ops xennet_e
 {
 	.get_link = ethtool_op_get_link,
 
+	.get_settings = xennet_get_settings,
 	.get_sset_count = xennet_get_sset_count,
 	.get_ethtool_stats = xennet_get_ethtool_stats,
 	.get_strings = xennet_get_strings,

On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> Add .get_settings function, return fake data so that ethtool can get
> enough information. For some application like VCS, this is useful,
> otherwise some of application logic will get panic.
> The reported data refers to VMWare vmxnet.
> 
> Signed-off-by: Xin Wei Hu <xwhu@suse.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
NAK, we should not just make things up.

Ben.
> ---
>  drivers/net/xen-netfront.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> Index: linux-3.2-rc2/drivers/net/xen-netfront.c
> ==================================================================> ---
linux-3.2-rc2.orig/drivers/net/xen-netfront.c
> +++ linux-3.2-rc2/drivers/net/xen-netfront.c
> @@ -1727,6 +1727,17 @@ static void netback_changed(struct xenbu
>  	}
>  }
>  
> +static int xennet_get_settings(struct net_device *netdev, struct
ethtool_cmd *ecmd)
> +{
> +	ecmd->supported = SUPPORTED_1000baseT_Full | SUPPORTED_TP;
> +	ecmd->advertising = ADVERTISED_TP;
> +	ecmd->port = PORT_TP;
> +	ecmd->transceiver = XCVR_INTERNAL;
> +	ecmd->speed = SPEED_1000;
> +	ecmd->duplex = DUPLEX_FULL;
> +	return 0;
> +}
> +
>  static const struct xennet_stat {
>  	char name[ETH_GSTRING_LEN];
>  	u16 offset;
> @@ -1774,6 +1785,7 @@ static const struct ethtool_ops xennet_e
>  {
>  	.get_link = ethtool_op_get_link,
>  
> +	.get_settings = xennet_get_settings,
>  	.get_sset_count = xennet_get_sset_count,
>  	.get_ethtool_stats = xennet_get_ethtool_stats,
>  	.get_strings = xennet_get_strings,
> --
> To unsubscribe from this list: send the line "unsubscribe netdev"
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that''s the marketing
department''s job.
They asked us to note that Solarflare product names are trademarked.

On Fri, Nov 18, Ben Hutchings wrote:
> On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> > The reported data refers to VMWare vmxnet.
> NAK, we should not just make things up.
So how about removing veth_get_settings, vmxnet3_get_settings,
tun_get_settings and other functions that escaped my grep?

Olaf

On 11/18/2011 09:46 AM, Ben Hutchings wrote:
> On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
>> Add .get_settings function, return fake data so that ethtool can get
>> enough information. For some application like VCS, this is useful,
>> otherwise some of application logic will get panic.
>> The reported data refers to VMWare vmxnet.
>>
>> Signed-off-by: Xin Wei Hu<xwhu@suse.com>
>> Signed-off-by: Chunyan Liu<cyliu@suse.com>
>> Signed-off-by: Olaf Hering<olaf@aepfle.de>
>
> NAK, we should not just make things up.
Which raises an interesting question for a virtual interface that isn''t
pretending to be a specific NIC type. What should the reported speed be? 
  Is it a 10/100 NIC?  A 1 or 10 GbE NIC? 3.14 GbE?  For other emulated 
interfaces, it rather falls-out from the emulation.  We can say that the 
driver may not make stuff up, but it would seem what is running in the 
host/hypervisor/dom0/whatever will have to.  It could I suppose, decide 
based on the physical NIC to which it is attached, so long as folks 
using the virtual NIC don''t expect its attributes to be the same from 
system to system.

rick

On 11/18/2011 10:44 AM, Rick Jones wrote:
>  It could I suppose, decide 
> based on the physical NIC to which it is attached, so long as folks 
> using the virtual NIC don''t expect its attributes to be the same
from
> system to system.
And assuming there''s a physical NIC at all.

    J

On 11/18/2011 10:46 AM, Jeremy Fitzhardinge wrote:
> On 11/18/2011 10:44 AM, Rick Jones wrote:
>>   It could I suppose, decide
>> based on the physical NIC to which it is attached, so long as folks
>> using the virtual NIC don''t expect its attributes to be the
same from
>> system to system.
>
> And assuming there''s a physical NIC at all.
It sounds like we need a way to specify "Indeterminate" for link
speed?
  Or some verbiage to that effect. Right now 0 and -1 cause ethtool to 
report "Unknown!"

         if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
                 fprintf(stdout, "Unknown!\n");
         else
                 fprintf(stdout, "%uMb/s\n", speed);


How about -2 for the u32 cast value of speed returning "Indeterminate"
or something like that?  Not in "proper" patch format:

	if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
		fprintf(stdout, "Unknown!\n");
	else if (speed == (u32)(-2))
		fprintf(stdout, "Indeterminate.");
	else
		fprintf(stdout, "%uMb/s\n", speed);

Signed-off-by: Rick Jones <rick.jones2@hp.com>	

rick jones

On Fri, 2011-11-18 at 19:43 +0100, Olaf Hering wrote:
> On Fri, Nov 18, Ben Hutchings wrote:
> 
> > On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> > > The reported data refers to VMWare vmxnet.
> > NAK, we should not just make things up.
> 
> So how about removing veth_get_settings, vmxnet3_get_settings,
> tun_get_settings and other functions that escaped my grep?
If they can''t provide meaningful information then maybe they should be
removed.  However, that could result in a regression for existing
working configurations.  (This isn''t the same as the case
you''re trying
to fix, since those applications have never worked with xen-netfront or
many other drivers that don''t implement get_settings.)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that''s the marketing
department''s job.
They asked us to note that Solarflare product names are trademarked.

On Fri 18. of November 2011 19:44:08 you wrote:
> On 11/18/2011 09:46 AM, Ben Hutchings wrote:
> > On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> >> Add .get_settings function, return fake data so that ethtool can
get
> >> enough information. For some application like VCS, this is useful,
> >> otherwise some of application logic will get panic.
> >> The reported data refers to VMWare vmxnet.
> >> 
> >> Signed-off-by: Xin Wei Hu<xwhu@suse.com>
> >> Signed-off-by: Chunyan Liu<cyliu@suse.com>
> >> Signed-off-by: Olaf Hering<olaf@aepfle.de>
> > 
> > NAK, we should not just make things up.
> 
> Which raises an interesting question for a virtual interface that
isn''t
> pretending to be a specific NIC type. What should the reported speed be?
>   Is it a 10/100 NIC?  A 1 or 10 GbE NIC? 3.14 GbE?  For other emulated
> interfaces, it rather falls-out from the emulation.  We can say that the
> driver may not make stuff up, but it would seem what is running in the
> host/hypervisor/dom0/whatever will have to.  It could I suppose, decide
> based on the physical NIC to which it is attached, so long as folks
> using the virtual NIC don''t expect its attributes to be the same
from
> system to system.
> 
> rick
Hmm,
two questions from me.
I think there is 10GbE link reported in Windows gplpv 0.11.0.322 driver.
Let say I have two virtual Windows running on XEN and they are in bridge with 
1GbE physical NIC.
Will the Windows talk to each other on 10GbE thru the bridge?
What should I do to give them 10GbE access to local samba server?
-- 
Pavel Mateja

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 18 Nov 2011 17:46:34 +0000
> On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
>> Add .get_settings function, return fake data so that ethtool can get
>> enough information. For some application like VCS, this is useful,
>> otherwise some of application logic will get panic.
>> The reported data refers to VMWare vmxnet.
>> 
>> Signed-off-by: Xin Wei Hu <xwhu@suse.com>
>> Signed-off-by: Chunyan Liu <cyliu@suse.com>
>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> NAK, we should not just make things up.
Agreed, if you cannot determine the values with certainty do not
implement this method.

Fix the tools which cannot function without this information.

On Fri, 2011-11-18 at 10:58 -0800, Rick Jones wrote:
> On 11/18/2011 10:46 AM, Jeremy Fitzhardinge wrote:
> > On 11/18/2011 10:44 AM, Rick Jones wrote:
> >>   It could I suppose, decide
> >> based on the physical NIC to which it is attached, so long as
folks
> >> using the virtual NIC don''t expect its attributes to be
the same from
> >> system to system.
> >
> > And assuming there''s a physical NIC at all.
> 
> It sounds like we need a way to specify "Indeterminate" for link
speed?
>   Or some verbiage to that effect. Right now 0 and -1 cause ethtool to 
> report "Unknown!"
> 
>          if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
>                  fprintf(stdout, "Unknown!\n");
>          else
>                  fprintf(stdout, "%uMb/s\n", speed);
> 
> 
> How about -2 for the u32 cast value of speed returning
"Indeterminate"
> or something like that?  Not in "proper" patch format:
> 
> 	if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
> 		fprintf(stdout, "Unknown!\n");
> 	else if (speed == (u32)(-2))
> 		fprintf(stdout, "Indeterminate.");
> 	else
> 		fprintf(stdout, "%uMb/s\n", speed);
I''m open to something like this, but the problem with assigning new
magic numbers is that older versions of ethtool won''t know to report
them as special.

We should also consider stacked drivers like bonding (and presumably
team) that expect real numbers when the link is up.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that''s the marketing
department''s job.
They asked us to note that Solarflare product names are trademarked.

On Fri, Nov 18, Ben Hutchings wrote:
> On Fri, 2011-11-18 at 19:43 +0100, Olaf Hering wrote:
> > On Fri, Nov 18, Ben Hutchings wrote:
> > 
> > > On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> > > > The reported data refers to VMWare vmxnet.
> > > NAK, we should not just make things up.
> > 
> > So how about removing veth_get_settings, vmxnet3_get_settings,
> > tun_get_settings and other functions that escaped my grep?
> 
> If they can''t provide meaningful information then maybe they
should be
> removed.  However, that could result in a regression for existing
> working configurations.  (This isn''t the same as the case
you''re trying
> to fix, since those applications have never worked with xen-netfront or
> many other drivers that don''t implement get_settings.)
That may be.
How about a new generic ethtool_op_get_settings_veth which returns fake
values for all relevant drivers (virtio, xen-netfront, and the ones
listed above)?

Olaf

On Fri, Nov 18, 2011 at 08:11:31PM +0100, Pavel Mat??ja
wrote:
> On Fri 18. of November 2011 19:44:08 you wrote:
> > On 11/18/2011 09:46 AM, Ben Hutchings wrote:
> > > On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> > >> Add .get_settings function, return fake data so that ethtool
can get
> > >> enough information. For some application like VCS, this is
useful,
> > >> otherwise some of application logic will get panic.
> > >> The reported data refers to VMWare vmxnet.
> > >> 
> > >> Signed-off-by: Xin Wei Hu<xwhu@suse.com>
> > >> Signed-off-by: Chunyan Liu<cyliu@suse.com>
> > >> Signed-off-by: Olaf Hering<olaf@aepfle.de>
> > > 
> > > NAK, we should not just make things up.
> > 
> > Which raises an interesting question for a virtual interface that
isn''t
> > pretending to be a specific NIC type. What should the reported speed
be?
> >   Is it a 10/100 NIC?  A 1 or 10 GbE NIC? 3.14 GbE?  For other
emulated
> > interfaces, it rather falls-out from the emulation.  We can say that
the
> > driver may not make stuff up, but it would seem what is running in the
> > host/hypervisor/dom0/whatever will have to.  It could I suppose,
decide
> > based on the physical NIC to which it is attached, so long as folks
> > using the virtual NIC don''t expect its attributes to be the
same from
> > system to system.
> > 
> > rick
> 
> Hmm,
> two questions from me.
> I think there is 10GbE link reported in Windows gplpv 0.11.0.322 driver.
> Let say I have two virtual Windows running on XEN and they are in bridge
with
> 1GbE physical NIC.
> Will the Windows talk to each other on 10GbE thru the bridge?
> What should I do to give them 10GbE access to local samba server?
>
The reported NIC speed in the VM does not mean anything really,
because it''s all virtual "hardware", and the reported speed
is not any kind of limit.

Even if windows VM says it''s 10 Mbit/sec NIC you can still talk
as fast as your system can go.

-- Pasi

> The reported NIC speed in the VM does not mean anything really, because
> it''s all virtual "hardware", and the reported speed is
not any kind of limit.
> 
> Even if windows VM says it''s 10 Mbit/sec NIC you can still talk as
fast as your
> system can go.
> 
That''s not quite true. Windows or some application might equate 10Mbit
== WAN and do things a bit differently. BITS certainly scales back throughput
but I think it does that based on actual tested throughput, although it might
use the link speed as an indication.

A HVM domain needs to report something because it is emulated hardware. A way to
pass that through from Dom0 would be nice, but probably a lot of work for little
gain vs just making up a reasonable figure.

James

> Hmm,
> two questions from me.
> I think there is 10GbE link reported in Windows gplpv 0.11.0.322 driver.
> Let say I have two virtual Windows running on XEN and they are in bridge
> with 1GbE physical NIC.
> Will the Windows talk to each other on 10GbE thru the bridge?
> What should I do to give them 10GbE access to local samba server?
I''ve never had GPLPV talking at 10Gb, but it can easily do 2-4Gb to
Dom0 or another DomU, so reporting a 1Gb figure is aiming a little low. Windows
basically demands that you give a value, and Hyper-V reports 10Gb so I do too.

James