2010-04-12 18:42:28

by Divyesh Shah

[permalink] [raw]
Subject: [PATCH][v2] Changes to more io-controller stats patches to address review comments.

Changelog from v1:
o Call blkiocg_update_idle_time_stats() at cfq_rq_enqueued() instead of at
dispatch time.

Changelog from original patchset: (in response to Vivek Goyal's comments)
o group blkiocg_update_blkio_group_dequeue_stats() with other DEBUG functions
o rename blkiocg_update_set_active_queue_stats() to
blkiocg_update_avg_queue_size_stats()
o s/request/io/ in blkiocg_update_request_add_stats() and
blkiocg_update_request_remove_stats()
o Call cfq_del_timer() at request dispatch() instead of
blkiocg_update_idle_time_stats()

Signed-off-by: Divyesh Shah<[email protected]>
---

block/blk-cgroup.c | 28 +++++++++++++---------------
block/blk-cgroup.h | 12 ++++++------
block/cfq-iosched.c | 20 +++++++++-----------
3 files changed, 28 insertions(+), 32 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 1ecff7a..fd428c1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -175,7 +175,7 @@ void blkiocg_update_idle_time_stats(struct blkio_group *blkg)
}
EXPORT_SYMBOL_GPL(blkiocg_update_idle_time_stats);

-void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg)
+void blkiocg_update_avg_queue_size_stats(struct blkio_group *blkg)
{
unsigned long flags;
struct blkio_group_stats *stats;
@@ -189,14 +189,21 @@ void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg)
blkio_update_group_wait_time(stats);
spin_unlock_irqrestore(&blkg->stats_lock, flags);
}
-EXPORT_SYMBOL_GPL(blkiocg_update_set_active_queue_stats);
+EXPORT_SYMBOL_GPL(blkiocg_update_avg_queue_size_stats);
+
+void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
+ unsigned long dequeue)
+{
+ blkg->stats.dequeue += dequeue;
+}
+EXPORT_SYMBOL_GPL(blkiocg_update_dequeue_stats);
#else
static inline void blkio_set_start_group_wait_time(struct blkio_group *blkg,
struct blkio_group *curr_blkg) {}
static inline void blkio_end_empty_time(struct blkio_group_stats *stats) {}
#endif

-void blkiocg_update_request_add_stats(struct blkio_group *blkg,
+void blkiocg_update_io_add_stats(struct blkio_group *blkg,
struct blkio_group *curr_blkg, bool direction,
bool sync)
{
@@ -209,9 +216,9 @@ void blkiocg_update_request_add_stats(struct blkio_group *blkg,
blkio_set_start_group_wait_time(blkg, curr_blkg);
spin_unlock_irqrestore(&blkg->stats_lock, flags);
}
-EXPORT_SYMBOL_GPL(blkiocg_update_request_add_stats);
+EXPORT_SYMBOL_GPL(blkiocg_update_io_add_stats);

-void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
+void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
bool direction, bool sync)
{
unsigned long flags;
@@ -221,7 +228,7 @@ void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
direction, sync);
spin_unlock_irqrestore(&blkg->stats_lock, flags);
}
-EXPORT_SYMBOL_GPL(blkiocg_update_request_remove_stats);
+EXPORT_SYMBOL_GPL(blkiocg_update_io_remove_stats);

