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

Groups > linux.kernel > #1601349 > unrolled thread

Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) Paolo Valente <paolo.valente@linaro.org> - 2017-03-15 13:10 +0100
    Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) Jens Axboe <axboe@kernel.dk> - 2017-03-15 17:00 +0100
      Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) Jens Axboe <axboe@kernel.dk> - 2017-03-15 17:40 +0100
        Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) Paolo Valente <paolo.valente@linaro.org> - 2017-03-15 18:10 +0100
          Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) Jens Axboe <axboe@kernel.dk> - 2017-03-15 22:10 +0100
            Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) Paolo Valente <paolo.valente@linaro.org> - 2017-03-18 11:40 +0100

#1601349 — Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM)

> Il giorno 07 mar 2017, alle ore 18:44, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 03/04/2017 09:01 AM, Paolo Valente wrote:
>> @@ -560,6 +600,15 @@ struct bfq_data {
>> 	struct bfq_io_cq *bio_bic;
>> 	/* bfqq associated with the task issuing current bio for merging */
>> 	struct bfq_queue *bio_bfqq;
>> +
>> +	/*
>> +	 * io context to put right after bfqd->lock is released. This
>> +	 * filed is used to perform put_io_context, when needed, to
>> +	 * after the scheduler lock has been released, and thus
>> +	 * prevent an ioc->lock from being possibly taken while the
>> +	 * scheduler lock is being held.
>> +	 */
>> +	struct io_context *ioc_to_put;
>> };
> 
> The logic around this is nasty, effectively you end up having locking
> around sections of code instea of structures, which is never a good
> idea.
> 
> The helper functions for unlocking and dropping the ioc add to the mess
> as well.
> 

Hi Jens,
fortunately I seem to have found and fixed the bug causing the failure
your reported in one of your previous emails, so I've started addressing
the issue you raise here.  But your suggestion below raised doubts
that I was not able to solve.  So I'm bailing out and asking for help.

> Can't we simply pass back a pointer to an ioc to free? That should be
> possible, given that we must have grabbed the bfqd lock ourselves
> further up in the call chain. So we _know_ that we'll drop it later on.
> If that wasn't the case, the existing logic wouldn't work.
> 

One of the two functions that discover that an ioc has to bee freed,
namely __bfq_bfqd_reset_in_service, is invoked at the end of several
relatively long chains of function invocations.  The heads of these
chains take and release the scheduler lock.  One example is:

bfq_dispatch_request -> __bfq_dispatch_request -> bfq_select_queue -> bfq_bfqq_expire  -> __bfq_bfqq_expire -> __bfq_bfqd_reset_in_service

To implement your proposal, all the functions involved in these chains
should be extended to pass back the ioc to put.  The resulting, heavy
version of the code seems really unadvisable, and prone to errors when
one modifies or adds some chain.

So I have certainly misunderstood something.  As usual, to help you
help me more quickly, here is a summary of what I have understood on
this matter.

1.  For similar, if not exactly the same, lock-nesting issue related
to io-context putting, deferred work is used.  Probably deferred work
is used also for other reasons, but for sure it does solve this issue too.

2.  My solution (which I'm not defending; I'm just trying to
understand) solves the same issue as above: put the io
context after the other lock is released.  But it solves it with no
work-queueing overhead.  Instead of queueing work, it 'queues' the ioc
to put, and puts it right after releasing the scheduler lock.

Where is my mistake?  And what is the correct interpretation of your
proposal to pass back the pointer (instead of storing it in a field of
the device data structure)?

Thanks,
Paolo

> -- 
> Jens Axboe
> 

[toc] | [next] | [standalone]

#1601508

On 03/15/2017 06:01 AM, Paolo Valente wrote:
> 
>> Il giorno 07 mar 2017, alle ore 18:44, Jens Axboe <axboe@kernel.dk> ha scritto:
>>
>> On 03/04/2017 09:01 AM, Paolo Valente wrote:
>>> @@ -560,6 +600,15 @@ struct bfq_data {
>>> 	struct bfq_io_cq *bio_bic;
>>> 	/* bfqq associated with the task issuing current bio for merging */
>>> 	struct bfq_queue *bio_bfqq;
>>> +
>>> +	/*
>>> +	 * io context to put right after bfqd->lock is released. This
>>> +	 * filed is used to perform put_io_context, when needed, to
>>> +	 * after the scheduler lock has been released, and thus
>>> +	 * prevent an ioc->lock from being possibly taken while the
>>> +	 * scheduler lock is being held.
>>> +	 */
>>> +	struct io_context *ioc_to_put;
>>> };
>>
>> The logic around this is nasty, effectively you end up having locking
>> around sections of code instea of structures, which is never a good
>> idea.
>>
>> The helper functions for unlocking and dropping the ioc add to the mess
>> as well.
>>
> 
> Hi Jens,
> fortunately I seem to have found and fixed the bug causing the failure
> your reported in one of your previous emails, so I've started addressing
> the issue you raise here.  But your suggestion below raised doubts
> that I was not able to solve.  So I'm bailing out and asking for help.

