block: export latency info for cgroups

PATCH In review Shaohua Li 4 participants 10 messages Oct 7, 2017 – Oct 11, 2017

Subsystems (2)

Block layer cgroup blkio

lore

Share & tools

lore

Shaohua Li

author

2017-10-07 00:55 UTC #0 lore raw

v2 cover block: export latency info for cgroups

From: Shaohua Li <[email protected]>

Hi,

latency info is a good sign to determine if IO is healthy. The patches export such info to cgroup io.stat.

I sent the first patch separately before, but since the latter depends on it, I include it here.

Thanks,
Shaohua

V1->V2: improve the scalability

Shaohua Li (3):
blk-stat: delete useless code
block: set request_list for request
blockcg: export latency info for each cgroup

 block/blk-cgroup.c         |  29 +++++++-
 block/blk-core.c           |   2 +-
 block/blk-mq.c             |   5 ++
 block/blk-stat.c           | 178 +++++++++++++++++++++++++++++++++++----------
 block/blk.h                |   5 ++
 include/linux/blk-cgroup.h |   9 +++
 include/linux/blk_types.h  |   5 +-
 7 files changed, 189 insertions(+), 44 deletions(-)

Signature · 1 line

2.9.5

Shaohua Li

author

2017-10-07 00:55 UTC #1 lore raw

PATCH v2 1/3 blk-stat: delete useless code

From: Shaohua Li <[email protected]>

Fix two issues:
- the per-cpu stat flush is unnecessary, nobody uses per-cpu stat except sum it to global stat. We can do the calculation there. The flush just wastes cpu time.
- some fields are signed int/s64. I don't see the point.

Cc Omar Sandoval <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 block/blk-stat.c          | 45 +++++++--------------------------------------
 include/linux/blk_types.h |  5 ++---
 2 files changed, 9 insertions(+), 41 deletions(-)

block/blk-stat.c +7 ~~-38~~

index c52356d..3a2f3c9 100644
@@ -11,8 +11,6 @@
 #include "blk-mq.h"
 #include "blk.h"
 
-#define BLK_RQ_STAT_BATCH	64
-
 struct blk_queue_stats {
 	struct list_head callbacks;
 	spinlock_t lock;

@@ -23,45 +21,21 @@ static void blk_stat_init(struct blk_rq_stat *stat)
 {
 	stat->min = -1ULL;
 	stat->max = stat->nr_samples = stat->mean = 0;
-	stat->batch = stat->nr_batch = 0;
-}
-
-static void blk_stat_flush_batch(struct blk_rq_stat *stat)
-{
-	const s32 nr_batch = READ_ONCE(stat->nr_batch);
-	const s32 nr_samples = READ_ONCE(stat->nr_samples);
-
-	if (!nr_batch)
-		return;
-	if (!nr_samples)
-		stat->mean = div64_s64(stat->batch, nr_batch);
-	else {
-		stat->mean = div64_s64((stat->mean * nr_samples) +
-					stat->batch,
-					nr_batch + nr_samples);
-	}
-
-	stat->nr_samples += nr_batch;
-	stat->nr_batch = stat->batch = 0;
+	stat->batch = 0;
 }
 
+/* src is a per-cpu stat, mean isn't initialized */
 static void blk_stat_sum(struct blk_rq_stat *dst, struct blk_rq_stat *src)
 {
-	blk_stat_flush_batch(src);
-
 	if (!src->nr_samples)
 		return;
 
 	dst->min = min(dst->min, src->min);
 	dst->max = max(dst->max, src->max);
 
-	if (!dst->nr_samples)
-		dst->mean = src->mean;
-	else {
-		dst->mean = div64_s64((src->mean * src->nr_samples) +
-					(dst->mean * dst->nr_samples),
-					dst->nr_samples + src->nr_samples);
-	}
+	dst->mean = div_u64(src->batch + dst->mean * dst->nr_samples,
+				dst->nr_samples + src->nr_samples);
+
 	dst->nr_samples += src->nr_samples;
 }

@@ -69,13 +43,8 @@ static void __blk_stat_add(struct blk_rq_stat *stat, u64 value)
 {
 	stat->min = min(stat->min, value);
 	stat->max = max(stat->max, value);
-
-	if (stat->batch + value < stat->batch ||
-	    stat->nr_batch + 1 == BLK_RQ_STAT_BATCH)
-		blk_stat_flush_batch(stat);
-
 	stat->batch += value;
-	stat->nr_batch++;
+	stat->nr_samples++;
 }
 
 void blk_stat_add(struct request *rq)

@@ -84,7 +53,7 @@ void blk_stat_add(struct request *rq)
 	struct blk_stat_callback *cb;
 	struct blk_rq_stat *stat;
 	int bucket;
-	s64 now, value;
+	u64 now, value;
 
 	now = __blk_stat_time(ktime_to_ns(ktime_get()));
 	if (now < blk_stat_time(&rq->issue_stat))

include/linux/blk_types.h +2 -3

index a2d2aa7..3385c89 100644
@@ -329,11 +329,10 @@ static inline bool blk_qc_t_is_internal(blk_qc_t cookie)
 }
 
 struct blk_rq_stat {
-	s64 mean;
+	u64 mean;
 	u64 min;
 	u64 max;
-	s32 nr_samples;
-	s32 nr_batch;
+	u32 nr_samples;
 	u64 batch;
 };

