| 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 |
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>
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>
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
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.
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
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 --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;
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(+)