[PATCH 0/2] cfq: Fixes for unaccounted_time variable

[PATCH 0/2] cfq: Fixes for unaccounted_time variable

[Justin TerAvest]

This should fix both issues that Vivek pointed out:
  * We can't set active queue at preempt time, because select queue
    might select a different queue than the one we chose to preempt.
  * Move the new variable to only be exported when debugging options are
    on.

[PATCH 1/2] cfq-iosched: Don't set active queue in preempt

[Justin TerAvest]

Commit "Add unaccounted time to timeslice_used" changed the behavior of
cfq_preempt_queue to set cfqq active. Vivek pointed out that other
preemption rules might get involved, so we shouldn't manually set which
queue is active.

This cleans up the code to just clear the queue stats at preemption
time.

Signed-off-by: Justin TerAvest <teravest@google.com>
---
 block/cfq-iosched.c |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 12e380b..69208d7 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1620,27 +1620,33 @@ static inline void cfq_del_timer(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	cfq_blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
 }
 
+static void cfq_clear_queue_stats(struct cfq_data *cfqd,
+				  struct cfq_queue *cfqq)
+{
+	cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
+	cfqq->slice_start = 0;
+	cfqq->dispatch_start = jiffies;
+	cfqq->allocated_slice = 0;
+	cfqq->slice_end = 0;
+	cfqq->slice_dispatch = 0;
+	cfqq->nr_sectors = 0;
+
+	cfq_clear_cfqq_wait_request(cfqq);
+	cfq_clear_cfqq_must_dispatch(cfqq);
+	cfq_clear_cfqq_must_alloc_slice(cfqq);
+	cfq_clear_cfqq_fifo_expire(cfqq);
+	cfq_mark_cfqq_slice_new(cfqq);
+
+	cfq_del_timer(cfqd, cfqq);
+}
+
 static void __cfq_set_active_queue(struct cfq_data *cfqd,
 				   struct cfq_queue *cfqq)
 {
 	if (cfqq) {
 		cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
 				cfqd->serving_prio, cfqd->serving_type);
-		cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
-		cfqq->slice_start = 0;
-		cfqq->dispatch_start = jiffies;
-		cfqq->allocated_slice = 0;
-		cfqq->slice_end = 0;
-		cfqq->slice_dispatch = 0;
-		cfqq->nr_sectors = 0;
-
-		cfq_clear_cfqq_wait_request(cfqq);
-		cfq_clear_cfqq_must_dispatch(cfqq);
-		cfq_clear_cfqq_must_alloc_slice(cfqq);
-		cfq_clear_cfqq_fifo_expire(cfqq);
-		cfq_mark_cfqq_slice_new(cfqq);
-
-		cfq_del_timer(cfqd, cfqq);
+		cfq_clear_queue_stats(cfqd, cfqq);
 	}
 
 	cfqd->active_queue = cfqq;
@@ -3332,7 +3338,8 @@ static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	BUG_ON(!cfq_cfqq_on_rr(cfqq));
 
 	cfq_service_tree_add(cfqd, cfqq, 1);