Signature · 1 line

2.9.5

Omar Sandoval

Reviewed-by on v2 1/3

2017-10-10 17:57 UTC #2 lore raw

Re: [PATCH V2 1/3] blk-stat: delete useless code

Quoted 6 lines · Shaohua Li

From: Shaohua Li <[email protected]>
Fix two issues:
- the per-cpu stat flush is unnecessary, nobody uses per-cpu stat except sum it to global stat. We can do the calculation there. The flush just wastes cpu time.

One thing that the stat flushing avoids is overflowing the batch counter, but with the current windows we're using, that seems really unlikely, so I think this is okay.

Since they diverged, I would kind of like to separate the types for the per-cpu buffer and the final stats, but this is fine for now.

Quoted 1 line

- some fields are signed int/s64. I don't see the point.

I like this part.

Reviewed-by Omar Sandoval <[email protected]>

Quoted 116 lines

Cc Omar Sandoval <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 block/blk-stat.c          | 45 +++++++--------------------------------------
 include/linux/blk_types.h |  5 ++---
 2 files changed, 9 insertions(+), 41 deletions(-)

block/blk-stat.c +7 ~~-38~~

index c52356d..3a2f3c9 100644
@@ -11,8 +11,6 @@
 #include "blk-mq.h"
 #include "blk.h"
 
