[PATCH v3] Don't update group weights when on service tree.

[PATCH v3] Don't update group weights when on service tree.

[Justin TerAvest]

Version 3 is updated to apply to for-2.6.39/core.

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>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/cfq-iosched.c |   53 +++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 89e0d1c..12e380b 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -178,6 +178,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;
@@ -853,7 +855,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;
@@ -874,13 +896,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;
 
@@ -892,9 +920,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);
 }
@@ -948,9 +974,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)) {
@@ -982,7 +1009,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 *
@@ -1255,7 +1284,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 *
@@ -1368,7 +1397,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--;
 	if (cfq_cfqq_sync(cfqq))
-- 
1.7.3.1

Re: [PATCH v3] Don't update group weights when on service tree.

[Jens Axboe]

On 2011-03-17 16:05, Justin TerAvest wrote:
> Version 3 is updated to apply to for-2.6.39/core.
> 
> 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.

Thanks, applied. In the future, please remember to prefix the subject
line with the area of interest. This one should be:

cfq-iosched: bla bla bla

so that people going through the log knows what that patch is touching.
I added it for you here.

-- 
Jens Axboe

Re: [PATCH v3] Don't update group weights when on service tree.

[Vivek Goyal]

On Thu, Mar 17, 2011 at 08:05:57AM -0700, Justin TerAvest wrote:
> Version 3 is updated to apply to for-2.6.39/core.
> 
> 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>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  block/cfq-iosched.c |   53 +++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 89e0d1c..12e380b 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -178,6 +178,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;
> @@ -853,7 +855,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;
> +	}
> +}

thinking more about it, looks like this code is still racy. If somebody
updates the weights again while we are about to process previous weight
change, we might lose the new weight and set needs_update=false. We might
have to use xchg() to update cfqg->needs_update.

Thanks
Vivek

Re: [PATCH v3] Don't update group weights when on service tree.

[Justin TerAvest]

On Mon, Mar 21, 2011 at 12:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Mar 17, 2011 at 08:05:57AM -0700, Justin TerAvest wrote:
>> Version 3 is updated to apply to for-2.6.39/core.
>>
>> 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>
>> Acked-by: Vivek Goyal <vgoyal@redhat.com>
>> ---
>>  block/cfq-iosched.c |   53 +++++++++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 41 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
>> index 89e0d1c..12e380b 100644
>> --- a/block/cfq-iosched.c
>> +++ b/block/cfq-iosched.c
>> @@ -178,6 +178,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;
>> @@ -853,7 +855,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;
>> +     }
>> +}
>
> thinking more about it, looks like this code is still racy. If somebody
> updates the weights again while we are about to process previous weight
> change, we might lose the new weight and set needs_update=false. We might
> have to use xchg() to update cfqg->needs_update.

I think you're right, Vivek.

I wish we could just take a lock on blkcg->lock when updating, we
should expect the weights to be updated that often, right? I'm not
sure if there's a feasible way to do that, though.

I'll explore both options, I'd just prefer to not add xchg() code if I
don't have to, as it requires a bit more thinking.

Thanks,
Justin


>
> Thanks
> Vivek
>

Re: [PATCH v3] Don't update group weights when on service tree.

[Vivek Goyal]

On Mon, Mar 21, 2011 at 01:15:33PM -0700, Justin TerAvest wrote:
> On Mon, Mar 21, 2011 at 12:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Mar 17, 2011 at 08:05:57AM -0700, Justin TerAvest wrote:
> >> Version 3 is updated to apply to for-2.6.39/core.
> >>
> >> 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>
> >> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> >> ---
> >>  block/cfq-iosched.c |   53 +++++++++++++++++++++++++++++++++++++++-----------
> >>  1 files changed, 41 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> >> index 89e0d1c..12e380b 100644
> >> --- a/block/cfq-iosched.c
> >> +++ b/block/cfq-iosched.c
> >> @@ -178,6 +178,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;
> >> @@ -853,7 +855,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;
> >> +     }
> >> +}
> >
> > thinking more about it, looks like this code is still racy. If somebody
> > updates the weights again while we are about to process previous weight
> > change, we might lose the new weight and set needs_update=false. We might
> > have to use xchg() to update cfqg->needs_update.
> 
> I think you're right, Vivek.
> 
> I wish we could just take a lock on blkcg->lock when updating, we
> should expect the weights to be updated that often, right? I'm not
> sure if there's a feasible way to do that, though.
> 

I think taking blkcg->lock while updating cfqg->needs_update in CFQ will
also solve the issue. We should already be holding blkcg->lock in cgroup
updation path.

Do put a comment explaining it well in case you end up taking this path.

Thanks
Vivek

> I'll explore both options, I'd just prefer to not add xchg() code if I
> don't have to, as it requires a bit more thinking.
> 
> Thanks,
> Justin
> 
> 
> >
> > Thanks
> > Vivek
> >