| Message ID | 1723654753-27893-1-git-send-email-ernis@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: netvsc: Update default VMBus channels | expand |
> -----Original Message----- > From: Erni Sri Satya Vennela <ernis@linux.microsoft.com> > Sent: Wednesday, August 14, 2024 12:59 PM > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui > <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; linux-hyperv@vger.kernel.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Erni Sri Satya Vennela <ernis@microsoft.com>; Erni Sri Satya Vennela > <ernis@linux.microsoft.com> > Subject: [PATCH v2] net: netvsc: Update default VMBus channels > > Change VMBus channels macro (VRSS_CHANNEL_DEFAULT) in > Linux netvsc from 8 to 16 to align with Azure Windows VM > and improve networking throughput. > > For VMs having less than 16 vCPUS, the channels depend > on number of vCPUs. Between 16 to 32 vCPUs, the channels > default to VRSS_CHANNEL_DEFAULT. For greater than 32 vCPUs, > set the channels to number of physical cores / 2 as a way > to optimize CPU resource utilization and scale for high-end > processors with many cores. > Maximum number of channels are by default set to 64. > > Based on this change the subchannel creation would change as follows: > > ------------------------------------------------------------- > |No. of vCPU |dev_info->num_chn |subchannel created | > ------------------------------------------------------------- > | 0-16 | 16 | vCPU | > | >16 & <=32 | 16 | 16 | > | >32 & <=128 | vCPU/2 | vCPU/2 | > | >128 | vCPU/2 | 64 | > ------------------------------------------------------------- > > Performance tests showed significant improvement in throughput: > - 0.54% for 16 vCPUs > - 0.83% for 32 vCPUs > - 1.76% for 48 vCPUs > - 10.35% for 64 vCPUs > - 13.47% for 96 vCPUs > > Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> Thanks.
From: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Sent: Wednesday, August 14, 2024 9:59 AM > > Change VMBus channels macro (VRSS_CHANNEL_DEFAULT) in > Linux netvsc from 8 to 16 to align with Azure Windows VM > and improve networking throughput. > > For VMs having less than 16 vCPUS, the channels depend > on number of vCPUs. Between 16 to 32 vCPUs, the channels > default to VRSS_CHANNEL_DEFAULT. For greater than 32 vCPUs, > set the channels to number of physical cores / 2 as a way > to optimize CPU resource utilization and scale for high-end > processors with many cores. > Maximum number of channels are by default set to 64. Where in the code is this enforced? It's not part of this patch. It might be in rndis_set_subchannel(), where a value larger than 64 could be sent to the Hyper-V host, expecting that the Hyper-V host will limit it to 64. But netvsc driver code is declaring an array of size VRSS_CHANNEL_MAX, and there's nothing that guarantees that Hyper-V will always limit the channel count to 64. But maybe the netvsc driver enforces the limit of VRSS_CHANNEL_MAX in a place that I didn't immediately see in a quick look at the code. > > Based on this change the subchannel creation would change as follows: > > ------------------------------------------------------------- > |No. of vCPU |dev_info->num_chn |subchannel created | > ------------------------------------------------------------- > | 0-16 | 16 | vCPU | > | >16 & <=32 | 16 | 16 | > | >32 & <=128 | vCPU/2 | vCPU/2 | > | >128 | vCPU/2 | 64 | > ------------------------------------------------------------- The terminology here is slightly wrong. A VMBus device has one primary channel plus zero or more subchannels. The chart above is specifying the total number of channels (primary plus subchannels), not the number of subchannels. Michael > > Performance tests showed significant improvement in throughput: > - 0.54% for 16 vCPUs > - 0.83% for 32 vCPUs > - 1.76% for 48 vCPUs > - 10.35% for 64 vCPUs > - 13.47% for 96 vCPUs > > Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com> > --- > Changes in v2: > * Set dev_info->num_chn based on vCPU count > --- > drivers/net/hyperv/hyperv_net.h | 2 +- > drivers/net/hyperv/netvsc_drv.c | 5 ++++- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index 810977952f95..e690b95b1bbb 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -882,7 +882,7 @@ struct nvsp_message { > > #define VRSS_SEND_TAB_SIZE 16 /* must be power of 2 */ > #define VRSS_CHANNEL_MAX 64 > -#define VRSS_CHANNEL_DEFAULT 8 > +#define VRSS_CHANNEL_DEFAULT 16 > > #define RNDIS_MAX_PKT_DEFAULT 8 > #define RNDIS_PKT_ALIGN_DEFAULT 8 > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 44142245343d..e32eb2997bf7 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -27,6 +27,7 @@ > #include <linux/rtnetlink.h> > #include <linux/netpoll.h> > #include <linux/bpf.h> > +#include <linux/cpumask.h> > > #include <net/arp.h> > #include <net/route.h> > @@ -987,7 +988,9 @@ struct netvsc_device_info *netvsc_devinfo_get(struct > netvsc_device *nvdev) > dev_info->bprog = prog; > } > } else { > - dev_info->num_chn = VRSS_CHANNEL_DEFAULT; > + int count = num_online_cpus(); > + > + dev_info->num_chn = (count < 32) ? VRSS_CHANNEL_DEFAULT : DIV_ROUND_UP(count, 2); > dev_info->send_sections = NETVSC_DEFAULT_TX; > dev_info->send_section_size = NETVSC_SEND_SECTION_SIZE; > dev_info->recv_sections = NETVSC_DEFAULT_RX; > -- > 2.34.1 >
> -----Original Message----- > From: Michael Kelley <mhklinux@outlook.com> > Sent: Wednesday, August 14, 2024 7:57 PM > To: Erni Sri Satya Vennela <ernis@linux.microsoft.com>; KY Srinivasan > <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; > wei.liu@kernel.org; Dexuan Cui <decui@microsoft.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; linux- > hyperv@vger.kernel.org; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org > Cc: Erni Sri Satya Vennela <ernis@microsoft.com> > Subject: RE: [PATCH v2] net: netvsc: Update default VMBus channels > > From: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Sent: Wednesday, > August 14, 2024 9:59 AM > > > > Change VMBus channels macro (VRSS_CHANNEL_DEFAULT) in > > Linux netvsc from 8 to 16 to align with Azure Windows VM > > and improve networking throughput. > > > > For VMs having less than 16 vCPUS, the channels depend > > on number of vCPUs. Between 16 to 32 vCPUs, the channels > > default to VRSS_CHANNEL_DEFAULT. For greater than 32 vCPUs, > > set the channels to number of physical cores / 2 as a way > > to optimize CPU resource utilization and scale for high-end > > processors with many cores. > > Maximum number of channels are by default set to 64. > > Where in the code is this enforced? It's not part of this patch. It > might be in rndis_set_subchannel(), where a value larger than > 64 could be sent to the Hyper-V host, expecting that the Hyper-V > host will limit it to 64. But netvsc driver code is declaring an array > of size VRSS_CHANNEL_MAX, and there's nothing that guarantees > that Hyper-V will always limit the channel count to 64. But maybe > the netvsc driver enforces the limit of VRSS_CHANNEL_MAX in a > place that I didn't immediately see in a quick look at the code. Yes, netvsc driver limits the num_chn to be <=64: #define VRSS_CHANNEL_MAX 64 /* This guarantees that num_possible_rss_qs <= num_online_cpus */ num_possible_rss_qs = min_t(u32, num_online_cpus(), rsscap.num_recv_que); net_device->max_chn = min_t(u32, VRSS_CHANNEL_MAX, num_possible_rss_qs); /* We will use the given number of channels if available. */ net_device->num_chn = min(net_device->max_chn, device_info->num_chn); Thanks, - Haiyang
On Wed, 14 Aug 2024 09:59:13 -0700 Erni Sri Satya Vennela wrote: > Change VMBus channels macro (VRSS_CHANNEL_DEFAULT) in > Linux netvsc from 8 to 16 to align with Azure Windows VM > and improve networking throughput. > > For VMs having less than 16 vCPUS, the channels depend > on number of vCPUs. Between 16 to 32 vCPUs, the channels > default to VRSS_CHANNEL_DEFAULT. For greater than 32 vCPUs, > set the channels to number of physical cores / 2 as a way > to optimize CPU resource utilization and scale for high-end > processors with many cores. > Maximum number of channels are by default set to 64. > > Based on this change the subchannel creation would change as follows: > > ------------------------------------------------------------- > |No. of vCPU |dev_info->num_chn |subchannel created | > ------------------------------------------------------------- > | 0-16 | 16 | vCPU | > | >16 & <=32 | 16 | 16 | > | >32 & <=128 | vCPU/2 | vCPU/2 | > | >128 | vCPU/2 | 64 | > ------------------------------------------------------------- > > Performance tests showed significant improvement in throughput: > - 0.54% for 16 vCPUs > - 0.83% for 32 vCPUs > - 1.76% for 48 vCPUs > - 10.35% for 64 vCPUs > - 13.47% for 96 vCPUs Is there anything that needs clarifying in my feedback on v1? https://lore.kernel.org/all/20240807201857.445f9f95@kernel.org/ Ignoring maintainer feedback is known to result in angry outbursts.
From: Haiyang Zhang <haiyangz@microsoft.com> Sent: Thursday, August 15, 2024 7:51 AM > > > From: Michael Kelley <mhklinux@outlook.com> Sent: Wednesday, August 14, 2024 7:57 PM > > > > From: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Sent: Wednesday, August 14, 2024 9:59 AM > > > > > > Change VMBus channels macro (VRSS_CHANNEL_DEFAULT) in > > > Linux netvsc from 8 to 16 to align with Azure Windows VM > > > and improve networking throughput. > > > > > > For VMs having less than 16 vCPUS, the channels depend > > > on number of vCPUs. Between 16 to 32 vCPUs, the channels > > > default to VRSS_CHANNEL_DEFAULT. For greater than 32 vCPUs, > > > set the channels to number of physical cores / 2 as a way > > > to optimize CPU resource utilization and scale for high-end > > > processors with many cores. > > > Maximum number of channels are by default set to 64. > > > > Where in the code is this enforced? It's not part of this patch. It > > might be in rndis_set_subchannel(), where a value larger than > > 64 could be sent to the Hyper-V host, expecting that the Hyper-V > > host will limit it to 64. But netvsc driver code is declaring an array > > of size VRSS_CHANNEL_MAX, and there's nothing that guarantees > > that Hyper-V will always limit the channel count to 64. But maybe > > the netvsc driver enforces the limit of VRSS_CHANNEL_MAX in a > > place that I didn't immediately see in a quick look at the code. > > Yes, netvsc driver limits the num_chn to be <=64: > > #define VRSS_CHANNEL_MAX 64 > > /* This guarantees that num_possible_rss_qs <= num_online_cpus */ > num_possible_rss_qs = min_t(u32, num_online_cpus(), > rsscap.num_recv_que); > > net_device->max_chn = min_t(u32, VRSS_CHANNEL_MAX, num_possible_rss_qs); > > /* We will use the given number of channels if available. */ > net_device->num_chn = min(net_device->max_chn, device_info->num_chn); > OK, right. Thanks for the identifying the code for me. :-) Michael
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, August 15, 2024 12:09 PM > To: Erni Sri Satya Vennela <ernis@linux.microsoft.com> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui > <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com; > pabeni@redhat.com; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; Erni Sri Satya Vennela <ernis@microsoft.com> > Subject: Re: [PATCH v2] net: netvsc: Update default VMBus channels > > On Wed, 14 Aug 2024 09:59:13 -0700 Erni Sri Satya Vennela wrote: > > Change VMBus channels macro (VRSS_CHANNEL_DEFAULT) in > > Linux netvsc from 8 to 16 to align with Azure Windows VM > > and improve networking throughput. > > > > For VMs having less than 16 vCPUS, the channels depend > > on number of vCPUs. Between 16 to 32 vCPUs, the channels > > default to VRSS_CHANNEL_DEFAULT. For greater than 32 vCPUs, > > set the channels to number of physical cores / 2 as a way > > to optimize CPU resource utilization and scale for high-end > > processors with many cores. > > Maximum number of channels are by default set to 64. > > > > Based on this change the subchannel creation would change as follows: > > > > ------------------------------------------------------------- > > |No. of vCPU |dev_info->num_chn |subchannel created | > > ------------------------------------------------------------- > > | 0-16 | 16 | vCPU | > > | >16 & <=32 | 16 | 16 | > > | >32 & <=128 | vCPU/2 | vCPU/2 | > > | >128 | vCPU/2 | 64 | > > ------------------------------------------------------------- > > > > Performance tests showed significant improvement in throughput: > > - 0.54% for 16 vCPUs > > - 0.83% for 32 vCPUs > > - 1.76% for 48 vCPUs > > - 10.35% for 64 vCPUs > > - 13.47% for 96 vCPUs > > Is there anything that needs clarifying in my feedback on v1? > > https://lore.kernel.org/all/20240807201857.445f9f95@kernel.org/ > > Ignoring maintainer feedback is known to result in angry outbursts. Your suggestion on netif_get_num_default_rss_queues() is not ignored. We discussed internally on the formula we used for the num_chn, and chose a similar formula for higher number of vCPUs as in netif_get_num_default_rss_queues(). For lower number of vCPUs, we use the same default as Windows guests, because we don't want any potential regression. So, the end result is a step function: > > ------------------------------------------------------------- > > |No. of vCPU |dev_info->num_chn |subchannel created | > > ------------------------------------------------------------- > > | 0-16 | 16 | vCPU | > > | >16 & <=32 | 16 | 16 | > > | >32 & <=128 | vCPU/2 | vCPU/2 | > > | >128 | vCPU/2 | 64 | > > ------------------------------------------------------------- @Erni Sri Satya Vennela Next time, please reply to maintainer's emails to LKML, regarding how you think of the suggestions. Also, netif_get_num_default_rss_queues() uses #phys cores, which is different from num_online_cpus(). You can try like below, in addition to your comparison test, see if it's better than the patch v2. dev_info->num_chn = netif_get_num_default_rss_queues(); if (dev_info->num_chn < VRSS_CHANNEL_DEFAULT) dev_info->num_chn = VRSS_CHANNEL_DEFAULT; Thanks, - Haiyang
On Thu, Aug 15, 2024 at 09:08:56AM -0700, Jakub Kicinski wrote: > On Wed, 14 Aug 2024 09:59:13 -0700 Erni Sri Satya Vennela wrote: > > Change VMBus channels macro (VRSS_CHANNEL_DEFAULT) in > > Linux netvsc from 8 to 16 to align with Azure Windows VM > > and improve networking throughput. > > > > For VMs having less than 16 vCPUS, the channels depend > > on number of vCPUs. Between 16 to 32 vCPUs, the channels > > default to VRSS_CHANNEL_DEFAULT. For greater than 32 vCPUs, > > set the channels to number of physical cores / 2 as a way > > to optimize CPU resource utilization and scale for high-end > > processors with many cores. > > Maximum number of channels are by default set to 64. > > > > Based on this change the subchannel creation would change as follows: > > > > ------------------------------------------------------------- > > |No. of vCPU |dev_info->num_chn |subchannel created | > > ------------------------------------------------------------- > > | 0-16 | 16 | vCPU | > > | >16 & <=32 | 16 | 16 | > > | >32 & <=128 | vCPU/2 | vCPU/2 | > > | >128 | vCPU/2 | 64 | > > ------------------------------------------------------------- > > > > Performance tests showed significant improvement in throughput: > > - 0.54% for 16 vCPUs > > - 0.83% for 32 vCPUs > > - 1.76% for 48 vCPUs > > - 10.35% for 64 vCPUs > > - 13.47% for 96 vCPUs > > Is there anything that needs clarifying in my feedback on v1? > > https://lore.kernel.org/all/20240807201857.445f9f95@kernel.org/ > > Ignoring maintainer feedback is known to result in angry outbursts. I sincerely apologize for the miss on my part. I will make sure this never happens again. As Haiyang mentioned, we were trying to use a similar logic as in netif_get_num_default_rss_queues(), and trying to make sure there are no potential regressions. I will work on Michael's and Haiyang's follow up comments. Please let us know your opinion on the same.
On Thu, 15 Aug 2024 19:23:50 +0000 Haiyang Zhang wrote: > Your suggestion on netif_get_num_default_rss_queues() is not ignored. > We discussed internally on the formula we used for the num_chn, and > chose a similar formula for higher number of vCPUs as in > netif_get_num_default_rss_queues(). > For lower number of vCPUs, we use the same default as Windows guests, > because we don't want any potential regression. Ideally you'd just use netif_get_num_default_rss_queues() but the code is close enough to that, and I don't have enough experience with the question of online CPUs vs physical CPUs. I would definitely advise you to try this on real workloads. While "iperf" looks great with a lot of rings, real workloads suffer measurably from having more channels eating up memory and generating interrupts. But if you're confident with the online_cpus() / 2, that's fine. You may be better off coding it up using max: dev_info->num_chn = max(DIV_ROUND_UP(num_online_cpus(), 2), VRSS_CHANNEL_DEFAULT);
On Fri, Aug 16, 2024 at 08:52:41AM -0700, Jakub Kicinski wrote: > On Thu, 15 Aug 2024 19:23:50 +0000 Haiyang Zhang wrote: > > Your suggestion on netif_get_num_default_rss_queues() is not ignored. > > We discussed internally on the formula we used for the num_chn, and > > chose a similar formula for higher number of vCPUs as in > > netif_get_num_default_rss_queues(). > > For lower number of vCPUs, we use the same default as Windows guests, > > because we don't want any potential regression. > > Ideally you'd just use netif_get_num_default_rss_queues() > but the code is close enough to that, and I don't have enough > experience with the question of online CPUs vs physical CPUs. > > I would definitely advise you to try this on real workloads. > While "iperf" looks great with a lot of rings, real workloads > suffer measurably from having more channels eating up memory > and generating interrupts. > > But if you're confident with the online_cpus() / 2, that's fine. > You may be better off coding it up using max: > > dev_info->num_chn = max(DIV_ROUND_UP(num_online_cpus(), 2), > VRSS_CHANNEL_DEFAULT); Due to hyper-threading, #of physical cores = online CPUs/2. Therefore, netif_get_num_default_rss_queues() returns #of physical cores/2 = online CPUs/4. In my testing, the throughput performance was similar for both the configurations even for higher SKUs.To utilize lesser CPU resources, will be using netif_get_num_default_rss_queues() for the next version of the patch.
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index 810977952f95..e690b95b1bbb 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -882,7 +882,7 @@ struct nvsp_message { #define VRSS_SEND_TAB_SIZE 16 /* must be power of 2 */ #define VRSS_CHANNEL_MAX 64 -#define VRSS_CHANNEL_DEFAULT 8 +#define VRSS_CHANNEL_DEFAULT 16 #define RNDIS_MAX_PKT_DEFAULT 8 #define RNDIS_PKT_ALIGN_DEFAULT 8 diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 44142245343d..e32eb2997bf7 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -27,6 +27,7 @@ #include <linux/rtnetlink.h> #include <linux/netpoll.h> #include <linux/bpf.h> +#include <linux/cpumask.h> #include <net/arp.h> #include <net/route.h> @@ -987,7 +988,9 @@ struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev) dev_info->bprog = prog; } } else { - dev_info->num_chn = VRSS_CHANNEL_DEFAULT; + int count = num_online_cpus(); + + dev_info->num_chn = (count < 32) ? VRSS_CHANNEL_DEFAULT : DIV_ROUND_UP(count, 2); dev_info->send_sections = NETVSC_DEFAULT_TX; dev_info->send_section_size = NETVSC_SEND_SECTION_SIZE; dev_info->recv_sections = NETVSC_DEFAULT_RX;
Change VMBus channels macro (VRSS_CHANNEL_DEFAULT) in Linux netvsc from 8 to 16 to align with Azure Windows VM and improve networking throughput. For VMs having less than 16 vCPUS, the channels depend on number of vCPUs. Between 16 to 32 vCPUs, the channels default to VRSS_CHANNEL_DEFAULT. For greater than 32 vCPUs, set the channels to number of physical cores / 2 as a way to optimize CPU resource utilization and scale for high-end processors with many cores. Maximum number of channels are by default set to 64. Based on this change the subchannel creation would change as follows: ------------------------------------------------------------- |No. of vCPU |dev_info->num_chn |subchannel created | ------------------------------------------------------------- | 0-16 | 16 | vCPU | | >16 & <=32 | 16 | 16 | | >32 & <=128 | vCPU/2 | vCPU/2 | | >128 | vCPU/2 | 64 | ------------------------------------------------------------- Performance tests showed significant improvement in throughput: - 0.54% for 16 vCPUs - 0.83% for 32 vCPUs - 1.76% for 48 vCPUs - 10.35% for 64 vCPUs - 13.47% for 96 vCPUs Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com> --- Changes in v2: * Set dev_info->num_chn based on vCPU count --- drivers/net/hyperv/hyperv_net.h | 2 +- drivers/net/hyperv/netvsc_drv.c | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-)