| Message ID | 20220305091205.4188398-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 05-03-22 17:11:56, Yu Kuai wrote: > 'entity->sched_data' is set to parent group's sched_data, thus it's NULL > for root group. And for_each_entity() is used widely to access > 'entity->sched_data', thus aplly news apis if root group is not ^^ apply > expected. Prepare to count root group into 'num_groups_with_pending_reqs'. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/bfq-iosched.c | 2 +- > block/bfq-iosched.h | 22 ++++++++-------------- > block/bfq-wf2q.c | 10 +++++----- > 3 files changed, 14 insertions(+), 20 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 69ddf6b0f01d..3bc7a7686aad 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -4393,7 +4393,7 @@ void bfq_bfqq_expire(struct bfq_data *bfqd, > * service with the same budget. > */ > entity = entity->parent; > - for_each_entity(entity) > + for_each_entity_not_root(entity) > entity->service = 0; > } So why is it a problem to clear the service for root cgroup here? > diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c > index f8eb340381cf..c4cb935a615a 100644 > --- a/block/bfq-wf2q.c > +++ b/block/bfq-wf2q.c > @@ -815,7 +815,7 @@ void bfq_bfqq_served(struct bfq_queue *bfqq, int served) > bfqq->service_from_wr += served; > > bfqq->service_from_backlogged += served; > - for_each_entity(entity) { > + for_each_entity_not_root(entity) { > st = bfq_entity_service_tree(entity); Hum, right so how come this was not crashing? Because entity->sched_data is indeed NULL for bfqd->root_group->entity and so bfq_entity_service_tree() returned some bogus pointer? Similarly for the cases you are changing below? Honza > > entity->service += served; > @@ -1201,7 +1201,7 @@ static void bfq_deactivate_entity(struct bfq_entity *entity, > struct bfq_sched_data *sd; > struct bfq_entity *parent = NULL; > > - for_each_entity_safe(entity, parent) { > + for_each_entity_not_root_safe(entity, parent) { > sd = entity->sched_data; > > if (!__bfq_deactivate_entity(entity, ins_into_idle_tree)) { > @@ -1270,7 +1270,7 @@ static void bfq_deactivate_entity(struct bfq_entity *entity, > * is not the case. > */ > entity = parent; > - for_each_entity(entity) { > + for_each_entity_not_root(entity) { > /* > * Invoke __bfq_requeue_entity on entity, even if > * already active, to requeue/reposition it in the > @@ -1570,7 +1570,7 @@ struct bfq_queue *bfq_get_next_queue(struct bfq_data *bfqd) > * We can finally update all next-to-serve entities along the > * path from the leaf entity just set in service to the root. > */ > - for_each_entity(entity) { > + for_each_entity_not_root(entity) { > struct bfq_sched_data *sd = entity->sched_data; > > if (!bfq_update_next_in_service(sd, NULL, false)) > @@ -1597,7 +1597,7 @@ bool __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) > * execute the final step: reset in_service_entity along the > * path from entity to the root. > */ > - for_each_entity(entity) > + for_each_entity_not_root(entity) > entity->sched_data->in_service_entity = NULL; > > /* > -- > 2.31.1 >
On Wed 13-04-22 11:50:44, Jan Kara wrote: > On Sat 05-03-22 17:11:56, Yu Kuai wrote: > > 'entity->sched_data' is set to parent group's sched_data, thus it's NULL > > for root group. And for_each_entity() is used widely to access > > 'entity->sched_data', thus aplly news apis if root group is not > ^^ apply > > > expected. Prepare to count root group into 'num_groups_with_pending_reqs'. > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > --- > > block/bfq-iosched.c | 2 +- > > block/bfq-iosched.h | 22 ++++++++-------------- > > block/bfq-wf2q.c | 10 +++++----- > > 3 files changed, 14 insertions(+), 20 deletions(-) > > > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > > index 69ddf6b0f01d..3bc7a7686aad 100644 > > --- a/block/bfq-iosched.c > > +++ b/block/bfq-iosched.c > > @@ -4393,7 +4393,7 @@ void bfq_bfqq_expire(struct bfq_data *bfqd, > > * service with the same budget. > > */ > > entity = entity->parent; > > - for_each_entity(entity) > > + for_each_entity_not_root(entity) > > entity->service = 0; > > } > > So why is it a problem to clear the service for root cgroup here? > > > diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c > > index f8eb340381cf..c4cb935a615a 100644 > > --- a/block/bfq-wf2q.c > > +++ b/block/bfq-wf2q.c > > @@ -815,7 +815,7 @@ void bfq_bfqq_served(struct bfq_queue *bfqq, int served) > > bfqq->service_from_wr += served; > > > > bfqq->service_from_backlogged += served; > > - for_each_entity(entity) { > > + for_each_entity_not_root(entity) { > > st = bfq_entity_service_tree(entity); > > Hum, right so how come this was not crashing? Because entity->sched_data is > indeed NULL for bfqd->root_group->entity and so bfq_entity_service_tree() > returned some bogus pointer? Similarly for the cases you are changing > below? Oh, I see now. Because for_each_entity() currently does not iterate through root cgroup because it has root_group->my_entity set to NULL and thus as a result immediate children of root_group will have their parent set to NULL as well. Honza
在 2022/04/13 18:59, Jan Kara 写道: > On Wed 13-04-22 11:50:44, Jan Kara wrote: >> On Sat 05-03-22 17:11:56, Yu Kuai wrote: >>> 'entity->sched_data' is set to parent group's sched_data, thus it's NULL >>> for root group. And for_each_entity() is used widely to access >>> 'entity->sched_data', thus aplly news apis if root group is not >> ^^ apply >> Hi, Thanks for spotting this. >>> expected. Prepare to count root group into 'num_groups_with_pending_reqs'. >>> >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>> --- >>> block/bfq-iosched.c | 2 +- >>> block/bfq-iosched.h | 22 ++++++++-------------- >>> block/bfq-wf2q.c | 10 +++++----- >>> 3 files changed, 14 insertions(+), 20 deletions(-) >>> >>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c >>> index 69ddf6b0f01d..3bc7a7686aad 100644 >>> --- a/block/bfq-iosched.c >>> +++ b/block/bfq-iosched.c >>> @@ -4393,7 +4393,7 @@ void bfq_bfqq_expire(struct bfq_data *bfqd, >>> * service with the same budget. >>> */ >>> entity = entity->parent; >>> - for_each_entity(entity) >>> + for_each_entity_not_root(entity) >>> entity->service = 0; >>> } >> >> So why is it a problem to clear the service for root cgroup here? This is not a problem in theory, however 'entity->service' should always be 0 for root_group. Thus I think there is no need to do this. >> >>> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c >>> index f8eb340381cf..c4cb935a615a 100644 >>> --- a/block/bfq-wf2q.c >>> +++ b/block/bfq-wf2q.c >>> @@ -815,7 +815,7 @@ void bfq_bfqq_served(struct bfq_queue *bfqq, int served) >>> bfqq->service_from_wr += served; >>> >>> bfqq->service_from_backlogged += served; >>> - for_each_entity(entity) { >>> + for_each_entity_not_root(entity) { >>> st = bfq_entity_service_tree(entity); >> >> Hum, right so how come this was not crashing? Because entity->sched_data is >> indeed NULL for bfqd->root_group->entity and so bfq_entity_service_tree() >> returned some bogus pointer? Similarly for the cases you are changing >> below? > > Oh, I see now. Because for_each_entity() currently does not iterate through > root cgroup because it has root_group->my_entity set to NULL and thus as a > result immediate children of root_group will have their parent set to NULL > as well. Yes, currently for_each_entity() and for_each_entity_not_root() are the same, they will stop before root_group. With patch 5, for_each_entity_not_root() will stay the same, while for_each_entity() will access root_group's entity in addition. And because bfq_entity_service_tree() will access 'entity->sched_data', thus I change to the new api here to avoid null-ptr-deref after patch 5. Same reasons for below changes. Thanks, Kuai
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 69ddf6b0f01d..3bc7a7686aad 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -4393,7 +4393,7 @@ void bfq_bfqq_expire(struct bfq_data *bfqd, * service with the same budget. */ entity = entity->parent; - for_each_entity(entity) + for_each_entity_not_root(entity) entity->service = 0; } diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h index d703492714e2..ddd8eff5c272 100644 --- a/block/bfq-iosched.h +++ b/block/bfq-iosched.h @@ -1024,25 +1024,22 @@ extern struct blkcg_policy blkcg_policy_bfq; /* - interface of the internal hierarchical B-WF2Q+ scheduler - */ #ifdef CONFIG_BFQ_GROUP_IOSCHED -/* both next loops stop at one of the child entities of the root group */ +/* stop at one of the child entities of the root group */ #define for_each_entity(entity) \ for (; entity ; entity = entity->parent) -/* - * For each iteration, compute parent in advance, so as to be safe if - * entity is deallocated during the iteration. Such a deallocation may - * happen as a consequence of a bfq_put_queue that frees the bfq_queue - * containing entity. - */ -#define for_each_entity_safe(entity, parent) \ - for (; entity && ({ parent = entity->parent; 1; }); entity = parent) - #define is_root_entity(entity) \ (entity->sched_data == NULL) #define for_each_entity_not_root(entity) \ for (; entity && !is_root_entity(entity); entity = entity->parent) +/* + * For each iteration, compute parent in advance, so as to be safe if + * entity is deallocated during the iteration. Such a deallocation may + * happen as a consequence of a bfq_put_queue that frees the bfq_queue + * containing entity. + */ #define for_each_entity_not_root_safe(entity, parent) \ for (; entity && !is_root_entity(entity) && \ ({ parent = entity->parent; 1; }); entity = parent) @@ -1050,16 +1047,13 @@ extern struct blkcg_policy blkcg_policy_bfq; #define is_root_entity(entity) (false) /* - * Next four macros are fake loops when cgroups support is not + * Next three macros are fake loops when cgroups support is not * enabled. I fact, in such a case, there is only one level to go up * (to reach the root group). */ #define for_each_entity(entity) \ for (; entity ; entity = NULL) -#define for_each_entity_safe(entity, parent) \ - for (parent = NULL; entity ; entity = parent) - #define for_each_entity_not_root(entity) \ for (; entity ; entity = NULL) diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c index f8eb340381cf..c4cb935a615a 100644 --- a/block/bfq-wf2q.c +++ b/block/bfq-wf2q.c @@ -815,7 +815,7 @@ void bfq_bfqq_served(struct bfq_queue *bfqq, int served) bfqq->service_from_wr += served; bfqq->service_from_backlogged += served; - for_each_entity(entity) { + for_each_entity_not_root(entity) { st = bfq_entity_service_tree(entity); entity->service += served; @@ -1201,7 +1201,7 @@ static void bfq_deactivate_entity(struct bfq_entity *entity, struct bfq_sched_data *sd; struct bfq_entity *parent = NULL; - for_each_entity_safe(entity, parent) { + for_each_entity_not_root_safe(entity, parent) { sd = entity->sched_data; if (!__bfq_deactivate_entity(entity, ins_into_idle_tree)) { @@ -1270,7 +1270,7 @@ static void bfq_deactivate_entity(struct bfq_entity *entity, * is not the case. */ entity = parent; - for_each_entity(entity) { + for_each_entity_not_root(entity) { /* * Invoke __bfq_requeue_entity on entity, even if * already active, to requeue/reposition it in the @@ -1570,7 +1570,7 @@ struct bfq_queue *bfq_get_next_queue(struct bfq_data *bfqd) * We can finally update all next-to-serve entities along the * path from the leaf entity just set in service to the root. */ - for_each_entity(entity) { + for_each_entity_not_root(entity) { struct bfq_sched_data *sd = entity->sched_data; if (!bfq_update_next_in_service(sd, NULL, false)) @@ -1597,7 +1597,7 @@ bool __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) * execute the final step: reset in_service_entity along the * path from entity to the root. */ - for_each_entity(entity) + for_each_entity_not_root(entity) entity->sched_data->in_service_entity = NULL; /*
'entity->sched_data' is set to parent group's sched_data, thus it's NULL for root group. And for_each_entity() is used widely to access 'entity->sched_data', thus aplly news apis if root group is not expected. Prepare to count root group into 'num_groups_with_pending_reqs'. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- block/bfq-iosched.c | 2 +- block/bfq-iosched.h | 22 ++++++++-------------- block/bfq-wf2q.c | 10 +++++----- 3 files changed, 14 insertions(+), 20 deletions(-)