Great (on fixing that other bug).

>> Can't we simply pass back a pointer to an ioc to free? That should be
>> possible, given that we must have grabbed the bfqd lock ourselves
>> further up in the call chain. So we _know_ that we'll drop it later on.
>> If that wasn't the case, the existing logic wouldn't work.
>>
> 
> One of the two functions that discover that an ioc has to bee freed,
> namely __bfq_bfqd_reset_in_service, is invoked at the end of several
> relatively long chains of function invocations.  The heads of these
> chains take and release the scheduler lock.  One example is:
> 
> bfq_dispatch_request -> __bfq_dispatch_request -> bfq_select_queue -> bfq_bfqq_expire  -> __bfq_bfqq_expire -> __bfq_bfqd_reset_in_service
> 
> To implement your proposal, all the functions involved in these chains
> should be extended to pass back the ioc to put.  The resulting, heavy
> version of the code seems really unadvisable, and prone to errors when
> one modifies or adds some chain.
> 
> So I have certainly misunderstood something.  As usual, to help you
> help me more quickly, here is a summary of what I have understood on
> this matter.
> 
> 1.  For similar, if not exactly the same, lock-nesting issue related
> to io-context putting, deferred work is used.  Probably deferred work
> is used also for other reasons, but for sure it does solve this issue too.
> 
> 2.  My solution (which I'm not defending; I'm just trying to
> understand) solves the same issue as above: put the io
> context after the other lock is released.  But it solves it with no
> work-queueing overhead.  Instead of queueing work, it 'queues' the ioc
> to put, and puts it right after releasing the scheduler lock.
> 
> Where is my mistake?  And what is the correct interpretation of your
> proposal to pass back the pointer (instead of storing it in a field of
> the device data structure)?

I think you understood me correctly. Currently I think the putting of
the io context is somewhat of a mess. You have seemingly random places
where you have to use special unlock functions, to ensure that you
notice that some caller deeper down has set ->ioc_to_put. I took a quick
look at it, and by far most of the cases can return an io_context to
free quite easily. You can mark these functions __must_check to ensure
that we don't drop an io_context, inadvertently. That's already a win
over the random ->ioc_to_put store. And you can then get rid of
bfq_unlock_put_ioc and it's irq variant as well.

The places where you are already returning a value, like off dispatch
for instance, you can just pass in a pointer to an io_context pointer.

If you get this right, it'll be a lot less fragile and hacky than your
current approach.

I'd avoid having to do deferred put from a workqueue at all costs. This
is an _expensive_ operation.

-- 
Jens Axboe

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

#1601537

On 03/15/2017 09:47 AM, Jens Axboe wrote:
> I think you understood me correctly. Currently I think the putting of
> the io context is somewhat of a mess. You have seemingly random places
> where you have to use special unlock functions, to ensure that you
> notice that some caller deeper down has set ->ioc_to_put. I took a quick
> look at it, and by far most of the cases can return an io_context to
> free quite easily. You can mark these functions __must_check to ensure
> that we don't drop an io_context, inadvertently. That's already a win
> over the random ->ioc_to_put store. And you can then get rid of
> bfq_unlock_put_ioc and it's irq variant as well.
> 
> The places where you are already returning a value, like off dispatch
> for instance, you can just pass in a pointer to an io_context pointer.
> 
> If you get this right, it'll be a lot less fragile and hacky than your
> current approach.

Even just looking a little closer, you also find cases where you
potentially twice store ->ioc_to_put. That kind of mixup can't happen if
you return it properly.