-	__cfq_set_active_queue(cfqd, cfqq);
+
+	cfq_clear_queue_stats(cfqd, cfqq);
 }
 
 /*
-- 
1.7.3.1

Re: [PATCH 1/2] cfq-iosched: Don't set active queue in preempt

[Vivek Goyal]

On Tue, Mar 22, 2011 at 01:17:29PM -0700, Justin TerAvest wrote:
> Commit "Add unaccounted time to timeslice_used" changed the behavior of
> cfq_preempt_queue to set cfqq active. Vivek pointed out that other
> preemption rules might get involved, so we shouldn't manually set which
> queue is active.
> 
> This cleans up the code to just clear the queue stats at preemption
> time.
> 
> Signed-off-by: Justin TerAvest <teravest@google.com>
> ---
>  block/cfq-iosched.c |   39 +++++++++++++++++++++++----------------
>  1 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 12e380b..69208d7 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -1620,27 +1620,33 @@ static inline void cfq_del_timer(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>  	cfq_blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
>  }
>  
> +static void cfq_clear_queue_stats(struct cfq_data *cfqd,
> +				  struct cfq_queue *cfqq)
> +{
> +	cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
> +	cfqq->slice_start = 0;
> +	cfqq->dispatch_start = jiffies;
> +	cfqq->allocated_slice = 0;
> +	cfqq->slice_end = 0;
> +	cfqq->slice_dispatch = 0;
> +	cfqq->nr_sectors = 0;
> +
> +	cfq_clear_cfqq_wait_request(cfqq);
> +	cfq_clear_cfqq_must_dispatch(cfqq);
> +	cfq_clear_cfqq_must_alloc_slice(cfqq);
> +	cfq_clear_cfqq_fifo_expire(cfqq);
> +	cfq_mark_cfqq_slice_new(cfqq);
> +
> +	cfq_del_timer(cfqd, cfqq);
> +}
> +
>  static void __cfq_set_active_queue(struct cfq_data *cfqd,
>  				   struct cfq_queue *cfqq)
>  {
>  	if (cfqq) {
>  		cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
>  				cfqd->serving_prio, cfqd->serving_type);
> -		cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
> -		cfqq->slice_start = 0;
> -		cfqq->dispatch_start = jiffies;
> -		cfqq->allocated_slice = 0;
> -		cfqq->slice_end = 0;
> -		cfqq->slice_dispatch = 0;
> -		cfqq->nr_sectors = 0;
> -
> -		cfq_clear_cfqq_wait_request(cfqq);
> -		cfq_clear_cfqq_must_dispatch(cfqq);
> -		cfq_clear_cfqq_must_alloc_slice(cfqq);
> -		cfq_clear_cfqq_fifo_expire(cfqq);
> -		cfq_mark_cfqq_slice_new(cfqq);
> -
> -		cfq_del_timer(cfqd, cfqq);
> +		cfq_clear_queue_stats(cfqd, cfqq);
>  	}
>  
>  	cfqd->active_queue = cfqq;
> @@ -3332,7 +3338,8 @@ static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>  	BUG_ON(!cfq_cfqq_on_rr(cfqq));
>  
>  	cfq_service_tree_add(cfqd, cfqq, 1);
> -	__cfq_set_active_queue(cfqd, cfqq);
> +
> +	cfq_clear_queue_stats(cfqd, cfqq);

Hi Justin,

Why do we have to clear queue stats here for the preempting queue?

Especially look at cfqq->dispatch_start = jiffies. We have not started
the dispatch yet. When this queue is selected next, then we will start
the dispatch.

So this patch has introduced another bug now. Now after preemption if
we don't select this group, then we have a queue with wrong dispatch
start and that will result in huge slice_used for the queue and
it will not get its fair share.

Thanks
Vivek

Re: [PATCH 1/2] cfq-iosched: Don't set active queue in preempt

[Justin TerAvest]

On Tue, Mar 22, 2011 at 1:59 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Mar 22, 2011 at 01:17:29PM -0700, Justin TerAvest wrote:
>> Commit "Add unaccounted time to timeslice_used" changed the behavior of
>> cfq_preempt_queue to set cfqq active. Vivek pointed out that other
>> preemption rules might get involved, so we shouldn't manually set which
>> queue is active.
>>
>> This cleans up the code to just clear the queue stats at preemption
>> time.
>>
>> Signed-off-by: Justin TerAvest <teravest@google.com>
>> ---
>>  block/cfq-iosched.c |   39 +++++++++++++++++++++++----------------
>>  1 files changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
>> index 12e380b..69208d7 100644
>> --- a/block/cfq-iosched.c
>> +++ b/block/cfq-iosched.c
>> @@ -1620,27 +1620,33 @@ static inline void cfq_del_timer(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>>       cfq_blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
>>  }
>>
>> +static void cfq_clear_queue_stats(struct cfq_data *cfqd,
>> +                               struct cfq_queue *cfqq)
>> +{
>> +     cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
>> +     cfqq->slice_start = 0;
>> +     cfqq->dispatch_start = jiffies;
>> +     cfqq->allocated_slice = 0;
>> +     cfqq->slice_end = 0;
>> +     cfqq->slice_dispatch = 0;
>> +     cfqq->nr_sectors = 0;
>> +
>> +     cfq_clear_cfqq_wait_request(cfqq);
>> +     cfq_clear_cfqq_must_dispatch(cfqq);
>> +     cfq_clear_cfqq_must_alloc_slice(cfqq);
>> +     cfq_clear_cfqq_fifo_expire(cfqq);
>> +     cfq_mark_cfqq_slice_new(cfqq);
>> +
>> +     cfq_del_timer(cfqd, cfqq);
>> +}
>> +
>>  static void __cfq_set_active_queue(struct cfq_data *cfqd,
>>                                  struct cfq_queue *cfqq)
>>  {
>>       if (cfqq) {
>>               cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
>>                               cfqd->serving_prio, cfqd->serving_type);
>> -             cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
>> -             cfqq->slice_start = 0;
>> -             cfqq->dispatch_start = jiffies;
>> -             cfqq->allocated_slice = 0;
>> -             cfqq->slice_end = 0;
>> -             cfqq->slice_dispatch = 0;
>> -             cfqq->nr_sectors = 0;
>> -
>> -             cfq_clear_cfqq_wait_request(cfqq);
>> -             cfq_clear_cfqq_must_dispatch(cfqq);
>> -             cfq_clear_cfqq_must_alloc_slice(cfqq);
>> -             cfq_clear_cfqq_fifo_expire(cfqq);
>> -             cfq_mark_cfqq_slice_new(cfqq);
>> -
>> -             cfq_del_timer(cfqd, cfqq);
>> +             cfq_clear_queue_stats(cfqd, cfqq);
>>       }
>>
>>       cfqd->active_queue = cfqq;
>> @@ -3332,7 +3338,8 @@ static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>>       BUG_ON(!cfq_cfqq_on_rr(cfqq));
>>
>>       cfq_service_tree_add(cfqd, cfqq, 1);
>> -     __cfq_set_active_queue(cfqd, cfqq);
>> +
>> +     cfq_clear_queue_stats(cfqd, cfqq);
>
> Hi Justin,
>
> Why do we have to clear queue stats here for the preempting queue?
>
> Especially look at cfqq->dispatch_start = jiffies. We have not started
> the dispatch yet. When this queue is selected next, then we will start
> the dispatch.
>
> So this patch has introduced another bug now. Now after preemption if
> we don't select this group, then we have a queue with wrong dispatch
> start and that will result in huge slice_used for the queue and
> it will not get its fair share.

Hi Vivek,

Ugh, you're right. Sorry, I had some bad ideas in my head for how
preemption worked that clearly aren't true.
I think that if the stats aren't cleared here, everything should then
be fine because jiffies will then be picked up when the active queue
is set. Does that sound sane to you?

Thanks for explaining this problem.

Justin

>
> Thanks
> Vivek
>

Re: [PATCH 1/2] cfq-iosched: Don't set active queue in preempt

[Vivek Goyal]

On Tue, Mar 22, 2011 at 02:58:48PM -0700, Justin TerAvest wrote:
[..]
> >>       cfqd->active_queue = cfqq;
> >> @@ -3332,7 +3338,8 @@ static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
> >>       BUG_ON(!cfq_cfqq_on_rr(cfqq));
> >>
> >>       cfq_service_tree_add(cfqd, cfqq, 1);
> >> -     __cfq_set_active_queue(cfqd, cfqq);
> >> +
> >> +     cfq_clear_queue_stats(cfqd, cfqq);
> >
> > Hi Justin,
> >
> > Why do we have to clear queue stats here for the preempting queue?
> >
> > Especially look at cfqq->dispatch_start = jiffies. We have not started
> > the dispatch yet. When this queue is selected next, then we will start
> > the dispatch.
> >
> > So this patch has introduced another bug now. Now after preemption if
> > we don't select this group, then we have a queue with wrong dispatch
> > start and that will result in huge slice_used for the queue and
> > it will not get its fair share.
> 
> Hi Vivek,
> 
> Ugh, you're right. Sorry, I had some bad ideas in my head for how
> preemption worked that clearly aren't true.
> I think that if the stats aren't cleared here, everything should then
> be fine because jiffies will then be picked up when the active queue
> is set. Does that sound sane to you?
> 

Yes, that makes sense. Primarily you are looking for unaccounted seek time
(slice_start - dispatch_start) and dispatch_start will be set when the
preempting queue is selected for dispatch. So I don't think you have to
clear up anything in cfq_preempt_queue().

Thanks
Vivek

[PATCH 2/2] blk-cgroup: Only give unaccounted_time under debug

[Justin TerAvest]

This change moves unaccounted_time to only be reported when
CONFIG_DEBUG_BLK_CGROUP is true.

Signed-off-by: Justin TerAvest <teravest@google.com>
---
 block/blk-cgroup.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 77ee3c1..2bef570 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -605,10 +605,10 @@ static uint64_t blkio_get_stat(struct blkio_group *blkg,
 	if (type == BLKIO_STAT_SECTORS)
 		return blkio_fill_stat(key_str, MAX_KEY_LEN - 1,
 					blkg->stats.sectors, cb, dev);
+#ifdef CONFIG_DEBUG_BLK_CGROUP
 	if (type == BLKIO_STAT_UNACCOUNTED_TIME)
 		return blkio_fill_stat(key_str, MAX_KEY_LEN - 1,
 					blkg->stats.unaccounted_time, cb, dev);
-#ifdef CONFIG_DEBUG_BLK_CGROUP
 	if (type == BLKIO_STAT_AVG_QUEUE_SIZE) {
 		uint64_t sum = blkg->stats.avg_queue_size_sum;
 		uint64_t samples = blkg->stats.avg_queue_size_samples;
@@ -1111,9 +1111,6 @@ static int blkiocg_file_read_map(struct cgroup *cgrp, struct cftype *cft,
 		case BLKIO_PROP_sectors:
 			return blkio_read_blkg_stats(blkcg, cft, cb,
 						BLKIO_STAT_SECTORS, 0);
-		case BLKIO_PROP_unaccounted_time:
-			return blkio_read_blkg_stats(blkcg, cft, cb,
-						BLKIO_STAT_UNACCOUNTED_TIME, 0);
 		case BLKIO_PROP_io_service_bytes:
 			return blkio_read_blkg_stats(blkcg, cft, cb,
 						BLKIO_STAT_SERVICE_BYTES, 1);
@@ -1133,6 +1130,9 @@ static int blkiocg_file_read_map(struct cgroup *cgrp, struct cftype *cft,
 			return blkio_read_blkg_stats(blkcg, cft, cb,
 						BLKIO_STAT_QUEUED, 1);
 #ifdef CONFIG_DEBUG_BLK_CGROUP
+		case BLKIO_PROP_unaccounted_time:
+			return blkio_read_blkg_stats(blkcg, cft, cb,
+						BLKIO_STAT_UNACCOUNTED_TIME, 0);
 		case BLKIO_PROP_dequeue:
 			return blkio_read_blkg_stats(blkcg, cft, cb,
 						BLKIO_STAT_DEQUEUE, 0);
@@ -1270,12 +1270,6 @@ struct cftype blkio_files[] = {
 		.read_map = blkiocg_file_read_map,
 	},
 	{
-		.name = "unaccounted_time",
-		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
-				BLKIO_PROP_unaccounted_time),
-		.read_map = blkiocg_file_read_map,
-	},
-	{
 		.name = "io_service_bytes",
 		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
 				BLKIO_PROP_io_service_bytes),
@@ -1396,6 +1390,12 @@ struct cftype blkio_files[] = {
 				BLKIO_PROP_dequeue),
 		.read_map = blkiocg_file_read_map,
 	},
+	{
+		.name = "unaccounted_time",
+		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
+				BLKIO_PROP_unaccounted_time),
+		.read_map = blkiocg_file_read_map,
+	},
 #endif
 };
 
-- 
1.7.3.1

Re: [PATCH 0/2] cfq: Fixes for unaccounted_time variable

[Jens Axboe]

On 2011-03-22 21:17, Justin TerAvest wrote:
> This should fix both issues that Vivek pointed out:
>   * We can't set active queue at preempt time, because select queue
>     might select a different queue than the one we chose to preempt.
>   * Move the new variable to only be exported when debugging options are
>     on.

Thanks, applied.

-- 
Jens Axboe