[RFC,v6,5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
diff mbox series

Message ID 1560421215-10750-6-git-send-email-yoshihiro.shimoda.uh@renesas.com
State New
Headers show
Series
  • treewide: improve R-Car SDHI performance
Related show

Commit Message

Yoshihiro Shimoda June 13, 2019, 10:20 a.m. UTC
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(-)

Comments

Wolfram Sang June 13, 2019, 7:58 p.m. UTC | #1
> -	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));
Christoph Hellwig June 14, 2019, 7:24 a.m. UTC | #2
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?
Wolfram Sang June 14, 2019, 10:42 a.m. UTC | #3
> > +		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.
Yoshihiro Shimoda June 17, 2019, 6:38 a.m. UTC | #4
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
Yoshihiro Shimoda June 17, 2019, 6:46 a.m. UTC | #5
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
Christoph Hellwig June 17, 2019, 6:53 a.m. UTC | #6
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;
}
Yoshihiro Shimoda June 17, 2019, 7:02 a.m. UTC | #7
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;
> }

Patch
diff mbox series

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 */