In __bfq_dispatch_request(), for instance. You call bfq_select_queue(),
and that in turn calls bfq_bfqq_expire(), which calls
__bfq_bfqq_expire() which can set ->ioc_to_put. But later on,
__bfq_dispatch_request() calls bfq_dispatch_rq_from_bfqq(), which in
turn calls bfq_bfqq_expire() that can also set ->ioc_to_put. There's no
"magic" bfq_unlock_and_put_ioc() in-between those. Maybe the former call
never sets ->ioc_to_put if it returns with bfqq == NULL? Hard to tell.

Or __bfq_insert_request(), it calls bfq_add_request(), which may set
->ioc_to_put through bfq_bfqq_handle_idle_busy_switch() ->
bfq_bfqq_expire(). And then from calling bfq_rq_enqueued() ->
bfq_bfqq_expire().

There might be more, but I think the above is plenty of evidence that
the current ->ioc_to_put solution is a bad hack, fragile, and already
has bugs.

How often do you expect this putting of the io_context to happen? If
it's not a very frequent occurence, maybe using a deferred workqueue to
put it IS the right solution. As it currently stands, the code doesn't
really work, and it's fragile. It can't be cleaned up without
refactoring, since the call paths are all extremely intermingled.

-- 
Jens Axboe

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

#1601575

> Il giorno 15 mar 2017, alle ore 17:30, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 03/15/2017 09:47 AM, Jens Axboe wrote:
>> I think you understood me correctly. Currently I think the putting of
>> the io context is somewhat of a mess. You have seemingly random places
>> where you have to use special unlock functions, to ensure that you
>> notice that some caller deeper down has set ->ioc_to_put. I took a quick
>> look at it, and by far most of the cases can return an io_context to
>> free quite easily. You can mark these functions __must_check to ensure
>> that we don't drop an io_context, inadvertently. That's already a win
>> over the random ->ioc_to_put store. And you can then get rid of
>> bfq_unlock_put_ioc and it's irq variant as well.
>> 
>> The places where you are already returning a value, like off dispatch
>> for instance, you can just pass in a pointer to an io_context pointer.
>> 
>> If you get this right, it'll be a lot less fragile and hacky than your
>> current approach.
> 
> Even just looking a little closer, you also find cases where you
> potentially twice store ->ioc_to_put. That kind of mixup can't happen if
> you return it properly.
> 
> In __bfq_dispatch_request(), for instance. You call bfq_select_queue(),
> and that in turn calls bfq_bfqq_expire(), which calls
> __bfq_bfqq_expire() which can set ->ioc_to_put. But later on,
> __bfq_dispatch_request() calls bfq_dispatch_rq_from_bfqq(), which in
> turn calls bfq_bfqq_expire() that can also set ->ioc_to_put. There's no
> "magic" bfq_unlock_and_put_ioc() in-between those. Maybe the former call
> never sets ->ioc_to_put if it returns with bfqq == NULL? Hard to tell.
> 
> Or __bfq_insert_request(), it calls bfq_add_request(), which may set
> ->ioc_to_put through bfq_bfqq_handle_idle_busy_switch() ->
> bfq_bfqq_expire(). And then from calling bfq_rq_enqueued() ->
> bfq_bfqq_expire().
> 

I have checked that.  Basically, since a queue can't be expired twice,
then it should never happen that ioc_to_put is set twice before being
used.  Yet, I do agree that using a shared field and exploiting
collateral effects makes code very complex and fragile (maybe even
buggy if my speculative check is wrong).  Just, it has been the best
solution I found, to avoid deferred work as you asked.  In fact, I
still find quite heavy the alternative of passing a pointer to an ioc
forth and back across seven or eight nested functions.

> There might be more, but I think the above is plenty of evidence that
> the current ->ioc_to_put solution is a bad hack, fragile, and already
> has bugs.
> 
> How often do you expect this putting of the io_context to happen?

Unfortunately often, as it must be done also every time the in-service
queue is reset.  But, in this respect, are we sure that we do need to
grab a reference to the ioc when we set a queue in service (as done in
cfq, and copied into bfq)?  I mean, we have the hook exit_ioc for
controlling the disappearing of an ioc.  Am I missing something here
too?

Thanks,
Paolo

> If
> it's not a very frequent occurence, maybe using a deferred workqueue to
> put it IS the right solution. As it currently stands, the code doesn't
> really work, and it's fragile. It can't be cleaned up without
> refactoring, since the call paths are all extremely intermingled.
> 
> -- 
> Jens Axboe
> 

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

