[PATCH v3] scsi: storvsc: Parameterize number hardware queues

[PATCH v3] scsi: storvsc: Parameterize number hardware queues

[Melanie Plageman]

From: "Melanie Plageman (Microsoft)" <melanieplageman@gmail.com>

Add ability to set the number of hardware queues with new module parameter,
storvsc_max_hw_queues. The default value remains the number of CPUs.  This
functionality is useful in some environments (e.g. Microsoft Azure) where
decreasing the number of hardware queues has been shown to improve
performance.

Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
---
 drivers/scsi/storvsc_drv.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 2e4fa77445fd..a64e6664c915 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -378,10 +378,14 @@ static u32 max_outstanding_req_per_channel;
 static int storvsc_change_queue_depth(struct scsi_device *sdev, int queue_depth);
 
 static int storvsc_vcpus_per_sub_channel = 4;
+static int storvsc_max_hw_queues = -1;
 
 module_param(storvsc_ringbuffer_size, int, S_IRUGO);
 MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
 
+module_param(storvsc_max_hw_queues, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(storvsc_max_hw_queues, "Maximum number of hardware queues");
+
 module_param(storvsc_vcpus_per_sub_channel, int, S_IRUGO);
 MODULE_PARM_DESC(storvsc_vcpus_per_sub_channel, "Ratio of VCPUs to subchannels");
 
@@ -1897,6 +1901,7 @@ static int storvsc_probe(struct hv_device *device,
 {
 	int ret;
 	int num_cpus = num_online_cpus();
+	int num_present_cpus = num_present_cpus();
 	struct Scsi_Host *host;
 	struct hv_host_device *host_dev;
 	bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
@@ -2004,8 +2009,19 @@ static int storvsc_probe(struct hv_device *device,
 	 * For non-IDE disks, the host supports multiple channels.
 	 * Set the number of HW queues we are supporting.
 	 */
-	if (!dev_is_ide)
-		host->nr_hw_queues = num_present_cpus();
+	if (!dev_is_ide) {
+		if (storvsc_max_hw_queues == -1)
+			host->nr_hw_queues = num_present_cpus;
+		else if (storvsc_max_hw_queues > num_present_cpus ||
+			 storvsc_max_hw_queues == 0 ||
+			storvsc_max_hw_queues < -1) {
+			storvsc_log(device, STORVSC_LOGGING_WARN,
+				"Resetting invalid storvsc_max_hw_queues value to default.\n");
+			host->nr_hw_queues = num_present_cpus;
+			storvsc_max_hw_queues = -1;
+		} else
+			host->nr_hw_queues = storvsc_max_hw_queues;
+	}
 
 	/*
 	 * Set the error handler work queue.
@@ -2169,6 +2185,14 @@ static int __init storvsc_drv_init(void)
 		vmscsi_size_delta,
 		sizeof(u64)));
 
+	if (storvsc_max_hw_queues > num_present_cpus() ||
+		storvsc_max_hw_queues == 0 ||
+		storvsc_max_hw_queues < -1) {
+		pr_warn("Setting storvsc_max_hw_queues to -1. %d is invalid.\n",
+			storvsc_max_hw_queues);
+		storvsc_max_hw_queues = -1;
+	}
+
 #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
 	fc_transport_template = fc_attach_transport(&fc_transport_functions);
 	if (!fc_transport_template)
-- 
2.20.1

RE: [PATCH v3] scsi: storvsc: Parameterize number hardware queues

[Michael Kelley]

From: Melanie Plageman <melanieplageman@gmail.com> Sent: Thursday, February 11, 2021 3:18 PM
> 
> Add ability to set the number of hardware queues with new module parameter,
> storvsc_max_hw_queues. The default value remains the number of CPUs.  This
> functionality is useful in some environments (e.g. Microsoft Azure) where
> decreasing the number of hardware queues has been shown to improve
> performance.
> 
> Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
> ---
>  drivers/scsi/storvsc_drv.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 2e4fa77445fd..a64e6664c915 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -378,10 +378,14 @@ static u32 max_outstanding_req_per_channel;
>  static int storvsc_change_queue_depth(struct scsi_device *sdev, int queue_depth);
> 
>  static int storvsc_vcpus_per_sub_channel = 4;
> +static int storvsc_max_hw_queues = -1;
> 
>  module_param(storvsc_ringbuffer_size, int, S_IRUGO);
>  MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
> 
> +module_param(storvsc_max_hw_queues, int, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(storvsc_max_hw_queues, "Maximum number of hardware
> queues");
> +

There's been an effort underway to not use the symbolic permissions in
module_param(), but to just use the octal digits (like 0600 for root only
access).   But I couldn't immediately find documentation on why this
change is being made.  And clearly it hasn't been applied to the
existing module_param() uses here in storvsc_drv.c.  But with this being
a new parameter, let's use the recommended octal digit format.

>  module_param(storvsc_vcpus_per_sub_channel, int, S_IRUGO);
>  MODULE_PARM_DESC(storvsc_vcpus_per_sub_channel, "Ratio of VCPUs to
> subchannels");
> 
> @@ -1897,6 +1901,7 @@ static int storvsc_probe(struct hv_device *device,
>  {
>  	int ret;
>  	int num_cpus = num_online_cpus();
> +	int num_present_cpus = num_present_cpus();
>  	struct Scsi_Host *host;
>  	struct hv_host_device *host_dev;
>  	bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
> @@ -2004,8 +2009,19 @@ static int storvsc_probe(struct hv_device *device,
>  	 * For non-IDE disks, the host supports multiple channels.
>  	 * Set the number of HW queues we are supporting.
>  	 */
> -	if (!dev_is_ide)
> -		host->nr_hw_queues = num_present_cpus();
> +	if (!dev_is_ide) {
> +		if (storvsc_max_hw_queues == -1)
> +			host->nr_hw_queues = num_present_cpus;
> +		else if (storvsc_max_hw_queues > num_present_cpus ||
> +			 storvsc_max_hw_queues == 0 ||
> +			storvsc_max_hw_queues < -1) {
> +			storvsc_log(device, STORVSC_LOGGING_WARN,
> +				"Resetting invalid storvsc_max_hw_queues value to default.\n");
> +			host->nr_hw_queues = num_present_cpus;
> +			storvsc_max_hw_queues = -1;
> +		} else
> +			host->nr_hw_queues = storvsc_max_hw_queues;
> +	}

I have a couple of thoughts about the above logic.  As the code is written,
valid values are integers from 1 to the number of CPUs, and -1.  The logic
would be simpler if the module parameter was an unsigned int instead of
a signed int, and zero was the marker for "use number of CPUs".  Then
you wouldn't have to check for negative values or have special handling
for -1.

Second, I think you can avoid intertwining the logic for checking for an
invalid value, and actually setting host->nr_hw_queues.  Check for an
invalid value first, then do the setting of host->hr_hw_queues.

Putting both thoughts together, you could get code like this:

	if (!dev_is ide) {
		if (storvsc_max_hw_queues > num_present_cpus) {
			storvsc_max_hw_queues = 0;
			storvsc_log(device, STORVSC_LOGGING_WARN,
				"Resetting invalid storvsc_max_hw_queues value to default.\n");
		}
		if (storvsc_max_hw_queues)
			host->nr_hw_queues = storvsc_max_hw_queues
		else
			host->hr_hw_queues = num_present_cpus;
	}

> 
>  	/*
>  	 * Set the error handler work queue.
> @@ -2169,6 +2185,14 @@ static int __init storvsc_drv_init(void)
>  		vmscsi_size_delta,
>  		sizeof(u64)));
> 
> +	if (storvsc_max_hw_queues > num_present_cpus() ||
> +		storvsc_max_hw_queues == 0 ||
> +		storvsc_max_hw_queues < -1) {
> +		pr_warn("Setting storvsc_max_hw_queues to -1. %d is invalid.\n",
> +			storvsc_max_hw_queues);
> +		storvsc_max_hw_queues = -1;
> +	}
> +

Is this check really needed?  Any usage of the value will be in
storvsc_probe() where the same check is performed.  I'm not seeing
a scenario where this check adds value over what's already being
done in storvsc_probe(), but maybe I'm missing it.

>  #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
>  	fc_transport_template = fc_attach_transport(&fc_transport_functions);
>  	if (!fc_transport_template)
> --
> 2.20.1

Re: [PATCH v3] scsi: storvsc: Parameterize number hardware queues

[Melanie Plageman]

On Fri, Feb 12, 2021 at 04:35:16PM +0000, Michael Kelley wrote:
> From: Melanie Plageman <melanieplageman@gmail.com> Sent: Thursday, February 11, 2021 3:18 PM
> > 
> > Add ability to set the number of hardware queues with new module parameter,
> > storvsc_max_hw_queues. The default value remains the number of CPUs.  This
> > functionality is useful in some environments (e.g. Microsoft Azure) where
> > decreasing the number of hardware queues has been shown to improve
> > performance.
> > 
> > Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
> > ---
> >  drivers/scsi/storvsc_drv.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 2e4fa77445fd..a64e6664c915 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -378,10 +378,14 @@ static u32 max_outstanding_req_per_channel;
> >  static int storvsc_change_queue_depth(struct scsi_device *sdev, int queue_depth);
> > 
> >  static int storvsc_vcpus_per_sub_channel = 4;
> > +static int storvsc_max_hw_queues = -1;
> > 
> >  module_param(storvsc_ringbuffer_size, int, S_IRUGO);
> >  MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
> > 
> > +module_param(storvsc_max_hw_queues, int, S_IRUGO|S_IWUSR);
> > +MODULE_PARM_DESC(storvsc_max_hw_queues, "Maximum number of hardware
> > queues");
> > +
> 
> There's been an effort underway to not use the symbolic permissions in
> module_param(), but to just use the octal digits (like 0600 for root only
> access).   But I couldn't immediately find documentation on why this
> change is being made.  And clearly it hasn't been applied to the
> existing module_param() uses here in storvsc_drv.c.  But with this being
> a new parameter, let's use the recommended octal digit format.

Thanks. I will update this in v4.

> 
> >  module_param(storvsc_vcpus_per_sub_channel, int, S_IRUGO);
> >  MODULE_PARM_DESC(storvsc_vcpus_per_sub_channel, "Ratio of VCPUs to
> > subchannels");
> > 
> > @@ -1897,6 +1901,7 @@ static int storvsc_probe(struct hv_device *device,
> >  {
> >  	int ret;
> >  	int num_cpus = num_online_cpus();
> > +	int num_present_cpus = num_present_cpus();
> >  	struct Scsi_Host *host;
> >  	struct hv_host_device *host_dev;
> >  	bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
> > @@ -2004,8 +2009,19 @@ static int storvsc_probe(struct hv_device *device,
> >  	 * For non-IDE disks, the host supports multiple channels.
> >  	 * Set the number of HW queues we are supporting.
> >  	 */
> > -	if (!dev_is_ide)
> > -		host->nr_hw_queues = num_present_cpus();
> > +	if (!dev_is_ide) {
> > +		if (storvsc_max_hw_queues == -1)
> > +			host->nr_hw_queues = num_present_cpus;
> > +		else if (storvsc_max_hw_queues > num_present_cpus ||
> > +			 storvsc_max_hw_queues == 0 ||
> > +			storvsc_max_hw_queues < -1) {
> > +			storvsc_log(device, STORVSC_LOGGING_WARN,
> > +				"Resetting invalid storvsc_max_hw_queues value to default.\n");
> > +			host->nr_hw_queues = num_present_cpus;
> > +			storvsc_max_hw_queues = -1;
> > +		} else
> > +			host->nr_hw_queues = storvsc_max_hw_queues;
> > +	}
> 
> I have a couple of thoughts about the above logic.  As the code is written,
> valid values are integers from 1 to the number of CPUs, and -1.  The logic
> would be simpler if the module parameter was an unsigned int instead of
> a signed int, and zero was the marker for "use number of CPUs".  Then
> you wouldn't have to check for negative values or have special handling
> for -1.

I used -1 because the linter ./scripts/checkpatch.pl throws an ERROR "do
not initialise statics to 0"

> 
> Second, I think you can avoid intertwining the logic for checking for an
> invalid value, and actually setting host->nr_hw_queues.  Check for an
> invalid value first, then do the setting of host->hr_hw_queues.
> 
> Putting both thoughts together, you could get code like this:
> 
> 	if (!dev_is ide) {
> 		if (storvsc_max_hw_queues > num_present_cpus) {
> 			storvsc_max_hw_queues = 0;
> 			storvsc_log(device, STORVSC_LOGGING_WARN,
> 				"Resetting invalid storvsc_max_hw_queues value to default.\n");
> 		}
> 		if (storvsc_max_hw_queues)
> 			host->nr_hw_queues = storvsc_max_hw_queues
> 		else
> 			host->hr_hw_queues = num_present_cpus;
> 	}

I will update the logic like this.

> 
> > 
> >  	/*
> >  	 * Set the error handler work queue.
> > @@ -2169,6 +2185,14 @@ static int __init storvsc_drv_init(void)
> >  		vmscsi_size_delta,
> >  		sizeof(u64)));
> > 
> > +	if (storvsc_max_hw_queues > num_present_cpus() ||
> > +		storvsc_max_hw_queues == 0 ||
> > +		storvsc_max_hw_queues < -1) {
> > +		pr_warn("Setting storvsc_max_hw_queues to -1. %d is invalid.\n",
> > +			storvsc_max_hw_queues);
> > +		storvsc_max_hw_queues = -1;
> > +	}
> > +
> 
> Is this check really needed?  Any usage of the value will be in
> storvsc_probe() where the same check is performed.  I'm not seeing
> a scenario where this check adds value over what's already being
> done in storvsc_probe(), but maybe I'm missing it.

It is not. I had initially added it because I did not plan on making the
parameter updatable and thought it would be better to only have one
message about the invalid value instead of #device messages (from each
probe()). But, after making it updatable, I had to add invalid value
checking to storvsc_probe() anyway, so, this is definitely unneeded. If
you specify the parameter at boot-time and this is here, you would only
get one instance of the logging message (because it resets the value of
storvsc_max_hw_queues to the default before probe() is called), but, I
don't think that is worth it.

> 
> >  #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
> >  	fc_transport_template = fc_attach_transport(&fc_transport_functions);
> >  	if (!fc_transport_template)
> > --
> > 2.20.1
> 

RE: [PATCH v3] scsi: storvsc: Parameterize number hardware queues

[Michael Kelley]

From: Melanie Plageman <melanieplageman@gmail.com> Sent: Wednesday, February 17, 2021 4:05 PM
> 
> On Fri, Feb 12, 2021 at 04:35:16PM +0000, Michael Kelley wrote:
> > From: Melanie Plageman <melanieplageman@gmail.com> Sent: Thursday, February 11,
> 2021 3:18 PM
> > >
> > > Add ability to set the number of hardware queues with new module parameter,
> > > storvsc_max_hw_queues. The default value remains the number of CPUs.  This
> > > functionality is useful in some environments (e.g. Microsoft Azure) where
> > > decreasing the number of hardware queues has been shown to improve
> > > performance.
> > >
> > > Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
> > > ---
> > >  drivers/scsi/storvsc_drv.c | 28 ++++++++++++++++++++++++++--
> > >  1 file changed, 26 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > index 2e4fa77445fd..a64e6664c915 100644
> > > --- a/drivers/scsi/storvsc_drv.c
> > > +++ b/drivers/scsi/storvsc_drv.c
> > > @@ -378,10 +378,14 @@ static u32 max_outstanding_req_per_channel;
> > >  static int storvsc_change_queue_depth(struct scsi_device *sdev, int queue_depth);
> > >
> > >  static int storvsc_vcpus_per_sub_channel = 4;
> > > +static int storvsc_max_hw_queues = -1;
> > >
> > >  module_param(storvsc_ringbuffer_size, int, S_IRUGO);
> > >  MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
> > >
> > > +module_param(storvsc_max_hw_queues, int, S_IRUGO|S_IWUSR);
> > > +MODULE_PARM_DESC(storvsc_max_hw_queues, "Maximum number of hardware
> > > queues");
> > > +
> >
> > There's been an effort underway to not use the symbolic permissions in
> > module_param(), but to just use the octal digits (like 0600 for root only
> > access).   But I couldn't immediately find documentation on why this
> > change is being made.  And clearly it hasn't been applied to the
> > existing module_param() uses here in storvsc_drv.c.  But with this being
> > a new parameter, let's use the recommended octal digit format.
> 
> Thanks. I will update this in v4.
> 
> >
> > >  module_param(storvsc_vcpus_per_sub_channel, int, S_IRUGO);
> > >  MODULE_PARM_DESC(storvsc_vcpus_per_sub_channel, "Ratio of VCPUs to
> > > subchannels");
> > >
> > > @@ -1897,6 +1901,7 @@ static int storvsc_probe(struct hv_device *device,
> > >  {
> > >  	int ret;
> > >  	int num_cpus = num_online_cpus();
> > > +	int num_present_cpus = num_present_cpus();
> > >  	struct Scsi_Host *host;
> > >  	struct hv_host_device *host_dev;
> > >  	bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
> > > @@ -2004,8 +2009,19 @@ static int storvsc_probe(struct hv_device *device,
> > >  	 * For non-IDE disks, the host supports multiple channels.
> > >  	 * Set the number of HW queues we are supporting.
> > >  	 */
> > > -	if (!dev_is_ide)
> > > -		host->nr_hw_queues = num_present_cpus();
> > > +	if (!dev_is_ide) {
> > > +		if (storvsc_max_hw_queues == -1)
> > > +			host->nr_hw_queues = num_present_cpus;
> > > +		else if (storvsc_max_hw_queues > num_present_cpus ||
> > > +			 storvsc_max_hw_queues == 0 ||
> > > +			storvsc_max_hw_queues < -1) {
> > > +			storvsc_log(device, STORVSC_LOGGING_WARN,
> > > +				"Resetting invalid storvsc_max_hw_queues value to default.\n");
> > > +			host->nr_hw_queues = num_present_cpus;
> > > +			storvsc_max_hw_queues = -1;
> > > +		} else
> > > +			host->nr_hw_queues = storvsc_max_hw_queues;
> > > +	}
> >
> > I have a couple of thoughts about the above logic.  As the code is written,
> > valid values are integers from 1 to the number of CPUs, and -1.  The logic
> > would be simpler if the module parameter was an unsigned int instead of
> > a signed int, and zero was the marker for "use number of CPUs".  Then
> > you wouldn't have to check for negative values or have special handling
> > for -1.
> 
> I used -1 because the linter ./scripts/checkpatch.pl throws an ERROR "do
> not initialise statics to 0"

OK, right.  The intent of that warning is not that using zero as a value
is bad.  The intent that to indicate that statics are by default initialized
to zero, so explicitly adding the "= 0" is unnecessary.  So feel free to
use "0" as the marker for "use numbers of CPUs".  Just don't
add the "= 0" in the variable declaration. :-)

> 
> >
> > Second, I think you can avoid intertwining the logic for checking for an
> > invalid value, and actually setting host->nr_hw_queues.  Check for an
> > invalid value first, then do the setting of host->hr_hw_queues.
> >
> > Putting both thoughts together, you could get code like this:
> >
> > 	if (!dev_is ide) {
> > 		if (storvsc_max_hw_queues > num_present_cpus) {
> > 			storvsc_max_hw_queues = 0;
> > 			storvsc_log(device, STORVSC_LOGGING_WARN,
> > 				"Resetting invalid storvsc_max_hw_queues value to default.\n");
> > 		}
> > 		if (storvsc_max_hw_queues)
> > 			host->nr_hw_queues = storvsc_max_hw_queues
> > 		else
> > 			host->hr_hw_queues = num_present_cpus;
> > 	}
> 
> I will update the logic like this.
> 
> >
> > >
> > >  	/*
> > >  	 * Set the error handler work queue.
> > > @@ -2169,6 +2185,14 @@ static int __init storvsc_drv_init(void)
> > >  		vmscsi_size_delta,
> > >  		sizeof(u64)));
> > >
> > > +	if (storvsc_max_hw_queues > num_present_cpus() ||
> > > +		storvsc_max_hw_queues == 0 ||
> > > +		storvsc_max_hw_queues < -1) {
> > > +		pr_warn("Setting storvsc_max_hw_queues to -1. %d is invalid.\n",
> > > +			storvsc_max_hw_queues);
> > > +		storvsc_max_hw_queues = -1;
> > > +	}
> > > +
> >
> > Is this check really needed?  Any usage of the value will be in
> > storvsc_probe() where the same check is performed.  I'm not seeing
> > a scenario where this check adds value over what's already being
> > done in storvsc_probe(), but maybe I'm missing it.
> 
> It is not. I had initially added it because I did not plan on making the
> parameter updatable and thought it would be better to only have one
> message about the invalid value instead of #device messages (from each
> probe()). But, after making it updatable, I had to add invalid value
> checking to storvsc_probe() anyway, so, this is definitely unneeded. If
> you specify the parameter at boot-time and this is here, you would only
> get one instance of the logging message (because it resets the value of
> storvsc_max_hw_queues to the default before probe() is called), but, I
> don't think that is worth it.

I agree.  And actually, if the code in storvsc_probe() "fixes" the bad
value, you would not get the warning on subsequent calls to
storvsc_probe().  So again, you would have only one warning unless
someone manually changed it to a bad value again via the /sys/module
interface.

Michael

> 
> >
> > >  #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
> > >  	fc_transport_template = fc_attach_transport(&fc_transport_functions);
> > >  	if (!fc_transport_template)
> > > --
> > > 2.20.1
> >