void blkiocg_update_timeslice_used(struct blkio_group *blkg, unsigned long time)
{
@@ -602,15 +609,6 @@ SHOW_FUNCTION_PER_GROUP(empty_time, BLKIO_STAT_EMPTY_TIME, 0);
#endif
#undef SHOW_FUNCTION_PER_GROUP

-#ifdef CONFIG_DEBUG_BLK_CGROUP
-void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
- unsigned long dequeue)
-{
- blkg->stats.dequeue += dequeue;
-}
-EXPORT_SYMBOL_GPL(blkiocg_update_dequeue_stats);
-#endif
-
struct cftype blkio_files[] = {
{
.name = "weight",
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index bfce085..18e031a 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -159,7 +159,7 @@ static inline char *blkg_path(struct blkio_group *blkg)
{
return blkg->path;
}
-void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg);
+void blkiocg_update_avg_queue_size_stats(struct blkio_group *blkg);
void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
unsigned long dequeue);
void blkiocg_update_set_idle_time_stats(struct blkio_group *blkg);
@@ -188,7 +188,7 @@ BLKG_FLAG_FNS(empty)
#undef BLKG_FLAG_FNS
#else
static inline char *blkg_path(struct blkio_group *blkg) { return NULL; }
-static inline void blkiocg_update_set_active_queue_stats(
+static inline void blkiocg_update_avg_queue_size_stats(
struct blkio_group *blkg) {}
static inline void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
unsigned long dequeue) {}
@@ -216,9 +216,9 @@ void blkiocg_update_completion_stats(struct blkio_group *blkg,
uint64_t start_time, uint64_t io_start_time, bool direction, bool sync);
void blkiocg_update_io_merged_stats(struct blkio_group *blkg, bool direction,
bool sync);
-void blkiocg_update_request_add_stats(struct blkio_group *blkg,
+void blkiocg_update_io_add_stats(struct blkio_group *blkg,
struct blkio_group *curr_blkg, bool direction, bool sync);
-void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
+void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
bool direction, bool sync);
#else
struct cgroup;
@@ -243,9 +243,9 @@ static inline void blkiocg_update_completion_stats(struct blkio_group *blkg,
bool sync) {}
static inline void blkiocg_update_io_merged_stats(struct blkio_group *blkg,
bool direction, bool sync) {}
-static inline void blkiocg_update_request_add_stats(struct blkio_group *blkg,
+static inline void blkiocg_update_io_add_stats(struct blkio_group *blkg,
struct blkio_group *curr_blkg, bool direction, bool sync) {}
-static inline void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
+static inline void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
bool direction, bool sync) {}
#endif
#endif /* _BLK_CGROUP_H */
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index b6e095c..cda6b29 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1381,10 +1381,10 @@ static void cfq_reposition_rq_rb(struct cfq_queue *cfqq, struct request *rq)
{
elv_rb_del(&cfqq->sort_list, rq);
cfqq->queued[rq_is_sync(rq)]--;
- blkiocg_update_request_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
+ blkiocg_update_io_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
rq_is_sync(rq));
cfq_add_rq_rb(rq);
- blkiocg_update_request_add_stats(
+ blkiocg_update_io_add_stats(
&cfqq->cfqg->blkg, &cfqq->cfqd->serving_group->blkg,
rq_data_dir(rq), rq_is_sync(rq));
}
@@ -1442,7 +1442,7 @@ static void cfq_remove_request(struct request *rq)
cfq_del_rq_rb(rq);

cfqq->cfqd->rq_queued--;
- blkiocg_update_request_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
+ blkiocg_update_io_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
rq_is_sync(rq));
if (rq_is_meta(rq)) {
WARN_ON(!cfqq->meta_pending);
@@ -1541,7 +1541,7 @@ static void __cfq_set_active_queue(struct cfq_data *cfqd,
if (cfqq) {
cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
cfqd->serving_prio, cfqd->serving_type);
- blkiocg_update_set_active_queue_stats(&cfqq->cfqg->blkg);
+ blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
cfqq->slice_start = 0;
cfqq->dispatch_start = jiffies;
cfqq->allocated_slice = 0;
@@ -2395,11 +2395,6 @@ static int cfq_dispatch_requests(struct request_queue *q, int force)
}

cfq_log_cfqq(cfqd, cfqq, "dispatched a request");
- /*
- * This is needed since we don't exactly match the mod_timer() and
- * del_timer() calls in CFQ.
- */
- blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
return 1;
}

