Date: Mon, 21 Mar 2011 15:27:57 -0400
From: Vivek Goyal <vgoyal@redhat.com>
To: Justin TerAvest <teravest@google.com>
Cc: jaxboe@fusionio.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] Don't update group weights when on service tree.
Message-ID: <20110321192757.GK2694@redhat.com>
References: <1300374357-30686-1-git-send-email-teravest@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1300374357-30686-1-git-send-email-teravest@google.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2152
Lines: 59

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
--
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/