RE: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
Hi Christoph,
> From: Christoph Hellwig, Sent: Monday, June 17, 2019 3:54 PM
>
> On Mon, Jun 17, 2019 at 06:46:33AM +, 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;
> }
Re: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
On Mon, Jun 17, 2019 at 06:46:33AM +, 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;
}
RE: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
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=156050608709643=2
Best regards,
Yoshihiro Shimoda
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
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
Re: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
> > + 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. signature.asc Description: PGP signature
Re: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
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?
Re: [RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
> - 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)); signature.asc Description: PGP signature
[RFC PATCH v6 5/5] mmc: queue: Use bigger segments if IOMMU can merge the segments
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
---
drivers/mmc/core/queue.c | 33 +
include/linux/mmc/host.h | 1 +
2 files changed, 30 insertions(+), 4 deletions(-)
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(>recovery_work, mmc_mq_recovery_handler);
INIT_WORK(>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(>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 intretune_now:1; /* do re-tuning at next req */
unsigned intretune_paused:1; /* re-tuning is temporarily
disabled */
unsigned intuse_blk_mq:1; /* use blk-mq */
+ unsigned intcan_merge:1;/* merging can be used */
int rescan_disable; /* disable card detection */
int rescan_entered; /* used with nonremovable
devices */
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