-#define BLK_RQ_STAT_BATCH	64
-
 struct blk_queue_stats {
 	struct list_head callbacks;
 	spinlock_t lock;

@@ -23,45 +21,21 @@ static void blk_stat_init(struct blk_rq_stat *stat)
 {
 	stat->min = -1ULL;
 	stat->max = stat->nr_samples = stat->mean = 0;
-	stat->batch = stat->nr_batch = 0;
-}
-
-static void blk_stat_flush_batch(struct blk_rq_stat *stat)
-{
-	const s32 nr_batch = READ_ONCE(stat->nr_batch);
-	const s32 nr_samples = READ_ONCE(stat->nr_samples);
-
-	if (!nr_batch)
-		return;
-	if (!nr_samples)
-		stat->mean = div64_s64(stat->batch, nr_batch);
-	else {
-		stat->mean = div64_s64((stat->mean * nr_samples) +
-					stat->batch,
-					nr_batch + nr_samples);
-	}
-
-	stat->nr_samples += nr_batch;
-	stat->nr_batch = stat->batch = 0;
+	stat->batch = 0;
 }
 
+/* src is a per-cpu stat, mean isn't initialized */
 static void blk_stat_sum(struct blk_rq_stat *dst, struct blk_rq_stat *src)
 {
-	blk_stat_flush_batch(src);
-
 	if (!src->nr_samples)
 		return;
 
 	dst->min = min(dst->min, src->min);
 	dst->max = max(dst->max, src->max);
 
-	if (!dst->nr_samples)
-		dst->mean = src->mean;
-	else {
-		dst->mean = div64_s64((src->mean * src->nr_samples) +
-					(dst->mean * dst->nr_samples),
-					dst->nr_samples + src->nr_samples);
-	}
+	dst->mean = div_u64(src->batch + dst->mean * dst->nr_samples,
+				dst->nr_samples + src->nr_samples);
+
 	dst->nr_samples += src->nr_samples;
 }

@@ -69,13 +43,8 @@ static void __blk_stat_add(struct blk_rq_stat *stat, u64 value)
 {
 	stat->min = min(stat->min, value);
 	stat->max = max(stat->max, value);
-
-	if (stat->batch + value < stat->batch ||
-	    stat->nr_batch + 1 == BLK_RQ_STAT_BATCH)
-		blk_stat_flush_batch(stat);
-
 	stat->batch += value;
-	stat->nr_batch++;
+	stat->nr_samples++;
 }
 
 void blk_stat_add(struct request *rq)

@@ -84,7 +53,7 @@ void blk_stat_add(struct request *rq)
 	struct blk_stat_callback *cb;
 	struct blk_rq_stat *stat;
 	int bucket;
-	s64 now, value;
+	u64 now, value;
 
 	now = __blk_stat_time(ktime_to_ns(ktime_get()));
 	if (now < blk_stat_time(&rq->issue_stat))

include/linux/blk_types.h +2 -3

index a2d2aa7..3385c89 100644
@@ -329,11 +329,10 @@ static inline bool blk_qc_t_is_internal(blk_qc_t cookie)
 }
 
 struct blk_rq_stat {
-	s64 mean;
+	u64 mean;
 	u64 min;
 	u64 max;
-	s32 nr_samples;
-	s32 nr_batch;
+	u32 nr_samples;
 	u64 batch;
 };

Signature · 1 line

2.9.5

Jens Axboe

maintainer

2017-10-10 18:02 UTC #3 lore raw

Re: [PATCH V2 1/3] blk-stat: delete useless code

In reply to Shaohua Li

Quoted 7 lines · Shaohua Li

From: Shaohua Li <[email protected]>
Fix two issues:
- the per-cpu stat flush is unnecessary, nobody uses per-cpu stat except sum it to global stat. We can do the calculation there. The flush just wastes cpu time.
- some fields are signed int/s64. I don't see the point.

Anecdotal, I had issues with the div yielding wrong results with an unsigned type. But I don't remember what or why right now, unfortunately, and this was before it was merged...

Signature · 1 line

Jens Axboe

Shaohua Li

author

2017-10-07 00:56 UTC #4 lore raw

PATCH v2 2/3 block: set request_list for request

In reply to Shaohua Li

From: Shaohua Li <[email protected]>

Legacy queue sets request's request_list, mq doesn't. This makes mq does the same thing, so we can find cgroup of a request. Note, we really only use blkg field of request_list, it's pointless to allocate mempool for request_list in mq case.

Signed-off-by Shaohua Li <[email protected]>

---
 block/blk-core.c | 2 +-
 block/blk-mq.c   | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

block/blk-core.c +1 -1