#1601739

On 03/15/2017 10:59 AM, Paolo Valente wrote:
> 
>> Il giorno 15 mar 2017, alle ore 17:30, Jens Axboe <axboe@kernel.dk> ha scritto:
>>
>> On 03/15/2017 09:47 AM, Jens Axboe wrote:
>>> I think you understood me correctly. Currently I think the putting of
>>> the io context is somewhat of a mess. You have seemingly random places
>>> where you have to use special unlock functions, to ensure that you
>>> notice that some caller deeper down has set ->ioc_to_put. I took a quick
>>> look at it, and by far most of the cases can return an io_context to
>>> free quite easily. You can mark these functions __must_check to ensure
>>> that we don't drop an io_context, inadvertently. That's already a win
>>> over the random ->ioc_to_put store. And you can then get rid of
>>> bfq_unlock_put_ioc and it's irq variant as well.
>>>
>>> The places where you are already returning a value, like off dispatch
>>> for instance, you can just pass in a pointer to an io_context pointer.
>>>
>>> If you get this right, it'll be a lot less fragile and hacky than your
>>> current approach.
>>
>> Even just looking a little closer, you also find cases where you
>> potentially twice store ->ioc_to_put. That kind of mixup can't happen if
>> you return it properly.
>>
>> In __bfq_dispatch_request(), for instance. You call bfq_select_queue(),
>> and that in turn calls bfq_bfqq_expire(), which calls
>> __bfq_bfqq_expire() which can set ->ioc_to_put. But later on,
>> __bfq_dispatch_request() calls bfq_dispatch_rq_from_bfqq(), which in
>> turn calls bfq_bfqq_expire() that can also set ->ioc_to_put. There's no
>> "magic" bfq_unlock_and_put_ioc() in-between those. Maybe the former call
>> never sets ->ioc_to_put if it returns with bfqq == NULL? Hard to tell.
>>
>> Or __bfq_insert_request(), it calls bfq_add_request(), which may set
>> ->ioc_to_put through bfq_bfqq_handle_idle_busy_switch() ->
>> bfq_bfqq_expire(). And then from calling bfq_rq_enqueued() ->
>> bfq_bfqq_expire().
>>
> 
> I have checked that.  Basically, since a queue can't be expired twice,
> then it should never happen that ioc_to_put is set twice before being
> used.  Yet, I do agree that using a shared field and exploiting
> collateral effects makes code very complex and fragile (maybe even
> buggy if my speculative check is wrong).  Just, it has been the best
> solution I found, to avoid deferred work as you asked.  In fact, I
> still find quite heavy the alternative of passing a pointer to an ioc
> forth and back across seven or eight nested functions.

It's not heavy at all, I went through all of it this morning. It's not
super pretty either, since you end up passing back an io_context which
is seemingly unrelated to what the functions otherwise do. But that's
mostly a reflection of the implementation, not that it's a bad way to go
about this in general. The worst bits are the places where you want to
add a

	WARN_ON(ret != NULL);

between two calls that potentially both drop the ioc. In terms of
overhead, it's not heavy. Punting to a workqueue would be orders of
magnitude more expensive.

>> There might be more, but I think the above is plenty of evidence that
>> the current ->ioc_to_put solution is a bad hack, fragile, and already
>> has bugs.
>>
>> How often do you expect this putting of the io_context to happen?
> 
> Unfortunately often, as it must be done also every time the in-service
> queue is reset.  But, in this respect, are we sure that we do need to
> grab a reference to the ioc when we set a queue in service (as done in
> cfq, and copied into bfq)?  I mean, we have the hook exit_ioc for
> controlling the disappearing of an ioc.  Am I missing something here
> too?

No, in fact that'd be perfectly fine. It's easier for CFQ to just retain
the reference so we know it's not going away, but for your case, it
might in fact make more sense to simply be able to de-service a queue if
the process exits. And if you do that, we can drop all this passing back
of ioc (or ->ioc_to_put) craziness, without having to punt to a
workqueue either.

This will be more efficient too, since it'll be a much more rare
occurence.

-- 
Jens Axboe

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

#1603759

