[PATCH 0/4] SYNC_NOIDLE preemption for ancestor cgroups

[PATCH 0/4] SYNC_NOIDLE preemption for ancestor cgroups

[Jan Kara]

Hello,

recently we have been debugging regression of basically any IO workload
when systemd started enabling blkio controller for user sessions (due to
delegation feature). Now using blkio controller certainly has its costs
but some of the hits seemed just too heavy - e.g. dbench4 throughput
dropped from ~150 MB/s to ~26 MB/s for ext4 with barrier=0 mount option on
an ordinary SATA drive. The reason for the drop is visible in the following
blktrace:

0.000383426  5122  A  WS 27691328 + 8 <- (259,851968) 21473600
0.000384039  5122  Q  WS 27691328 + 8 [jbd2/sdb3-8]
0.000385944  5122  G  WS 27691328 + 8 [jbd2/sdb3-8]
0.000386315  5122  P   N [jbd2/sdb3-8]
...
0.000394031  5122  A  WS 27691384 + 8 <- (259,851968) 21473656
0.000394210  5122  Q  WS 27691384 + 8 [jbd2/sdb3-8]
0.000394569  5122  M  WS 27691384 + 8 [jbd2/sdb3-8]
0.000395239  5122  I  WS 27691328 + 64 [jbd2/sdb3-8]
0.000396572     0  m   N cfq5122SN / insert_request
0.000397389     0  m   N cfq5122SN / add_to_rr
0.000398458  5122  U   N [jbd2/sdb3-8] 1

<<< Here we wait for 7.5 ms for idle timer on dbench sync-noidle queue to fire

0.008001111     0  m   N cfq idle timer fired
0.008003152     0  m   N cfq5174SN /dbench slice expired t=0
0.008004871     0  m   N /dbench served: vt=24796020 min_vt=24771438
0.008006508     0  m   N cfq5174SN /dbench sl_used=2 disp=1 charge=2 iops=0 sect=24
0.008007509     0  m   N cfq5174SN /dbench del_from_rr
0.008008197     0  m   N /dbench del_from_rr group
0.008008771     0  m   N cfq schedule dispatch
0.008013506     0  m   N cfq workload slice:16
0.008014979     0  m   N cfq5122SN / set_active wl_class:0 wl_type:1
0.008017229     0  m   N cfq5122SN / fifo=          (null)
0.008018149     0  m   N cfq5122SN / dispatch_insert
0.008019863     0  m   N cfq5122SN / dispatched a request
0.008020829     0  m   N cfq5122SN / activate rq, drv=1
0.008021578   389  D  WS 27691328 + 64 [kworker/5:1H]
0.008491262     0  C  WS 27691328 + 64 [0]
0.008498654     0  m   N cfq5122SN / complete rqnoidle 1
0.008500202     0  m   N cfq5122SN / set_slice=19
0.008501797     0  m   N cfq5122SN / arm_idle: 2 group_idle: 0
0.008502073     0  m   N cfq schedule dispatch
0.008517281  5122  A  WS 27691392 + 8 <- (259,851968) 21473664
0.008517627  5122  Q  WS 27691392 + 8 [jbd2/sdb3-8]
0.008519126  5122  G  WS 27691392 + 8 [jbd2/sdb3-8]
0.008519534  5122  I  WS 27691392 + 8 [jbd2/sdb3-8]
0.008520560     0  m   N cfq5122SN / insert_request
0.008521908     0  m   N cfq5122SN / dispatch_insert
0.008522798     0  m   N cfq5122SN / dispatched a request
0.008523558     0  m   N cfq5122SN / activate rq, drv=1
0.008523841  5122  D  WS 27691392 + 8 [jbd2/sdb3-8]
0.008718527     0  C  WS 27691392 + 8 [0]
0.008721911     0  m   N cfq5122SN / complete rqnoidle 1
0.008723186     0  m   N cfq5122SN / arm_idle: 2 group_idle: 0
0.008723578     0  m   N cfq schedule dispatch
0.009062333  5174  A  WS 23276680 + 24 <- (259,851968) 17058952
0.009062950  5174  Q  WS 23276680 + 24 [dbench4]
0.009065427  5174  G  WS 23276680 + 24 [dbench4]
0.009065717  5174  P   N [dbench4]
0.009067472  5174  I  WS 23276680 + 24 [dbench4]
0.009069038     0  m   N cfq5174SN /dbench insert_request
0.009069913     0  m   N cfq5174SN /dbench add_to_rr
0.009071190  5174  U   N [dbench4] 1

