diff mbox series

[net-next] net: hyperv: Add attributes to show RX/TX indirection table

Message ID HK0P153MB027502644323A21B09F6DA60987C0@HK0P153MB0275.APCP153.PROD.OUTLOOK.COM
State Changes Requested
Delegated to: David Miller
Headers show
Series [net-next] net: hyperv: Add attributes to show RX/TX indirection table | expand

Commit Message

Chi Song July 17, 2020, 6:04 a.m. UTC 
The network is observed with low performance, if TX indirection table is imbalance.
But the table is in memory and set in runtime, it's hard to know. Add them to attributes can help on troubleshooting.
---
 drivers/net/hyperv/netvsc_drv.c | 46 +++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Wei Liu July 17, 2020, 10:43 a.m. UTC | #1 
Hi Chi

Thanks for your patch. A few things need to be fixed before it can be
accepted upstream.

On Fri, Jul 17, 2020 at 06:04:31AM +0000, Chi Song wrote:
> The network is observed with low performance, if TX indirection table
> is imbalance.  But the table is in memory and set in runtime, it's
> hard to know. Add them to attributes can help on troubleshooting.

Missing Signed-off-by here. I assume you wrote this patch so please add

    Signed-off-by: Chi Song <song.chi@microsoft.com>

If there are other authors, please add their SoBs too.

Please wrap the commit message to around 72 to 80 columns wide.

I notice you only talked about TX table but in the code your also added
support for RX table.

I would also suggest changing the commit message a bit to:

    An imbalanced TX indirection table causes netvsc to have low
    performance. This table is created and managed during runtime. To help
    better diagnose performance issues caused by imbalanced tables, add
    device attributes to show the content of TX and RX indirection tables.

Perhaps RX table causes low performance as well? If so, the above
message needs further adjustment to account for that too.

I will leave reviewing the code to Haiyang and Stephen.

Wei.