@@ -3208,8 +3203,11 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
cfq_del_timer(cfqd, cfqq);
cfq_clear_cfqq_wait_request(cfqq);
__blk_run_queue(cfqd->queue);
- } else
+ } else {
+ blkiocg_update_idle_time_stats(
+ &cfqq->cfqg->blkg);
cfq_mark_cfqq_must_dispatch(cfqq);
+ }
}
} else if (cfq_should_preempt(cfqd, cfqq, rq)) {
/*
@@ -3235,7 +3233,7 @@ static void cfq_insert_request(struct request_queue *q, struct request *rq)
list_add_tail(&rq->queuelist, &cfqq->fifo);
cfq_add_rq_rb(rq);

- blkiocg_update_request_add_stats(&cfqq->cfqg->blkg,
+ blkiocg_update_io_add_stats(&cfqq->cfqg->blkg,
&cfqd->serving_group->blkg, rq_data_dir(rq),
rq_is_sync(rq));
cfq_rq_enqueued(cfqd, cfqq, rq);


2010-04-12 22:12:08

by Vivek Goyal

[permalink] [raw]
Subject: Re: [PATCH][v2] Changes to more io-controller stats patches to address review comments.

On Mon, Apr 12, 2010 at 11:41:49AM -0700, Divyesh Shah wrote:
> Changelog from v1:
> o Call blkiocg_update_idle_time_stats() at cfq_rq_enqueued() instead of at
> dispatch time.
>
> Changelog from original patchset: (in response to Vivek Goyal's comments)
> o group blkiocg_update_blkio_group_dequeue_stats() with other DEBUG functions
> o rename blkiocg_update_set_active_queue_stats() to
> blkiocg_update_avg_queue_size_stats()
> o s/request/io/ in blkiocg_update_request_add_stats() and
> blkiocg_update_request_remove_stats()
> o Call cfq_del_timer() at request dispatch() instead of
> blkiocg_update_idle_time_stats()
>
> Signed-off-by: Divyesh Shah<[email protected]>
> ---

Looks good to me.

Acked-by: Vivek Goyal <[email protected]>

Thanks
Vivek

>
> block/blk-cgroup.c | 28 +++++++++++++---------------
> block/blk-cgroup.h | 12 ++++++------
> block/cfq-iosched.c | 20 +++++++++-----------
> 3 files changed, 28 insertions(+), 32 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 1ecff7a..fd428c1 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -175,7 +175,7 @@ void blkiocg_update_idle_time_stats(struct blkio_group *blkg)
> }
> EXPORT_SYMBOL_GPL(blkiocg_update_idle_time_stats);
>
> -void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg)
> +void blkiocg_update_avg_queue_size_stats(struct blkio_group *blkg)
> {
> unsigned long flags;
> struct blkio_group_stats *stats;
> @@ -189,14 +189,21 @@ void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg)
> blkio_update_group_wait_time(stats);
> spin_unlock_irqrestore(&blkg->stats_lock, flags);
> }
> -EXPORT_SYMBOL_GPL(blkiocg_update_set_active_queue_stats);
> +EXPORT_SYMBOL_GPL(blkiocg_update_avg_queue_size_stats);
> +
> +void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
> + unsigned long dequeue)
> +{
> + blkg->stats.dequeue += dequeue;
> +}
> +EXPORT_SYMBOL_GPL(blkiocg_update_dequeue_stats);
> #else
> static inline void blkio_set_start_group_wait_time(struct blkio_group *blkg,
> struct blkio_group *curr_blkg) {}
> static inline void blkio_end_empty_time(struct blkio_group_stats *stats) {}
> #endif
>
> -void blkiocg_update_request_add_stats(struct blkio_group *blkg,
> +void blkiocg_update_io_add_stats(struct blkio_group *blkg,
> struct blkio_group *curr_blkg, bool direction,
> bool sync)
> {
> @@ -209,9 +216,9 @@ void blkiocg_update_request_add_stats(struct blkio_group *blkg,
> blkio_set_start_group_wait_time(blkg, curr_blkg);
> spin_unlock_irqrestore(&blkg->stats_lock, flags);
> }
> -EXPORT_SYMBOL_GPL(blkiocg_update_request_add_stats);
> +EXPORT_SYMBOL_GPL(blkiocg_update_io_add_stats);
>
> -void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
> +void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
> bool direction, bool sync)
> {
> unsigned long flags;
> @@ -221,7 +228,7 @@ void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
> direction, sync);
> spin_unlock_irqrestore(&blkg->stats_lock, flags);
> }
> -EXPORT_SYMBOL_GPL(blkiocg_update_request_remove_stats);
> +EXPORT_SYMBOL_GPL(blkiocg_update_io_remove_stats);
>
> void blkiocg_update_timeslice_used(struct blkio_group *blkg, unsigned long time)
> {
> @@ -602,15 +609,6 @@ SHOW_FUNCTION_PER_GROUP(empty_time, BLKIO_STAT_EMPTY_TIME, 0);
> #endif
> #undef SHOW_FUNCTION_PER_GROUP
>
> -#ifdef CONFIG_DEBUG_BLK_CGROUP
> -void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
> - unsigned long dequeue)
> -{
> - blkg->stats.dequeue += dequeue;
> -}
> -EXPORT_SYMBOL_GPL(blkiocg_update_dequeue_stats);
> -#endif
> -
> struct cftype blkio_files[] = {
> {
> .name = "weight",
> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
> index bfce085..18e031a 100644
> --- a/block/blk-cgroup.h
> +++ b/block/blk-cgroup.h
> @@ -159,7 +159,7 @@ static inline char *blkg_path(struct blkio_group *blkg)
> {
> return blkg->path;
> }
> -void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg);
> +void blkiocg_update_avg_queue_size_stats(struct blkio_group *blkg);
> void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
> unsigned long dequeue);
> void blkiocg_update_set_idle_time_stats(struct blkio_group *blkg);
> @@ -188,7 +188,7 @@ BLKG_FLAG_FNS(empty)
> #undef BLKG_FLAG_FNS
> #else
> static inline char *blkg_path(struct blkio_group *blkg) { return NULL; }
> -static inline void blkiocg_update_set_active_queue_stats(
> +static inline void blkiocg_update_avg_queue_size_stats(
> struct blkio_group *blkg) {}
> static inline void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
> unsigned long dequeue) {}
> @@ -216,9 +216,9 @@ void blkiocg_update_completion_stats(struct blkio_group *blkg,
> uint64_t start_time, uint64_t io_start_time, bool direction, bool sync);
> void blkiocg_update_io_merged_stats(struct blkio_group *blkg, bool direction,
> bool sync);
> -void blkiocg_update_request_add_stats(struct blkio_group *blkg,
> +void blkiocg_update_io_add_stats(struct blkio_group *blkg,
> struct blkio_group *curr_blkg, bool direction, bool sync);
> -void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
> +void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
> bool direction, bool sync);
> #else
> struct cgroup;
> @@ -243,9 +243,9 @@ static inline void blkiocg_update_completion_stats(struct blkio_group *blkg,
> bool sync) {}
> static inline void blkiocg_update_io_merged_stats(struct blkio_group *blkg,
> bool direction, bool sync) {}
> -static inline void blkiocg_update_request_add_stats(struct blkio_group *blkg,
> +static inline void blkiocg_update_io_add_stats(struct blkio_group *blkg,
> struct blkio_group *curr_blkg, bool direction, bool sync) {}
> -static inline void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
> +static inline void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
> bool direction, bool sync) {}
> #endif
> #endif /* _BLK_CGROUP_H */
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index b6e095c..cda6b29 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -1381,10 +1381,10 @@ static void cfq_reposition_rq_rb(struct cfq_queue *cfqq, struct request *rq)
> {
> elv_rb_del(&cfqq->sort_list, rq);
> cfqq->queued[rq_is_sync(rq)]--;
> - blkiocg_update_request_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
> + blkiocg_update_io_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
> rq_is_sync(rq));
> cfq_add_rq_rb(rq);
> - blkiocg_update_request_add_stats(
> + blkiocg_update_io_add_stats(
> &cfqq->cfqg->blkg, &cfqq->cfqd->serving_group->blkg,
> rq_data_dir(rq), rq_is_sync(rq));
> }
> @@ -1442,7 +1442,7 @@ static void cfq_remove_request(struct request *rq)
> cfq_del_rq_rb(rq);
>
> cfqq->cfqd->rq_queued--;
> - blkiocg_update_request_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
> + blkiocg_update_io_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
> rq_is_sync(rq));
> if (rq_is_meta(rq)) {
> WARN_ON(!cfqq->meta_pending);
> @@ -1541,7 +1541,7 @@ static void __cfq_set_active_queue(struct cfq_data *cfqd,
> if (cfqq) {
> cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
> cfqd->serving_prio, cfqd->serving_type);
> - blkiocg_update_set_active_queue_stats(&cfqq->cfqg->blkg);
> + blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
> cfqq->slice_start = 0;
> cfqq->dispatch_start = jiffies;
> cfqq->allocated_slice = 0;
> @@ -2395,11 +2395,6 @@ static int cfq_dispatch_requests(struct request_queue *q, int force)
> }
>
> cfq_log_cfqq(cfqd, cfqq, "dispatched a request");
> - /*
> - * This is needed since we don't exactly match the mod_timer() and
> - * del_timer() calls in CFQ.
> - */
> - blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
> return 1;
> }
>
> @@ -3208,8 +3203,11 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> cfq_del_timer(cfqd, cfqq);
> cfq_clear_cfqq_wait_request(cfqq);
> __blk_run_queue(cfqd->queue);
> - } else
> + } else {
> + blkiocg_update_idle_time_stats(
> + &cfqq->cfqg->blkg);
> cfq_mark_cfqq_must_dispatch(cfqq);
> + }
> }
> } else if (cfq_should_preempt(cfqd, cfqq, rq)) {
> /*
> @@ -3235,7 +3233,7 @@ static void cfq_insert_request(struct request_queue *q, struct request *rq)
> list_add_tail(&rq->queuelist, &cfqq->fifo);
> cfq_add_rq_rb(rq);
>
> - blkiocg_update_request_add_stats(&cfqq->cfqg->blkg,
> + blkiocg_update_io_add_stats(&cfqq->cfqg->blkg,
> &cfqd->serving_group->blkg, rq_data_dir(rq),
> rq_is_sync(rq));
> cfq_rq_enqueued(cfqd, cfqq, rq);

2010-04-13 17:26:54

by Divyesh Shah

[permalink] [raw]
Subject: Re: [PATCH][v2] Changes to more io-controller stats patches to address review comments.

Hi Jens,
If this looks ok to you, can you please queue it up? Btw, after
Gui's patches there is 1 merge conflict in your for-2.6.35 branch when
applying this patch but it seems pretty straightforward to resolve. In
case you'd prefer a clean patch that takes care of the merge conflict
please let me know.

Thanks,
Divyesh

On Mon, Apr 12, 2010 at 3:11 PM, Vivek Goyal <[email protected]> wrote:
> On Mon, Apr 12, 2010 at 11:41:49AM -0700, Divyesh Shah wrote:
>> Changelog from v1:
>> o Call blkiocg_update_idle_time_stats() at cfq_rq_enqueued() instead of at
>> ? dispatch time.
>>
>> Changelog from original patchset: (in response to Vivek Goyal's comments)
>> o group blkiocg_update_blkio_group_dequeue_stats() with other DEBUG functions
>> o rename blkiocg_update_set_active_queue_stats() to
>> ? blkiocg_update_avg_queue_size_stats()
>> o s/request/io/ in blkiocg_update_request_add_stats() and
>> ? blkiocg_update_request_remove_stats()
>> o Call cfq_del_timer() at request dispatch() instead of
>> ? blkiocg_update_idle_time_stats()
>>
>> Signed-off-by: Divyesh Shah<[email protected]>
>> ---
>
> Looks good to me.
>
> Acked-by: Vivek Goyal <[email protected]>
>
> Thanks
> Vivek
>
>>
>> ?block/blk-cgroup.c ?| ? 28 +++++++++++++---------------
>> ?block/blk-cgroup.h ?| ? 12 ++++++------
>> ?block/cfq-iosched.c | ? 20 +++++++++-----------
>> ?3 files changed, 28 insertions(+), 32 deletions(-)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 1ecff7a..fd428c1 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -175,7 +175,7 @@ void blkiocg_update_idle_time_stats(struct blkio_group *blkg)
>> ?}
>> ?EXPORT_SYMBOL_GPL(blkiocg_update_idle_time_stats);
>>
>> -void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg)
>> +void blkiocg_update_avg_queue_size_stats(struct blkio_group *blkg)
>> ?{
>> ? ? ? unsigned long flags;
>> ? ? ? struct blkio_group_stats *stats;
>> @@ -189,14 +189,21 @@ void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg)
>> ? ? ? blkio_update_group_wait_time(stats);
>> ? ? ? spin_unlock_irqrestore(&blkg->stats_lock, flags);
>> ?}
>> -EXPORT_SYMBOL_GPL(blkiocg_update_set_active_queue_stats);
>> +EXPORT_SYMBOL_GPL(blkiocg_update_avg_queue_size_stats);
>> +
>> +void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
>> + ? ? ? ? ? ? ? ? ? ? unsigned long dequeue)
>> +{
>> + ? ? blkg->stats.dequeue += dequeue;
>> +}
>> +EXPORT_SYMBOL_GPL(blkiocg_update_dequeue_stats);
>> ?#else
>> ?static inline void blkio_set_start_group_wait_time(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct blkio_group *curr_blkg) {}
>> ?static inline void blkio_end_empty_time(struct blkio_group_stats *stats) {}
>> ?#endif
>>
>> -void blkiocg_update_request_add_stats(struct blkio_group *blkg,
>> +void blkiocg_update_io_add_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? struct blkio_group *curr_blkg, bool direction,
>> ? ? ? ? ? ? ? ? ? ? ? bool sync)
>> ?{
>> @@ -209,9 +216,9 @@ void blkiocg_update_request_add_stats(struct blkio_group *blkg,
>> ? ? ? blkio_set_start_group_wait_time(blkg, curr_blkg);
>> ? ? ? spin_unlock_irqrestore(&blkg->stats_lock, flags);
>> ?}
>> -EXPORT_SYMBOL_GPL(blkiocg_update_request_add_stats);
>> +EXPORT_SYMBOL_GPL(blkiocg_update_io_add_stats);
>>
>> -void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
>> +void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bool direction, bool sync)
>> ?{
>> ? ? ? unsigned long flags;
>> @@ -221,7 +228,7 @@ void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? direction, sync);
>> ? ? ? spin_unlock_irqrestore(&blkg->stats_lock, flags);
>> ?}
>> -EXPORT_SYMBOL_GPL(blkiocg_update_request_remove_stats);
>> +EXPORT_SYMBOL_GPL(blkiocg_update_io_remove_stats);
>>
>> ?void blkiocg_update_timeslice_used(struct blkio_group *blkg, unsigned long time)
>> ?{
>> @@ -602,15 +609,6 @@ SHOW_FUNCTION_PER_GROUP(empty_time, BLKIO_STAT_EMPTY_TIME, 0);
>> ?#endif
>> ?#undef SHOW_FUNCTION_PER_GROUP
>>
>> -#ifdef CONFIG_DEBUG_BLK_CGROUP
>> -void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
>> - ? ? ? ? ? ? ? ? ? ? unsigned long dequeue)
>> -{
>> - ? ? blkg->stats.dequeue += dequeue;
>> -}
>> -EXPORT_SYMBOL_GPL(blkiocg_update_dequeue_stats);
>> -#endif
>> -
>> ?struct cftype blkio_files[] = {
>> ? ? ? {
>> ? ? ? ? ? ? ? .name = "weight",
>> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
>> index bfce085..18e031a 100644
>> --- a/block/blk-cgroup.h
>> +++ b/block/blk-cgroup.h
>> @@ -159,7 +159,7 @@ static inline char *blkg_path(struct blkio_group *blkg)
>> ?{
>> ? ? ? return blkg->path;
>> ?}
>> -void blkiocg_update_set_active_queue_stats(struct blkio_group *blkg);
>> +void blkiocg_update_avg_queue_size_stats(struct blkio_group *blkg);
>> ?void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned long dequeue);
>> ?void blkiocg_update_set_idle_time_stats(struct blkio_group *blkg);
>> @@ -188,7 +188,7 @@ BLKG_FLAG_FNS(empty)
>> ?#undef BLKG_FLAG_FNS
>> ?#else
>> ?static inline char *blkg_path(struct blkio_group *blkg) { return NULL; }
>> -static inline void blkiocg_update_set_active_queue_stats(
>> +static inline void blkiocg_update_avg_queue_size_stats(
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct blkio_group *blkg) {}
>> ?static inline void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned long dequeue) {}
>> @@ -216,9 +216,9 @@ void blkiocg_update_completion_stats(struct blkio_group *blkg,
>> ? ? ? uint64_t start_time, uint64_t io_start_time, bool direction, bool sync);
>> ?void blkiocg_update_io_merged_stats(struct blkio_group *blkg, bool direction,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bool sync);
>> -void blkiocg_update_request_add_stats(struct blkio_group *blkg,
>> +void blkiocg_update_io_add_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? struct blkio_group *curr_blkg, bool direction, bool sync);
>> -void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
>> +void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bool direction, bool sync);
>> ?#else
>> ?struct cgroup;
>> @@ -243,9 +243,9 @@ static inline void blkiocg_update_completion_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? bool sync) {}
>> ?static inline void blkiocg_update_io_merged_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bool direction, bool sync) {}
>> -static inline void blkiocg_update_request_add_stats(struct blkio_group *blkg,
>> +static inline void blkiocg_update_io_add_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? struct blkio_group *curr_blkg, bool direction, bool sync) {}
>> -static inline void blkiocg_update_request_remove_stats(struct blkio_group *blkg,
>> +static inline void blkiocg_update_io_remove_stats(struct blkio_group *blkg,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bool direction, bool sync) {}
>> ?#endif
>> ?#endif /* _BLK_CGROUP_H */
>> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
>> index b6e095c..cda6b29 100644
>> --- a/block/cfq-iosched.c
>> +++ b/block/cfq-iosched.c
>> @@ -1381,10 +1381,10 @@ static void cfq_reposition_rq_rb(struct cfq_queue *cfqq, struct request *rq)
>> ?{
>> ? ? ? elv_rb_del(&cfqq->sort_list, rq);
>> ? ? ? cfqq->queued[rq_is_sync(rq)]--;
>> - ? ? blkiocg_update_request_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
>> + ? ? blkiocg_update_io_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? rq_is_sync(rq));
>> ? ? ? cfq_add_rq_rb(rq);
>> - ? ? blkiocg_update_request_add_stats(
>> + ? ? blkiocg_update_io_add_stats(
>> ? ? ? ? ? ? ? ? ? ? ? &cfqq->cfqg->blkg, &cfqq->cfqd->serving_group->blkg,
>> ? ? ? ? ? ? ? ? ? ? ? rq_data_dir(rq), rq_is_sync(rq));
>> ?}
>> @@ -1442,7 +1442,7 @@ static void cfq_remove_request(struct request *rq)
>> ? ? ? cfq_del_rq_rb(rq);
>>
>> ? ? ? cfqq->cfqd->rq_queued--;
>> - ? ? blkiocg_update_request_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
>> + ? ? blkiocg_update_io_remove_stats(&cfqq->cfqg->blkg, rq_data_dir(rq),
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? rq_is_sync(rq));
>> ? ? ? if (rq_is_meta(rq)) {
>> ? ? ? ? ? ? ? WARN_ON(!cfqq->meta_pending);
>> @@ -1541,7 +1541,7 @@ static void __cfq_set_active_queue(struct cfq_data *cfqd,
>> ? ? ? if (cfqq) {
>> ? ? ? ? ? ? ? cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cfqd->serving_prio, cfqd->serving_type);
>> - ? ? ? ? ? ? blkiocg_update_set_active_queue_stats(&cfqq->cfqg->blkg);
>> + ? ? ? ? ? ? blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
>> ? ? ? ? ? ? ? cfqq->slice_start = 0;
>> ? ? ? ? ? ? ? cfqq->dispatch_start = jiffies;
>> ? ? ? ? ? ? ? cfqq->allocated_slice = 0;
>> @@ -2395,11 +2395,6 @@ static int cfq_dispatch_requests(struct request_queue *q, int force)
>> ? ? ? }
>>
>> ? ? ? cfq_log_cfqq(cfqd, cfqq, "dispatched a request");
>> - ? ? /*
>> - ? ? ?* This is needed since we don't exactly match the mod_timer() and
>> - ? ? ?* del_timer() calls in CFQ.
>> - ? ? ?*/
>> - ? ? blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
>> ? ? ? return 1;
>> ?}
>>
>> @@ -3208,8 +3203,11 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cfq_del_timer(cfqd, cfqq);
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cfq_clear_cfqq_wait_request(cfqq);
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __blk_run_queue(cfqd->queue);
>> - ? ? ? ? ? ? ? ? ? ? } else
>> + ? ? ? ? ? ? ? ? ? ? } else {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? blkiocg_update_idle_time_stats(
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cfqq->cfqg->blkg);
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cfq_mark_cfqq_must_dispatch(cfqq);
>> + ? ? ? ? ? ? ? ? ? ? }
>> ? ? ? ? ? ? ? }
>> ? ? ? } else if (cfq_should_preempt(cfqd, cfqq, rq)) {
>> ? ? ? ? ? ? ? /*
>> @@ -3235,7 +3233,7 @@ static void cfq_insert_request(struct request_queue *q, struct request *rq)
>> ? ? ? list_add_tail(&rq->queuelist, &cfqq->fifo);
>> ? ? ? cfq_add_rq_rb(rq);
>>
>> - ? ? blkiocg_update_request_add_stats(&cfqq->cfqg->blkg,
>> + ? ? blkiocg_update_io_add_stats(&cfqq->cfqg->blkg,
>> ? ? ? ? ? ? ? ? ? ? ? &cfqd->serving_group->blkg, rq_data_dir(rq),
>> ? ? ? ? ? ? ? ? ? ? ? rq_is_sync(rq));
>> ? ? ? cfq_rq_enqueued(cfqd, cfqq, rq);
>

2010-04-13 18:15:32

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH][v2] Changes to more io-controller stats patches to address review comments.

On Tue, Apr 13 2010, Divyesh Shah wrote:
> Hi Jens,
> If this looks ok to you, can you please queue it up? Btw, after
> Gui's patches there is 1 merge conflict in your for-2.6.35 branch when
> applying this patch but it seems pretty straightforward to resolve. In
> case you'd prefer a clean patch that takes care of the merge conflict
> please let me know.

I hand applied that one hunk, and committed it.

But...

block/blk-cgroup.c:271: error: redefinition of ?blkiocg_set_start_empty_time?
block/blk-cgroup.h:208: note: previous definition of ?blkiocg_set_start_empty_time? was here
block/blk-cgroup.c: In function ?blkiocg_set_start_empty_time?:
block/blk-cgroup.c:291: error: implicit declaration of function ?blkio_blkg_empty?
block/blk-cgroup.c:292: error: ?struct blkio_group_stats? has no member named ?start_empty_time?
block/blk-cgroup.c:293: error: implicit declaration of function ?blkio_mark_blkg_empty?
make[1]: *** [block/blk-cgroup.o] Error 1
make: *** [block] Error 2

I merged mainline into for-2.6.35 to resolve a merge conflict, but that
was trivial.

--
Jens Axboe

2010-04-13 21:29:23

by Divyesh Shah

[permalink] [raw]
Subject: Re: [PATCH][v2] Changes to more io-controller stats patches to address review comments.

On Tue, Apr 13, 2010 at 11:15 AM, Jens Axboe <[email protected]> wrote:
> On Tue, Apr 13 2010, Divyesh Shah wrote:
>> Hi Jens,
>> ? ? ? ?If this looks ok to you, can you please queue it up? Btw, after
>> Gui's patches there is 1 merge conflict in your for-2.6.35 branch when
>> applying this patch but it seems pretty straightforward to resolve. In
>> case you'd prefer a clean patch that takes care of the merge conflict
>> please let me know.
>
> I hand applied that one hunk, and committed it.
>
> But...
>
> block/blk-cgroup.c:271: error: redefinition of ?blkiocg_set_start_empty_time?
> block/blk-cgroup.h:208: note: previous definition of ?blkiocg_set_start_empty_time? was here
> block/blk-cgroup.c: In function ?blkiocg_set_start_empty_time?:
> block/blk-cgroup.c:291: error: implicit declaration of function ?blkio_blkg_empty?
> block/blk-cgroup.c:292: error: ?struct blkio_group_stats? has no member named ?start_empty_time?
> block/blk-cgroup.c:293: error: implicit declaration of function ?blkio_mark_blkg_empty?
> make[1]: *** [block/blk-cgroup.o] Error 1
> make: *** [block] Error 2

Ahh.. I see this now with CONFIG_DEBUG_CFQ_IOSCHED not set.
blkiocg_set_empty_start_time() should be within the #ifdef
CONFIG_DEBUG_BLK_CGROUP block.

>
> I merged mainline into for-2.6.35 to resolve a merge conflict, but that
> was trivial.

block/cfq-iosched.c: In function 'cfq_forced_dispatch':
block/cfq-iosched.c:2234: error: too few arguments to function
'cfq_slice_expired'

Another compile error after fixing this was due to the merge from
mainline. cfq_slice_expired() now has an extra argument. Fixed that
too. Will send a patch with the 2 fixes in a bit. Sorry for the
breakage.

>
> --
> Jens Axboe
>
>