diff mbox series

[v3] net: netvsc: Update default VMBus channels

Message ID 1724339168-20913-1-git-send-email-ernis@linux.microsoft.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v3] net: netvsc: Update default VMBus channels | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-25--21-00 (tests: 714)

Commit Message

Erni Sri Satya Vennela Aug. 22, 2024, 3:06 p.m. UTC 
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 64 vCPUs, the channels
default to VRSS_CHANNEL_DEFAULT. For greater than 64 vCPUs,
set the channels to number of physical cores / 2 returned by
netif_get_num_default_rss_queues() as a way to optimize CPU
resource utilization and scale for high-end processors with
many cores. Due to hyper-threading, the number of
physical cores = vCPUs/2.
Maximum number of channels are by default set to 64.

Based on this change the channel creation would change as follows:

-------------------------------------------------------------
| No. of vCPU	|  dev_info->num_chn	| channels created  |
-------------------------------------------------------------
|  0-16		|       16		|       vCPU        |
| >16 & <=64	|       16		|       16          |
| >64 & <=256	|       vCPU/4		|       vCPU/4      |
| >256		|       vCPU/4		|       64          |
-------------------------------------------------------------

Performance tests showed significant improvement in throughput:
- 0.54% for 16 vCPUs
- 0.83% for 32 vCPUs
- 0.86% for 48 vCPUs
- 9.72% for 64 vCPUs
- 13.57% for 96 vCPUs

Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
---
Changes in v3:
* Use netif_get_num_default_rss_queues() to set channels
* Change terminology for channels in commit message
---
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 | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

Comments

Michael Kelley Aug. 23, 2024, 4:37 p.m. UTC | #1 
From: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Sent: Thursday, August 22, 2024 8:06 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 64 vCPUs, the channels
> default to VRSS_CHANNEL_DEFAULT. For greater than 64 vCPUs,
> set the channels to number of physical cores / 2 returned by
> netif_get_num_default_rss_queues() as a way to optimize CPU
> resource utilization and scale for high-end processors with
> many cores. Due to hyper-threading, the number of
> physical cores = vCPUs/2.

But note that a given physical processor may or may not support
hyper-threading. For example, the physical processor used for
ARM64 VMs in Azure does not have hyper-threading. And even
if the physical processor supports hyper-threading, the VM might
not see hyper-threading as enabled. Many Azure GPU-based VM
sizes see only full cores, with no hyper-threading. It's also possible
to boot Linux with hyper-threading disabled even if the VM sees
hyper-threaded cores (the "nosmt" or "smt=1" kernel boot option).

Your code below probably isn't affected when hyper-threading
isn't present. But in the interest of accuracy, the discussion here
in the commit message should qualify the use of "vCPU/4" as
the number of channels. It might be "vCPU/2" when
hyper-threading isn't present or is disabled, and for vCPU
counts between 16 and 64, you'll get more than 16 channels.

> Maximum number of channels are by default set to 64.
> 
> Based on this change the channel creation would change as follows:
> 
> -------------------------------------------------------------
> | No. of vCPU	|  dev_info->num_chn	| channels created  |
> -------------------------------------------------------------
> |  0-16		|       16		|       vCPU        |

Nit: Presumably we won't ever have 0 vCPUs.  :-)

