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

[Yu Kuai]

在 2022/05/31 17:19, Paolo Valente 写道:
> > Il giorno 31 mag 2022, alle ore 11:06, Yu Kuai <yukuai3@xxxxxxxxxx> ha scritto:
> >
> > 在 2022/05/31 16:36, Paolo VALENTE 写道:
> > > > Il giorno 30 mag 2022, alle ore 10:40, Yu Kuai <yukuai3@xxxxxxxxxx> ha scritto:
> > > >
> > > > 在 2022/05/30 16:34, Yu Kuai 写道:
> > > > > 在 2022/05/30 16:10, Paolo Valente 写道:
> > > > > > > Il giorno 28 mag 2022, alle ore 11:50, Yu Kuai <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 at least one bfqq that is marked busy.
> > > > > > > 3) Don't count if bfqg doesn't have any busy bfqqs.
> > > > > >
> > > > > > Unfortunately, I see a last problem here. I see a double change:
> > > > > > (1) a bfqg is now counted only as a function of the state of its child
> > > > > >       queues, and not of also its child bfqgs
> > > > > > (2) the state considered for counting a bfqg moves from having pending
> > > > > >       requests to having busy queues
> > > > > >
> > > > > > I'm ok with with (1), which is a good catch (you are lady explained
> > > > > > the idea to me some time ago IIRC).
> > > > > >
> > > > > > Yet I fear that (2) is not ok.  A bfqq can become non busy even if it
> > > > > > still has in-flight I/O, i.e.  I/O being served in the drive.  The
> > > > > > weight of such a bfqq must still be considered in the weights_tree,
> > > > > > and the group containing such a queue must still be counted when
> > > > > > checking whether the scenario is asymmetric.  Otherwise service
> > > > > > guarantees are broken.  The reason is that, if a scenario is deemed as
> > > > > > symmetric because in-flight I/O is not taken into account, then idling
> > > > > > will not be performed to protect some bfqq, and in-flight I/O may
> > > > > > steal bandwidth to that bfqq in an uncontrolled way.
> > > > > Hi, Paolo
> > > > > Thanks for your explanation.
> > > > > My orginal thoughts was using weights_tree insertion/removal, however,
> > > > > Jan convinced me that using bfq_add/del_bfqq_busy() is ok.
> > > > >  From what I see, when bfqq dispatch the last request,
> > > > > bfq_del_bfqq_busy() will not be called from __bfq_bfqq_expire() if
> > > > > idling is needed, and it will delayed to when such bfqq get scheduled as
> > > > > in-service queue again. Which means the weight of such bfqq should still
> > > > > be considered in the weights_tree.
> > > > > I also run some tests on null_blk with "irqmode=2
> > > > > completion_nsec=100000000(100ms) hw_queue_depth=1", and tests show
> > > > > that service guarantees are still preserved on slow device.
> > > > > Do you this is strong enough to cover your concern?
> > > Unfortunately it is not.  Your very argument is what made be believe
> > > that considering busy queues was enough, in the first place.  But, as
> > > I found out, the problem is caused by the queues that do not enjoy
> > > idling.  With your patch (as well as in my initial version) they are
> > > not counted when they remain without requests queued.  And this makes
> > > asymmetric scenarios be considered erroneously as symmetric.  The
> > > consequence is that idling gets switched off when it had to be kept
> > > on, and control on bandwidth is lost for the victim in-service queues.
> >
> > Hi，Paolo
> >
> > Thanks for your explanation, are you thinking that if bfqq doesn't enjoy
> > idling, then such bfqq will clear busy after dispatching the last
> > request?
> >
> > Please kindly correct me if I'm wrong in the following process:
> >
> > If there are more than one bfqg that is activatied, then bfqqs that are
> > not enjoying idle are still left busy after dispatching the last
> > request.
> >
> > details in __bfq_bfqq_expire:
> >
> >         if (RB_EMPTY_ROOT(&bfqq->sort_list) &&
> >         ┊   !(reason == BFQQE_PREEMPTED &&
> >         ┊     idling_needed_for_service_guarantees(bfqd, bfqq))) {
> > -> idling_needed_for_service_guarantees will always return true,
>
> It returns true only is the scenario is symmetric.  Not counting bfqqs
> with in-flight requests makes an asymmetric scenario be considered
> wrongly symmetric.  See function bfq_asymmetric_scenario().
Hi,

Yes, with this patchset, If there are more than one bfqg that is
activatied(contain busy bfqq), bfq_asymmetric_scenario() will return
true:

bfq_asymmetric_scenario()
 	return varied_queue_weights || multiple_classes_busy
 #ifdef CONFIG_BFQ_GROUP_IOSCHED
	       || bfqd->num_groups_with_busy_queues > 1
 #endif

From what I see, bfqd->num_groups_with_busy_queues > 1 is always true...
> Paolo
>
> > bfqq(whether or not enjoy idling) will stay busy.
> >                 if (bfqq->dispatched == 0)
> >                         /*
> >                         ┊* Overloading budget_timeout field 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_del_bfqq_busy(bfqd, bfqq, true);
> >
> > Thanks,
> > Kuai
>
> .