Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1543862 > unrolled thread

[PATCHSET v4] blk-mq-scheduling framework

Back to article view | Back to linux.kernel

Contents

  [PATCHSET v4] blk-mq-scheduling framework Jens Axboe <axboe@fb.com> - 2016-12-17 01:20 +0100
    [PATCH 2/8] blk-mq: make mq_ops a const pointer Jens Axboe <axboe@fb.com> - 2016-12-17 01:20 +0100
    Re: [PATCHSET v4] blk-mq-scheduling framework Paolo Valente <paolo.valente@linaro.org> - 2016-12-19 12:40 +0100
      Re: [PATCHSET v4] blk-mq-scheduling framework Paolo Valente <paolo.valente@linaro.org> - 2016-12-19 19:30 +0100
        Re: [PATCHSET v4] blk-mq-scheduling framework Paolo Valente <paolo.valente@linaro.org> - 2016-12-22 16:30 +0100
    Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO  schedulers Jens Axboe <axboe@fb.com> - 2016-12-21 03:30 +0100
      Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers Paolo Valente <paolo.valente@linaro.org> - 2016-12-22 16:30 +0100
    Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers Paolo Valente <paolo.valente@linaro.org> - 2016-12-22 11:00 +0100
      Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers Paolo Valente <paolo.valente@linaro.org> - 2016-12-22 12:20 +0100
      Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers Paolo Valente <paolo.valente@linaro.org> - 2016-12-23 11:20 +0100
    Re: [PATCHSET v4] blk-mq-scheduling framework Omar Sandoval <osandov@osandov.com> - 2016-12-22 18:00 +0100
      Re: [PATCHSET v4] blk-mq-scheduling framework Omar Sandoval <osandov@osandov.com> - 2016-12-22 18:20 +0100

#1543862 — [PATCHSET v4] blk-mq-scheduling framework

This is version 4 of this patchset, version 3 was posted here:

https://marc.info/?l=linux-block&m=148178513407631&w=2

From the discussion last time, I looked into the feasibility of having
two sets of tags for the same request pool, to avoid having to copy
some of the request fields at dispatch and completion time. To do that,
we'd have to replace the driver tag map(s) with our own, and augment
that with tag map(s) on the side representing the device queue depth.
Queuing IO with the scheduler would allocate from the new map, and
dispatching would acquire the "real" tag. We would need to change
drivers to do this, or add an extra indirection table to map a real
tag to the scheduler tag. We would also need a 1:1 mapping between
scheduler and hardware tag pools, or additional info to track it.
Unless someone can convince me otherwise, I think the current approach
is cleaner.

I wasn't going to post v4 so soon, but I discovered a bug that led
to drastically decreased merging. Especially on rotating storage,
this release should be fast, and on par with the merging that we
get through the legacy schedulers.

Changes since v3:

- Keep the blk_mq_free_request/__blk_mq_free_request() as the
  interface, and have those functions call the scheduler API
  instead.

- Add insertion merging from unplugging.

- Ensure that RQF_STARTED is cleared when we get a new shadow
  request, or merging will fail if it is already set.

- Improve the blk_mq_sched_init_hctx_data() implementation. From Omar.

- Make the shadow alloc/free interface more usable by schedulers
  that use the software queues. From Omar.

- Fix a bug in the io context code.

- Put the is_shadow() helper in generic code, instead of in mq-deadline.

- Add prep patch that unexports blk_mq_free_hctx_request(), it's not
  used by anyone.

- Remove the magic '256' queue depth from mq-deadline, replace with a
  module parameter, 'queue_depth', that defaults to 256.

- Various cleanups.

[toc] | [next] | [standalone]

#1543863 — [PATCH 2/8] blk-mq: make mq_ops a const pointer

We never change it, make that clear.

