| Message ID | 1560421215-10750-6-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | treewide: improve R-Car SDHI performance | expand |
> - blk_queue_max_segments(mq->queue, host->max_segs); > + /* blk_queue_can_use_iommu_merging() should succeed if can_merge = 1 */ > + if (host->can_merge && > + !blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host))) > + WARN_ON(1); > + blk_queue_max_segments(mq->queue, mmc_get_max_segments(host)); Maybe we could use WARN here to save the comment and move the info to the printout? - blk_queue_max_segments(mq->queue, host->max_segs); + if (host->can_merge) + WARN(!blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host)), + "merging was advertised but not possible\n"); + blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
On Thu, Jun 13, 2019 at 07:20:15PM +0900, Yoshihiro Shimoda wrote: > +static unsigned int mmc_get_max_segments(struct mmc_host *host) > +{ > + return host->can_merge ? BLK_MAX_SEGMENTS : host->max_segs; > +} Note that BLK_MAX_SEGMENTS is really a little misnamed, it just is a BLK_DEFAULT_SEGMENTS. I think you are better of picking your own value here (even if 128 ends up ok) than reusing this somewhat confusing constant. > + /* > + * Since blk_mq_alloc_tag_set() calls .init_request() of mmc_mq_ops, > + * the host->can_merge should be set before to get max_segs from > + * mmc_get_max_segments(). > + */ > + if (host->max_segs < BLK_MAX_SEGMENTS && > + device_iommu_mapped(mmc_dev(host))) > + host->can_merge = 1; > + else > + host->can_merge = 0; > + can_merge seems a little too generic a name to me. Maybe can_iommu_merge?
> > + host->can_merge = 1; > > + else > > + host->can_merge = 0; > > + > > can_merge seems a little too generic a name to me. Maybe can_iommu_merge? Ack.
Hi Wolfram-san, > From: Wolfram Sang, Sent: Friday, June 14, 2019 4:59 AM > > > - blk_queue_max_segments(mq->queue, host->max_segs); > > + /* blk_queue_can_use_iommu_merging() should succeed if can_merge = 1 */ > > + if (host->can_merge && > > + !blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host))) > > + WARN_ON(1); > > + blk_queue_max_segments(mq->queue, mmc_get_max_segments(host)); > > Maybe we could use WARN here to save the comment and move the info to > the printout? > > - blk_queue_max_segments(mq->queue, host->max_segs); > + if (host->can_merge) > + WARN(!blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host)), > + "merging was advertised but not possible\n"); > + blk_queue_max_segments(mq->queue, mmc_get_max_segments(host)); Thank you for the suggestion. It's a good idea! I'll fix the patch. Best regards, Yoshihiro Shimoda
Hi Christoph, > From: Christoph Hellwig, Sent: Friday, June 14, 2019 4:25 PM > > On Thu, Jun 13, 2019 at 07:20:15PM +0900, Yoshihiro Shimoda wrote: > > +static unsigned int mmc_get_max_segments(struct mmc_host *host) > > +{ > > + return host->can_merge ? BLK_MAX_SEGMENTS : host->max_segs; > > +} > > Note that BLK_MAX_SEGMENTS is really a little misnamed, it just > is a BLK_DEFAULT_SEGMENTS. I think you are better of picking your > own value here (even if 128 ends up ok) than reusing this somewhat > confusing constant. Thank you for your comments. I got it. I'll fix this. > > + /* > > + * Since blk_mq_alloc_tag_set() calls .init_request() of mmc_mq_ops, > > + * the host->can_merge should be set before to get max_segs from > > + * mmc_get_max_segments(). > > + */ > > + if (host->max_segs < BLK_MAX_SEGMENTS && > > + device_iommu_mapped(mmc_dev(host))) > > + host->can_merge = 1; > > + else > > + host->can_merge = 0; > > + > > can_merge seems a little too generic a name to me. Maybe can_iommu_merge? I'll fix the name. Also, only the device_iommu_mapped() condition wiil cause a problem on iommu=pt [1]. So, I'll add another condition here. [1] https://marc.info/?l=linux-mmc&m=156050608709643&w=2 Best regards, Yoshihiro Shimoda
On Mon, Jun 17, 2019 at 06:46:33AM +0000, Yoshihiro Shimoda wrote: > > can_merge seems a little too generic a name to me. Maybe can_iommu_merge? > > I'll fix the name. Also, only the device_iommu_mapped() condition wiil cause > a problem on iommu=pt [1]. So, I'll add another condition here. Instead of adding another condition here I think we need to properly abstract it out in the DMA layer. E.g. have a unsigned long dma_get_merge_boundary(struct device *dev) { const struct dma_map_ops *ops = get_dma_ops(dev); if (!ops || !ops->get_merge_boundary) return 0; /* can't merge */ return ops->get_merge_boundary(dev); } and then implement the method in dma-iommu.c. blk_queue_can_use_iommu_merging then comes: bool blk_queue_enable_iommu_merging(struct request_queue *q, struct device *dev) { unsigned long boundary = dma_get_merge_boundary(dev); if (!boundary) return false; blk_queue_virt_boundary(q, boundary); return true; }
Hi Christoph, > From: Christoph Hellwig, Sent: Monday, June 17, 2019 3:54 PM > > On Mon, Jun 17, 2019 at 06:46:33AM +0000, Yoshihiro Shimoda wrote: > > > can_merge seems a little too generic a name to me. Maybe can_iommu_merge? > > > > I'll fix the name. Also, only the device_iommu_mapped() condition wiil cause > > a problem on iommu=pt [1]. So, I'll add another condition here. > > Instead of adding another condition here I think we need to properly > abstract it out in the DMA layer. E.g. have a Thank you for your comment and sample code! I'll add such functions on next patch series. Best regards, Yoshihiro Shimoda > unsigned long dma_get_merge_boundary(struct device *dev) > { > const struct dma_map_ops *ops = get_dma_ops(dev); > > if (!ops || !ops->get_merge_boundary) > return 0; /* can't merge */ > return ops->get_merge_boundary(dev); > } > > and then implement the method in dma-iommu.c. > > blk_queue_can_use_iommu_merging then comes: > > bool blk_queue_enable_iommu_merging(struct request_queue *q, > struct device *dev) > { > unsigned long boundary = dma_get_merge_boundary(dev); > > if (!boundary) > return false; > blk_queue_virt_boundary(q, boundary); > return true; > }
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c index b5b9c61..59d7606 100644 --- a/drivers/mmc/core/queue.c +++ b/drivers/mmc/core/queue.c @@ -196,6 +196,11 @@ static void mmc_queue_setup_discard(struct request_queue *q, blk_queue_flag_set(QUEUE_FLAG_SECERASE, q); } +static unsigned int mmc_get_max_segments(struct mmc_host *host) +{ + return host->can_merge ? BLK_MAX_SEGMENTS : host->max_segs; +} + /** * mmc_init_request() - initialize the MMC-specific per-request data * @q: the request queue @@ -209,7 +214,7 @@ static int __mmc_init_request(struct mmc_queue *mq, struct request *req, struct mmc_card *card = mq->card; struct mmc_host *host = card->host; - mq_rq->sg = mmc_alloc_sg(host->max_segs, gfp); + mq_rq->sg = mmc_alloc_sg(mmc_get_max_segments(host), gfp); if (!mq_rq->sg) return -ENOMEM; @@ -368,15 +373,24 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card) blk_queue_bounce_limit(mq->queue, limit); blk_queue_max_hw_sectors(mq->queue, min(host->max_blk_count, host->max_req_size / 512)); - blk_queue_max_segments(mq->queue, host->max_segs); + /* blk_queue_can_use_iommu_merging() should succeed if can_merge = 1 */ + if (host->can_merge && + !blk_queue_can_use_iommu_merging(mq->queue, mmc_dev(host))) + WARN_ON(1); + blk_queue_max_segments(mq->queue, mmc_get_max_segments(host)); if (mmc_card_mmc(card)) block_size = card->ext_csd.data_sector_size; blk_queue_logical_block_size(mq->queue, block_size); - blk_queue_max_segment_size(mq->queue, + /* + * After blk_queue_can_use_iommu_merging() was called with succeed, + * since it calls blk_queue_virt_boundary for IOMMU, the mmc should + * not call blk_queue_max_segment_size(). + */ + if (!host->can_merge) + blk_queue_max_segment_size(mq->queue, round_down(host->max_seg_size, block_size)); - INIT_WORK(&mq->recovery_work, mmc_mq_recovery_handler); INIT_WORK(&mq->complete_work, mmc_blk_mq_complete_work); @@ -422,6 +436,17 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) mq->tag_set.cmd_size = sizeof(struct mmc_queue_req); mq->tag_set.driver_data = mq; + /* + * Since blk_mq_alloc_tag_set() calls .init_request() of mmc_mq_ops, + * the host->can_merge should be set before to get max_segs from + * mmc_get_max_segments(). + */ + if (host->max_segs < BLK_MAX_SEGMENTS && + device_iommu_mapped(mmc_dev(host))) + host->can_merge = 1; + else + host->can_merge = 0; + ret = blk_mq_alloc_tag_set(&mq->tag_set); if (ret) return ret; diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 43d0f0c..84b9bef 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -398,6 +398,7 @@ struct mmc_host { unsigned int retune_now:1; /* do re-tuning at next req */ unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ unsigned int use_blk_mq:1; /* use blk-mq */ + unsigned int can_merge:1; /* merging can be used */ int rescan_disable; /* disable card detection */ int rescan_entered; /* used with nonremovable devices */
If max_segs of a mmc host is smaller than BLK_MAX_SEGMENTS, the mmc subsystem tries to use such bigger segments by using IOMMU subsystem, and then the mmc subsystem exposes such information to the block layer by using blk_queue_can_use_iommu_merging(). Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/mmc/core/queue.c | 33 +++++++++++++++++++++++++++++---- include/linux/mmc/host.h | 1 + 2 files changed, 30 insertions(+), 4 deletions(-)