netdev - Re: [PATCH v3] net: netvsc: Update default VMBus channels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240827045521.GA17487@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Date: Mon, 26 Aug 2024 21:55:21 -0700
From: Erni Sri Satya Vennela <ernis@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>
Cc: "kys@...rosoft.com" <kys@...rosoft.com>,
	"haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
	"wei.liu@...nel.org" <wei.liu@...nel.org>,
	"decui@...rosoft.com" <decui@...rosoft.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"ernis@...rosoft.com" <ernis@...rosoft.com>
Subject: Re: [PATCH v3] net: netvsc: Update default VMBus channels

On Fri, Aug 23, 2024 at 04:37:17PM +0000, Michael Kelley wrote:
> From: Erni Sri Satya Vennela <ernis@...ux.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@...ux.microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@...rosoft.com>
> > Reviewed-by: Shradha Gupta <shradhagupta@...ux.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.