index adb064a..f88afd1 100644
@@ -718,7 +718,7 @@ static void free_request_size(void *element, void *data)
 int blk_init_rl(struct request_list *rl, struct request_queue *q,
 		gfp_t gfp_mask)
 {
-	if (unlikely(rl->rq_pool))
+	if (unlikely(rl->rq_pool) || q->mq_ops)
 		return 0;
 
 	rl->q = q;

block/blk-mq.c +5 -0

index 7f01d69..c3b5876 100644
@@ -481,6 +481,9 @@ void blk_mq_free_request(struct request *rq)
 
 	wbt_done(q->rq_wb, &rq->issue_stat);
 
+	if (blk_rq_rl(rq))
+		blk_put_rl(blk_rq_rl(rq));
+
 	clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
 	clear_bit(REQ_ATOM_POLL_SLEPT, &rq->atomic_flags);
 	if (rq->tag != -1)

@@ -1504,6 +1507,8 @@ static void blk_mq_bio_to_request(struct request *rq, struct bio *bio)
 {
 	blk_init_request_from_bio(rq, bio);
 
+	blk_rq_set_rl(rq, blk_get_rl(rq->q, bio));
+
 	blk_account_io_start(rq, true);
 }

Signature · 1 line

2.9.5

Jens Axboe

maintainer

2017-10-10 19:47 UTC #5 lore raw

Re: [PATCH V2 2/3] block: set request_list for request

Quoted 6 lines · Shaohua Li

From: Shaohua Li <[email protected]>
Legacy queue sets request's request_list, mq doesn't. This makes mq does the same thing, so we can find cgroup of a request. Note, we really only use blkg field of request_list, it's pointless to allocate mempool for request_list in mq case.

Looks OK to me.

Signature · 1 line

Jens Axboe

Shaohua Li

author

2017-10-07 00:56 UTC #6 lore raw

PATCH v2 3/3 blockcg: export latency info for each cgroup

In reply to Shaohua Li

From: Shaohua Li <[email protected]>

Export the latency info to user. The latency is a good sign to indicate if IO is congested or not. User can use the info to make decisions like adjust cgroup settings.

Existing io.stat shows accumulated IO bytes and requests, but accumulated value for latency doesn't make much sense. This patch exports the latency info in a 100ms interval.

To reduce overhead, latency info of children is propagated to parents every 10ms. This means the parents's latency could lost 10ms info of its children in 100ms. This should be ok, as we don't need precise latency info.

A micro benchmark running fio test against null_blk in a sixth level cgroup doesn't show obvious regression. perf shows a little bit overhead in blk_stat_add (~1%) and blkg_lookup (~1%), which is unavoidable right now.

With this patch, the io.stat will show:
8:0 rbytes=7282688 wbytes=0 rios=83 wios=0 rlat_mean=2720 rlat_min=183 rlat_max=14880 wlat_mean=0 wlat_min=0 wlat_max=0 The new fields will display read/write average/minimum/maximum latency
within 100ms. The latency is us.

Signed-off-by Shaohua Li <[email protected]>

---
 block/blk-cgroup.c         |  29 +++++++++-
 block/blk-stat.c           | 135 ++++++++++++++++++++++++++++++++++++++++++++-
 block/blk.h                |   5 ++
 include/linux/blk-cgroup.h |   9 +++
 4 files changed, 175 insertions(+), 3 deletions(-)

block/blk-cgroup.c +27 -2

index d3f56ba..89c5075 100644
@@ -78,6 +78,7 @@ static void blkg_free(struct blkcg_gq *blkg)
 
 	blkg_rwstat_exit(&blkg->stat_ios);
 	blkg_rwstat_exit(&blkg->stat_bytes);
+	blkg_rq_stat_exit(blkg);
 	kfree(blkg);
 }

@@ -104,6 +105,8 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct request_queue *q,
 	    blkg_rwstat_init(&blkg->stat_ios, gfp_mask))
 		goto err_free;
 
+	if (blkg_rq_stat_init(blkg, gfp_mask))
+		goto err_free;
 	blkg->q = q;
 	INIT_LIST_HEAD(&blkg->q_node);
 	blkg->blkcg = blkcg;

@@ -952,6 +955,8 @@ static int blkcg_print_stat(struct seq_file *sf, void *v)
 		const char *dname;
 		struct blkg_rwstat rwstat;
 		u64 rbytes, wbytes, rios, wios;
+		u64 rmean = 0, rmin = 0, rmax = 0;
+		u64 wmean = 0, wmin = 0, wmax = 0;
 
 		dname = blkg_dev_name(blkg);
 		if (!dname)

