diff mbox series

[3/4] scsi: core: Cap shost max_sectors according to DMA optimum mapping limits

Message ID 1653035003-70312-4-git-send-email-john.garry@huawei.com
State New
Headers show
Series DMA mapping changes for SCSI core | expand

Commit Message

John Garry May 20, 2022, 8:23 a.m. UTC 
Streaming DMA mappings may be considerably slower when mappings go through
an IOMMU and the total mapping length is somewhat long. This is because the
IOMMU IOVA code allocates and free an IOVA for each mapping, which may
affect performance.

For performance reasons set the request_queue max_sectors from
dma_opt_mapping_size(), which knows this mapping limit.

In addition, the shost->max_sectors is repeatedly set for each sdev in
__scsi_init_queue(). This is unnecessary, so set once when adding the
host.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hosts.c    | 5 +++++
 drivers/scsi/scsi_lib.c | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Damien Le Moal May 20, 2022, 11:30 p.m. UTC | #1 
On 5/20/22 17:23, John Garry wrote:
> Streaming DMA mappings may be considerably slower when mappings go through
> an IOMMU and the total mapping length is somewhat long. This is because the
> IOMMU IOVA code allocates and free an IOVA for each mapping, which may
> affect performance.
> 
> For performance reasons set the request_queue max_sectors from
> dma_opt_mapping_size(), which knows this mapping limit.
> 
> In addition, the shost->max_sectors is repeatedly set for each sdev in
> __scsi_init_queue(). This is unnecessary, so set once when adding the
> host.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hosts.c    | 5 +++++
>  drivers/scsi/scsi_lib.c | 4 ----
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index f69b77cbf538..a3ae6345473b 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -225,6 +225,11 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>  	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
>  				   shost->can_queue);
>  
> +	if (dma_dev->dma_mask) {
> +		shost->max_sectors = min_t(unsigned int, shost->max_sectors,
> +				dma_opt_mapping_size(dma_dev) >> SECTOR_SHIFT);
> +	}

Nit: you could drop the curly brackets here.

> +
>  	error = scsi_init_sense_cache(shost);
>  	if (error)
>  		goto fail;
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8d18cc7e510e..2d43bb8799bd 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1884,10 +1884,6 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
>  		blk_queue_max_integrity_segments(q, shost->sg_prot_tablesize);
>  	}
>  
> -	if (dev->dma_mask) {
> -		shost->max_sectors = min_t(unsigned int, shost->max_sectors,
> -				dma_max_mapping_size(dev) >> SECTOR_SHIFT);
> -	}
>  	blk_queue_max_hw_sectors(q, shost->max_sectors);
>  	blk_queue_segment_boundary(q, shost->dma_boundary);
>  	dma_set_seg_boundary(dev, shost->dma_boundary);

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
John Garry May 23, 2022, 6:53 a.m. UTC | #2 
On 21/05/2022 00:30, Damien Le Moal wrote:
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index f69b77cbf538..a3ae6345473b 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -225,6 +225,11 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>>   	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
>>   				   shost->can_queue);
>>   

Hi Damien,

>> +	if (dma_dev->dma_mask) {
>> +		shost->max_sectors = min_t(unsigned int, shost->max_sectors,
>> +				dma_opt_mapping_size(dma_dev) >> SECTOR_SHIFT);
>> +	}
> Nit: you could drop the curly brackets here.