> | >16 & <=64	|       16		|       16          |
> | >64 & <=256	|       vCPU/4		|       vCPU/4      |
> | >256		|       vCPU/4		|       64          |
> -------------------------------------------------------------
> 
> Performance tests showed significant improvement in throughput:
> - 0.54% for 16 vCPUs
> - 0.83% for 32 vCPUs
> - 0.86% for 48 vCPUs
> - 9.72% for 64 vCPUs
> - 13.57% for 96 vCPUs
> 
> Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reviewed-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
> ---
> Changes in v3:
> * Use netif_get_num_default_rss_queues() to set channels
> * Change terminology for channels in commit message
> ---
> 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 | 3 ++-
>  2 files changed, 3 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..a6482afe4217 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -987,7 +987,8 @@ struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev)
>  			dev_info->bprog = prog;
>  		}
>  	} else {
> -		dev_info->num_chn = VRSS_CHANNEL_DEFAULT;
> +		dev_info->num_chn = max(VRSS_CHANNEL_DEFAULT,
> +					netif_get_num_default_rss_queues());
>  		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
>
Erni Sri Satya Vennela Aug. 27, 2024, 4:55 a.m. UTC | #2 
On Fri, Aug 23, 2024 at 04:37:17PM +0000, Michael Kelley wrote:
> From: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Sent: Thursday, August 22, 2024 8:06 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 64 vCPUs, the channels
> > default to VRSS_CHANNEL_DEFAULT. For greater than 64 vCPUs,
> > set the channels to number of physical cores / 2 returned by
> > netif_get_num_default_rss_queues() as a way to optimize CPU
> > resource utilization and scale for high-end processors with
> > many cores. Due to hyper-threading, the number of
> > physical cores = vCPUs/2.
> 
> But note that a given physical processor may or may not support
> hyper-threading. For example, the physical processor used for
> ARM64 VMs in Azure does not have hyper-threading. And even
> if the physical processor supports hyper-threading, the VM might
> not see hyper-threading as enabled. Many Azure GPU-based VM
> sizes see only full cores, with no hyper-threading. It's also possible
> to boot Linux with hyper-threading disabled even if the VM sees
> hyper-threaded cores (the "nosmt" or "smt=1" kernel boot option).
> 
> Your code below probably isn't affected when hyper-threading
> isn't present. But in the interest of accuracy, the discussion here
> in the commit message should qualify the use of "vCPU/4" as
> the number of channels. It might be "vCPU/2" when
> hyper-threading isn't present or is disabled, and for vCPU
> counts between 16 and 64, you'll get more than 16 channels.
> 
> > Maximum number of channels are by default set to 64.
> > 
> > Based on this change the channel creation would change as follows:
> > 
> > -------------------------------------------------------------
> > | No. of vCPU	|  dev_info->num_chn	| channels created  |
> > -------------------------------------------------------------
> > |  0-16		|       16		|       vCPU        |
> 
> Nit: Presumably we won't ever have 0 vCPUs.  :-)
> 
> > | >16 & <=64	|       16		|       16          |
> > | >64 & <=256	|       vCPU/4		|       vCPU/4      |
> > | >256		|       vCPU/4		|       64          |
> > -------------------------------------------------------------
> > 
> > Performance tests showed significant improvement in throughput:
> > - 0.54% for 16 vCPUs
> > - 0.83% for 32 vCPUs
> > - 0.86% for 48 vCPUs
> > - 9.72% for 64 vCPUs
> > - 13.57% for 96 vCPUs
> > 
> > Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Reviewed-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
> > ---
> > Changes in v3:
> > * Use netif_get_num_default_rss_queues() to set channels
> > * Change terminology for channels in commit message
> > ---
> > 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 | 3 ++-
> >  2 files changed, 3 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..a6482afe4217 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -987,7 +987,8 @@ struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev)
> >  			dev_info->bprog = prog;
> >  		}
> >  	} else {
> > -		dev_info->num_chn = VRSS_CHANNEL_DEFAULT;
> > +		dev_info->num_chn = max(VRSS_CHANNEL_DEFAULT,
> > +					netif_get_num_default_rss_queues());
> >  		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
> > 
Thank you for the knowledge you shared. I’ll update the patch
with your suggestions in the next version.
diff mbox series

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..a6482afe4217 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -987,7 +987,8 @@  struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev)
 			dev_info->bprog = prog;
 		}
 	} else {
-		dev_info->num_chn = VRSS_CHANNEL_DEFAULT;
+		dev_info->num_chn = max(VRSS_CHANNEL_DEFAULT,
+					netif_get_num_default_rss_queues());
 		dev_info->send_sections = NETVSC_DEFAULT_TX;
 		dev_info->send_section_size = NETVSC_SEND_SECTION_SIZE;
 		dev_info->recv_sections = NETVSC_DEFAULT_RX;