> ---
>  drivers/net/hyperv/netvsc_drv.c | 46 +++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 6267f706e8ee..cd6fe96e10c1 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -2370,6 +2370,51 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
>  	return NOTIFY_OK;
>  }
>  
> +static ssize_t tx_indirection_table_show(struct device *dev,
> +					 struct device_attribute *dev_attr,
> +					 char *buf)
> +{
> +	struct net_device *ndev = to_net_dev(dev);
> +	struct net_device_context *ndc = netdev_priv(ndev);
> +	int i = 0;
> +	ssize_t offset = 0;
> +
> +	for (i = 0; i < VRSS_SEND_TAB_SIZE; i++)
> +		offset += sprintf(buf + offset, "%u ", ndc->tx_table[i]);
> +	buf[offset - 1] = '\n';
> +
> +	return offset;
> +}
> +static DEVICE_ATTR_RO(tx_indirection_table);
> +
> +static ssize_t rx_indirection_table_show(struct device *dev,
> +					 struct device_attribute *dev_attr,
> +					 char *buf)
> +{
> +	struct net_device *ndev = to_net_dev(dev);
> +	struct net_device_context *ndc = netdev_priv(ndev);
> +	int i = 0;
> +	ssize_t offset = 0;
> +
> +	for (i = 0; i < ITAB_NUM; i++)
> +		offset += sprintf(buf + offset, "%u ", ndc->rx_table[i]);
> +	buf[offset - 1] = '\n';
> +
> +	return offset;
> +}
> +static DEVICE_ATTR_RO(rx_indirection_table);
> +
> +static struct attribute *netvsc_dev_attrs[] = {
> +	&dev_attr_tx_indirection_table.attr,
> +	&dev_attr_rx_indirection_table.attr,
> +	NULL
> +};
> +
> +const struct attribute_group netvsc_dev_group = {
> +	.name = NULL,
> +	.attrs = netvsc_dev_attrs,
> +};
> +
>  static int netvsc_probe(struct hv_device *dev,
>  			const struct hv_vmbus_device_id *dev_id)
>  {
> @@ -2410,6 +2455,7 @@ static int netvsc_probe(struct hv_device *dev,
>  
>  	net->netdev_ops = &device_ops;
>  	net->ethtool_ops = &ethtool_ops;
> +	net->sysfs_groups[0] = &netvsc_dev_group;
>  	SET_NETDEV_DEV(net, &dev->device);
>  
>  	/* We always need headroom for rndis header */
> -- 
> 2.25.1
Stephen Hemminger July 17, 2020, 3:24 p.m. UTC | #2 
On Fri, 17 Jul 2020 06:04:31 +0000
Chi Song <Song.Chi@microsoft.com> wrote:

> The network is observed with low performance, if TX indirection table is imbalance.
> But the table is in memory and set in runtime, it's hard to know. Add them to attributes can help on troubleshooting.


The receive indirection table comes from RSS configuration.
The RSS configuration is already visible via ethtool so adding sysfs support
for that is redundant.

The transmit indirection table comes from the host, and is unique
to this driver. So adding a sysfs file for that makes sense.

The format of sysfs files is that in general there should be
one value per file.

One other possibility would be to make these as attributes under each
queues. But that is harder.
Haiyang Zhang July 17, 2020, 4:18 p.m. UTC | #3 
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, July 17, 2020 11:25 AM
> To: Chi Song <Song.Chi@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> Wei Liu <wei.liu@kernel.org>; David S. Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; Alexei Starovoitov <ast@kernel.org>; Daniel
> Borkmann <daniel@iogearbox.net>; Martin KaFai Lau <kafai@fb.com>; Song
> Liu <songliubraving@fb.com>; Yonghong Song <yhs@fb.com>; Andrii Nakryiko
> <andriin@fb.com>; John Fastabend <john.fastabend@gmail.com>; KP Singh
> <kpsingh@chromium.org>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next] net: hyperv: Add attributes to show RX/TX
> indirection table
> 
> On Fri, 17 Jul 2020 06:04:31 +0000
> Chi Song <Song.Chi@microsoft.com> wrote:
> 
> > The network is observed with low performance, if TX indirection table is
> imbalance.
> > But the table is in memory and set in runtime, it's hard to know. Add them to
> attributes can help on troubleshooting.
> 
> 
> The receive indirection table comes from RSS configuration.
> The RSS configuration is already visible via ethtool so adding sysfs support for
> that is redundant.
> 
> The transmit indirection table comes from the host, and is unique to this driver.
> So adding a sysfs file for that makes sense.
> 
> The format of sysfs files is that in general there should be one value per file.
> 
> One other possibility would be to make these as attributes under each queues.
> But that is harder.

The vmbus has per channel sysfs entries, but the channels are numbered by 
Rel_ID globally (not the subchannel index). Also the TX table is a many to one
mapping from table index to channel index, ie. Multiple table entries map to 
one channel. So display the TX indirection table entries under each channel 
will requires additional steps to figure out the actual table contents.
In sysfs, most files are short. But there are existing examples, such as " uevent" 
contains multiple values, even name and value pairs.

We knew the ethtool can output the RX table. But for Azure telemetry tools, 
it prefers to access the info from sysfs. Also in some minimal installation, 
"ethtool" may not always be installed. It will be more reliable for Azure 
telemetry tools to have the data in sysfs as well.

Thanks,
- Haiyang
David Miller July 17, 2020, 4:55 p.m. UTC | #4 
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Fri, 17 Jul 2020 16:18:11 +0000

> Also in some minimal installation, "ethtool" may not always be
> installed.

This is never an argument against using the most well suited API for
exporting information to the user.

You can write "minimal" tools that just perform the ethtool netlink
operations you require for information retrieval, you don't have to
have the ethtool utility installed.
Haiyang Zhang July 17, 2020, 4:59 p.m. UTC | #5 
> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Friday, July 17, 2020 12:56 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: stephen@networkplumber.org; Chi Song <Song.Chi@microsoft.com>; KY
> Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; wei.liu@kernel.org; kuba@kernel.org;
> ast@kernel.org; daniel@iogearbox.net; kafai@fb.com; songliubraving@fb.com;
> yhs@fb.com; andriin@fb.com; john.fastabend@gmail.com;
> kpsingh@chromium.org; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next] net: hyperv: Add attributes to show RX/TX
> indirection table
> 
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Fri, 17 Jul 2020 16:18:11 +0000
> 
> > Also in some minimal installation, "ethtool" may not always be
> > installed.
> 
> This is never an argument against using the most well suited API for
> exporting information to the user.
> 
> You can write "minimal" tools that just perform the ethtool netlink
> operations you require for information retrieval, you don't have to
> have the ethtool utility installed.

This option is being considered by us as well. 

