| Message ID | 1653035003-70312-1-git-send-email-john.garry@huawei.com |
|---|---|
| Headers | show |
| Series | DMA mapping changes for SCSI core | expand |
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>
On 5/20/22 17:23, John Garry wrote: > Streaming DMA mapping involving an IOMMU may be much slower for larger > total mapping size. This is because every IOMMU DMA mapping requires an > IOVA to be allocated and freed. IOVA sizes above a certain limit are not > cached, which can have a big impact on DMA mapping performance. > > Provide an API for device drivers to know this "optimal" limit, such that > they may try to produce mapping which don't exceed it. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > Documentation/core-api/dma-api.rst | 9 +++++++++ > include/linux/dma-map-ops.h | 1 + > include/linux/dma-mapping.h | 5 +++++ > kernel/dma/mapping.c | 12 ++++++++++++ > 4 files changed, 27 insertions(+) > > diff --git a/Documentation/core-api/dma-api.rst b/Documentation/core-api/dma-api.rst > index 6d6d0edd2d27..b3cd9763d28b 100644 > --- a/Documentation/core-api/dma-api.rst > +++ b/Documentation/core-api/dma-api.rst > @@ -204,6 +204,15 @@ Returns the maximum size of a mapping for the device. The size parameter > of the mapping functions like dma_map_single(), dma_map_page() and > others should not be larger than the returned value. > > +:: > + > + size_t > + dma_opt_mapping_size(struct device *dev); > + > +Returns the maximum optimal size of a mapping for the device. Mapping large > +buffers may take longer so device drivers are advised to limit total DMA > +streaming mappings length to the returned value. > + > :: > > bool > diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h > index 0d5b06b3a4a6..98ceba6fa848 100644 > --- a/include/linux/dma-map-ops.h > +++ b/include/linux/dma-map-ops.h > @@ -69,6 +69,7 @@ struct dma_map_ops { > int (*dma_supported)(struct device *dev, u64 mask); > u64 (*get_required_mask)(struct device *dev); > size_t (*max_mapping_size)(struct device *dev); > + size_t (*opt_mapping_size)(void); > unsigned long (*get_merge_boundary)(struct device *dev); > }; > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index dca2b1355bb1..fe3849434b2a 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -144,6 +144,7 @@ int dma_set_mask(struct device *dev, u64 mask); > int dma_set_coherent_mask(struct device *dev, u64 mask); > u64 dma_get_required_mask(struct device *dev); > size_t dma_max_mapping_size(struct device *dev); > +size_t dma_opt_mapping_size(struct device *dev); > bool dma_need_sync(struct device *dev, dma_addr_t dma_addr); > unsigned long dma_get_merge_boundary(struct device *dev); > struct sg_table *dma_alloc_noncontiguous(struct device *dev, size_t size, > @@ -266,6 +267,10 @@ static inline size_t dma_max_mapping_size(struct device *dev) > { > return 0; > } > +static inline size_t dma_opt_mapping_size(struct device *dev) > +{ > + return 0; > +} > static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr) > { > return false; > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c > index db7244291b74..1bfe11b1edb6 100644 > --- a/kernel/dma/mapping.c > +++ b/kernel/dma/mapping.c > @@ -773,6 +773,18 @@ size_t dma_max_mapping_size(struct device *dev) > } > EXPORT_SYMBOL_GPL(dma_max_mapping_size); > > +size_t dma_opt_mapping_size(struct device *dev) > +{ > + const struct dma_map_ops *ops = get_dma_ops(dev); > + size_t size = SIZE_MAX; > + > + if (ops && ops->opt_mapping_size) > + size = ops->opt_mapping_size(); > + > + return min(dma_max_mapping_size(dev), size); > +} > +EXPORT_SYMBOL_GPL(dma_opt_mapping_size); > + > bool dma_need_sync(struct device *dev, dma_addr_t dma_addr) > { > const struct dma_map_ops *ops = get_dma_ops(dev); Looks OK to me. Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
The whole series looks fine to me. I'll happily queue it up in the dma-mapping tree if the SCSI and ATA maintainers are ok with that.
On 2022/05/22 22:13, Christoph Hellwig wrote: > The whole series looks fine to me. I'll happily queue it up in the > dma-mapping tree if the SCSI and ATA maintainers are ok with that. > Fine with me. I sent an acked-by for the libata bit.
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;
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; >
On 22/05/2022 23:22, Damien Le Moal wrote: > On 2022/05/22 22:13, Christoph Hellwig wrote: >> The whole series looks fine to me. I'll happily queue it up in the >> dma-mapping tree if the SCSI and ATA maintainers are ok with that. >> > > Fine with me. I sent an acked-by for the libata bit. > Thanks, I'm going to have to post a v2 and I figure that with the timing that I'll have to wait for v5.20 now. Cheers, John
Christoph, > The whole series looks fine to me. I'll happily queue it up in the > dma-mapping tree if the SCSI and ATA maintainers are ok with that. Works for me. Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
As reported in [0], DMA mappings whose size exceeds the IOMMU IOVA caching limit may see a big performance hit. This series introduces a new DMA mapping API, dma_opt_mapping_size(), so that drivers may know this limit when performance is a factor in the mapping. Robin didn't like using dma_max_mapping_size() for this [1]. An alternative to adding the new API would be to add a "hard_limit" arg to dma_max_mapping_size(). This would mean fixing up all current users, but it would be good to do that anyway as not all users require a hard limit. The SCSI core coded is modified to use this limit. I also added a patch for libata-scsi as it does not currently honour the shost max_sectors limit. [0] https://lore.kernel.org/linux-iommu/20210129092120.1482-1-thunder.leizhen@huawei.com/ [1] https://lore.kernel.org/linux-iommu/f5b78c9c-312e-70ab-ecbb-f14623a4b6e3@arm.com/ John Garry (4): dma-mapping: Add dma_opt_mapping_size() dma-iommu: Add iommu_dma_opt_mapping_size() scsi: core: Cap shost max_sectors according to DMA optimum mapping limits libata-scsi: Cap ata_device->max_sectors according to shost->max_sectors Documentation/core-api/dma-api.rst | 9 +++++++++ drivers/ata/libata-scsi.c | 1 + drivers/iommu/dma-iommu.c | 6 ++++++ drivers/iommu/iova.c | 5 +++++ drivers/scsi/hosts.c | 5 +++++ drivers/scsi/scsi_lib.c | 4 ---- include/linux/dma-map-ops.h | 1 + include/linux/dma-mapping.h | 5 +++++ include/linux/iova.h | 2 ++ kernel/dma/mapping.c | 12 ++++++++++++ 10 files changed, 46 insertions(+), 4 deletions(-)