| Message ID | 20211014014556.3597008-2-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | optimize the bfq queue idle judgment | expand |
> Il giorno 14 ott 2021, alle ore 03:45, Yu Kuai <yukuai3@huawei.com> ha scritto: > > 'num_groups_with_pending_reqs' represents how many groups that are > not root group and have pending requests. This patch also counted > root group into 'num_groups_with_pending_reqs'. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/bfq-iosched.c | 36 ++++++++++++++++++++++++++------ > block/bfq-wf2q.c | 50 +++++++++++++++++++++++++++++++++------------ > 2 files changed, 67 insertions(+), 19 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index fec18118dc30..d251735383f7 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -852,6 +852,16 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd, > bfq_put_queue(bfqq); > } > > +static inline void > +bfq_clear_group_with_pending_reqs(struct bfq_data *bfqd, > + struct bfq_entity *entity) > +{ > + if (entity->in_groups_with_pending_reqs) { > + entity->in_groups_with_pending_reqs = false; > + bfqd->num_groups_with_pending_reqs--; > + } > +} > + > /* > * Invoke __bfq_weights_tree_remove on bfqq and decrement the number > * of active groups for each queue's inactive parent entity. > @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, > struct bfq_queue *bfqq) > { > struct bfq_entity *entity = bfqq->entity.parent; > + struct bfq_sched_data *sd; > + > + /* > + * If the bfq queue is in root group, the decrement of > + * num_groups_with_pending_reqs is performed immediately upon the > + * deactivation of entity. > + */ > + if (!entity) { > + entity = &bfqd->root_group->entity; > + sd = entity->my_sched_data; > + > + if (!sd->in_service_entity) > + bfq_clear_group_with_pending_reqs(bfqd, entity); > + > + return; > + } > > for_each_entity(entity) { > - struct bfq_sched_data *sd = entity->my_sched_data; > + sd = entity->my_sched_data; > > if (sd->next_in_service || sd->in_service_entity) { > /* > @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, > } > > /* > - * The decrement of num_groups_with_pending_reqs is > + * If the bfq queue is not in root group, > + * the decrement of num_groups_with_pending_reqs is I'm sorry if I didn't notice this before, but why do you postpone the decrement only for queues not in root group? If I'm not missing anything, the active (i.e., with pending reqs) state of the root group is to be computed as that of ay other group. Thanks, Paolo > * 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 > @@ -889,10 +916,7 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, > * 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--; > - } > + bfq_clear_group_with_pending_reqs(bfqd, entity); > } > > /* > diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c > index b74cc0da118e..3e9e672aa302 100644 > --- a/block/bfq-wf2q.c > +++ b/block/bfq-wf2q.c > @@ -945,6 +945,42 @@ static void bfq_update_fin_time_enqueue(struct bfq_entity *entity, > > bfq_active_insert(st, entity); > } > +#ifdef CONFIG_BFQ_GROUP_IOSCHED > +static inline void > +bfq_set_group_with_pending_reqs(struct bfq_data *bfqd, > + struct bfq_entity *entity) > +{ > + if (!entity->in_groups_with_pending_reqs) { > + entity->in_groups_with_pending_reqs = true; > + bfqd->num_groups_with_pending_reqs++; > + } > +} > + > +static void bfq_update_groups_with_pending_reqs(struct bfq_entity *entity) > +{ > + struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity); > + > + if (bfqq) { > + /* > + * If the entity represents bfq_queue, and the queue belongs to > + * root cgroup. > + */ > + if (!entity->parent) > + bfq_set_group_with_pending_reqs(bfqq->bfqd, > + &bfqq->bfqd->root_group->entity); > + } else { > + /* If the entity represents bfq_group. */ > + struct bfq_group *bfqg = > + container_of(entity, struct bfq_group, entity); > + struct bfq_data *bfqd = bfqg->bfqd; > + > + bfq_set_group_with_pending_reqs(bfqd, entity); > + } > +} > +#else > +#define bfq_update_groups_with_pending_reqs(entity) \ > + do {} while (0) > +#endif > > /** > * __bfq_activate_entity - handle activation of entity. > @@ -999,19 +1035,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity, > entity->on_st_or_in_serv = true; > } > > -#ifdef CONFIG_BFQ_GROUP_IOSCHED > - if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */ > - struct bfq_group *bfqg = > - container_of(entity, struct bfq_group, entity); > - struct bfq_data *bfqd = bfqg->bfqd; > - > - if (!entity->in_groups_with_pending_reqs) { > - entity->in_groups_with_pending_reqs = true; > - bfqd->num_groups_with_pending_reqs++; > - } > - } > -#endif > - > + bfq_update_groups_with_pending_reqs(entity); > bfq_update_fin_time_enqueue(entity, st, backshifted); > } > > -- > 2.31.1 >
On 2021/10/20 16:51, Paolo Valente wrote: >> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >> struct bfq_queue *bfqq) >> { >> struct bfq_entity *entity = bfqq->entity.parent; >> + struct bfq_sched_data *sd; >> + >> + /* >> + * If the bfq queue is in root group, the decrement of >> + * num_groups_with_pending_reqs is performed immediately upon the >> + * deactivation of entity. >> + */ >> + if (!entity) { >> + entity = &bfqd->root_group->entity; >> + sd = entity->my_sched_data; >> + >> + if (!sd->in_service_entity) >> + bfq_clear_group_with_pending_reqs(bfqd, entity); >> + >> + return; >> + } >> >> for_each_entity(entity) { >> - struct bfq_sched_data *sd = entity->my_sched_data; >> + sd = entity->my_sched_data; >> >> if (sd->next_in_service || sd->in_service_entity) { >> /* >> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >> } >> >> /* >> - * The decrement of num_groups_with_pending_reqs is >> + * If the bfq queue is not in root group, >> + * the decrement of num_groups_with_pending_reqs is > > > I'm sorry if I didn't notice this before, but why do you postpone the > decrement only for queues not in root group? If I'm not missing > anything, the active (i.e., with pending reqs) state of the root group > is to be computed as that of ay other group. Hi, Paolo I thought if queue is in root group, then bfqq->entity.parent is NULL, and such case is handled above, which is separate from previous implementation for queues that are not in root group. Is this the wrong way to handle root group? Thanks, Kuai
> Il giorno 20 ott 2021, alle ore 11:20, yukuai (C) <yukuai3@huawei.com> ha scritto: > > On 2021/10/20 16:51, Paolo Valente wrote: > >>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >>> struct bfq_queue *bfqq) >>> { >>> struct bfq_entity *entity = bfqq->entity.parent; >>> + struct bfq_sched_data *sd; >>> + >>> + /* >>> + * If the bfq queue is in root group, the decrement of >>> + * num_groups_with_pending_reqs is performed immediately upon the >>> + * deactivation of entity. >>> + */ >>> + if (!entity) { >>> + entity = &bfqd->root_group->entity; >>> + sd = entity->my_sched_data; >>> + >>> + if (!sd->in_service_entity) >>> + bfq_clear_group_with_pending_reqs(bfqd, entity); >>> + >>> + return; >>> + } >>> >>> for_each_entity(entity) { >>> - struct bfq_sched_data *sd = entity->my_sched_data; >>> + sd = entity->my_sched_data; >>> >>> if (sd->next_in_service || sd->in_service_entity) { >>> /* >>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >>> } >>> >>> /* >>> - * The decrement of num_groups_with_pending_reqs is >>> + * If the bfq queue is not in root group, >>> + * the decrement of num_groups_with_pending_reqs is >> I'm sorry if I didn't notice this before, but why do you postpone the >> decrement only for queues not in root group? If I'm not missing >> anything, the active (i.e., with pending reqs) state of the root group >> is to be computed as that of ay other group. > > Hi, Paolo > > I thought if queue is in root group, then bfqq->entity.parent is NULL, > and such case is handled above, which is separate from previous > implementation for queues that are not in root group. > > Is this the wrong way to handle root group? > I think that, if we want to count also the root group among the active ones, then the logic for tagging the root group as active must be the same as the other groups. Or am I missing something? Thanks, Paolo > Thanks, > Kuai
On 2021/10/20 17:29, Paolo Valente wrote: > > >> Il giorno 20 ott 2021, alle ore 11:20, yukuai (C) <yukuai3@huawei.com> ha scritto: >> >> On 2021/10/20 16:51, Paolo Valente wrote: >> >>>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >>>> struct bfq_queue *bfqq) >>>> { >>>> struct bfq_entity *entity = bfqq->entity.parent; >>>> + struct bfq_sched_data *sd; >>>> + >>>> + /* >>>> + * If the bfq queue is in root group, the decrement of >>>> + * num_groups_with_pending_reqs is performed immediately upon the >>>> + * deactivation of entity. >>>> + */ >>>> + if (!entity) { >>>> + entity = &bfqd->root_group->entity; >>>> + sd = entity->my_sched_data; >>>> + >>>> + if (!sd->in_service_entity) >>>> + bfq_clear_group_with_pending_reqs(bfqd, entity); >>>> + >>>> + return; >>>> + } >>>> >>>> for_each_entity(entity) { >>>> - struct bfq_sched_data *sd = entity->my_sched_data; >>>> + sd = entity->my_sched_data; >>>> >>>> if (sd->next_in_service || sd->in_service_entity) { >>>> /* >>>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >>>> } >>>> >>>> /* >>>> - * The decrement of num_groups_with_pending_reqs is >>>> + * If the bfq queue is not in root group, >>>> + * the decrement of num_groups_with_pending_reqs is >>> I'm sorry if I didn't notice this before, but why do you postpone the >>> decrement only for queues not in root group? If I'm not missing >>> anything, the active (i.e., with pending reqs) state of the root group >>> is to be computed as that of ay other group. >> >> Hi, Paolo >> >> I thought if queue is in root group, then bfqq->entity.parent is NULL, >> and such case is handled above, which is separate from previous >> implementation for queues that are not in root group. >> >> Is this the wrong way to handle root group? >> > > I think that, if we want to count also the root group among the active > ones, then the logic for tagging the root group as active must be the > same as the other groups. Or am I missing something? Hi, Paolo Currently, if queue is in root group, bfqq->entity.parent is NULL, and this makes it hard to keep the same logic. Can we store root_group->my_entity to bfqq->entity.parent if the queue is in root group? Thanks, Kuai
> Il giorno 20 ott 2021, alle ore 11:38, yukuai (C) <yukuai3@huawei.com> ha scritto: > > On 2021/10/20 17:29, Paolo Valente wrote: >>> Il giorno 20 ott 2021, alle ore 11:20, yukuai (C) <yukuai3@huawei.com> ha scritto: >>> >>> On 2021/10/20 16:51, Paolo Valente wrote: >>> >>>>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >>>>> struct bfq_queue *bfqq) >>>>> { >>>>> struct bfq_entity *entity = bfqq->entity.parent; >>>>> + struct bfq_sched_data *sd; >>>>> + >>>>> + /* >>>>> + * If the bfq queue is in root group, the decrement of >>>>> + * num_groups_with_pending_reqs is performed immediately upon the >>>>> + * deactivation of entity. >>>>> + */ >>>>> + if (!entity) { >>>>> + entity = &bfqd->root_group->entity; >>>>> + sd = entity->my_sched_data; >>>>> + >>>>> + if (!sd->in_service_entity) >>>>> + bfq_clear_group_with_pending_reqs(bfqd, entity); >>>>> + >>>>> + return; >>>>> + } >>>>> >>>>> for_each_entity(entity) { >>>>> - struct bfq_sched_data *sd = entity->my_sched_data; >>>>> + sd = entity->my_sched_data; >>>>> >>>>> if (sd->next_in_service || sd->in_service_entity) { >>>>> /* >>>>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >>>>> } >>>>> >>>>> /* >>>>> - * The decrement of num_groups_with_pending_reqs is >>>>> + * If the bfq queue is not in root group, >>>>> + * the decrement of num_groups_with_pending_reqs is >>>> I'm sorry if I didn't notice this before, but why do you postpone the >>>> decrement only for queues not in root group? If I'm not missing >>>> anything, the active (i.e., with pending reqs) state of the root group >>>> is to be computed as that of ay other group. >>> >>> Hi, Paolo >>> >>> I thought if queue is in root group, then bfqq->entity.parent is NULL, >>> and such case is handled above, which is separate from previous >>> implementation for queues that are not in root group. >>> >>> Is this the wrong way to handle root group? >>> >> I think that, if we want to count also the root group among the active >> ones, then the logic for tagging the root group as active must be the >> same as the other groups. Or am I missing something? > > Hi, Paolo > > Currently, if queue is in root group, bfqq->entity.parent is NULL, and > this makes it hard to keep the same logic. > > Can we store root_group->my_entity to bfqq->entity.parent if the queue > is in root group? > Any sensible implementation is ok for me. Usually, stuff for root group is in the bfqd. Thanks, Paolo > Thanks, > Kuai
On 2021/10/20 17:43, Paolo Valente wrote: > > >> Il giorno 20 ott 2021, alle ore 11:38, yukuai (C) <yukuai3@huawei.com> ha scritto: >> >> On 2021/10/20 17:29, Paolo Valente wrote: >>>> Il giorno 20 ott 2021, alle ore 11:20, yukuai (C) <yukuai3@huawei.com> ha scritto: >>>> >>>> On 2021/10/20 16:51, Paolo Valente wrote: >>>> >>>>>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >>>>>> struct bfq_queue *bfqq) >>>>>> { >>>>>> struct bfq_entity *entity = bfqq->entity.parent; >>>>>> + struct bfq_sched_data *sd; >>>>>> + >>>>>> + /* >>>>>> + * If the bfq queue is in root group, the decrement of >>>>>> + * num_groups_with_pending_reqs is performed immediately upon the >>>>>> + * deactivation of entity. >>>>>> + */ >>>>>> + if (!entity) { >>>>>> + entity = &bfqd->root_group->entity; >>>>>> + sd = entity->my_sched_data; >>>>>> + >>>>>> + if (!sd->in_service_entity) >>>>>> + bfq_clear_group_with_pending_reqs(bfqd, entity); >>>>>> + >>>>>> + return; >>>>>> + } >>>>>> >>>>>> for_each_entity(entity) { >>>>>> - struct bfq_sched_data *sd = entity->my_sched_data; >>>>>> + sd = entity->my_sched_data; >>>>>> >>>>>> if (sd->next_in_service || sd->in_service_entity) { >>>>>> /* >>>>>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, >>>>>> } >>>>>> >>>>>> /* >>>>>> - * The decrement of num_groups_with_pending_reqs is >>>>>> + * If the bfq queue is not in root group, >>>>>> + * the decrement of num_groups_with_pending_reqs is >>>>> I'm sorry if I didn't notice this before, but why do you postpone the >>>>> decrement only for queues not in root group? If I'm not missing >>>>> anything, the active (i.e., with pending reqs) state of the root group >>>>> is to be computed as that of ay other group. >>>> >>>> Hi, Paolo >>>> >>>> I thought if queue is in root group, then bfqq->entity.parent is NULL, >>>> and such case is handled above, which is separate from previous >>>> implementation for queues that are not in root group. >>>> >>>> Is this the wrong way to handle root group? >>>> >>> I think that, if we want to count also the root group among the active >>> ones, then the logic for tagging the root group as active must be the >>> same as the other groups. Or am I missing something? >> >> Hi, Paolo >> >> Currently, if queue is in root group, bfqq->entity.parent is NULL, and >> this makes it hard to keep the same logic. >> >> Can we store root_group->my_entity to bfqq->entity.parent if the queue >> is in root group? >> > > Any sensible implementation is ok for me. Usually, stuff for root > group is in the bfqd. > I'll try to do implement that way, Thanks, Kuai > Thanks, > Paolo > >> Thanks, >> Kuai > > . >
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index fec18118dc30..d251735383f7 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -852,6 +852,16 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd, bfq_put_queue(bfqq); } +static inline void +bfq_clear_group_with_pending_reqs(struct bfq_data *bfqd, + struct bfq_entity *entity) +{ + if (entity->in_groups_with_pending_reqs) { + entity->in_groups_with_pending_reqs = false; + bfqd->num_groups_with_pending_reqs--; + } +} + /* * Invoke __bfq_weights_tree_remove on bfqq and decrement the number * of active groups for each queue's inactive parent entity. @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, struct bfq_queue *bfqq) { struct bfq_entity *entity = bfqq->entity.parent; + struct bfq_sched_data *sd; + + /* + * If the bfq queue is in root group, the decrement of + * num_groups_with_pending_reqs is performed immediately upon the + * deactivation of entity. + */ + if (!entity) { + entity = &bfqd->root_group->entity; + sd = entity->my_sched_data; + + if (!sd->in_service_entity) + bfq_clear_group_with_pending_reqs(bfqd, entity); + + return; + } for_each_entity(entity) { - struct bfq_sched_data *sd = entity->my_sched_data; + sd = entity->my_sched_data; if (sd->next_in_service || sd->in_service_entity) { /* @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, } /* - * The decrement of num_groups_with_pending_reqs is + * If the bfq queue is not in root group, + * 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 @@ -889,10 +916,7 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd, * 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--; - } + bfq_clear_group_with_pending_reqs(bfqd, entity); } /* diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c index b74cc0da118e..3e9e672aa302 100644 --- a/block/bfq-wf2q.c +++ b/block/bfq-wf2q.c @@ -945,6 +945,42 @@ static void bfq_update_fin_time_enqueue(struct bfq_entity *entity, bfq_active_insert(st, entity); } +#ifdef CONFIG_BFQ_GROUP_IOSCHED +static inline void +bfq_set_group_with_pending_reqs(struct bfq_data *bfqd, + struct bfq_entity *entity) +{ + if (!entity->in_groups_with_pending_reqs) { + entity->in_groups_with_pending_reqs = true; + bfqd->num_groups_with_pending_reqs++; + } +} + +static void bfq_update_groups_with_pending_reqs(struct bfq_entity *entity) +{ + struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity); + + if (bfqq) { + /* + * If the entity represents bfq_queue, and the queue belongs to + * root cgroup. + */ + if (!entity->parent) + bfq_set_group_with_pending_reqs(bfqq->bfqd, + &bfqq->bfqd->root_group->entity); + } else { + /* If the entity represents bfq_group. */ + struct bfq_group *bfqg = + container_of(entity, struct bfq_group, entity); + struct bfq_data *bfqd = bfqg->bfqd; + + bfq_set_group_with_pending_reqs(bfqd, entity); + } +} +#else +#define bfq_update_groups_with_pending_reqs(entity) \ + do {} while (0) +#endif /** * __bfq_activate_entity - handle activation of entity. @@ -999,19 +1035,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity, entity->on_st_or_in_serv = true; } -#ifdef CONFIG_BFQ_GROUP_IOSCHED - if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */ - struct bfq_group *bfqg = - container_of(entity, struct bfq_group, entity); - struct bfq_data *bfqd = bfqg->bfqd; - - if (!entity->in_groups_with_pending_reqs) { - entity->in_groups_with_pending_reqs = true; - bfqd->num_groups_with_pending_reqs++; - } - } -#endif - + bfq_update_groups_with_pending_reqs(entity); bfq_update_fin_time_enqueue(entity, st, backshifted); }
'num_groups_with_pending_reqs' represents how many groups that are not root group and have pending requests. This patch also counted root group into 'num_groups_with_pending_reqs'. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- block/bfq-iosched.c | 36 ++++++++++++++++++++++++++------ block/bfq-wf2q.c | 50 +++++++++++++++++++++++++++++++++------------ 2 files changed, 67 insertions(+), 19 deletions(-)