Thanks,
- Haiyang
Stephen Hemminger July 17, 2020, 5:15 p.m. UTC | #6 
On Fri, 17 Jul 2020 09:55:35 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Fri, 17 Jul 2020 16:18:11 +0000
> 
> > Also in some minimal installation, "ethtool" may not always be
> > installed.  
> 
> This is never an argument against using the most well suited API for
> exporting information to the user.
> 
> You can write "minimal" tools that just perform the ethtool netlink
> operations you require for information retrieval, you don't have to
> have the ethtool utility installed.

Would it be better in the long term to make the transmit indirection
table available under the new rt_netlink based API's for ethtool?

I can imagine that other hardware or hypervisors might have the
same kind of transmit mapping.

Alternatively, the hyperv network driver could integrate/replace the
indirection table with something based on current receive flow steering.
Haiyang Zhang July 17, 2020, 5:33 p.m. UTC | #7 
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, July 17, 2020 1:15 PM
> To: David Miller <davem@davemloft.net>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; Chi Song
> <Song.Chi@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; wei.liu@kernel.org;
> kuba@kernel.org; ast@kernel.org; daniel@iogearbox.net; kafai@fb.com;
> songliubraving@fb.com; yhs@fb.com; andriin@fb.com;
> john.fastabend@gmail.com; kpsingh@chromium.org; linux-
> hyperv@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next] net: hyperv: Add attributes to show RX/TX
> indirection table
> 
> On Fri, 17 Jul 2020 09:55:35 -0700 (PDT) David Miller <davem@davemloft.net>
> wrote:
> 
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> > Date: Fri, 17 Jul 2020 16:18:11 +0000
> >
> > > Also in some minimal installation, "ethtool" may not always be
> > > installed.
> >
> > This is never an argument against using the most well suited API for
> > exporting information to the user.
> >
> > You can write "minimal" tools that just perform the ethtool netlink
> > operations you require for information retrieval, you don't have to
> > have the ethtool utility installed.
> 
> Would it be better in the long term to make the transmit indirection table
> available under the new rt_netlink based API's for ethtool?
> 
> I can imagine that other hardware or hypervisors might have the same kind of
> transmit mapping.

I think it should be a good long term plan, if going forward more NIC 
drivers start to use TX table in the future.

Thanks,
- Haiyang
diff mbox series

Patch

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 6267f706e8ee..cd6fe96e10c1 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2370,6 +2370,51 @@  static int netvsc_unregister_vf(struct net_device *vf_netdev)
 	return NOTIFY_OK;
 }
 
+static ssize_t tx_indirection_table_show(struct device *dev,
+					 struct device_attribute *dev_attr,
+					 char *buf)
+{
+	struct net_device *ndev = to_net_dev(dev);
+	struct net_device_context *ndc = netdev_priv(ndev);
+	int i = 0;
+	ssize_t offset = 0;
+
+	for (i = 0; i < VRSS_SEND_TAB_SIZE; i++)
+		offset += sprintf(buf + offset, "%u ", ndc->tx_table[i]);
+	buf[offset - 1] = '\n';
+
+	return offset;
+}
+static DEVICE_ATTR_RO(tx_indirection_table);
+
+static ssize_t rx_indirection_table_show(struct device *dev,
+					 struct device_attribute *dev_attr,
+					 char *buf)
+{
+	struct net_device *ndev = to_net_dev(dev);
+	struct net_device_context *ndc = netdev_priv(ndev);
+	int i = 0;
+	ssize_t offset = 0;
+
+	for (i = 0; i < ITAB_NUM; i++)
+		offset += sprintf(buf + offset, "%u ", ndc->rx_table[i]);
+	buf[offset - 1] = '\n';
+
+	return offset;
+}
+static DEVICE_ATTR_RO(rx_indirection_table);
+
+static struct attribute *netvsc_dev_attrs[] = {
+	&dev_attr_tx_indirection_table.attr,
+	&dev_attr_rx_indirection_table.attr,
+	NULL
+};
+
+const struct attribute_group netvsc_dev_group = {
+	.name = NULL,
+	.attrs = netvsc_dev_attrs,
+};
+
 static int netvsc_probe(struct hv_device *dev,
 			const struct hv_vmbus_device_id *dev_id)
 {
@@ -2410,6 +2455,7 @@  static int netvsc_probe(struct hv_device *dev,
 
 	net->netdev_ops = &device_ops;
 	net->ethtool_ops = &ethtool_ops;
+	net->sysfs_groups[0] = &netvsc_dev_group;
 	SET_NETDEV_DEV(net, &dev->device);
 
 	/* We always need headroom for rndis header */