Some people prefer this style - multi-line statements have curly 
brackets, while single-line statements conform to the official coding 
style (and don't use brackets).

I'll just stick with what we have unless there is a consensus to change.

Thanks,
John

> 
>> +
>>   	error = scsi_init_sense_cache(shost);
>>   	if (error)
>>   		goto fail;
Damien Le Moal May 23, 2022, 7:33 a.m. UTC | #3 
On 2022/05/23 15:53, John Garry wrote:
> On 21/05/2022 00:30, Damien Le Moal wrote:
>>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>>> index f69b77cbf538..a3ae6345473b 100644
>>> --- a/drivers/scsi/hosts.c
>>> +++ b/drivers/scsi/hosts.c
>>> @@ -225,6 +225,11 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>>>   	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
>>>   				   shost->can_queue);
>>>   
> 
> Hi Damien,
> 
>>> +	if (dma_dev->dma_mask) {
>>> +		shost->max_sectors = min_t(unsigned int, shost->max_sectors,
>>> +				dma_opt_mapping_size(dma_dev) >> SECTOR_SHIFT);
>>> +	}
>> Nit: you could drop the curly brackets here.
> 
> Some people prefer this style - multi-line statements have curly 
> brackets, while single-line statements conform to the official coding 
> style (and don't use brackets).

OK.

> 
> I'll just stick with what we have unless there is a consensus to change.
> 
> Thanks,
> John
> 
>>
>>> +
>>>   	error = scsi_init_sense_cache(shost);
>>>   	if (error)
>>>   		goto fail;
>
Dan Carpenter May 23, 2022, 11:08 a.m. UTC | #4 
Hi John,

url:    https://github.com/intel-lab-lkp/linux/commits/John-Garry/DMA-mapping-changes-for-SCSI-core/20220520-163049
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: s390-randconfig-m031-20220519 (https://download.01.org/0day-ci/archive/20220521/202205210545.gkS834ds-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 11.3.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/scsi/hosts.c:243 scsi_add_host_with_dma() warn: variable dereferenced before check 'dma_dev' (see line 228)

vim +/dma_dev +243 drivers/scsi/hosts.c

d139b9bd0e52dd James Bottomley   2009-11-05  209  int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
d139b9bd0e52dd James Bottomley   2009-11-05  210  			   struct device *dma_dev)
^1da177e4c3f41 Linus Torvalds    2005-04-16  211  {
^1da177e4c3f41 Linus Torvalds    2005-04-16  212  	struct scsi_host_template *sht = shost->hostt;
^1da177e4c3f41 Linus Torvalds    2005-04-16  213  	int error = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  214  
91921e016a2199 Hannes Reinecke   2014-06-25  215  	shost_printk(KERN_INFO, shost, "%s\n",
^1da177e4c3f41 Linus Torvalds    2005-04-16  216  			sht->info ? sht->info(shost) : sht->name);
^1da177e4c3f41 Linus Torvalds    2005-04-16  217  
^1da177e4c3f41 Linus Torvalds    2005-04-16  218  	if (!shost->can_queue) {
91921e016a2199 Hannes Reinecke   2014-06-25  219  		shost_printk(KERN_ERR, shost,
91921e016a2199 Hannes Reinecke   2014-06-25  220  			     "can_queue = 0 no longer supported\n");
542bd1377a9630 James Bottomley   2008-04-21  221  		goto fail;
^1da177e4c3f41 Linus Torvalds    2005-04-16  222  	}
^1da177e4c3f41 Linus Torvalds    2005-04-16  223  
50b6cb3516365c Dexuan Cui        2021-10-07  224  	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
50b6cb3516365c Dexuan Cui        2021-10-07  225  	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
ea2f0f77538c50 John Garry        2021-05-19  226  				   shost->can_queue);
ea2f0f77538c50 John Garry        2021-05-19  227  
2ad7ba6ca08593 John Garry        2022-05-20 @228  	if (dma_dev->dma_mask) {
                                                            ^^^^^^^^^^^^^^^^^
The patch adds a new unchecked dereference

2ad7ba6ca08593 John Garry        2022-05-20  229  		shost->max_sectors = min_t(unsigned int, shost->max_sectors,
2ad7ba6ca08593 John Garry        2022-05-20  230  				dma_opt_mapping_size(dma_dev) >> SECTOR_SHIFT);
2ad7ba6ca08593 John Garry        2022-05-20  231  	}
2ad7ba6ca08593 John Garry        2022-05-20  232  
0a6ac4ee7c2109 Christoph Hellwig 2017-01-03  233  	error = scsi_init_sense_cache(shost);
0a6ac4ee7c2109 Christoph Hellwig 2017-01-03  234  	if (error)
0a6ac4ee7c2109 Christoph Hellwig 2017-01-03  235  		goto fail;
0a6ac4ee7c2109 Christoph Hellwig 2017-01-03  236  
d285203cf647d7 Christoph Hellwig 2014-01-17  237  	error = scsi_mq_setup_tags(shost);
542bd1377a9630 James Bottomley   2008-04-21  238  	if (error)
542bd1377a9630 James Bottomley   2008-04-21  239  		goto fail;
d285203cf647d7 Christoph Hellwig 2014-01-17  240  
^1da177e4c3f41 Linus Torvalds    2005-04-16  241  	if (!shost->shost_gendev.parent)
^1da177e4c3f41 Linus Torvalds    2005-04-16  242  		shost->shost_gendev.parent = dev ? dev : &platform_bus;
3c8d9a957d0ae6 James Bottomley   2012-05-04 @243  	if (!dma_dev)
                                                            ^^^^^^^^
The old code checked for NULL

3c8d9a957d0ae6 James Bottomley   2012-05-04  244  		dma_dev = shost->shost_gendev.parent;
3c8d9a957d0ae6 James Bottomley   2012-05-04  245  
d139b9bd0e52dd James Bottomley   2009-11-05  246  	shost->dma_dev = dma_dev;
^1da177e4c3f41 Linus Torvalds    2005-04-16  247  
5c6fab9d558470 Mika Westerberg   2016-02-18  248  	/*
5c6fab9d558470 Mika Westerberg   2016-02-18  249  	 * Increase usage count temporarily here so that calling
5c6fab9d558470 Mika Westerberg   2016-02-18  250  	 * scsi_autopm_put_host() will trigger runtime idle if there is
5c6fab9d558470 Mika Westerberg   2016-02-18  251  	 * nothing else preventing suspending the device.
5c6fab9d558470 Mika Westerberg   2016-02-18  252  	 */
5c6fab9d558470 Mika Westerberg   2016-02-18  253  	pm_runtime_get_noresume(&shost->shost_gendev);
bc4f24014de58f Alan Stern        2010-06-17  254  	pm_runtime_set_active(&shost->shost_gendev);
bc4f24014de58f Alan Stern        2010-06-17  255  	pm_runtime_enable(&shost->shost_gendev);
bc4f24014de58f Alan Stern        2010-06-17  256  	device_enable_async_suspend(&shost->shost_gendev);
bc4f24014de58f Alan Stern        2010-06-17  257
John Garry May 23, 2022, 11:56 a.m. UTC | #5 
On 23/05/2022 12:08, Dan Carpenter wrote:

Thanks for the report

> 50b6cb3516365c Dexuan Cui        2021-10-07  224  	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
> 50b6cb3516365c Dexuan Cui        2021-10-07  225  	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
> ea2f0f77538c50 John Garry        2021-05-19  226  				   shost->can_queue);
> ea2f0f77538c50 John Garry        2021-05-19  227
> 2ad7ba6ca08593 John Garry        2022-05-20 @228  	if (dma_dev->dma_mask) {
>                                                              ^^^^^^^^^^^^^^^^^

I knew that we fixed up dma_dev to be non-NULL, but I thought it was 
earlier in this function...

> The patch adds a new unchecked dereference
> 
> 2ad7ba6ca08593 John Garry        2022-05-20  229  		shost->max_sectors = min_t(unsigned int, shost->max_sectors,
> 2ad7ba6ca08593 John Garry        2022-05-20  230  				dma_opt_mapping_size(dma_dev) >> SECTOR_SHIFT);
> 2ad7ba6ca08593 John Garry        2022-05-20  231  	}
> 2ad7ba6ca08593 John Garry        2022-05-20  232
> 0a6ac4ee7c2109 Christoph Hellwig 2017-01-03  233  	error = scsi_init_sense_cache(shost);
> 0a6ac4ee7c2109 Christoph Hellwig 2017-01-03  234  	if (error)
> 0a6ac4ee7c2109 Christoph Hellwig 2017-01-03  235  		goto fail;
> 0a6ac4ee7c2109 Christoph Hellwig 2017-01-03  236
> d285203cf647d7 Christoph Hellwig 2014-01-17  237  	error = scsi_mq_setup_tags(shost);
> 542bd1377a9630 James Bottomley   2008-04-21  238  	if (error)
> 542bd1377a9630 James Bottomley   2008-04-21  239  		goto fail;
> d285203cf647d7 Christoph Hellwig 2014-01-17  240
> ^1da177e4c3f41 Linus Torvalds    2005-04-16  241  	if (!shost->shost_gendev.parent)
> ^1da177e4c3f41 Linus Torvalds    2005-04-16  242  		shost->shost_gendev.parent = dev ? dev : &platform_bus;
> 3c8d9a957d0ae6 James Bottomley   2012-05-04 @243  	if (!dma_dev)
>                                                              ^^^^^^^^

Cheers,
John
diff mbox series

Patch

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index f69b77cbf538..a3ae6345473b 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -225,6 +225,11 @@  int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
 				   shost->can_queue);
 
+	if (dma_dev->dma_mask) {
+		shost->max_sectors = min_t(unsigned int, shost->max_sectors,
+				dma_opt_mapping_size(dma_dev) >> SECTOR_SHIFT);
+	}
+
 	error = scsi_init_sense_cache(shost);
 	if (error)
 		goto fail;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8d18cc7e510e..2d43bb8799bd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1884,10 +1884,6 @@  void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 		blk_queue_max_integrity_segments(q, shost->sg_prot_tablesize);
 	}
 
-	if (dev->dma_mask) {
-		shost->max_sectors = min_t(unsigned int, shost->max_sectors,
-				dma_max_mapping_size(dev) >> SECTOR_SHIFT);
-	}
 	blk_queue_max_hw_sectors(q, shost->max_sectors);
 	blk_queue_segment_boundary(q, shost->dma_boundary);
 	dma_set_seg_boundary(dev, shost->dma_boundary);