Messages in this thread | | | From | Justin TerAvest <> | Date | Wed, 16 Mar 2011 13:28:38 -0700 | Subject | Re: [PATCH v2 1/1] Don't update group weights when on service tree. |
| |
Jens,
Could you please take a look and apply if this looks good to you?
On Mon, Feb 14, 2011 at 8:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Fri, Feb 11, 2011 at 01:09:03PM -0800, Justin TerAvest wrote: >> For version 2, I took Vivek's advice and made sure we update the group >> weight from cfq_group_service_tree_add(). >> >> If a weight was updated while a group is on the service tree, the >> calculation for the total weight of the service tree can be adjusted >> improperly, which either leads to bad service tree weights, or >> potentially crashes (if total_weight becomes 0). >> >> This patch defers updates to the weight until a group is off the service >> tree. >> >> Signed-off-by: Justin TerAvest <teravest@google.com> >> --- > > Thanks Justin. Looks good to me. > > Acked-by: Vivek Goyal <vgoyal@redhat.com> > > Vivek > >> block/cfq-iosched.c | 57 ++++++++++++++++++++++++++++++++++++++------------ >> 1 files changed, 43 insertions(+), 14 deletions(-) >> >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c >> index 501ffdf..868f9cf 100644 >> --- a/block/cfq-iosched.c >> +++ b/block/cfq-iosched.c >> @@ -179,6 +179,8 @@ struct cfq_group { >> /* group service_tree key */ >> u64 vdisktime; >> unsigned int weight; >> + unsigned int new_weight; >> + bool needs_update; >> >> /* number of cfqq currently on this group */ >> int nr_cfqq; >> @@ -863,7 +865,27 @@ __cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg) >> } >> >> static void >> -cfq_group_service_tree_add(struct cfq_data *cfqd, struct cfq_group *cfqg) >> +cfq_update_group_weight(struct cfq_group *cfqg) >> +{ >> + BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node)); >> + if (cfqg->needs_update) { >> + cfqg->weight = cfqg->new_weight; >> + cfqg->needs_update = false; >> + } >> +} >> + >> +static void >> +cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg) >> +{ >> + BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node)); >> + >> + cfq_update_group_weight(cfqg); >> + __cfq_group_service_tree_add(st, cfqg); >> + st->total_weight += cfqg->weight; >> +} >> + >> +static void >> +cfq_group_notify_queue_add(struct cfq_data *cfqd, struct cfq_group *cfqg) >> { >> struct cfq_rb_root *st = &cfqd->grp_service_tree; >> struct cfq_group *__cfqg; >> @@ -884,13 +906,19 @@ cfq_group_service_tree_add(struct cfq_data *cfqd, struct cfq_group *cfqg) >> cfqg->vdisktime = __cfqg->vdisktime + CFQ_IDLE_DELAY; >> } else >> cfqg->vdisktime = st->min_vdisktime; >> + cfq_group_service_tree_add(st, cfqg); >> +} >> >> - __cfq_group_service_tree_add(st, cfqg); >> - st->total_weight += cfqg->weight; >> +static void >> +cfq_group_service_tree_del(struct cfq_rb_root *st, struct cfq_group *cfqg) >> +{ >> + st->total_weight -= cfqg->weight; >> + if (!RB_EMPTY_NODE(&cfqg->rb_node)) >> + cfq_rb_erase(&cfqg->rb_node, st); >> } >> >> static void >> -cfq_group_service_tree_del(struct cfq_data *cfqd, struct cfq_group *cfqg) >> +cfq_group_notify_queue_del(struct cfq_data *cfqd, struct cfq_group *cfqg) >> { >> struct cfq_rb_root *st = &cfqd->grp_service_tree; >> >> @@ -902,9 +930,7 @@ cfq_group_service_tree_del(struct cfq_data *cfqd, struct cfq_group *cfqg) >> return; >> >> cfq_log_cfqg(cfqd, cfqg, "del_from_rr group"); >> - st->total_weight -= cfqg->weight; >> - if (!RB_EMPTY_NODE(&cfqg->rb_node)) >> - cfq_rb_erase(&cfqg->rb_node, st); >> + cfq_group_service_tree_del(st, cfqg); >> cfqg->saved_workload_slice = 0; >> cfq_blkiocg_update_dequeue_stats(&cfqg->blkg, 1); >> } >> @@ -952,9 +978,10 @@ static void cfq_group_served(struct cfq_data *cfqd, struct cfq_group *cfqg, >> charge = cfqq->allocated_slice; >> >> /* Can't update vdisktime while group is on service tree */ >> - cfq_rb_erase(&cfqg->rb_node, st); >> + cfq_group_service_tree_del(st, cfqg); >> cfqg->vdisktime += cfq_scale_slice(charge, cfqg); >> - __cfq_group_service_tree_add(st, cfqg); >> + /* If a new weight was requested, update now, off tree */ >> + cfq_group_service_tree_add(st, cfqg); >> >> /* This group is being expired. Save the context */ >> if (time_after(cfqd->workload_expires, jiffies)) { >> @@ -985,7 +1012,9 @@ static inline struct cfq_group *cfqg_of_blkg(struct blkio_group *blkg) >> void cfq_update_blkio_group_weight(void *key, struct blkio_group *blkg, >> unsigned int weight) >> { >> - cfqg_of_blkg(blkg)->weight = weight; >> + struct cfq_group *cfqg = cfqg_of_blkg(blkg); >> + cfqg->new_weight = weight; >> + cfqg->needs_update = true; >> } >> >> static struct cfq_group * >> @@ -1194,7 +1223,7 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq, >> /* Move this cfq to root group */ >> cfq_log_cfqq(cfqd, cfqq, "moving to root group"); >> if (!RB_EMPTY_NODE(&cfqq->rb_node)) >> - cfq_group_service_tree_del(cfqd, cfqq->cfqg); >> + cfq_group_notify_queue_del(cfqd, cfqq->cfqg); >> cfqq->orig_cfqg = cfqq->cfqg; >> cfqq->cfqg = &cfqd->root_group; >> cfqd->root_group.ref++; >> @@ -1204,7 +1233,7 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq, >> /* cfqq is sequential now needs to go to its original group */ >> BUG_ON(cfqq->cfqg != &cfqd->root_group); >> if (!RB_EMPTY_NODE(&cfqq->rb_node)) >> - cfq_group_service_tree_del(cfqd, cfqq->cfqg); >> + cfq_group_notify_queue_del(cfqd, cfqq->cfqg); >> cfq_put_cfqg(cfqq->cfqg); >> cfqq->cfqg = cfqq->orig_cfqg; >> cfqq->orig_cfqg = NULL; >> @@ -1284,7 +1313,7 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq, >> service_tree->count++; >> if ((add_front || !new_cfqq) && !group_changed) >> return; >> - cfq_group_service_tree_add(cfqd, cfqq->cfqg); >> + cfq_group_notify_queue_add(cfqd, cfqq->cfqg); >> } >> >> static struct cfq_queue * >> @@ -1395,7 +1424,7 @@ static void cfq_del_cfqq_rr(struct cfq_data *cfqd, struct cfq_queue *cfqq) >> cfqq->p_root = NULL; >> } >> >> - cfq_group_service_tree_del(cfqd, cfqq->cfqg); >> + cfq_group_notify_queue_del(cfqd, cfqq->cfqg); >> BUG_ON(!cfqd->busy_queues); >> cfqd->busy_queues--; >> } >> -- >> 1.7.3.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |