* [PATCH 1/4] cfq-iosched: Don't group_idle if cfqq has big thinktime
2015-11-25 14:22 [PATCH 0/4] SYNC_NOIDLE preemption for ancestor cgroups Jan Kara
@ 2015-11-25 14:22 ` Jan Kara
2015-11-25 14:22 ` [PATCH 2/4] cfq-iosched: Reorder checks in cfq_should_preempt() Jan Kara
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Jan Kara @ 2015-11-25 14:22 UTC (permalink / raw)
To: axboe; +Cc: Jeff Moyer, LKML, Tejun Heo, 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 2/4] cfq-iosched: Reorder checks in cfq_should_preempt()
2015-11-25 14:22 [PATCH 0/4] SYNC_NOIDLE preemption for ancestor cgroups Jan Kara
2015-11-25 14:22 ` [PATCH 1/4] cfq-iosched: Don't group_idle if cfqq has big thinktime Jan Kara
@ 2015-11-25 14:22 ` Jan Kara
2015-11-25 14:22 ` [PATCH 3/4] cfq-iosched: Allow sync noidle workloads to preempt each other Jan Kara
2015-11-25 14:22 ` [PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child Jan Kara
3 siblings, 0 replies; 7+ messages in thread
From: Jan Kara @ 2015-11-25 14:22 UTC (permalink / raw)
To: axboe; +Cc: Jeff Moyer, LKML, Tejun Heo, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/4] cfq-iosched: Allow sync noidle workloads to preempt each other
2015-11-25 14:22 [PATCH 0/4] SYNC_NOIDLE preemption for ancestor cgroups Jan Kara
2015-11-25 14:22 ` [PATCH 1/4] cfq-iosched: Don't group_idle if cfqq has big thinktime Jan Kara
2015-11-25 14:22 ` [PATCH 2/4] cfq-iosched: Reorder checks in cfq_should_preempt() Jan Kara
@ 2015-11-25 14:22 ` Jan Kara
2015-11-25 14:22 ` [PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child Jan Kara
3 siblings, 0 replies; 7+ messages in thread
From: Jan Kara @ 2015-11-25 14:22 UTC (permalink / raw)
To: axboe; +Cc: Jeff Moyer, LKML, Tejun Heo, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child
2015-11-25 14:22 [PATCH 0/4] SYNC_NOIDLE preemption for ancestor cgroups Jan Kara
` (2 preceding siblings ...)
2015-11-25 14:22 ` [PATCH 3/4] cfq-iosched: Allow sync noidle workloads to preempt each other Jan Kara
@ 2015-11-25 14:22 ` Jan Kara
2015-11-25 20:24 ` kbuild test robot
3 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2015-11-25 14:22 UTC (permalink / raw)
To: axboe; +Cc: Jeff Moyer, LKML, Tejun Heo, 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child
2015-11-25 14:22 ` [PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child Jan Kara
@ 2015-11-25 20:24 ` kbuild test robot
2015-11-26 13:22 ` Jan Kara
0 siblings, 1 reply; 7+ messages in thread
From: kbuild test robot @ 2015-11-25 20:24 UTC (permalink / raw)
To: Jan Kara; +Cc: kbuild-all, axboe, Jeff Moyer, LKML, Tejun Heo, Jan Kara
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 4/4] cfq-iosched: Allow parent cgroup to preempt its child
2015-11-25 20:24 ` kbuild test robot
@ 2015-11-26 13:22 ` Jan Kara
0 siblings, 0 replies; 7+ messages in thread
From: Jan Kara @ 2015-11-26 13:22 UTC (permalink / raw)
To: kbuild test robot
Cc: Jan Kara, kbuild-all, axboe, Jeff Moyer, LKML, Tejun Heo, 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
^ permalink raw reply [flat|nested] 7+ messages in thread