| Message ID | 20220416093753.3054696-3-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support concurrent sync io for bfq on a specail occasion | expand |
On Sat 16-04-22 17:37:50, Yu Kuai wrote: > Weight-raised queue is not inserted to weights_tree, which makes it > impossible to track how many queues have pending requests through > weights_tree insertion and removel. This patch add fake weight_counter > for weight-raised queue to do that. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> This is a bit hacky. I was looking into a better place where to hook to count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() and bfq_del_bfqq_busy() are ideal for this. It also makes better sense conceptually than hooking into weights tree handling. Other than this the rest of the series looks fine to me. Honza > --- > block/bfq-iosched.c | 11 +++++++++++ > block/bfq-wf2q.c | 5 ++--- > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 2deea2d07a1f..a2977c938c70 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -134,6 +134,8 @@ > #include "bfq-iosched.h" > #include "blk-wbt.h" > > +#define BFQ_FAKE_WEIGHT_COUNTER ((void *) POISON_INUSE) > + > #define BFQ_BFQQ_FNS(name) \ > void bfq_mark_bfqq_##name(struct bfq_queue *bfqq) \ > { \ > @@ -884,6 +886,12 @@ void bfq_weights_tree_add(struct bfq_data *bfqd, struct bfq_queue *bfqq) > if (bfqq->weight_counter) > return; > > + if (bfqq->wr_coeff != 1) { > + bfqq->weight_counter = BFQ_FAKE_WEIGHT_COUNTER; > + bfqq->ref++; > + return; > + } > + > while (*new) { > struct bfq_weight_counter *__counter = container_of(*new, > struct bfq_weight_counter, > @@ -943,6 +951,9 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd, struct bfq_queue *bfqq) > if (!bfqq->weight_counter) > return; > > + if (bfqq->weight_counter == BFQ_FAKE_WEIGHT_COUNTER) > + goto reset_entity_pointer; > + > root = &bfqd->queue_weights_tree; > bfqq->weight_counter->num_active--; > if (bfqq->weight_counter->num_active > 0) > diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c > index a1296058c1ec..ae12c6b2c525 100644 > --- a/block/bfq-wf2q.c > +++ b/block/bfq-wf2q.c > @@ -776,7 +776,7 @@ __bfq_entity_update_weight_prio(struct bfq_service_tree *old_st, > * Add the entity, if it is not a weight-raised queue, > * to the counter associated with its new weight. > */ > - if (prev_weight != new_weight && bfqq && bfqq->wr_coeff == 1) > + if (prev_weight != new_weight && bfqq) > bfq_weights_tree_add(bfqd, bfqq); > > new_st->wsum += entity->weight; > @@ -1680,8 +1680,7 @@ void bfq_add_bfqq_busy(struct bfq_data *bfqd, struct bfq_queue *bfqq) > bfqd->busy_queues[bfqq->ioprio_class - 1]++; > > if (!bfqq->dispatched) > - if (bfqq->wr_coeff == 1) > - bfq_weights_tree_add(bfqd, bfqq); > + bfq_weights_tree_add(bfqd, bfqq); > > if (bfqq->wr_coeff > 1) > bfqd->wr_busy_queues++; > -- > 2.31.1 >
在 2022/04/25 17:48, Jan Kara 写道: > On Sat 16-04-22 17:37:50, Yu Kuai wrote: >> Weight-raised queue is not inserted to weights_tree, which makes it >> impossible to track how many queues have pending requests through >> weights_tree insertion and removel. This patch add fake weight_counter >> for weight-raised queue to do that. >> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > This is a bit hacky. I was looking into a better place where to hook to > count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() > and bfq_del_bfqq_busy() are ideal for this. It also makes better sense > conceptually than hooking into weights tree handling. > Hi, bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are dispatched, however there might still some reqs are't completed yet. Here what we want to track is how many bfqqs have pending reqs, specifically if the bfqq have reqs are't complted. Thus I think bfq_del_bfqq_busy() is not the right place to do that. Thanks, Kuai > Other than this the rest of the series looks fine to me. > > Honza
在 2022/04/25 21:34, yukuai (C) 写道: > 在 2022/04/25 17:48, Jan Kara 写道: >> On Sat 16-04-22 17:37:50, Yu Kuai wrote: >>> Weight-raised queue is not inserted to weights_tree, which makes it >>> impossible to track how many queues have pending requests through >>> weights_tree insertion and removel. This patch add fake weight_counter >>> for weight-raised queue to do that. >>> >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> >> This is a bit hacky. I was looking into a better place where to hook to >> count entities in a bfq_group with requests and I think >> bfq_add_bfqq_busy() >> and bfq_del_bfqq_busy() are ideal for this. It also makes better sense >> conceptually than hooking into weights tree handling. >> > Hi, > > bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are > dispatched, however there might still some reqs are't completed yet. > > Here what we want to track is how many bfqqs have pending reqs, > specifically if the bfqq have reqs are't complted. > > Thus I think bfq_del_bfqq_busy() is not the right place to do that. BTW, there is a counter 'dispatched' in bfqq, how about we rename it to 'inflight', and inc when adding req to bfqq, dec the same as 'dispatched' ? This way we can count bfqq when adding 'inflight' from 0 to 1, and stop when decreasing 'inflight' from 1 to 0.
Hello! On Mon 25-04-22 21:34:16, yukuai (C) wrote: > 在 2022/04/25 17:48, Jan Kara 写道: > > On Sat 16-04-22 17:37:50, Yu Kuai wrote: > > > Weight-raised queue is not inserted to weights_tree, which makes it > > > impossible to track how many queues have pending requests through > > > weights_tree insertion and removel. This patch add fake weight_counter > > > for weight-raised queue to do that. > > > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > > > This is a bit hacky. I was looking into a better place where to hook to > > count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() > > and bfq_del_bfqq_busy() are ideal for this. It also makes better sense > > conceptually than hooking into weights tree handling. > > bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are > dispatched, however there might still some reqs are't completed yet. > > Here what we want to track is how many bfqqs have pending reqs, > specifically if the bfqq have reqs are't complted. > > Thus I think bfq_del_bfqq_busy() is not the right place to do that. Yes, I'm aware there will be a difference. But note that bfqq can stay busy with only dispatched requests because the logic in __bfq_bfqq_expire() will not call bfq_del_bfqq_busy() if idling is needed for service guarantees. So I think using bfq_add/del_bfqq_busy() would work OK. Honza
On Mon 25-04-22 21:55:46, yukuai (C) wrote: > 在 2022/04/25 21:34, yukuai (C) 写道: > > 在 2022/04/25 17:48, Jan Kara 写道: > > > On Sat 16-04-22 17:37:50, Yu Kuai wrote: > > > > Weight-raised queue is not inserted to weights_tree, which makes it > > > > impossible to track how many queues have pending requests through > > > > weights_tree insertion and removel. This patch add fake weight_counter > > > > for weight-raised queue to do that. > > > > > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > > > > > This is a bit hacky. I was looking into a better place where to hook to > > > count entities in a bfq_group with requests and I think > > > bfq_add_bfqq_busy() > > > and bfq_del_bfqq_busy() are ideal for this. It also makes better sense > > > conceptually than hooking into weights tree handling. > > > > > Hi, > > > > bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are > > dispatched, however there might still some reqs are't completed yet. > > > > Here what we want to track is how many bfqqs have pending reqs, > > specifically if the bfqq have reqs are't complted. > > > > Thus I think bfq_del_bfqq_busy() is not the right place to do that. > > BTW, there is a counter 'dispatched' in bfqq, how about we rename it > to 'inflight', and inc when adding req to bfqq, dec the same as > 'dispatched' ? > > This way we can count bfqq when adding 'inflight' from 0 to 1, and > stop when decreasing 'inflight' from 1 to 0. Well, but 'dispatched' is used in quite a few places and it would require quite some thinking to decide which impact using 'inflight' has there... But we also have 'bfqq->entity.allocated' which is number of requests in some state associated with bfqq and we could use that. But as I wrote in my previous email, I'm not convinced it is really necessary... Honza
在 2022/04/26 0:16, Jan Kara 写道: > Hello! > > On Mon 25-04-22 21:34:16, yukuai (C) wrote: >> 在 2022/04/25 17:48, Jan Kara 写道: >>> On Sat 16-04-22 17:37:50, Yu Kuai wrote: >>>> Weight-raised queue is not inserted to weights_tree, which makes it >>>> impossible to track how many queues have pending requests through >>>> weights_tree insertion and removel. This patch add fake weight_counter >>>> for weight-raised queue to do that. >>>> >>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>> >>> This is a bit hacky. I was looking into a better place where to hook to >>> count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() >>> and bfq_del_bfqq_busy() are ideal for this. It also makes better sense >>> conceptually than hooking into weights tree handling. >> >> bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are >> dispatched, however there might still some reqs are't completed yet. >> >> Here what we want to track is how many bfqqs have pending reqs, >> specifically if the bfqq have reqs are't complted. >> >> Thus I think bfq_del_bfqq_busy() is not the right place to do that. > > Yes, I'm aware there will be a difference. But note that bfqq can stay busy > with only dispatched requests because the logic in __bfq_bfqq_expire() will > not call bfq_del_bfqq_busy() if idling is needed for service guarantees. So > I think using bfq_add/del_bfqq_busy() would work OK. Hi, I didn't think of that before. If bfqq stay busy after dispathing all the requests, there are two other places that bfqq can clear busy: 1) bfq_remove_request(), bfqq has to insert a new req while it's not in service. 2) bfq_release_process_ref(), user thread is gone / moved, or old bfqq is gone due to merge / ioprio change. I wonder, will bfq_del_bfqq_busy() be called immediately when requests are completed? (It seems not to me...). For example, a user thread issue a sync io just once, and it keep running without issuing new io, then when does the bfqq clears the busy state? Thanks, Kuai > > Honza >
On Tue 26-04-22 09:49:04, yukuai (C) wrote: > 在 2022/04/26 0:16, Jan Kara 写道: > > Hello! > > > > On Mon 25-04-22 21:34:16, yukuai (C) wrote: > > > 在 2022/04/25 17:48, Jan Kara 写道: > > > > On Sat 16-04-22 17:37:50, Yu Kuai wrote: > > > > > Weight-raised queue is not inserted to weights_tree, which makes it > > > > > impossible to track how many queues have pending requests through > > > > > weights_tree insertion and removel. This patch add fake weight_counter > > > > > for weight-raised queue to do that. > > > > > > > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > > > > > > > This is a bit hacky. I was looking into a better place where to hook to > > > > count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() > > > > and bfq_del_bfqq_busy() are ideal for this. It also makes better sense > > > > conceptually than hooking into weights tree handling. > > > > > > bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are > > > dispatched, however there might still some reqs are't completed yet. > > > > > > Here what we want to track is how many bfqqs have pending reqs, > > > specifically if the bfqq have reqs are't complted. > > > > > > Thus I think bfq_del_bfqq_busy() is not the right place to do that. > > > > Yes, I'm aware there will be a difference. But note that bfqq can stay busy > > with only dispatched requests because the logic in __bfq_bfqq_expire() will > > not call bfq_del_bfqq_busy() if idling is needed for service guarantees. So > > I think using bfq_add/del_bfqq_busy() would work OK. > Hi, > > I didn't think of that before. If bfqq stay busy after dispathing all > the requests, there are two other places that bfqq can clear busy: > > 1) bfq_remove_request(), bfqq has to insert a new req while it's not in > service. Yes and the request then would have to be dispatched or merged. Which generally means another bfqq from the same bfqg is currently active and thus this should have no impact on service guarantees we are interested in. > 2) bfq_release_process_ref(), user thread is gone / moved, or old bfqq > is gone due to merge / ioprio change. Yes, here there's no new IO for the bfqq so no point in maintaining any service guarantees to it. > I wonder, will bfq_del_bfqq_busy() be called immediately when requests > are completed? (It seems not to me...). For example, a user thread > issue a sync io just once, and it keep running without issuing new io, > then when does the bfqq clears the busy state? No, when bfqq is kept busy, it will get scheduled as in-service queue in the future. Then what happens depends on whether it will get more requests or not. But generally its busy state will get cleared once it is expired for other reason than preemption. Honza
在 2022/04/26 15:40, Jan Kara 写道: > On Tue 26-04-22 09:49:04, yukuai (C) wrote: >> 在 2022/04/26 0:16, Jan Kara 写道: >>> Hello! >>> >>> On Mon 25-04-22 21:34:16, yukuai (C) wrote: >>>> 在 2022/04/25 17:48, Jan Kara 写道: >>>>> On Sat 16-04-22 17:37:50, Yu Kuai wrote: >>>>>> Weight-raised queue is not inserted to weights_tree, which makes it >>>>>> impossible to track how many queues have pending requests through >>>>>> weights_tree insertion and removel. This patch add fake weight_counter >>>>>> for weight-raised queue to do that. >>>>>> >>>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>>>> >>>>> This is a bit hacky. I was looking into a better place where to hook to >>>>> count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() >>>>> and bfq_del_bfqq_busy() are ideal for this. It also makes better sense >>>>> conceptually than hooking into weights tree handling. >>>> >>>> bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are >>>> dispatched, however there might still some reqs are't completed yet. >>>> >>>> Here what we want to track is how many bfqqs have pending reqs, >>>> specifically if the bfqq have reqs are't complted. >>>> >>>> Thus I think bfq_del_bfqq_busy() is not the right place to do that. >>> >>> Yes, I'm aware there will be a difference. But note that bfqq can stay busy >>> with only dispatched requests because the logic in __bfq_bfqq_expire() will >>> not call bfq_del_bfqq_busy() if idling is needed for service guarantees. So >>> I think using bfq_add/del_bfqq_busy() would work OK. >> Hi, >> >> I didn't think of that before. If bfqq stay busy after dispathing all >> the requests, there are two other places that bfqq can clear busy: >> >> 1) bfq_remove_request(), bfqq has to insert a new req while it's not in >> service. > > Yes and the request then would have to be dispatched or merged. Which > generally means another bfqq from the same bfqg is currently active and > thus this should have no impact on service guarantees we are interested in. > >> 2) bfq_release_process_ref(), user thread is gone / moved, or old bfqq >> is gone due to merge / ioprio change. > > Yes, here there's no new IO for the bfqq so no point in maintaining any > service guarantees to it. > >> I wonder, will bfq_del_bfqq_busy() be called immediately when requests >> are completed? (It seems not to me...). For example, a user thread >> issue a sync io just once, and it keep running without issuing new io, >> then when does the bfqq clears the busy state? > > No, when bfqq is kept busy, it will get scheduled as in-service queue in > the future. Then what happens depends on whether it will get more requests > or not. But generally its busy state will get cleared once it is expired > for other reason than preemption. Thanks for your explanation. I think in normal case using bfq_add/del_bfqq_busy() if fine. There is one last situation that I'm worried: If some disk are very slow that the dispatched reqs are not completed when the bfqq is rescheduled as in-service queue, and thus busy state can be cleared while reqs are not completed. Using bfq_del_bfqq_busy() will change behaviour in this specail case, do you think service guarantees will be broken? Thanks, Kuai
On Tue 26-04-22 16:27:46, yukuai (C) wrote: > 在 2022/04/26 15:40, Jan Kara 写道: > > On Tue 26-04-22 09:49:04, yukuai (C) wrote: > > > 在 2022/04/26 0:16, Jan Kara 写道: > > > > Hello! > > > > > > > > On Mon 25-04-22 21:34:16, yukuai (C) wrote: > > > > > 在 2022/04/25 17:48, Jan Kara 写道: > > > > > > On Sat 16-04-22 17:37:50, Yu Kuai wrote: > > > > > > > Weight-raised queue is not inserted to weights_tree, which makes it > > > > > > > impossible to track how many queues have pending requests through > > > > > > > weights_tree insertion and removel. This patch add fake weight_counter > > > > > > > for weight-raised queue to do that. > > > > > > > > > > > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > > > > > > > > > > > This is a bit hacky. I was looking into a better place where to hook to > > > > > > count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() > > > > > > and bfq_del_bfqq_busy() are ideal for this. It also makes better sense > > > > > > conceptually than hooking into weights tree handling. > > > > > > > > > > bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are > > > > > dispatched, however there might still some reqs are't completed yet. > > > > > > > > > > Here what we want to track is how many bfqqs have pending reqs, > > > > > specifically if the bfqq have reqs are't complted. > > > > > > > > > > Thus I think bfq_del_bfqq_busy() is not the right place to do that. > > > > > > > > Yes, I'm aware there will be a difference. But note that bfqq can stay busy > > > > with only dispatched requests because the logic in __bfq_bfqq_expire() will > > > > not call bfq_del_bfqq_busy() if idling is needed for service guarantees. So > > > > I think using bfq_add/del_bfqq_busy() would work OK. > > > Hi, > > > > > > I didn't think of that before. If bfqq stay busy after dispathing all > > > the requests, there are two other places that bfqq can clear busy: > > > > > > 1) bfq_remove_request(), bfqq has to insert a new req while it's not in > > > service. > > > > Yes and the request then would have to be dispatched or merged. Which > > generally means another bfqq from the same bfqg is currently active and > > thus this should have no impact on service guarantees we are interested in. > > > > > 2) bfq_release_process_ref(), user thread is gone / moved, or old bfqq > > > is gone due to merge / ioprio change. > > > > Yes, here there's no new IO for the bfqq so no point in maintaining any > > service guarantees to it. > > > > > I wonder, will bfq_del_bfqq_busy() be called immediately when requests > > > are completed? (It seems not to me...). For example, a user thread > > > issue a sync io just once, and it keep running without issuing new io, > > > then when does the bfqq clears the busy state? > > > > No, when bfqq is kept busy, it will get scheduled as in-service queue in > > the future. Then what happens depends on whether it will get more requests > > or not. But generally its busy state will get cleared once it is expired > > for other reason than preemption. > > Thanks for your explanation. > > I think in normal case using bfq_add/del_bfqq_busy() if fine. > > There is one last situation that I'm worried: If some disk are very > slow that the dispatched reqs are not completed when the bfqq is > rescheduled as in-service queue, and thus busy state can be cleared > while reqs are not completed. > > Using bfq_del_bfqq_busy() will change behaviour in this specail case, > do you think service guarantees will be broken? Well, I don't think so. Because slow disks don't tend to do a lot of internal scheduling (or have deep IO queues for that matter). Also note that generally bfq_select_queue() will not even expire a queue (despite it not having any requests to dispatch) when we should not dispatch other requests to maintain service guarantees. So I think service guarantees will be generally preserved. Obviously I could be wrong, we we will not know until we try it :). Honza
在 2022/04/26 17:15, Jan Kara 写道: > On Tue 26-04-22 16:27:46, yukuai (C) wrote: >> 在 2022/04/26 15:40, Jan Kara 写道: >>> On Tue 26-04-22 09:49:04, yukuai (C) wrote: >>>> 在 2022/04/26 0:16, Jan Kara 写道: >>>>> Hello! >>>>> >>>>> On Mon 25-04-22 21:34:16, yukuai (C) wrote: >>>>>> 在 2022/04/25 17:48, Jan Kara 写道: >>>>>>> On Sat 16-04-22 17:37:50, Yu Kuai wrote: >>>>>>>> Weight-raised queue is not inserted to weights_tree, which makes it >>>>>>>> impossible to track how many queues have pending requests through >>>>>>>> weights_tree insertion and removel. This patch add fake weight_counter >>>>>>>> for weight-raised queue to do that. >>>>>>>> >>>>>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>>>>>> >>>>>>> This is a bit hacky. I was looking into a better place where to hook to >>>>>>> count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() >>>>>>> and bfq_del_bfqq_busy() are ideal for this. It also makes better sense >>>>>>> conceptually than hooking into weights tree handling. >>>>>> >>>>>> bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are >>>>>> dispatched, however there might still some reqs are't completed yet. >>>>>> >>>>>> Here what we want to track is how many bfqqs have pending reqs, >>>>>> specifically if the bfqq have reqs are't complted. >>>>>> >>>>>> Thus I think bfq_del_bfqq_busy() is not the right place to do that. >>>>> >>>>> Yes, I'm aware there will be a difference. But note that bfqq can stay busy >>>>> with only dispatched requests because the logic in __bfq_bfqq_expire() will >>>>> not call bfq_del_bfqq_busy() if idling is needed for service guarantees. So >>>>> I think using bfq_add/del_bfqq_busy() would work OK. >>>> Hi, >>>> >>>> I didn't think of that before. If bfqq stay busy after dispathing all >>>> the requests, there are two other places that bfqq can clear busy: >>>> >>>> 1) bfq_remove_request(), bfqq has to insert a new req while it's not in >>>> service. >>> >>> Yes and the request then would have to be dispatched or merged. Which >>> generally means another bfqq from the same bfqg is currently active and >>> thus this should have no impact on service guarantees we are interested in. >>> >>>> 2) bfq_release_process_ref(), user thread is gone / moved, or old bfqq >>>> is gone due to merge / ioprio change. >>> >>> Yes, here there's no new IO for the bfqq so no point in maintaining any >>> service guarantees to it. >>> >>>> I wonder, will bfq_del_bfqq_busy() be called immediately when requests >>>> are completed? (It seems not to me...). For example, a user thread >>>> issue a sync io just once, and it keep running without issuing new io, >>>> then when does the bfqq clears the busy state? >>> >>> No, when bfqq is kept busy, it will get scheduled as in-service queue in >>> the future. Then what happens depends on whether it will get more requests >>> or not. But generally its busy state will get cleared once it is expired >>> for other reason than preemption. >> >> Thanks for your explanation. >> >> I think in normal case using bfq_add/del_bfqq_busy() if fine. >> >> There is one last situation that I'm worried: If some disk are very >> slow that the dispatched reqs are not completed when the bfqq is >> rescheduled as in-service queue, and thus busy state can be cleared >> while reqs are not completed. >> >> Using bfq_del_bfqq_busy() will change behaviour in this specail case, >> do you think service guarantees will be broken? > > Well, I don't think so. Because slow disks don't tend to do a lot of > internal scheduling (or have deep IO queues for that matter). Also note > that generally bfq_select_queue() will not even expire a queue (despite it > not having any requests to dispatch) when we should not dispatch other > requests to maintain service guarantees. So I think service guarantees will > be generally preserved. Obviously I could be wrong, we we will not know > until we try it :). Thanks a lot for your explanation, I'll do some tests. And i'll send a new version if tests look good.
> Il giorno 26 apr 2022, alle ore 11:15, Jan Kara <jack@suse.cz> ha scritto: > > On Tue 26-04-22 16:27:46, yukuai (C) wrote: >> 在 2022/04/26 15:40, Jan Kara 写道: >>> On Tue 26-04-22 09:49:04, yukuai (C) wrote: >>>> 在 2022/04/26 0:16, Jan Kara 写道: >>>>> Hello! >>>>> >>>>> On Mon 25-04-22 21:34:16, yukuai (C) wrote: >>>>>> 在 2022/04/25 17:48, Jan Kara 写道: >>>>>>> On Sat 16-04-22 17:37:50, Yu Kuai wrote: >>>>>>>> Weight-raised queue is not inserted to weights_tree, which makes it >>>>>>>> impossible to track how many queues have pending requests through >>>>>>>> weights_tree insertion and removel. This patch add fake weight_counter >>>>>>>> for weight-raised queue to do that. >>>>>>>> >>>>>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>>>>>> >>>>>>> This is a bit hacky. I was looking into a better place where to hook to >>>>>>> count entities in a bfq_group with requests and I think bfq_add_bfqq_busy() >>>>>>> and bfq_del_bfqq_busy() are ideal for this. It also makes better sense >>>>>>> conceptually than hooking into weights tree handling. >>>>>> >>>>>> bfq_del_bfqq_busy() will be called when all the reqs in the bfqq are >>>>>> dispatched, however there might still some reqs are't completed yet. >>>>>> >>>>>> Here what we want to track is how many bfqqs have pending reqs, >>>>>> specifically if the bfqq have reqs are't complted. >>>>>> >>>>>> Thus I think bfq_del_bfqq_busy() is not the right place to do that. >>>>> >>>>> Yes, I'm aware there will be a difference. But note that bfqq can stay busy >>>>> with only dispatched requests because the logic in __bfq_bfqq_expire() will >>>>> not call bfq_del_bfqq_busy() if idling is needed for service guarantees. So >>>>> I think using bfq_add/del_bfqq_busy() would work OK. >>>> Hi, >>>> >>>> I didn't think of that before. If bfqq stay busy after dispathing all >>>> the requests, there are two other places that bfqq can clear busy: >>>> >>>> 1) bfq_remove_request(), bfqq has to insert a new req while it's not in >>>> service. >>> >>> Yes and the request then would have to be dispatched or merged. Which >>> generally means another bfqq from the same bfqg is currently active and >>> thus this should have no impact on service guarantees we are interested in. >>> >>>> 2) bfq_release_process_ref(), user thread is gone / moved, or old bfqq >>>> is gone due to merge / ioprio change. >>> >>> Yes, here there's no new IO for the bfqq so no point in maintaining any >>> service guarantees to it. >>> >>>> I wonder, will bfq_del_bfqq_busy() be called immediately when requests >>>> are completed? (It seems not to me...). For example, a user thread >>>> issue a sync io just once, and it keep running without issuing new io, >>>> then when does the bfqq clears the busy state? >>> >>> No, when bfqq is kept busy, it will get scheduled as in-service queue in >>> the future. Then what happens depends on whether it will get more requests >>> or not. But generally its busy state will get cleared once it is expired >>> for other reason than preemption. >> >> Thanks for your explanation. >> >> I think in normal case using bfq_add/del_bfqq_busy() if fine. >> >> There is one last situation that I'm worried: If some disk are very >> slow that the dispatched reqs are not completed when the bfqq is >> rescheduled as in-service queue, and thus busy state can be cleared >> while reqs are not completed. >> >> Using bfq_del_bfqq_busy() will change behaviour in this specail case, >> do you think service guarantees will be broken? > > Well, I don't think so. Because slow disks don't tend to do a lot of > internal scheduling (or have deep IO queues for that matter). Also note > that generally bfq_select_queue() will not even expire a queue (despite it > not having any requests to dispatch) when we should not dispatch other > requests to maintain service guarantees. So I think service guarantees will > be generally preserved. Obviously I could be wrong, we we will not know > until we try it :). > I have nothing to add ... You guys are getting better than me about BFQ :) Thanks, Paolo > Honza > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 2deea2d07a1f..a2977c938c70 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -134,6 +134,8 @@ #include "bfq-iosched.h" #include "blk-wbt.h" +#define BFQ_FAKE_WEIGHT_COUNTER ((void *) POISON_INUSE) + #define BFQ_BFQQ_FNS(name) \ void bfq_mark_bfqq_##name(struct bfq_queue *bfqq) \ { \ @@ -884,6 +886,12 @@ void bfq_weights_tree_add(struct bfq_data *bfqd, struct bfq_queue *bfqq) if (bfqq->weight_counter) return; + if (bfqq->wr_coeff != 1) { + bfqq->weight_counter = BFQ_FAKE_WEIGHT_COUNTER; + bfqq->ref++; + return; + } + while (*new) { struct bfq_weight_counter *__counter = container_of(*new, struct bfq_weight_counter, @@ -943,6 +951,9 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd, struct bfq_queue *bfqq) if (!bfqq->weight_counter) return; + if (bfqq->weight_counter == BFQ_FAKE_WEIGHT_COUNTER) + goto reset_entity_pointer; + root = &bfqd->queue_weights_tree; bfqq->weight_counter->num_active--; if (bfqq->weight_counter->num_active > 0) diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c index a1296058c1ec..ae12c6b2c525 100644 --- a/block/bfq-wf2q.c +++ b/block/bfq-wf2q.c @@ -776,7 +776,7 @@ __bfq_entity_update_weight_prio(struct bfq_service_tree *old_st, * Add the entity, if it is not a weight-raised queue, * to the counter associated with its new weight. */ - if (prev_weight != new_weight && bfqq && bfqq->wr_coeff == 1) + if (prev_weight != new_weight && bfqq) bfq_weights_tree_add(bfqd, bfqq); new_st->wsum += entity->weight; @@ -1680,8 +1680,7 @@ void bfq_add_bfqq_busy(struct bfq_data *bfqd, struct bfq_queue *bfqq) bfqd->busy_queues[bfqq->ioprio_class - 1]++; if (!bfqq->dispatched) - if (bfqq->wr_coeff == 1) - bfq_weights_tree_add(bfqd, bfqq); + bfq_weights_tree_add(bfqd, bfqq); if (bfqq->wr_coeff > 1) bfqd->wr_busy_queues++;
Weight-raised queue is not inserted to weights_tree, which makes it impossible to track how many queues have pending requests through weights_tree insertion and removel. This patch add fake weight_counter for weight-raised queue to do that. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- block/bfq-iosched.c | 11 +++++++++++ block/bfq-wf2q.c | 5 ++--- 2 files changed, 13 insertions(+), 3 deletions(-)