<<<< Here we wait another 7 ms for idle timer on jbd2 sync-noidle queue to fire

0.016001504     0  m   N cfq idle timer fired
0.016002924     0  m   N cfq5122SN / slice expired t=0
0.016004424     0  m   N / served: vt=24783779 min_vt=24771488
0.016005888     0  m   N cfq5122SN / sl_used=2 disp=2 charge=2 iops=0 sect=72
0.016006635     0  m   N cfq5122SN / del_from_rr
0.016007152     0  m   N / del_from_rr group
0.016007613     0  m   N cfq schedule dispatch
0.016014571     0  m   N cfq workload slice:24
0.016015679     0  m   N cfq5174SN /dbench set_active wl_class:0 wl_type:1
0.016016794     0  m   N cfq5174SN /dbench fifo=          (null)
0.016017652     0  m   N cfq5174SN /dbench dispatch_insert
0.016018883     0  m   N cfq5174SN /dbench dispatched a request
0.016019714     0  m   N cfq5174SN /dbench activate rq, drv=1
0.016019973   382  D  WS 23276680 + 24 [kworker/6:1H]
0.016347056     0  C  WS 23276680 + 24 [0]
0.016357022     0  m   N cfq5174SN /dbench complete rqnoidle 1
0.016358509     0  m   N cfq5174SN /dbench set_slice=24
0.016360127     0  m   N cfq5174SN /dbench arm_idle: 2 group_idle: 0
0.016360508     0  m   N cfq schedule dispatch
...

When dbench isn't in a separate cgroup, dbench and jbd2 sync-noidle queues just
freely preempt each other. When dbench gets contained in a dedicated blkio
cgroup, preemption is not allowed and the throughput dropped.

The idling is happening because we want to provide separation of IO between
different blkio cgroups and thus we idle to avoid starving one cgroup where
process is submitting only dependent IO. I am of the opinion that in case
ancestor would like to preempt a descendant cgroup, there is no strong reason
to provide the separation and we can save at least one of the idle times
(when switching from dbench to jbd2 thread). Thus the following patch set
which improves the throughput of dbench4 from ~26 MB/s to ~48 MB/s.

The first patch in the patch set is just unrelated improvement where I've
spotted some asymetry in how slice_idle and group_idle are handled. Patches two
and three prepare cfq_should_preempt() to be able to work on service trees of
different cgroups, patch 4 then adds the logic in cfq_should_preempt() to allow
preemption by ancestor cgroup.

Comments welcome!

								Honza

[PATCH 1/4] cfq-iosched: Don't group_idle if cfqq has big thinktime

[Jan Kara]

From: Jan Kara <jack@suse.com>

There is no point in idling on a cfq group if the only cfq queue that is
there has too big thinktime.

