Re: [PATCH v1] scsi: storvsc: Parameterize nr_hw_queues

Re: [PATCH v1] scsi: storvsc: Parameterize nr_hw_queues

[Andres Freund]

Hi,

On 2021-02-02 15:19:08 -0500, Melanie Plageman wrote:
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 2e4fa77445fd..d72ab6daf9ae 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_nr_hw_queues = -1;
>  
>  module_param(storvsc_ringbuffer_size, int, S_IRUGO);
>  MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
>  
> +module_param(storvsc_nr_hw_queues, int, S_IRUGO);
> +MODULE_PARM_DESC(storvsc_nr_hw_queues, "Number of hardware queues");

Would be nice if the parameter could be changed in a running
system. Obviously that's only going to affect devices that'll be
attached in the future (or detached and reattached), but that's still
nicer than not being able to change at all.


> @@ -2155,6 +2163,7 @@ static struct fc_function_template fc_transport_functions = {
>  static int __init storvsc_drv_init(void)
>  {
>  	int ret;
> +	int ncpus = num_present_cpus();
>  
>  	/*
>  	 * Divide the ring buffer data size (which is 1 page less
> @@ -2169,6 +2178,14 @@ static int __init storvsc_drv_init(void)
>  		vmscsi_size_delta,
>  		sizeof(u64)));
>  
> +	if (storvsc_nr_hw_queues > ncpus || storvsc_nr_hw_queues == 0 ||
> +			storvsc_nr_hw_queues < -1) {
> +		printk(KERN_ERR "storvsc: Invalid storvsc_nr_hw_queues value of %d.
> +						Valid values include -1 and 1-%d.\n",
> +				storvsc_nr_hw_queues, ncpus);
> +		return -EINVAL;
> +	}
> +

I have a mild preference for just capping the effective value to
num_present_cpus(), rather than erroring out. Would allow you to
e.g. cap the number of queues to 4, with the same param specified on
smaller and bigger systems.  Perhaps renaming it to max_hw_queues would
make that more obvious?

Greetings,

Andres Freund

[PATCH v2 0/2] scsi: storvsc: Parameterize nr_hw_queues

[Melanie Plageman (Microsoft)]

Though the convention on this list seems to be to start a new thread for
a new version of the patch, because I made the mistake of sending the
first patch as an attachment, I've included both the original patch
inline as well as the updates in this patchset.

Melanie Plageman (Microsoft) (2):
  scsi: storvsc: Parameterize number hardware queues
  scsi: storvsc: Make max hw queues updatable

 drivers/scsi/storvsc_drv.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH 1/2] scsi: storvsc: Parameterize number hardware queues

[Melanie Plageman (Microsoft)]

Add ability to set the number of hardware queues. The default value
remains the number of CPUs.  This functionality is useful in some
environments (e.g. Microsoft Azure) when 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 | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 2e4fa77445fd..d72ab6daf9ae 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_nr_hw_queues = -1;
 
 module_param(storvsc_ringbuffer_size, int, S_IRUGO);
 MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
 
+module_param(storvsc_nr_hw_queues, int, S_IRUGO);
+MODULE_PARM_DESC(storvsc_nr_hw_queues, "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");
 
@@ -2004,8 +2008,12 @@ 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_nr_hw_queues == -1)
+			host->nr_hw_queues = num_present_cpus();
+		else
+			host->nr_hw_queues = storvsc_nr_hw_queues;
+	}
 
 	/*
 	 * Set the error handler work queue.
@@ -2155,6 +2163,7 @@ static struct fc_function_template fc_transport_functions = {
 static int __init storvsc_drv_init(void)
 {
 	int ret;
+	int ncpus = num_present_cpus();
 
 	/*
 	 * Divide the ring buffer data size (which is 1 page less
@@ -2169,6 +2178,14 @@ static int __init storvsc_drv_init(void)
 		vmscsi_size_delta,
 		sizeof(u64)));
 
+	if (storvsc_nr_hw_queues > ncpus || storvsc_nr_hw_queues == 0 ||
+			storvsc_nr_hw_queues < -1) {
+		printk(KERN_ERR "storvsc: Invalid storvsc_nr_hw_queues value of %d.
+						Valid values include -1 and 1-%d.\n",
+				storvsc_nr_hw_queues, ncpus);
+		return -EINVAL;
+	}
+
 #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
 	fc_transport_template = fc_attach_transport(&fc_transport_functions);
 	if (!fc_transport_template)
-- 
2.20.1

[PATCH 2/2] scsi: storvsc: Make max hw queues updatable

[Melanie Plageman (Microsoft)]

- Change name of parameter to storvsc_max_hw_queues
- Make the parameter updatable on a running system
- Change error to warning for an invalid value of the parameter at
  driver init time

Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
---

Andres wrote:
>> On 2021-02-02 15:19:08 -0500, Melanie Plageman wrote:
>> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
>> index 2e4fa77445fd..d72ab6daf9ae 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_nr_hw_queues = -1;
>>
>>  module_param(storvsc_ringbuffer_size, int, S_IRUGO);
>>  MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size
>>  (bytes)");
>>
>> +module_param(storvsc_nr_hw_queues, int, S_IRUGO);
>> +MODULE_PARM_DESC(storvsc_nr_hw_queues, "Number of hardware queues");
>
> Would be nice if the parameter could be changed in a running
> system. Obviously that's only going to affect devices that'll be
> attached in the future (or detached and reattached), but that's still
> nicer than not being able to change at all.

My problem with making it updatable is that the user can set it to an
invalid value and then that is the value he/she/they will see in
/sys/module/hv_storvsc/parameters/storvsc_max_hw_queues at that point.
However, if they had set it as a boot-time kernel parameter, then the
value would have been corrected to a valid value.

On probe of a new device, if the value of storvsc_max_hw_queues is
invalid, the value of nr_hw_queues would be set to NCPUs, but, at that
point, do I also change the value in
/sys/module/hv_storvsc/parameters/storvsc_max_hw_queues?
If so, there is a weird intermediate period where it has a value that
isn't actually used or valid.
If not, you can update it to an invalid value while the system is
running but not if you set it at boot-time. Despite the fact that this
invalid value is not used at any point by the driver, it seems weird
that you can get it into a state where it doesn't reflect reality
depending on how you set it.

Maybe this isn't too big of a deal. I noticed the other module parameter
than can be set on a running system in storvsc is logging_level, and,
because of the do_logging() function, it doesn't really matter if the
user set it to a number higher or lower than one of the pre-defined log
levels. This behavior makes sense to me for logging, but I'm not sure
how I feel about going with a similar strategy for
storvsc_max_hw_queues.

I've gone with making it updatable and not updating the value of
storvsc_max_hw_queues during storvsc_probe(), even if the user had
updated storvsc_max_hw_queues to an invalid value. I just use NCPUs for
the value of nr_hw_queues.

Is there any user facing documentation on any of this? It would be nice
to add a comment there about what this parameter is and note that if you
change it, you need to reinitiate a probe of your device for it to take
effect and with details on the valid values for it and when they will be
reflected in /sys/module...

Andres wrote:
>> @@ -2155,6 +2163,7 @@ static struct fc_function_template
>> fc_transport_functions = {
>>  static int __init storvsc_drv_init(void)
>>  {
>>   int ret;
>> + int ncpus = num_present_cpus();
>>
>>   /*
>>    * Divide the ring buffer data size (which is 1 page less
>> @@ -2169,6 +2178,14 @@ static int __init storvsc_drv_init(void)
>>     vmscsi_size_delta,
>>     sizeof(u64)));
>>
>> + if (storvsc_nr_hw_queues > ncpus || storvsc_nr_hw_queues == 0 ||
>> +     storvsc_nr_hw_queues < -1) {
>> +   printk(KERN_ERR "storvsc: Invalid storvsc_nr_hw_queues value of
>> %d.
>> +           Valid values include -1 and 1-%d.\n",
>> +       storvsc_nr_hw_queues, ncpus);
>> +   return -EINVAL;
>> + }
>> +
>
> I have a mild preference for just capping the effective value to
> num_present_cpus(), rather than erroring out. Would allow you to
> e.g. cap the number of queues to 4, with the same param specified on
> smaller and bigger systems.  Perhaps renaming it to max_hw_queues
> would
> make that more obvious?

I like max_hw_queues -- I've updated the name to that.

I changed it to update storvsc_max_hw_queues to NCPUs if an invalid
value was provided.
I agree that it was pretty nasty to have it error out. I wanted to avoid
unexpected behavior for the user.
I changed the error to a warning, so, provided the sufficient log level,
a user could find out why the value they set storvsc_max_hw_queues to
does not match the one they see in
/sys/module/hv_storvsc/parameters/storvsc_max_hw_queues

 drivers/scsi/storvsc_drv.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index d72ab6daf9ae..e41f2461851e 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -378,13 +378,13 @@ 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_nr_hw_queues = -1;
+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_nr_hw_queues, int, S_IRUGO);
-MODULE_PARM_DESC(storvsc_nr_hw_queues, "Number of hardware queues");
+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");
@@ -1901,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);
@@ -2009,10 +2010,18 @@ static int storvsc_probe(struct hv_device *device,
 	 * Set the number of HW queues we are supporting.
 	 */
 	if (!dev_is_ide) {
-		if (storvsc_nr_hw_queues == -1)
-			host->nr_hw_queues = num_present_cpus();
+		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,
+				"Setting nr_hw_queues to %d. storvsc_max_hw_queues value %d is invalid.\n",
+				num_present_cpus, storvsc_max_hw_queues);
+			host->nr_hw_queues = num_present_cpus;
+		}
 		else
-			host->nr_hw_queues = storvsc_nr_hw_queues;
+			host->nr_hw_queues = storvsc_max_hw_queues;
 	}
 
 	/*
@@ -2178,12 +2187,11 @@ static int __init storvsc_drv_init(void)
 		vmscsi_size_delta,
 		sizeof(u64)));
 
-	if (storvsc_nr_hw_queues > ncpus || storvsc_nr_hw_queues == 0 ||
-			storvsc_nr_hw_queues < -1) {
-		printk(KERN_ERR "storvsc: Invalid storvsc_nr_hw_queues value of %d.
-						Valid values include -1 and 1-%d.\n",
-				storvsc_nr_hw_queues, ncpus);
-		return -EINVAL;
+	if (storvsc_max_hw_queues > ncpus || storvsc_max_hw_queues == 0 ||
+			storvsc_max_hw_queues < -1) {
+		printk(KERN_WARNING "storvsc: Setting storvsc_max_hw_queues to %d. %d is invalid.\n",
+			ncpus, storvsc_max_hw_queues);
+		storvsc_max_hw_queues = ncpus;
 	}
 
 #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
-- 
2.20.1

Re: [PATCH 2/2] scsi: storvsc: Make max hw queues updatable

[Wei Liu]

Hi Melanie

On Fri, Feb 05, 2021 at 09:24:47PM +0000, Melanie Plageman (Microsoft) wrote:
> - Change name of parameter to storvsc_max_hw_queues
> - Make the parameter updatable on a running system
> - Change error to warning for an invalid value of the parameter at
>   driver init time
> 
> Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>

This patch contains a section which seems to be a copy&paste error from
another email.

Also, please use get_maintainers.pl to CC the correct maintainers. You
can configure git-sendemail such that the correct people are CC'ed
automatically. The storvsc code normally goes via the storage
maintainers' tree.

If you have any question about how to use the get_maintainers.pl script
or the upstreaming process in general, let me know.

Wei.

> ---
> 
> Andres wrote:
> >> On 2021-02-02 15:19:08 -0500, Melanie Plageman wrote:
> >> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> >> index 2e4fa77445fd..d72ab6daf9ae 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
[...]

Re: [PATCH 2/2] scsi: storvsc: Make max hw queues updatable

[Melanie Plageman]

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

> This patch contains a section which seems to be a copy&paste error
> from another email.

I don't think that there are any unintentional portions to the email.
Andres gave me feedback on my patch. I addressed his feedback in an
updated version of the patch but wanted to include additional context
and points which did not belong in the commit message. After some
research online, I was led to believe that doing so below the commit
message was an appropriate way to add relevant context to the patch
which may not belong in the commit message (using --annotate or
--compose with git-send-email). The patch still applied cleanly for me.

My intent was the following:
I received feedback on the code in the patch and wanted to address that
feedback with an updated version of the patch. I also wanted to respond
inline to the specific comments with text to continue the discussion.
Additionally, I wanted to make sure that the new patch showed a clear
diff with the old patch so that it was clear what I changed in response
to feedback. If I had started a new thread with this new version, I
feared a two-patch set would no longer make sense, as the two commits
should be fixed up together in a final version.

What would have been the appropriate way to achieve this that aligns
more closely with the convention of the list?

> Also, please use get_maintainers.pl to CC the correct maintainers. You
> can configure git-sendemail such that the correct people are CC'ed
> automatically. The storvsc code normally goes via the storage
> maintainers' tree.
>
> If you have any question about how to use the get_maintainers.pl script
> or the upstreaming process in general, let me know.

I created this CC list after running get_maintainers.pl on my patch (and
then used git-send-email to send it). It returned these people and
lists:

K. Y. Srinivasan [kys@microsoft.com]
Haiyang Zhang [haiyangz@microsoft.com]
Stephen Hemminger [sthemmin@microsoft.com]
Sasha Levin [sashal@kernel.org]

James E.J. Bottomley [jejb@linux.ibm.com]
Martin K. Petersen [martin.petersen@oracle.com]

[linux-hyperv@vger.kernel.org]
[linux-scsi@vger.kernel.org]
[linux-kernel@vger.kernel.org]

I chose to CC the four people listed as suporters for Hyper-V core and drivers
and exclude SCSI maintainers, as this patch doesn't seem to have much impact
outside of storvsc. Same rationale for excluding the SCSI list and main linux
kernel list.

Regards,
Melanie