Signed-off-by: Jens Axboe <axboe@fb.com>
Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 block/blk-mq.c         | 2 +-
 include/linux/blk-mq.h | 2 +-
 include/linux/blkdev.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index d79fdc11b1ee..87b7eaa1cb74 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -639,7 +639,7 @@ struct blk_mq_timeout_data {
 
 void blk_mq_rq_timed_out(struct request *req, bool reserved)
 {
-	struct blk_mq_ops *ops = req->q->mq_ops;
+	const struct blk_mq_ops *ops = req->q->mq_ops;
 	enum blk_eh_timer_return ret = BLK_EH_RESET_TIMER;
 
 	/*
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 4a2ab5d99ff7..afc81d77e471 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -60,7 +60,7 @@ struct blk_mq_hw_ctx {
 
 struct blk_mq_tag_set {
 	unsigned int		*mq_map;
-	struct blk_mq_ops	*ops;
+	const struct blk_mq_ops	*ops;
 	unsigned int		nr_hw_queues;
 	unsigned int		queue_depth;	/* max hw supported */
 	unsigned int		reserved_tags;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 286b2a264383..7c40fb838b44 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -408,7 +408,7 @@ struct request_queue {
 	dma_drain_needed_fn	*dma_drain_needed;
 	lld_busy_fn		*lld_busy_fn;
 
-	struct blk_mq_ops	*mq_ops;
+	const struct blk_mq_ops	*mq_ops;
 
 	unsigned int		*mq_map;
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1544473

> Il giorno 17 dic 2016, alle ore 01:12, Jens Axboe <axboe@fb.com> ha scritto:
> 
> This is version 4 of this patchset, version 3 was posted here:
> 
> https://marc.info/?l=linux-block&m=148178513407631&w=2
> 
> From the discussion last time, I looked into the feasibility of having
> two sets of tags for the same request pool, to avoid having to copy
> some of the request fields at dispatch and completion time. To do that,
> we'd have to replace the driver tag map(s) with our own, and augment
> that with tag map(s) on the side representing the device queue depth.
> Queuing IO with the scheduler would allocate from the new map, and
> dispatching would acquire the "real" tag. We would need to change
> drivers to do this, or add an extra indirection table to map a real
> tag to the scheduler tag. We would also need a 1:1 mapping between
> scheduler and hardware tag pools, or additional info to track it.
> Unless someone can convince me otherwise, I think the current approach
> is cleaner.
> 
> I wasn't going to post v4 so soon, but I discovered a bug that led
> to drastically decreased merging. Especially on rotating storage,
> this release should be fast, and on par with the merging that we
> get through the legacy schedulers.
> 

I'm to modifying bfq.  You mentioned other missing pieces to come.  Do
you already have an idea of what they are, so that I am somehow
prepared to what won't work even if my changes are right?

Thanks,
Paolo

> Changes since v3:
> 
> - Keep the blk_mq_free_request/__blk_mq_free_request() as the
>  interface, and have those functions call the scheduler API
>  instead.
> 
> - Add insertion merging from unplugging.
> 
> - Ensure that RQF_STARTED is cleared when we get a new shadow
>  request, or merging will fail if it is already set.
> 
> - Improve the blk_mq_sched_init_hctx_data() implementation. From Omar.
> 
> - Make the shadow alloc/free interface more usable by schedulers
>  that use the software queues. From Omar.
> 
> - Fix a bug in the io context code.
> 
> - Put the is_shadow() helper in generic code, instead of in mq-deadline.
> 
> - Add prep patch that unexports blk_mq_free_hctx_request(), it's not
>  used by anyone.
> 
> - Remove the magic '256' queue depth from mq-deadline, replace with a
>  module parameter, 'queue_depth', that defaults to 256.
> 
> - Various cleanups.
> 

[toc] | [prev] | [next] | [standalone]

#1544731

> Il giorno 19 dic 2016, alle ore 16:20, Jens Axboe <axboe@fb.com> ha scritto:
> 
> On 12/19/2016 04:32 AM, Paolo Valente wrote:
>> 
>>> Il giorno 17 dic 2016, alle ore 01:12, Jens Axboe <axboe@fb.com> ha scritto:
>>> 
>>> This is version 4 of this patchset, version 3 was posted here:
>>> 
>>> https://marc.info/?l=linux-block&m=148178513407631&w=2
>>> 
>>> From the discussion last time, I looked into the feasibility of having
>>> two sets of tags for the same request pool, to avoid having to copy
>>> some of the request fields at dispatch and completion time. To do that,
>>> we'd have to replace the driver tag map(s) with our own, and augment
>>> that with tag map(s) on the side representing the device queue depth.
>>> Queuing IO with the scheduler would allocate from the new map, and
>>> dispatching would acquire the "real" tag. We would need to change
>>> drivers to do this, or add an extra indirection table to map a real
>>> tag to the scheduler tag. We would also need a 1:1 mapping between
>>> scheduler and hardware tag pools, or additional info to track it.
>>> Unless someone can convince me otherwise, I think the current approach
>>> is cleaner.
>>> 
>>> I wasn't going to post v4 so soon, but I discovered a bug that led
>>> to drastically decreased merging. Especially on rotating storage,
>>> this release should be fast, and on par with the merging that we
>>> get through the legacy schedulers.
>>> 
>> 
>> I'm to modifying bfq.  You mentioned other missing pieces to come.  Do
>> you already have an idea of what they are, so that I am somehow
>> prepared to what won't work even if my changes are right?
> 
> I'm mostly talking about elevator ops hooks that aren't there in the new
> framework, but exist in the old one. There should be no hidden
> surprises, if that's what you are worried about.
> 
> On the ops side, the only ones I can think of are the activate and
> deactivate, and those can be done in the dispatch_request hook for
> activate, and put/requeue for deactivate.
> 

You mean that there is no conceptual problem in moving the code of the
activate interface function into the dispatch function, and the code
of the deactivate into the put_request? (for a requeue it is a little
less clear to me, so one step at a time)  Or am I missing
something more complex?

> Outside of that, some of them have been renamed, and some have been
> collapsed (like activate/deactivate), and yet others again work a little
> differently (like merging). See the mq-deadline conversion, and just
> work through them one at the time.
> 

That's how I'm proceeding, thanks.

Thank you,
Paolo

> -- 
> Jens Axboe
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1546416

> Il giorno 19 dic 2016, alle ore 22:05, Jens Axboe <axboe@fb.com> ha scritto:
> 
> On 12/19/2016 11:21 AM, Paolo Valente wrote:
>> 
>>> Il giorno 19 dic 2016, alle ore 16:20, Jens Axboe <axboe@fb.com> ha scritto:
>>> 
>>> On 12/19/2016 04:32 AM, Paolo Valente wrote:
>>>> 
>>>>> Il giorno 17 dic 2016, alle ore 01:12, Jens Axboe <axboe@fb.com> ha scritto:
>>>>> 
>>>>> This is version 4 of this patchset, version 3 was posted here:
>>>>> 
>>>>> https://marc.info/?l=linux-block&m=148178513407631&w=2
>>>>> 
>>>>> From the discussion last time, I looked into the feasibility of having
>>>>> two sets of tags for the same request pool, to avoid having to copy
>>>>> some of the request fields at dispatch and completion time. To do that,
>>>>> we'd have to replace the driver tag map(s) with our own, and augment
>>>>> that with tag map(s) on the side representing the device queue depth.
>>>>> Queuing IO with the scheduler would allocate from the new map, and
>>>>> dispatching would acquire the "real" tag. We would need to change
>>>>> drivers to do this, or add an extra indirection table to map a real
>>>>> tag to the scheduler tag. We would also need a 1:1 mapping between
>>>>> scheduler and hardware tag pools, or additional info to track it.
>>>>> Unless someone can convince me otherwise, I think the current approach
>>>>> is cleaner.
>>>>> 
>>>>> I wasn't going to post v4 so soon, but I discovered a bug that led
>>>>> to drastically decreased merging. Especially on rotating storage,
>>>>> this release should be fast, and on par with the merging that we
>>>>> get through the legacy schedulers.
>>>>> 
>>>> 
>>>> I'm to modifying bfq.  You mentioned other missing pieces to come.  Do
>>>> you already have an idea of what they are, so that I am somehow
>>>> prepared to what won't work even if my changes are right?
>>> 
>>> I'm mostly talking about elevator ops hooks that aren't there in the new
>>> framework, but exist in the old one. There should be no hidden
>>> surprises, if that's what you are worried about.
>>> 
>>> On the ops side, the only ones I can think of are the activate and
>>> deactivate, and those can be done in the dispatch_request hook for
>>> activate, and put/requeue for deactivate.
>>> 
>> 
>> You mean that there is no conceptual problem in moving the code of the
>> activate interface function into the dispatch function, and the code
>> of the deactivate into the put_request? (for a requeue it is a little
>> less clear to me, so one step at a time)  Or am I missing
>> something more complex?
> 
> Yes, what I mean is that there isn't a 1:1 mapping between the old ops
> and the new ops. So you'll have to consider the cases.
> 
> 

Problem: whereas it seems easy and safe to do somewhere else the
simple increment that was done in activate_request, I wonder if it may
happen that a request is deactivate before being completed.  In it may
happen, then, without a deactivate_request hook, the increments would
remain unbalanced.  Or are request completions always guaranteed till
no hw/sw components breaks?

Thanks,
Paolo 

> -- 
> Jens Axboe
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1545551 — Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers

On Tue, Dec 20 2016, Paolo Valente wrote:
> > +	else
> > +		rq = __blk_mq_alloc_request(data, op);
> > +
> > +	if (rq) {
> > +		rq->elv.icq = NULL;
> > +		if (e && e->type->icq_cache)
> > +			blk_mq_sched_assign_ioc(q, rq, bio);
> 
> bfq needs rq->elv.icq to be consistent in bfq_get_request, but the
> needed initialization seems to occur only after mq.get_request is
> invoked.

Can you do it from get/put_rq_priv? The icq is assigned there. If not,
we can redo this part, not a big deal.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1546414 — Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers

> Il giorno 21 dic 2016, alle ore 03:22, Jens Axboe <axboe@fb.com> ha scritto:
> 
> On Tue, Dec 20 2016, Paolo Valente wrote:
>>> +	else
>>> +		rq = __blk_mq_alloc_request(data, op);
>>> +
>>> +	if (rq) {
>>> +		rq->elv.icq = NULL;
>>> +		if (e && e->type->icq_cache)
>>> +			blk_mq_sched_assign_ioc(q, rq, bio);
>> 
>> bfq needs rq->elv.icq to be consistent in bfq_get_request, but the
>> needed initialization seems to occur only after mq.get_request is
>> invoked.
> 
> Can you do it from get/put_rq_priv?

Definitely, I just overlooked them, sorry :(

Thanks,
Paolo

> The icq is assigned there. If not,
> we can redo this part, not a big deal.
> 
> -- 
> Jens Axboe
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1546251 — Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers

> Il giorno 17 dic 2016, alle ore 01:12, Jens Axboe <axboe@fb.com> ha scritto:
> 
> This adds a set of hooks that intercepts the blk-mq path of
> allocating/inserting/issuing/completing requests, allowing
> us to develop a scheduler within that framework.
> 
> We reuse the existing elevator scheduler API on the registration
> side, but augment that with the scheduler flagging support for
> the blk-mq interfce, and with a separate set of ops hooks for MQ
> devices.
> 
> Schedulers can opt in to using shadow requests. Shadow requests
> are internal requests that the scheduler uses for for the allocate
> and insert part, which are then mapped to a real driver request
> at dispatch time. This is needed to separate the device queue depth
> from the pool of requests that the scheduler has to work with.
> 
> Signed-off-by: Jens Axboe <axboe@fb.com>
> 
...

> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> new file mode 100644
> index 000000000000..b7e1839d4785
> --- /dev/null
> +++ b/block/blk-mq-sched.c

> ...
> +static inline bool
> +blk_mq_sched_allow_merge(struct request_queue *q, struct request *rq,
> +			 struct bio *bio)
> +{
> +	struct elevator_queue *e = q->elevator;
> +
> +	if (e && e->type->ops.mq.allow_merge)
> +		return e->type->ops.mq.allow_merge(q, rq, bio);
> +
> +	return true;
> +}
> +

Something does not seem to add up here:
e->type->ops.mq.allow_merge may be called only in
blk_mq_sched_allow_merge, which, in its turn, may be called only in
blk_mq_attempt_merge, which, finally, may be called only in
blk_mq_merge_queue_io.  Yet the latter may be called only if there is
no elevator (line 1399 and 1507 in blk-mq.c).

Therefore, e->type->ops.mq.allow_merge can never be called, both if
there is and if there is not an elevator.  Be patient if I'm missing
something huge, but I thought it was worth reporting this.

Paolo

[toc] | [prev] | [next] | [standalone]

#1546276 — Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers

> Il giorno 22 dic 2016, alle ore 10:59, Paolo Valente <paolo.valente@linaro.org> ha scritto:
> 
>> 
>> Il giorno 17 dic 2016, alle ore 01:12, Jens Axboe <axboe@fb.com> ha scritto:
>> 
>> This adds a set of hooks that intercepts the blk-mq path of
>> allocating/inserting/issuing/completing requests, allowing
>> us to develop a scheduler within that framework.
>> 
>> We reuse the existing elevator scheduler API on the registration
>> side, but augment that with the scheduler flagging support for
>> the blk-mq interfce, and with a separate set of ops hooks for MQ
>> devices.
>> 
>> Schedulers can opt in to using shadow requests. Shadow requests
>> are internal requests that the scheduler uses for for the allocate
>> and insert part, which are then mapped to a real driver request
>> at dispatch time. This is needed to separate the device queue depth
>> from the pool of requests that the scheduler has to work with.
>> 
>> Signed-off-by: Jens Axboe <axboe@fb.com>
>> 
> ...
> 
>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>> new file mode 100644
>> index 000000000000..b7e1839d4785
>> --- /dev/null
>> +++ b/block/blk-mq-sched.c
> 
>> ...
>> +static inline bool
>> +blk_mq_sched_allow_merge(struct request_queue *q, struct request *rq,
>> +			 struct bio *bio)
>> +{
>> +	struct elevator_queue *e = q->elevator;
>> +
>> +	if (e && e->type->ops.mq.allow_merge)
>> +		return e->type->ops.mq.allow_merge(q, rq, bio);
>> +
>> +	return true;
>> +}
>> +
> 
> Something does not seem to add up here:
> e->type->ops.mq.allow_merge may be called only in
> blk_mq_sched_allow_merge, which, in its turn, may be called only in
> blk_mq_attempt_merge, which, finally, may be called only in
> blk_mq_merge_queue_io.  Yet the latter may be called only if there is
> no elevator (line 1399 and 1507 in blk-mq.c).
> 
> Therefore, e->type->ops.mq.allow_merge can never be called, both if
> there is and if there is not an elevator.  Be patient if I'm missing
> something huge, but I thought it was worth reporting this.
> 

Just another detail: if e->type->ops.mq.allow_merge does get invoked
from the above path, then it is invoked of course without the
scheduler lock held.  In contrast, if this function gets invoked
from dd_bio_merge, then the scheduler lock is held.

To handle this opposite alternatives, I don't know whether checking if
the lock is held (and possibly taking it) from inside
e->type->ops.mq.allow_merge is a good solution.  In any case, before
possibly trying it, I will wait for some feedback on the main problem,
i.e., on the fact that e->type->ops.mq.allow_merge
seems unreachable in the above path.

Thanks,
Paolo

> Paolo
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1546808 — Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers

> Il giorno 22 dic 2016, alle ore 10:59, Paolo Valente <paolo.valente@linaro.org> ha scritto:
> 
>> 
>> Il giorno 17 dic 2016, alle ore 01:12, Jens Axboe <axboe@fb.com> ha scritto:
>> 
>> This adds a set of hooks that intercepts the blk-mq path of
>> allocating/inserting/issuing/completing requests, allowing
>> us to develop a scheduler within that framework.
>> 
>> We reuse the existing elevator scheduler API on the registration
>> side, but augment that with the scheduler flagging support for
>> the blk-mq interfce, and with a separate set of ops hooks for MQ
>> devices.
>> 
>> Schedulers can opt in to using shadow requests. Shadow requests
>> are internal requests that the scheduler uses for for the allocate
>> and insert part, which are then mapped to a real driver request
>> at dispatch time. This is needed to separate the device queue depth
>> from the pool of requests that the scheduler has to work with.
>> 
>> Signed-off-by: Jens Axboe <axboe@fb.com>
>> 
> ...
> 
>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>> new file mode 100644
>> index 000000000000..b7e1839d4785
>> --- /dev/null
>> +++ b/block/blk-mq-sched.c
> 
>> ...
>> +static inline bool
>> +blk_mq_sched_allow_merge(struct request_queue *q, struct request *rq,
>> +			 struct bio *bio)
>> +{
>> +	struct elevator_queue *e = q->elevator;
>> +
>> +	if (e && e->type->ops.mq.allow_merge)
>> +		return e->type->ops.mq.allow_merge(q, rq, bio);
>> +
>> +	return true;
>> +}
>> +
> 
> Something does not seem to add up here:
> e->type->ops.mq.allow_merge may be called only in
> blk_mq_sched_allow_merge, which, in its turn, may be called only in
> blk_mq_attempt_merge, which, finally, may be called only in
> blk_mq_merge_queue_io.  Yet the latter may be called only if there is
> no elevator (line 1399 and 1507 in blk-mq.c).
> 
> Therefore, e->type->ops.mq.allow_merge can never be called, both if
> there is and if there is not an elevator.  Be patient if I'm missing
> something huge, but I thought it was worth reporting this.
> 

Jens,
I forgot to add that I'm willing (and would be happy) to propose a fix
to this, and possibly the other problems too, on my own.  Just, I'm
not yet expert enough to do it with having first received some
feedback or instructions from you.  In this specific case, I don't
even know yet whether this is really a bug.

Thanks, and merry Christmas if we don't get in touch before,
Paolo

> Paolo
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1546494

On Thu, Dec 22, 2016 at 04:23:24PM +0000, Bart Van Assche wrote:
> On Fri, 2016-12-16 at 17:12 -0700, Jens Axboe wrote:
> > From the discussion last time, I looked into the feasibility of having
> > two sets of tags for the same request pool, to avoid having to copy
> > some of the request fields at dispatch and completion time. To do that,
> > we'd have to replace the driver tag map(s) with our own, and augment
> > that with tag map(s) on the side representing the device queue depth.
> > Queuing IO with the scheduler would allocate from the new map, and
> > dispatching would acquire the "real" tag. We would need to change
> > drivers to do this, or add an extra indirection table to map a real
> > tag to the scheduler tag. We would also need a 1:1 mapping between
> > scheduler and hardware tag pools, or additional info to track it.
> > Unless someone can convince me otherwise, I think the current approach
> > is cleaner.
> 
> Hello Jens,
> 
> Can you have a look at the attached patches? These implement the "two tags
> per request" approach without a table that maps one tag type to the other
> or any other ugly construct. __blk_mq_alloc_request() is modified such that
> it assigns rq->sched_tag and sched_tags->rqs[] instead of rq->tag and
> tags->rqs[]. rq->tag and tags->rqs[] are assigned just before dispatch by
> blk_mq_assign_drv_tag(). This approach results in significantly less code
> than the approach proposed in v4 of your blk-mq-sched patch series. Memory
> usage is lower because only a single set of requests is allocated. The
> runtime overhead is lower because request fields no longer have to be
> copied between the requests owned by the block driver and the requests
> owned by the I/O scheduler. I can boot a VM from the virtio-blk driver but
> otherwise the attached patches have not yet been tested.
> 
> Thanks,
> 
> Bart.

Hey, Bart,

This approach occurred to us, but we couldn't figure out a way to make
blk_mq_tag_to_rq() work with it. From skimming over the patches, I
didn't see a solution to that problem.

[toc] | [prev] | [next] | [standalone]

#1546501

On Thu, Dec 22, 2016 at 04:57:36PM +0000, Bart Van Assche wrote:
> On Thu, 2016-12-22 at 08:52 -0800, Omar Sandoval wrote:
> > This approach occurred to us, but we couldn't figure out a way to make
> > blk_mq_tag_to_rq() work with it. From skimming over the patches, I
> > didn't see a solution to that problem.
> 
> Hello Omar,
> 
> Can you clarify your comment? Since my patches initialize both tags->rqs[]
> and sched_tags->rqs[] the function blk_mq_tag_to_rq() should still work.
> 
> Bart.

Sorry, you're right, it does work, but tags->rqs[] ends up being the
extra lookup table. I suspect that the runtime overhead of keeping that
up to date could be worse than copying the rq fields if you have lots of
CPUs but only one hardware queue.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web