Signed-off-by: Jan Kara <jack@suse.com>
---
 block/cfq-iosched.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 1f9093e901da..fcef1786edf3 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2897,6 +2897,7 @@ static bool cfq_should_idle(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 static void cfq_arm_slice_timer(struct cfq_data *cfqd)
 {
 	struct cfq_queue *cfqq = cfqd->active_queue;
+	struct cfq_rb_root *st = cfqq->service_tree;
 	struct cfq_io_cq *cic;
 	unsigned long sl, group_idle = 0;
 
@@ -2947,8 +2948,13 @@ static void cfq_arm_slice_timer(struct cfq_data *cfqd)
 		return;
 	}
 
-	/* There are other queues in the group, don't do group idle */
-	if (group_idle && cfqq->cfqg->nr_cfqq > 1)
+	/*
+	 * There are other queues in the group or this is the only group and
+	 * it has too big thinktime, don't do group idle.
+	 */
+	if (group_idle &&
+	    (cfqq->cfqg->nr_cfqq > 1 ||
+	     !cfq_io_thinktime_big(cfqd, &st->ttime, true)))
 		return;
 
 	cfq_mark_cfqq_wait_request(cfqq);
-- 
2.1.4

[PATCH 2/4] cfq-iosched: Reorder checks in cfq_should_preempt()

[Jan Kara]

From: Jan Kara <jack@suse.com>

Move check for preemption by rt class up. There is no functional change
but it makes arguing about conditions simpler since we can be sure both
cfq queues are from the same ioprio class.

Signed-off-by: Jan Kara <jack@suse.com>
---
 block/cfq-iosched.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index fcef1786edf3..b78a82acf99c 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -3959,6 +3959,13 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
 	if (cfq_slice_used(cfqq))
 		return true;
 
+	/*
+	 * Allow an RT request to pre-empt an ongoing non-RT cfqq timeslice.
+	 */
+	if (cfq_class_rt(new_cfqq) && !cfq_class_rt(cfqq))
+		return true;
+
+	WARN_ON_ONCE(cfqq->ioprio_class != new_cfqq->ioprio_class);
 	/* Allow preemption only if we are idling on sync-noidle tree */
 	if (cfqd->serving_wl_type == SYNC_NOIDLE_WORKLOAD &&
 	    cfqq_type(new_cfqq) == SYNC_NOIDLE_WORKLOAD &&
@@ -3973,12 +3980,6 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
 	if ((rq->cmd_flags & REQ_PRIO) && !cfqq->prio_pending)
 		return true;
 
-	/*
-	 * Allow an RT request to pre-empt an ongoing non-RT cfqq timeslice.
-	 */
-	if (cfq_class_rt(new_cfqq) && !cfq_class_rt(cfqq))
-		return true;
-
 	/* An idle queue should not be idle now for some reason */
 	if (RB_EMPTY_ROOT(&cfqq->sort_list) && !cfq_should_idle(cfqd, cfqq))
 		return true;
-- 
2.1.4

[PATCH 3/4] cfq-iosched: Allow sync noidle workloads to preempt each other

[Jan Kara]

From: Jan Kara <jack@suse.com>

The original idea with preemption of sync noidle queues (introduced in
commit 718eee0579b8 "cfq-iosched: fairness for sync no-idle queues") was
that we service all sync noidle queues together, we don't idle on any of
the queues individually and we idle only if there is no sync noidle
queue to be served. This intention also matches the original test:

	if (cfqd->serving_type == SYNC_NOIDLE_WORKLOAD
	   && new_cfqq->service_tree == cfqq->service_tree)
		return true;

However since at that time cfqq->service_tree was not set for idling
queues, this test was unreliable and was replaced in commit e4a229196a7c
"cfq-iosched: fix no-idle preemption logic" by:

	if (cfqd->serving_type == SYNC_NOIDLE_WORKLOAD &&
	    cfqq_type(new_cfqq) == SYNC_NOIDLE_WORKLOAD &&
	    new_cfqq->service_tree->count == 1)
		return true;

That was a reliable test but was actually doing something different -
now we preempt sync noidle queue only if the new queue is the only one
busy in the service tree.

These days cfq queue is kept in service tree even if it is idling and
thus the original check would be safe again. But since we actually check
that cfq queues are in the same cgroup, of the same priority class and
workload type (sync noidle), we know that new_cfqq is fine to preempt
cfqq. So just remove the service tree check.

Signed-off-by: Jan Kara <jack@suse.com>
---
 block/cfq-iosched.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index b78a82acf99c..6c8bd1025c62 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -3969,7 +3969,6 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
 	/* Allow preemption only if we are idling on sync-noidle tree */
 	if (cfqd->serving_wl_type == SYNC_NOIDLE_WORKLOAD &&
 	    cfqq_type(new_cfqq) == SYNC_NOIDLE_WORKLOAD &&
-	    new_cfqq->service_tree->count == 2 &&
 	    RB_EMPTY_ROOT(&cfqq->sort_list))
 		return true;
 
-- 
2.1.4

[PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child

[Jan Kara]

From: Jan Kara <jack@suse.com>

Currently we don't allow sync workload of one cgroup to preempt sync
workload of any other cgroup. This is because we want to achieve service
separation between cgroups. However in cases where cgroup preempting is
ancestor of the current cgroup, there is no need of separation and
idling introduces unnecessary overhead. This is hurts for example the
case when workload is isolated within a cgroup but journalling threads
are in root cgroup. Simple way to demostrate the issue is using:

dbench4 -c /usr/share/dbench4/client.txt -t 10 -D /mnt 1

on ext4 filesystem on plain SATA driver (mounted with barrier=0 to make
difference more visible). When all processes are in the root cgroup,
reported throughput is 153.132 MB/sec. When dbench process gets its own
blkio cgroup, reported throughput drops to 26.1006 MB/sec.

Fix the problem by making check in cfq_should_preempt() more benevolent
and allow preemption by ancestor cgroup. This improves the throughput
reported by dbench4 to 48.9106 MB/sec.

Signed-off-by: Jan Kara <jack@suse.com>
---
 block/cfq-iosched.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 6c8bd1025c62..0961b84a9107 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -632,6 +632,13 @@ static inline struct cfq_group *cfqg_parent(struct cfq_group *cfqg)
 	return pblkg ? blkg_to_cfqg(pblkg) : NULL;
 }
 
+static inline bool cfqg_is_descendant(struct cfq_group *cfqg,
+				      struct cfq_group *ancestor)
+{
+	return cgroup_is_descendant(cfqg_to_blkg(cfqg)->blkcg->css.cgroup,
+				    cfqg_to_blkg(ancestor)->blkcg->css.cgroup);
+}
+
 static inline void cfqg_get(struct cfq_group *cfqg)
 {
 	return blkg_get(cfqg_to_blkg(cfqg));
@@ -758,6 +765,11 @@ static void cfqg_stats_xfer_dead(struct cfq_group *cfqg)
 #else	/* CONFIG_CFQ_GROUP_IOSCHED */
 
 static inline struct cfq_group *cfqg_parent(struct cfq_group *cfqg) { return NULL; }
+static inline bool cfqg_is_descendant(struct cfq_group *cfqg,
+				      struct cfq_group *ancestor)
+{
+	return true;
+}
 static inline void cfqg_get(struct cfq_group *cfqg) { }
 static inline void cfqg_put(struct cfq_group *cfqg) { }
 
@@ -3953,7 +3965,12 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
 	if (rq_is_sync(rq) && !cfq_cfqq_sync(cfqq))
 		return true;
 
-	if (new_cfqq->cfqg != cfqq->cfqg)
+	/*
+	 * Treat ancestors of current cgroup the same way as current cgroup.
+	 * For anybody else we disallow preemption to guarantee service
+	 * fairness among cgroups.
+	 */
+	if (!cfqg_is_descendant(cfqq->cfqg, new_cfqq->cfqg))
 		return false;
 
 	if (cfq_slice_used(cfqq))
-- 
2.1.4

Re: [PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

Hi Jan,

[auto build test ERROR on block/for-next]
[also build test ERROR on v4.4-rc2 next-20151124]

url:    https://github.com/0day-ci/linux/commits/Jan-Kara/SYNC_NOIDLE-preemption-for-ancestor-cgroups/20151125-222645
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: i386-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "cgroup_is_descendant" undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 52591 bytes --]

Re: [PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Thu 26-11-15 04:24:23, kbuild test robot wrote:
> Hi Jan,
> 
> [auto build test ERROR on block/for-next]
> [also build test ERROR on v4.4-rc2 next-20151124]
> 
> url:    https://github.com/0day-ci/linux/commits/Jan-Kara/SYNC_NOIDLE-preemption-for-ancestor-cgroups/20151125-222645
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
> config: i386-allmodconfig (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
> >> ERROR: "cgroup_is_descendant" undefined!

Ah, I didn't notice the function is not exported. Attached patch fixes the
build.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-cgroup-Export-cgroup_is_descendant.patch --]
[-- Type: text/x-patch, Size: 760 bytes --]

>From 9a489728dbae48311e2ee0fff3c1793dd1e97392 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Thu, 26 Nov 2015 13:29:21 +0100
Subject: [PATCH] cgroup: Export cgroup_is_descendant()

CFQ IO scheduler will use cgroup_is_descendant() to decide whether one
queue can preempt another once.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 kernel/cgroup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index f1603c153890..6b61f847b040 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -477,6 +477,7 @@ bool cgroup_is_descendant(struct cgroup *cgrp, struct cgroup *ancestor)
 	}
 	return false;
 }
+EXPORT_SYMBOL_GPL(cgroup_is_descendant);
 
 static int notify_on_release(const struct cgroup *cgrp)
 {
-- 
2.1.4