@@ -969,11 +974,30 @@ static int blkcg_print_stat(struct seq_file *sf, void *v)
 		rios = atomic64_read(&rwstat.aux_cnt[BLKG_RWSTAT_READ]);
 		wios = atomic64_read(&rwstat.aux_cnt[BLKG_RWSTAT_WRITE]);
 
+		if (blkg->rq_stat.stat[0].nr_samples) {
+			rmean = blkg->rq_stat.stat[0].mean;
+			do_div(rmean, 1000);
+			rmin = blkg->rq_stat.stat[0].min;
+			do_div(rmin, 1000);
+			rmax = blkg->rq_stat.stat[0].max;
+			do_div(rmax, 1000);
+		}
+		if (blkg->rq_stat.stat[1].nr_samples) {
+			wmean = blkg->rq_stat.stat[1].mean;
+			do_div(wmean, 1000);
+			wmin = blkg->rq_stat.stat[1].min;
+			do_div(wmin, 1000);
+			wmax = blkg->rq_stat.stat[1].max;
+			do_div(wmax, 1000);
+		}
 		spin_unlock_irq(blkg->q->queue_lock);
 
 		if (rbytes || wbytes || rios || wios)
-			seq_printf(sf, "%s rbytes=%llu wbytes=%llu rios=%llu wios=%llu\n",
-				   dname, rbytes, wbytes, rios, wios);
+			seq_printf(sf, "%s rbytes=%llu wbytes=%llu rios=%llu wios=%llu "
+				   "rlat_mean=%llu rlat_min=%llu rlat_max=%llu "
+				   "wlat_mean=%llu wlat_min=%llu wlat_max=%llu\n",
+				   dname, rbytes, wbytes, rios, wios,
+				   rmean, rmin, rmax, wmean, wmin, wmax);
 	}
 
 	rcu_read_unlock();

@@ -1167,6 +1191,7 @@ int blkcg_init_queue(struct request_queue *q)
 		blkg_destroy_all(q);
 		spin_unlock_irq(q->queue_lock);
 	}
+	blk_stat_enable_accounting(q);
 	return ret;
 }

block/blk-stat.c +134 -1

160 lines not on this page · raw message

block/blk.h +5 -0

index fda5a46..4d76a971 100644
@@ -309,6 +309,11 @@ static inline void blk_throtl_bio_endio(struct bio *bio) { }
 static inline void blk_throtl_stat_add(struct request *rq, u64 time) { }
 #endif
 
+#ifdef CONFIG_BLK_CGROUP
+extern int blkg_rq_stat_init(struct blkcg_gq *blkg, gfp_t gfp);
+extern void blkg_rq_stat_exit(struct blkcg_gq *blkg);
+#endif
+
 #ifdef CONFIG_BOUNCE
 extern int init_emergency_isa_pool(void);
 extern void blk_queue_bounce(struct request_queue *q, struct bio **bio);

include/linux/blk-cgroup.h +9 -0

index f57e54d..58f3d25 100644
@@ -102,6 +102,14 @@ struct blkcg_policy_data {
 	int				plid;
 };
 
