diff mbox series

[v6,4/6] scsi: sd: Allow max_sectors be capped at DMA optimal size limit

Message ID 1657797329-98541-5-git-send-email-john.garry@huawei.com (mailing list archive)
State Not Applicable
Headers show
Series DMA mapping changes for SCSI core | expand

Commit Message

John Garry July 14, 2022, 11:15 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.

New member Scsi_Host.opt_sectors is added, which is the optimal host
max_sectors, and use this value to cap the request queue max_sectors when
set.

It could be considered to have request queues io_opt value initially
set at Scsi_Host.opt_sectors in __scsi_init_queue(), but that is not
really the purpose of io_opt.

Finally, even though Scsi_Host.opt_sectors value should never be greater
than the request queue max_hw_sectors value, continue to limit to this
value for safety.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/sd.c        | 2 ++
 include/scsi/scsi_host.h | 1 +
 2 files changed, 3 insertions(+)

Comments

Damien Le Moal July 18, 2022, 10:47 a.m. UTC | #1
On 7/14/22 20:15, 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.
> 
> New member Scsi_Host.opt_sectors is added, which is the optimal host
> max_sectors, and use this value to cap the request queue max_sectors when
> set.
> 
> It could be considered to have request queues io_opt value initially
> set at Scsi_Host.opt_sectors in __scsi_init_queue(), but that is not
> really the purpose of io_opt.
> 
> Finally, even though Scsi_Host.opt_sectors value should never be greater
> than the request queue max_hw_sectors value, continue to limit to this
> value for safety.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/sd.c        | 2 ++
>  include/scsi/scsi_host.h | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a1a2ac09066f..3eaee1f7aaca 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3296,6 +3296,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  				      (sector_t)BLK_DEF_MAX_SECTORS);
>  	}
>  
> +	rw_max = min_not_zero(rw_max, sdp->host->opt_sectors);
> +

Adding a comment explaining what the cap is would be nice.

>  	/* Do not exceed controller limit */
>  	rw_max = min(rw_max, queue_max_hw_sectors(q));
>  
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 667d889b92b5..d32a84b2bb40 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -607,6 +607,7 @@ struct Scsi_Host {
>  	short unsigned int sg_tablesize;
>  	short unsigned int sg_prot_tablesize;
>  	unsigned int max_sectors;
> +	unsigned int opt_sectors;
>  	unsigned int max_segment_size;
>  	unsigned long dma_boundary;
>  	unsigned long virt_boundary_mask;

Otherwise, looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Martin K. Petersen July 19, 2022, 2:30 a.m. UTC | #2
John,

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a1a2ac09066f..3eaee1f7aaca 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3296,6 +3296,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  				      (sector_t)BLK_DEF_MAX_SECTORS);
>  	}
>  
> +	rw_max = min_not_zero(rw_max, sdp->host->opt_sectors);
> +
>  	/* Do not exceed controller limit */
>  	rw_max = min(rw_max, queue_max_hw_sectors(q));

I'm OK with this approach.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
John Garry July 19, 2022, 7:05 a.m. UTC | #3
On 18/07/2022 11:47, Damien Le Moal wrote:
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index a1a2ac09066f..3eaee1f7aaca 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -3296,6 +3296,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
>>   				      (sector_t)BLK_DEF_MAX_SECTORS);
>>   	}

Hi Damien,

>>   
>> +	rw_max = min_not_zero(rw_max, sdp->host->opt_sectors);
>> +
> Adding a comment explaining what the cap is would be nice.
> 


Christoph has now applied this (thanks, BTW), so would you like me to 
follow up with a patch on top with a comment?

Thanks,
John
Christoph Hellwig July 19, 2022, 7:10 a.m. UTC | #4
On Tue, Jul 19, 2022 at 08:05:30AM +0100, John Garry wrote:
> Christoph has now applied this (thanks, BTW), so would you like me to 
> follow up with a patch on top with a comment?

Please do.
John Garry July 19, 2022, 9:10 a.m. UTC | #5
On 19/07/2022 08:10, Christoph Hellwig wrote:
> On Tue, Jul 19, 2022 at 08:05:30AM +0100, John Garry wrote:
>> Christoph has now applied this (thanks, BTW), so would you like me to
>> follow up with a patch on top with a comment?
> Please do.

ok, fine, I'll do it now.

Just saying in case it's an issue - I was looking at 
http://git.infradead.org/users/hch/dma-mapping.git/log/refs/heads/for-next 
and the order is not the same as this series and would cause an 
intermediate build breakage at 9f5ec52ae501 ("scsi: scsi_transport_sas: 
cap shost opt_sectors according to DMA optimal limit")

Cheers,
John
Christoph Hellwig July 19, 2022, 9:17 a.m. UTC | #6
On Tue, Jul 19, 2022 at 10:10:22AM +0100, John Garry wrote:
> Just saying in case it's an issue - I was looking at 
> http://git.infradead.org/users/hch/dma-mapping.git/log/refs/heads/for-next 
> and the order is not the same as this series and would cause an 
> intermediate build breakage at 9f5ec52ae501 ("scsi: scsi_transport_sas: cap 
> shost opt_sectors according to DMA optimal limit")

No idea what git-am did there, I've fixed it up now.
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a1a2ac09066f..3eaee1f7aaca 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3296,6 +3296,8 @@  static int sd_revalidate_disk(struct gendisk *disk)
 				      (sector_t)BLK_DEF_MAX_SECTORS);
 	}
 
+	rw_max = min_not_zero(rw_max, sdp->host->opt_sectors);
+
 	/* Do not exceed controller limit */
 	rw_max = min(rw_max, queue_max_hw_sectors(q));
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 667d889b92b5..d32a84b2bb40 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -607,6 +607,7 @@  struct Scsi_Host {
 	short unsigned int sg_tablesize;
 	short unsigned int sg_prot_tablesize;
 	unsigned int max_sectors;
+	unsigned int opt_sectors;
 	unsigned int max_segment_size;
 	unsigned long dma_boundary;
 	unsigned long virt_boundary_mask;