> Il giorno 15 mar 2017, alle ore 21:00, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 03/15/2017 10:59 AM, Paolo Valente wrote:
>> 
>>> Il giorno 15 mar 2017, alle ore 17:30, Jens Axboe <axboe@kernel.dk> ha scritto:
>>> 
>>> On 03/15/2017 09:47 AM, Jens Axboe wrote:
>>>> I think you understood me correctly. Currently I think the putting of
>>>> the io context is somewhat of a mess. You have seemingly random places
>>>> where you have to use special unlock functions, to ensure that you
>>>> notice that some caller deeper down has set ->ioc_to_put. I took a quick
>>>> look at it, and by far most of the cases can return an io_context to
>>>> free quite easily. You can mark these functions __must_check to ensure
>>>> that we don't drop an io_context, inadvertently. That's already a win
>>>> over the random ->ioc_to_put store. And you can then get rid of
>>>> bfq_unlock_put_ioc and it's irq variant as well.
>>>> 
>>>> The places where you are already returning a value, like off dispatch
>>>> for instance, you can just pass in a pointer to an io_context pointer.
>>>> 
>>>> If you get this right, it'll be a lot less fragile and hacky than your
>>>> current approach.
>>> 
>>> Even just looking a little closer, you also find cases where you
>>> potentially twice store ->ioc_to_put. That kind of mixup can't happen if
>>> you return it properly.
>>> 
>>> In __bfq_dispatch_request(), for instance. You call bfq_select_queue(),
>>> and that in turn calls bfq_bfqq_expire(), which calls
>>> __bfq_bfqq_expire() which can set ->ioc_to_put. But later on,
>>> __bfq_dispatch_request() calls bfq_dispatch_rq_from_bfqq(), which in
>>> turn calls bfq_bfqq_expire() that can also set ->ioc_to_put. There's no
>>> "magic" bfq_unlock_and_put_ioc() in-between those. Maybe the former call
>>> never sets ->ioc_to_put if it returns with bfqq == NULL? Hard to tell.
>>> 
>>> Or __bfq_insert_request(), it calls bfq_add_request(), which may set
>>> ->ioc_to_put through bfq_bfqq_handle_idle_busy_switch() ->
>>> bfq_bfqq_expire(). And then from calling bfq_rq_enqueued() ->
>>> bfq_bfqq_expire().
>>> 
>> 
>> I have checked that.  Basically, since a queue can't be expired twice,
>> then it should never happen that ioc_to_put is set twice before being
>> used.  Yet, I do agree that using a shared field and exploiting
>> collateral effects makes code very complex and fragile (maybe even
>> buggy if my speculative check is wrong).  Just, it has been the best
>> solution I found, to avoid deferred work as you asked.  In fact, I
>> still find quite heavy the alternative of passing a pointer to an ioc
>> forth and back across seven or eight nested functions.
> 
> It's not heavy at all, I went through all of it this morning.

Yes, sorry. I meant heavy in terms of code complexity.

> It's not
> super pretty either, since you end up passing back an io_context which
> is seemingly unrelated to what the functions otherwise do.

Exactly.

> But that's
> mostly a reflection of the implementation, not that it's a bad way to go
> about this in general. The worst bits are the places where you want to
> add a
> 
> 	WARN_ON(ret != NULL);
> 
> between two calls that potentially both drop the ioc. In terms of
> overhead, it's not heavy. Punting to a workqueue would be orders of
> magnitude more expensive.
> 
>>> There might be more, but I think the above is plenty of evidence that
>>> the current ->ioc_to_put solution is a bad hack, fragile, and already
>>> has bugs.
>>> 
>>> How often do you expect this putting of the io_context to happen?
>> 
>> Unfortunately often, as it must be done also every time the in-service
>> queue is reset.  But, in this respect, are we sure that we do need to
>> grab a reference to the ioc when we set a queue in service (as done in
>> cfq, and copied into bfq)?  I mean, we have the hook exit_ioc for
>> controlling the disappearing of an ioc.  Am I missing something here
>> too?
> 
> No, in fact that'd be perfectly fine. It's easier for CFQ to just retain
> the reference so we know it's not going away, but for your case, it
> might in fact make more sense to simply be able to de-service a queue if
> the process exits. And if you do that, we can drop all this passing back
> of ioc (or ->ioc_to_put) craziness, without having to punt to a
> workqueue either.
> 

Done, and ... well, it seems to work :)

I'm striving to have a new patch series ready before Monday, but I'm
not confident I'll make it.

Thanks,
Paolo

> This will be more efficient too, since it'll be a much more rare
> occurence.
> 
> -- 
> Jens Axboe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web