[PATCH 0/5 v3] SYNC_NOIDLE preemption for ancestor cgroups

[PATCH 0/5 v3] SYNC_NOIDLE preemption for ancestor cgroups

[Jan Kara]

Hello Jens,

This is v3 of the patch series. 

Changes since v2:
* added acks from Tejun
* fixed some typos pointed out by Tejun
* fixed condition checking for too long think time in patch 1/5

Changes since v1:
* added missing export of cgroup_is_descendant()

If you find the series too hasty for the current merge window, just postpone it
for the next one.

---

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/5] 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..0a6a70a9bca8 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.6.2

[PATCH 2/5] 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.

Acked-by: Tejun Heo <tj@kernel.org>
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 0a6a70a9bca8..2544c219c00c 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.6.2

[PATCH 3/5] 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.

Acked-by: Tejun Heo <tj@kernel.org>
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 2544c219c00c..98569c2f373e 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.6.2

[PATCH 4/5] cgroup: Export cgroup_is_descendant()

[Jan Kara]

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

Acked-by: Tejun Heo <tj@kernel.org>
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 470f6536b9e8..c4d74c10dfcb 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -483,6 +483,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.6.2

[PATCH 5/5] 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 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 drive (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.

Acked-by: Tejun Heo <tj@kernel.org>
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 98569c2f373e..e3c591dd8f19 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.6.2

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

[Tejun Heo]

On Tue, Jan 12, 2016 at 04:24:15PM +0100, Jan Kara wrote:
> 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>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 0/5 v3] SYNC_NOIDLE preemption for ancestor cgroups

[Jens Axboe]

On 01/12/2016 08:24 AM, Jan Kara wrote:
> Hello Jens,
>
> This is v3 of the patch series.
>
> Changes since v2:
> * added acks from Tejun
> * fixed some typos pointed out by Tejun
> * fixed condition checking for too long think time in patch 1/5
>
> Changes since v1:
> * added missing export of cgroup_is_descendant()
>
> If you find the series too hasty for the current merge window, just postpone it
> for the next one.

This series looks good to me, I'd like to push it in this round. Good 
work, Jan!

-- 
Jens Axboe

Re: [PATCH 0/5 v3] SYNC_NOIDLE preemption for ancestor cgroups

[Jan Kara]

On Tue 12-01-16 10:10:35, Jens Axboe wrote:
> On 01/12/2016 08:24 AM, Jan Kara wrote:
> >Hello Jens,
> >
> >This is v3 of the patch series.
> >
> >Changes since v2:
> >* added acks from Tejun
> >* fixed some typos pointed out by Tejun
> >* fixed condition checking for too long think time in patch 1/5
> >
> >Changes since v1:
> >* added missing export of cgroup_is_descendant()
> >
> >If you find the series too hasty for the current merge window, just postpone it
> >for the next one.
> 
> This series looks good to me, I'd like to push it in this round. Good work,
> Jan!

So did this happen? I don't see patches anywhere in your tree or in Linus'?

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

Re: [PATCH 0/5 v3] SYNC_NOIDLE preemption for ancestor cgroups

[Jens Axboe]

On 02/04/2016 09:43 AM, Jan Kara wrote:
> On Tue 12-01-16 10:10:35, Jens Axboe wrote:
>> On 01/12/2016 08:24 AM, Jan Kara wrote:
>>> Hello Jens,
>>>
>>> This is v3 of the patch series.
>>>
>>> Changes since v2:
>>> * added acks from Tejun
>>> * fixed some typos pointed out by Tejun
>>> * fixed condition checking for too long think time in patch 1/5
>>>
>>> Changes since v1:
>>> * added missing export of cgroup_is_descendant()
>>>
>>> If you find the series too hasty for the current merge window, just postpone it
>>> for the next one.
>>
>> This series looks good to me, I'd like to push it in this round. Good work,
>> Jan!
>
> So did this happen? I don't see patches anywhere in your tree or in Linus'?

Spinning it off real soon, it'll get there.


-- 
Jens Axboe