Re: [PATCH -next v10 3/4] block, bfq: refactor the counting of 'num_groups_with_pending_reqs'

[Yu Kuai]

Hi

在 2022/07/20 19:24, Paolo VALENTE 写道:
> > Il giorno 12 lug 2022, alle ore 15:30, Yu Kuai 
> > <yukuai1@xxxxxxxxxxxxxxx <mailto:yukuai1@xxxxxxxxxxxxxxx>> ha scritto:
> >
> > Hi!
> >
> > I'm copying my reply with new mail address, because Paolo seems
> > didn't receive my reply.
> >
> > 在 2022/06/23 23:32, Paolo Valente 写道:
> > > Sorry for the delay.
> > > > Il giorno 10 giu 2022, alle ore 04:17, Yu Kuai <yukuai3@xxxxxxxxxx 
> > > > <mailto:yukuai3@xxxxxxxxxx>> ha scritto:
> > > >
> > > > Currently, bfq can't handle sync io concurrently as long as they
> > > > are not issued from root group. This is because
> > > > 'bfqd->num_groups_with_pending_reqs > 0' is always true in
> > > > bfq_asymmetric_scenario().
> > > >
> > > > The way that bfqg is counted into 'num_groups_with_pending_reqs':
> > > >
> > > > Before this patch:
> > > > 1) root group will never be counted.
> > > > 2) Count if bfqg or it's child bfqgs have pending requests.
> > > > 3) Don't count if bfqg and it's child bfqgs complete all the requests.
> > > >
> > > > After this patch:
> > > > 1) root group is counted.
> > > > 2) Count if bfqg have pending requests.
> > > > 3) Don't count if bfqg complete all the requests.
> > > >
> > > > With this change, the occasion that only one group is activated can be
> > > > detected, and next patch will support concurrent sync io in the
> > > > occasion.
> > > >
> > > > Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx <mailto:yukuai3@xxxxxxxxxx>>
> > > > Reviewed-by: Jan Kara <jack@xxxxxxx <mailto:jack@xxxxxxx>>
> > > > ---
> > > > block/bfq-iosched.c | 42 ------------------------------------------
> > > > block/bfq-iosched.h | 18 +++++++++---------
> > > > block/bfq-wf2q.c    | 19 ++++---------------
> > > > 3 files changed, 13 insertions(+), 66 deletions(-)
> > > >
> > > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> > > > index 0ec21018daba..03b04892440c 100644
> > > > --- a/block/bfq-iosched.c
> > > > +++ b/block/bfq-iosched.c
> > > > @@ -970,48 +970,6 @@ void __bfq_weights_tree_remove(struct bfq_data 
> > > > *bfqd,
> > > > void bfq_weights_tree_remove(struct bfq_data *bfqd,
> > > >     struct bfq_queue *bfqq)
> > > > {
> > > > -struct bfq_entity *entity = bfqq->entity.parent;
> > > > -
> > > > -for_each_entity(entity) {
> > > > -struct bfq_sched_data *sd = entity->my_sched_data;
> > > > -
> > > > -if (sd->next_in_service || sd->in_service_entity) {
> > > > -/*
> > > > -* entity is still active, because either
> > > > -* next_in_service or in_service_entity is not
> > > > -* NULL (see the comments on the definition of
> > > > -* next_in_service for details on why
> > > > -* in_service_entity must be checked too).
> > > > -*
> > > > -* As a consequence, its parent entities are
> > > > -* active as well, and thus this loop must
> > > > -* stop here.
> > > > -*/
> > > > -break;
> > > > -}
> > > > -
> > > > -/*
> > > > -* The decrement of num_groups_with_pending_reqs is
> > > > -* not performed immediately upon the deactivation of
> > > > -* entity, but it is delayed to when it also happens
> > > > -* that the first leaf descendant bfqq of entity gets
> > > > -* all its pending requests completed. The following
> > > > -* instructions perform this delayed decrement, if
> > > > -* needed. See the comments on
> > > > -* num_groups_with_pending_reqs for details.
> > > > -*/
> > > > -if (entity->in_groups_with_pending_reqs) {
> > > > -entity->in_groups_with_pending_reqs = false;
> > > > -bfqd->num_groups_with_pending_reqs--;
> > > > -}
> > > > -}
> > > With this part removed, I'm missing how you handle the following
> > > sequence of events:
> > > 1.  a queue Q becomes non busy but still has dispatched requests, so
> > > it must not be removed from the counter of queues with pending reqs
> > > yet
> > > 2.  the last request of Q is completed with Q being still idle (non
> > > busy).  At this point Q must be removed from the counter.  It seems to
> > > me that this case is not handled any longer
> > Hi, Paolo
> >
> > 1) At first, patch 1 support to track if bfqq has pending requests, it's
> > done by setting the flag 'entity->in_groups_with_pending_reqs' when the
> > first request is inserted to bfqq, and it's cleared when the last
> > request is completed(based on weights_tree insertion and removal).
>
> In patch 1 I don't see the flag cleared for the request-completion event :(
>
> The piece of code involved is this:
>
> static void bfq_completed_request(struct bfq_queue *bfqq, struct 
> bfq_data *bfqd)
> {
> u64 now_ns;
> u32 delta_us;
>
> bfq_update_hw_tag(bfqd);
>
> bfqd->rq_in_driver[bfqq->actuator_idx]--;
> bfqd->tot_rq_in_driver--;
> bfqq->dispatched--;
>
> if (!bfqq->dispatched && !bfq_bfqq_busy(bfqq)) {
> /*
> * Set budget_timeout (which we overload to store the
> * time at which the queue remains with no backlog and
> * no outstanding request; used by the weight-raising
> * mechanism).
> */
> bfqq->budget_timeout = jiffies;
>
> bfq_weights_tree_remove(bfqd, bfqq);
> }
> ...
>
> Am I missing something?

I add a new api bfq_del_bfqq_in_groups_with_pending_reqs() in patch 1
to clear the flag, and it's called both from bfq_del_bfqq_busy() and
bfq_completed_request(). I think you may miss the later:

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 0d46cb728bbf..0ec21018daba 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6263,6 +6263,7 @@ static void bfq_completed_request(struct bfq_queue 
*bfqq, struct bfq_data *bfqd)
 		 */
 		bfqq->budget_timeout = jiffies;

+		bfq_del_bfqq_in_groups_with_pending_reqs(bfqq);
 		bfq_weights_tree_remove(bfqd, bfqq);
 	}

Thanks,
Kuai
> Thanks,
> Paolo