Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table
@ 2020-07-17  6:04 Chi Song
  2020-07-17 10:43 ` Wei Liu
  2020-07-17 15:24 ` Stephen Hemminger
  0 siblings, 2 replies; 8+ messages in thread
From: Chi Song @ 2020-07-17  6:04 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrii Nakryiko, John Fastabend, KP Singh
  Cc: linux-hyperv, netdev, linux-kernel, bpf

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(+)

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table
  2020-07-17  6:04 [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table Chi Song
@ 2020-07-17 10:43 ` Wei Liu
  2020-07-17 15:24 ` Stephen Hemminger
  1 sibling, 0 replies; 8+ messages in thread
From: Wei Liu @ 2020-07-17 10:43 UTC (permalink / raw)
  To: Chi Song
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrii Nakryiko, John Fastabend, KP Singh, linux-hyperv, netdev,
	linux-kernel, bpf

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table
  2020-07-17  6:04 [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table Chi Song
  2020-07-17 10:43 ` Wei Liu
@ 2020-07-17 15:24 ` Stephen Hemminger
  2020-07-17 16:18   ` Haiyang Zhang
  1 sibling, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2020-07-17 15:24 UTC (permalink / raw)
  To: Chi Song
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrii Nakryiko, John Fastabend, KP Singh, linux-hyperv, netdev

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table
  2020-07-17 15:24 ` Stephen Hemminger
@ 2020-07-17 16:18   ` Haiyang Zhang
  2020-07-17 16:55     ` David Miller
  0 siblings, 1 reply; 8+ messages in thread
From: Haiyang Zhang @ 2020-07-17 16:18 UTC (permalink / raw)
  To: Stephen Hemminger, Chi Song
  Cc: KY Srinivasan, Stephen Hemminger, Wei Liu, David S. Miller,
	Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, linux-hyperv, netdev



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table
  2020-07-17 16:18   ` Haiyang Zhang
@ 2020-07-17 16:55     ` David Miller
  2020-07-17 16:59       ` Haiyang Zhang
  2020-07-17 17:15       ` Stephen Hemminger
  0 siblings, 2 replies; 8+ messages in thread
From: David Miller @ 2020-07-17 16:55 UTC (permalink / raw)
  To: haiyangz
  Cc: stephen, Song.Chi, kys, sthemmin, wei.liu, kuba, ast, daniel,
	kafai, songliubraving, yhs, andriin, john.fastabend, kpsingh,
	linux-hyperv, netdev

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table
  2020-07-17 16:55     ` David Miller
@ 2020-07-17 16:59       ` Haiyang Zhang
  2020-07-17 17:15       ` Stephen Hemminger
  1 sibling, 0 replies; 8+ messages in thread
From: Haiyang Zhang @ 2020-07-17 16:59 UTC (permalink / raw)
  To: David Miller
  Cc: stephen, Chi Song, KY Srinivasan, Stephen Hemminger, wei.liu,
	kuba, ast, daniel, kafai, songliubraving, yhs, andriin,
	john.fastabend, kpsingh, linux-hyperv, netdev



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table
  2020-07-17 16:55     ` David Miller
  2020-07-17 16:59       ` Haiyang Zhang
@ 2020-07-17 17:15       ` Stephen Hemminger
  2020-07-17 17:33         ` Haiyang Zhang
  1 sibling, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2020-07-17 17:15 UTC (permalink / raw)
  To: David Miller
  Cc: haiyangz, Song.Chi, kys, sthemmin, wei.liu, kuba, ast, daniel,
	kafai, songliubraving, yhs, andriin, john.fastabend, kpsingh,
	linux-hyperv, netdev

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table
  2020-07-17 17:15       ` Stephen Hemminger
@ 2020-07-17 17:33         ` Haiyang Zhang
  0 siblings, 0 replies; 8+ messages in thread
From: Haiyang Zhang @ 2020-07-17 17:33 UTC (permalink / raw)
  To: Stephen Hemminger, David Miller
  Cc: Chi Song, KY Srinivasan, Stephen Hemminger, wei.liu, kuba, ast,
	daniel, kafai, songliubraving, yhs, andriin, john.fastabend,
	kpsingh, linux-hyperv, netdev



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17  6:04 [PATCH net-next] net: hyperv: Add attributes to show RX/TX indirection table Chi Song
2020-07-17 10:43 ` Wei Liu
2020-07-17 15:24 ` Stephen Hemminger
2020-07-17 16:18   ` Haiyang Zhang
2020-07-17 16:55     ` David Miller
2020-07-17 16:59       ` Haiyang Zhang
2020-07-17 17:15       ` Stephen Hemminger
2020-07-17 17:33         ` Haiyang Zhang

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git