lkml.org 
[lkml]   [2011]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>


\
 
 \ /
  Last update: 2011-02-12 03:25    [W:0.094 / U:20.492 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site