+struct blkcg_gq_rq_stat {
+	u64 last_flush_time;
+	struct blk_rq_stat stat[2];
+	struct blk_rq_stat __percpu *cpu_stat;
+	struct blk_rq_stat __percpu *cpu_propagate_stat;
+	u64 __percpu *cpu_propagate_time;
+};
+
 /* association between a blk cgroup and a request queue */
 struct blkcg_gq {
 	/* Pointer to the associated request_queue */

@@ -130,6 +138,7 @@ struct blkcg_gq {
 
 	struct blkg_rwstat		stat_bytes;
 	struct blkg_rwstat		stat_ios;
+	struct blkcg_gq_rq_stat		rq_stat;
 
 	struct blkg_policy_data		*pd[BLKCG_MAX_POLS];

Signature · 1 line

2.9.5

weiping zhang

2017-10-10 17:35 UTC #7 lore raw

Re: [PATCH V2 3/3] blockcg: export latency info for each cgroup

Quoted 5 lines · Shaohua Li

From: Shaohua Li <[email protected]>
Export the latency info to user. The latency is a good sign to indicate if IO is congested or not. User can use the info to make decisions like adjust cgroup settings.

Hi Shaohua,
How to check IO congested or not by latency ? Different SSD has different latency especially when mixed sequence and random IO operatons.

Quoted 3 lines

Existing io.stat shows accumulated IO bytes and requests, but accumulated value for latency doesn't make much sense. This patch

How to understand "accumulated value for latency doesn't make much sense" could you give an example? I think iostat's r_await and w_await are nearly equal to rlat_mean and wlat_mean if there is no too much request starved.

Quoted 12 lines

exports the latency info in a 100ms interval.
To reduce overhead, latency info of children is propagated to parents every 10ms. This means the parents's latency could lost 10ms info of its children in 100ms. This should be ok, as we don't need precise latency info.
A micro benchmark running fio test against null_blk in a sixth level cgroup doesn't show obvious regression. perf shows a little bit overhead in blk_stat_add (~1%) and blkg_lookup (~1%), which is unavoidable right now.

Quoted 5 lines

With this patch, the io.stat will show:
8:0 rbytes=7282688 wbytes=0 rios=83 wios=0 rlat_mean=2720 rlat_min=183 rlat_max=14880 wlat_mean=0 wlat_min=0 wlat_max=0 The new fields will display read/write average/minimum/maximum latency
within 100ms. The latency is us.

Thanks
Weiping

Shaohua Li

author

2017-10-10 18:23 UTC #8 lore raw

Re: [PATCH V2 3/3] blockcg: export latency info for each cgroup

Quoted 10 lines · weiping zhang

Quoted 5 lines · Shaohua Li
From: Shaohua Li <[email protected]>
Export the latency info to user. The latency is a good sign to indicate if IO is congested or not. User can use the info to make decisions like adjust cgroup settings.
Hi Shaohua,
How to check IO congested or not by latency ? Different SSD has different latency especially when mixed sequence and random IO operatons.

There is no magic here, you should know the SSD characteristics first. The idea is observing the latency when the system isn't overcommited, for example, running the workload in a one-cgroup setup, then using the observed latency to guide configuration for multi-cgroup settings or check if IO is healthy in cgroups.

Quoted 5 lines

Quoted 3 lines
Existing io.stat shows accumulated IO bytes and requests, but accumulated value for latency doesn't make much sense. This patch
How to understand "accumulated value for latency doesn't make much sense" could you give an example?

rbytes and wbytes in io.stat are accumulated value. If such value of latency isn't valuable.

Quoted 3 lines

I think iostat's r_await and w_await
are nearly equal to rlat_mean and wlat_mean if there is no too much request starved.

yes, pretty similar. io.stat is per-cgroup though

Thanks,
Shaohua

weiping zhang

2017-10-11 14:41 UTC #9 lore raw

Re: [PATCH V2 3/3] blockcg: export latency info for each cgroup

2017-10-11 2:23 GMT+08:00 Shaohua Li <[email protected]>:

Quoted 12 lines · weiping zhang

Quoted 10 lines · Shaohua Li
Quoted 5 lines
From: Shaohua Li <[email protected]>
Export the latency info to user. The latency is a good sign to indicate if IO is congested or not. User can use the info to make decisions like adjust cgroup settings.
Hi Shaohua,
How to check IO congested or not by latency ? Different SSD has different latency especially when mixed sequence and random IO operatons.

Quoted 6 lines

There is no magic here, you should know the SSD characteristics first. The idea is observing the latency when the system isn't overcommited, for example, running the workload in a one-cgroup setup, then using the observed latency to guide configuration for multi-cgroup settings or check if IO is healthy in cgroups.

Could you please give more detail about how to get the SSD characteristics ? I cann't find a good way to get the reasonable latency at mixed sequence , random,
read, and write. Is there a value or a range represent the reasonable latency at mixed circurmastance ?

Thanks
Weiping