Re: [PATCH 10/19] io-conroller: Prepare elevator layer for single queue schedulers

Gui Jianfeng
2009-06-11T08:13:01Z

Vivek Goyal wrote:
...
>
> /*
> @@ -1296,6 +1302,13 @@ void io_group_chain_link(struct request_queue *q, void *key,
> iog = io_cgroup_lookup_group(iocg, key);
> io_group_set_parent(prev, iog);
> }
> +
> + if (unlikely(efqd->only_root_group))
> + /*
> + * TODO: Take care of force expiry of existing queue before
> + * new queue is queued.
> + */
> + efqd->only_root_group = 0;

Hi Vivek,

This flag isn't set back when all child groups go away. Am i missing something?
BTW, why not just determine "only root group" by cgroup itself? Although there might be
some impact if cgroup is built but no request goes into it. but i think this isn't a big
deal. How about the following patch?

Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
---
block/elevator-fq.c | 21 ++++++++++-----------
block/elevator-fq.h | 1 -
2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/block/elevator-fq.c b/block/elevator-fq.c
index a516dce..f33155c 100644
--- a/block/elevator-fq.c
+++ b/block/elevator-fq.c
@@ -76,7 +76,6 @@ void elv_del_ioq_busy(struct elevator_queue *e, struct io_queue *ioq,
void elv_activate_ioq(struct io_queue *ioq, int add_front);
void elv_deactivate_ioq(struct elv_fq_data *efqd, struct io_queue *ioq,
int requeue);
-
static int bfq_update_next_active(struct io_sched_data *sd)
{
struct io_group *iog;
@@ -1131,6 +1130,14 @@ struct io_cgroup io_root_cgroup = {
.ioprio_class = IO_DEFAULT_GRP_CLASS,
};

+static int is_only_root_group(void)
+{
+ if (list_empty(&io_root_cgroup.css.cgroup->children))
+ return 1;
+
+ return 0;
+}
+
void bfq_init_entity(struct io_entity *entity, struct io_group *iog)
{
entity->ioprio = entity->new_ioprio;
@@ -1543,13 +1550,6 @@ void io_group_chain_link(struct request_queue *q, void *key,
iog = io_cgroup_lookup_group(iocg, key);
io_group_set_parent(prev, iog);
}
-
- if (unlikely(efqd->only_root_group))
- /*
- * TODO: Take care of force expiry of existing queue before
- * new queue is queued.
- */
- efqd->only_root_group = 0;
}

/**
@@ -3601,7 +3601,7 @@ void *elv_fq_select_ioq(struct request_queue *q, int force)
* overhead.
*/

- if (efqd->only_root_group && elv_iosched_single_ioq(q->elevator)) {
+ if (is_only_root_group() && elv_iosched_single_ioq(q->elevator)) {
elv_log_ioq(efqd, ioq, "select: only root group, no expiry");
goto keep_queue;
}
@@ -3872,7 +3872,7 @@ void elv_ioq_completed_request(struct request_queue *q, struct request *rq)
* unnecessary overhead.
*/

- if (efqd->only_root_group &&
+ if (is_only_root_group() &&
elv_iosched_single_ioq(q->elevator)) {
elv_log_ioq(efqd, ioq, "select: only root group,"
" no expiry");
@@ -4019,7 +4019,6 @@ int elv_init_fq_data(struct request_queue *q, struct elevator_queue *e)

/* For the time being keep fairness enabled by default */
efqd->fairness = 1;
- efqd->only_root_group = 1;
return 0;
}

diff --git a/block/elevator-fq.h b/block/elevator-fq.h
index 1b1fb0b..0939735 100644
--- a/block/elevator-fq.h
+++ b/block/elevator-fq.h
@@ -368,7 +368,6 @@ struct elv_fq_data {
* queues.
*/
unsigned int fairness;
- int only_root_group;
};

extern int elv_slice_idle;
--
1.5.4.rc3