| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20110214165050.GD13097@redhat.com>
Date: Mon, 14 Feb 2011 11:50:50 -0500
From: Vivek Goyal <vgoyal@...hat.com>
To: Justin TerAvest <teravest@...gle.com>
Cc: jaxboe@...ionio.com, ctalbott@...gle.com, mrubin@...gle.com,
jmoyer@...hat.com, guijianfeng@...fujitsu.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] Don't update group weights when on service tree.
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@...gle.com>
> ---
Thanks Justin. Looks good to me.
Acked-by: Vivek Goyal <vgoyal@...hat.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists