Messages in this thread | | | Date | Sat, 12 Feb 2011 10:21:47 +0800 | From | Gui Jianfeng <> | Subject | Re: [PATCH 5/6 v4] cfq-iosched: CFQ group hierarchical scheduling and use_hierarchy interface |
| |
Vivek Goyal wrote: > On Thu, Feb 10, 2011 at 03:47:45PM +0800, Gui Jianfeng wrote: >> CFQ group hierarchical scheduling and use_hierarchy interface. >> > > Hi Gui, > > I have done a quick high level review. Some minor comments inline. > > [..] >> struct cfq_data { >> struct request_queue *queue; >> - /* Root service tree for cfq_groups */ >> - struct cfq_rb_root grp_service_tree; >> struct cfq_group root_group; >> >> + /* cfq group schedule in flat or hierarchy manner. */ >> + bool use_hierarchy; >> + > > This seems to be redundant now? Nobody is using it?
Ahh, I think so.
> >> /* >> * The priority currently being served >> */ >> @@ -246,6 +251,9 @@ struct cfq_data { >> unsigned long workload_expires; >> struct cfq_group *serving_group; >> >> + /* Service tree for cfq group flat scheduling mode. */ >> + struct cfq_rb_root grp_service_tree; > > Above comment is misleading. This service tree is now used both for > flat as well as hierarhical mode.
Will modify.
> > [..] >> static void >> cfq_group_service_tree_add(struct cfq_data *cfqd, struct cfq_group *cfqg) >> { >> - struct cfq_rb_root *st = &cfqd->grp_service_tree; >> struct cfq_entity *cfqe = &cfqg->cfqe; >> - struct cfq_entity *__cfqe; >> struct rb_node *n; >> + struct cfq_entity *entity; >> + struct cfq_rb_root *st; >> + struct cfq_group *__cfqg; >> >> cfqg->nr_cfqq++; >> + >> if (!RB_EMPTY_NODE(&cfqe->rb_node)) >> return; >> >> /* >> - * Currently put the group at the end. Later implement something >> - * so that groups get lesser vtime based on their weights, so that >> - * if group does not loose all if it was not continously backlogged. >> + * Enqueue this group and its ancestors onto their service tree. >> */ >> - n = rb_last(&st->rb); >> - if (n) { >> - __cfqe = rb_entry_entity(n); >> - cfqe->vdisktime = __cfqe->vdisktime + CFQ_IDLE_DELAY; >> - } else >> - cfqe->vdisktime = st->min_vdisktime; >> + while (cfqe) { >> + if (!RB_EMPTY_NODE(&cfqe->rb_node)) >> + return; >> >> - cfq_entity_service_tree_add(st, cfqe); >> + /* >> + * Currently put the group at the end. Later implement >> + * something so that groups get lesser vtime based on >> + * their weights, so that if group does not loose all >> + * if it was not continously backlogged. >> + */ > > Can we use vdisktime boost logic for groups also? I think it can be a separate > patch in the series (the last one). Keeping it as a separate patch will > also help you to coordinate with chad's patch.
Make a separete patch make more sense, will do it as soon as this series gets merged.
> >> + st = cfqe->service_tree; > > Group entity set their service tree when they get allocated and retain > this pointer even when they get deleted from serivce tree. Queue entities > seem to have it NULL when they get deleted from service tree and it > gets set again when queue is getting inserted. It would be nice if we > can fix this discrepancy and keep it consistent. I think clearing up > cfqe->service_tree is a better idea and then calculate it again for > group also.
Ok, will consider to change.
> > [..] >> >> -static struct cfq_group * >> -cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create) >> +static void init_cfqe(struct blkio_cgroup *blkcg, >> + struct cfq_group *cfqg) > > As you are using this function for initializing group entity, possibly > rename it to init_group_entity() or init_group_cfqe() etc.
Sure.
> > [..] >> +static struct cfq_group * >> +cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create) >> +{ >> + struct blkio_cgroup *blkcg = cgroup_to_blkio_cgroup(cgroup); >> + struct cfq_group *cfqg = NULL; >> + void *key = cfqd; >> + struct backing_dev_info *bdi = &cfqd->queue->backing_dev_info; >> + unsigned int major, minor; >> + >> + cfqg = cfqg_of_blkg(blkiocg_lookup_group(blkcg, key)); >> + if (cfqg && !cfqg->blkg.dev && bdi->dev && dev_name(bdi->dev)) { >> + sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor); >> + cfqg->blkg.dev = MKDEV(major, minor); >> + goto done; >> + } > > Should we make this updation of this info hierarhical?
IMHO, it's fine to defer the updation when we really get the cfqg.
Will post an updated version.
Thanks, Gui
> > Thanks > Vivek >
| |