[PATCH 0/3] cfq-iosched: Fair cross-group preemption

[PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Chad Talbott]

This patchset introduces fair cross-group preemption.  Right now, we
don't have strict isolation between processes in different cgroups.
For example: currently an RT ioprio thread in one group can preempt a
BE ioprio thread in another group.  We would like to have an
application specify the relative priorities of its threads, but still
allow strict isolation between applications.

With this patch series, we can configure an entire cgroup as needing
low-latency service.  Then that group will be able to preempt another
group.  To prevent a runaway application from starving other
applications, we allow this preemption only until it has exceeded its
fair share (as specified by its relative weight).  So a rate-limited,
but latency sensative application (like streaming audio or video) can
get front-of-the-line service without fear of it hogging a whole
disk's IO.

Note that this series is targeted to shallow-queue devices (e.g. a
single spinning disk).  Without hardware support, it is much more
difficult to provide low-latency on a device with a deep request
queue.

The following bash script demonstrates the feature:

cd /dev/cgroup
mkdir c1 c2

(
    echo '8:16 900' > c1/blkio.weight_device
    echo '8:16 2' > c1/blkio.class
    echo $BASHPID > c1/tasks
    dd if=/dev/sdb of=/dev/null bs=1M iflag=direct
) & 
(
    echo '8:16 100' > c2/blkio.weight_device
    echo '8:16 1' > c2/blkio.class
    echo $BASHPID > c2/tasks
    while : ; do dd if=/dev/sdb of=/dev/null bs=1M count=1 iflag=direct; sleep .1 ; done
) &
sleep 10
kill %1 %2

grep 8:16 c?/blkio*
# end

Since the c2 reader is "well behaved" and only reads 10MB/s, it will
be able to consistenly preempt c1, and therefore see low latency for
its requests.

[PATCH 1/3] cfq-iosched: Fair cross-group preemption (interface and documentation)

[Chad Talbott]

blkio.class is a new interface that allows you to specify a cgroup as
requiring low-latency service on a specific block device.  This patch
includes only the interface and documentation.

Signed-off-by: Chad Talbott <ctalbott@google.com>
---
 Documentation/cgroups/blkio-controller.txt |   16 ++++
 block/blk-cgroup.c                         |  121 +++++++++++++++++++++-------
 block/blk-cgroup.h                         |   36 +++++++--
 block/cfq-iosched.c                        |   12 +++-
 4 files changed, 145 insertions(+), 40 deletions(-)

diff --git a/Documentation/cgroups/blkio-controller.txt b/Documentation/cgroups/blkio-controller.txt
index 4ed7b5c..7acf9d2 100644
--- a/Documentation/cgroups/blkio-controller.txt
+++ b/Documentation/cgroups/blkio-controller.txt
@@ -169,6 +169,22 @@ Proportional weight policy files
 	  dev     weight
 	  8:16    300
 
+- blkio.class
+	- Specifies the per-cgroup, per-device service class.  There
+	  are currently two service classes: real-time (1) and
+	  best-effort (2).  By default, all groups start as
+	  best-effort.
+
+	  #echo dev_maj:dev_minor class > /path/to/cgroup/blkio.class
+	  Configure class=real-time on /dev/sdb (8:16) in this cgroup
+	  # echo '8:16 1' > blkio.class
+	  # cat blkio.class
+	  8:16    1
+
+	  A real-time group can preempt a best-effort group, only as
+	  long as it's within its assigned weight; i.e. fairness is
+	  preserved.
+
 - blkio.time
 	- disk time allocated to cgroup per device in milliseconds. First
 	  two fields specify the major and minor number of the device and
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 455768a..f2c553e 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -129,6 +129,21 @@ blkio_update_group_weight(struct blkio_group *blkg, unsigned int weight)
 	}
 }
 
+static inline void
+blkio_update_group_class(struct blkio_group *blkg, unsigned int class)
+{
+	struct blkio_policy_type *blkiop;
+
+	list_for_each_entry(blkiop, &blkio_list, list) {
+		/* If this policy does not own the blkg, do not send updates */
+		if (blkiop->plid != blkg->plid)
+			continue;
+		if (blkiop->ops.blkio_update_group_class_fn)
+			blkiop->ops.blkio_update_group_class_fn(blkg->key,
+								blkg, class);
+	}
+}
+
 static inline void blkio_update_group_bps(struct blkio_group *blkg, u64 bps,
 				int fileid)
 {
@@ -708,15 +723,29 @@ static int blkio_policy_parse_and_set(char *buf,
 
 	switch (plid) {
 	case BLKIO_POLICY_PROP:
-		ret = strict_strtoul(s[1], 10, &temp);
-		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
-			temp > BLKIO_WEIGHT_MAX)
-			return -EINVAL;
-
-		newpn->plid = plid;
-		newpn->fileid = fileid;
-		newpn->val.weight = temp;
-		break;
+		switch (fileid) {
+		case BLKIO_PROP_weight_device:
+			ret = strict_strtoul(s[1], 10, &temp);
+			if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
+			    temp > BLKIO_WEIGHT_MAX)
+				return -EINVAL;
+
+			newpn->plid = plid;
+			newpn->fileid = fileid;
+			newpn->val.prop.weight = temp;
+			break;
+		case BLKIO_PROP_class:
+			ret = strict_strtoul(s[1], 10, &temp);
+			if (ret ||
+			    temp < BLKIO_RT_CLASS ||
+			    temp > BLKIO_BE_CLASS)
+				return -EINVAL;
+
+			newpn->plid = plid;
+			newpn->fileid = fileid;
+			newpn->val.prop.class = temp;
+			break;
+		}
 	case BLKIO_POLICY_THROTL:
 		switch(fileid) {
 		case BLKIO_THROTL_read_bps_device:
@@ -727,7 +756,7 @@ static int blkio_policy_parse_and_set(char *buf,
 
 			newpn->plid = plid;
 			newpn->fileid = fileid;
-			newpn->val.bps = bps;
+			newpn->val.throtl.bps = bps;
 			break;
 		case BLKIO_THROTL_read_iops_device:
 		case BLKIO_THROTL_write_iops_device:
@@ -740,7 +769,7 @@ static int blkio_policy_parse_and_set(char *buf,
 
 			newpn->plid = plid;
 			newpn->fileid = fileid;
-			newpn->val.iops = (unsigned int)iops;
+			newpn->val.throtl.iops = (unsigned int)iops;
 			break;
 		}
 		break;
@@ -759,20 +788,34 @@ unsigned int blkcg_get_weight(struct blkio_cgroup *blkcg,
 	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_PROP,
 				BLKIO_PROP_weight_device);
 	if (pn)
-		return pn->val.weight;
+		return pn->val.prop.weight;
 	else
 		return blkcg->weight;
 }
 EXPORT_SYMBOL_GPL(blkcg_get_weight);
 
-uint64_t blkcg_get_read_bps(struct blkio_cgroup *blkcg, dev_t dev)
+enum blkcg_class blkcg_get_class(struct blkio_cgroup *blkcg,
+			     dev_t dev)
 {
 	struct blkio_policy_node *pn;
+	enum blkcg_class class = BLKIO_DEFAULT_CLASS;
+
+	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_PROP,
+				BLKIO_PROP_class);
+	if (pn)
+		class = pn->val.prop.class;
 
+	return class;
+}
+EXPORT_SYMBOL_GPL(blkcg_get_class);
+
+uint64_t blkcg_get_read_bps(struct blkio_cgroup *blkcg, dev_t dev)
+{
+	struct blkio_policy_node *pn;
 	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_THROTL,
 				BLKIO_THROTL_read_bps_device);
 	if (pn)
-		return pn->val.bps;
+		return pn->val.throtl.bps;
 	else
 		return -1;
 }
@@ -783,7 +826,7 @@ uint64_t blkcg_get_write_bps(struct blkio_cgroup *blkcg, dev_t dev)
 	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_THROTL,
 				BLKIO_THROTL_write_bps_device);
 	if (pn)
-		return pn->val.bps;
+		return pn->val.throtl.bps;
 	else
 		return -1;
 }
@@ -795,7 +838,7 @@ unsigned int blkcg_get_read_iops(struct blkio_cgroup *blkcg, dev_t dev)
 	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_THROTL,
 				BLKIO_THROTL_read_iops_device);
 	if (pn)
-		return pn->val.iops;
+		return pn->val.throtl.iops;
 	else
 		return -1;
 }
@@ -806,7 +849,7 @@ unsigned int blkcg_get_write_iops(struct blkio_cgroup *blkcg, dev_t dev)
 	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_THROTL,
 				BLKIO_THROTL_write_iops_device);
 	if (pn)
-		return pn->val.iops;
+		return pn->val.throtl.iops;
 	else
 		return -1;
 }
@@ -816,19 +859,19 @@ static bool blkio_delete_rule_command(struct blkio_policy_node *pn)
 {
 	switch(pn->plid) {
 	case BLKIO_POLICY_PROP:
-		if (pn->val.weight == 0)
+		if (pn->val.prop.weight == 0)
 			return 1;
 		break;
 	case BLKIO_POLICY_THROTL:
 		switch(pn->fileid) {
 		case BLKIO_THROTL_read_bps_device:
 		case BLKIO_THROTL_write_bps_device:
-			if (pn->val.bps == 0)
+			if (pn->val.throtl.bps == 0)
 				return 1;
 			break;
 		case BLKIO_THROTL_read_iops_device:
 		case BLKIO_THROTL_write_iops_device:
-			if (pn->val.iops == 0)
+			if (pn->val.throtl.iops == 0)
 				return 1;
 		}
 		break;
@@ -844,17 +887,17 @@ static void blkio_update_policy_rule(struct blkio_policy_node *oldpn,
 {
 	switch(oldpn->plid) {
 	case BLKIO_POLICY_PROP:
-		oldpn->val.weight = newpn->val.weight;
+		oldpn->val.prop.weight = newpn->val.prop.weight;
 		break;
 	case BLKIO_POLICY_THROTL:
 		switch(newpn->fileid) {
 		case BLKIO_THROTL_read_bps_device:
 		case BLKIO_THROTL_write_bps_device:
-			oldpn->val.bps = newpn->val.bps;
+			oldpn->val.throtl.bps = newpn->val.throtl.bps;
 			break;
 		case BLKIO_THROTL_read_iops_device:
 		case BLKIO_THROTL_write_iops_device:
-			oldpn->val.iops = newpn->val.iops;
+			oldpn->val.throtl.iops = newpn->val.throtl.iops;
 		}
 		break;
 	default:
@@ -872,22 +915,24 @@ static void blkio_update_blkg_policy(struct blkio_cgroup *blkcg,
 	unsigned int weight, iops;
 	u64 bps;
 
+
 	switch(pn->plid) {
 	case BLKIO_POLICY_PROP:
-		weight = pn->val.weight ? pn->val.weight :
+		weight = pn->val.prop.weight ? pn->val.prop.weight :
 				blkcg->weight;
 		blkio_update_group_weight(blkg, weight);
+		blkio_update_group_class(blkg, pn->val.prop.class);
 		break;
 	case BLKIO_POLICY_THROTL:
 		switch(pn->fileid) {
 		case BLKIO_THROTL_read_bps_device:
 		case BLKIO_THROTL_write_bps_device:
-			bps = pn->val.bps ? pn->val.bps : (-1);
+			bps = pn->val.throtl.bps ? pn->val.throtl.bps : (-1);
 			blkio_update_group_bps(blkg, bps, pn->fileid);
 			break;
 		case BLKIO_THROTL_read_iops_device:
 		case BLKIO_THROTL_write_iops_device:
-			iops = pn->val.iops ? pn->val.iops : (-1);
+			iops = pn->val.throtl.iops ? pn->val.throtl.iops : (-1);
 			blkio_update_group_iops(blkg, iops, pn->fileid);
 			break;
 		}
@@ -984,21 +1029,28 @@ blkio_print_policy_node(struct seq_file *m, struct blkio_policy_node *pn)
 {
 	switch(pn->plid) {
 		case BLKIO_POLICY_PROP:
-			if (pn->fileid == BLKIO_PROP_weight_device)
+			switch(pn->fileid) {
+			case BLKIO_PROP_weight_device:
+				seq_printf(m, "%u:%u\t%u\n", MAJOR(pn->dev),
+					   MINOR(pn->dev), pn->val.prop.weight);
+				break;
+			case BLKIO_PROP_class:
 				seq_printf(m, "%u:%u\t%u\n", MAJOR(pn->dev),
-					MINOR(pn->dev), pn->val.weight);
+					   MINOR(pn->dev), pn->val.prop.class);
+				break;
+			};
 			break;
 		case BLKIO_POLICY_THROTL:
 			switch(pn->fileid) {
 			case BLKIO_THROTL_read_bps_device:
 			case BLKIO_THROTL_write_bps_device:
 				seq_printf(m, "%u:%u\t%llu\n", MAJOR(pn->dev),
-					MINOR(pn->dev), pn->val.bps);
+					   MINOR(pn->dev), pn->val.throtl.bps);
 				break;
 			case BLKIO_THROTL_read_iops_device:
 			case BLKIO_THROTL_write_iops_device:
 				seq_printf(m, "%u:%u\t%u\n", MAJOR(pn->dev),
-					MINOR(pn->dev), pn->val.iops);
+					   MINOR(pn->dev), pn->val.throtl.iops);
 				break;
 			}
 			break;
@@ -1037,6 +1089,7 @@ static int blkiocg_file_read(struct cgroup *cgrp, struct cftype *cft,
 	case BLKIO_POLICY_PROP:
 		switch(name) {
 		case BLKIO_PROP_weight_device:
+		case BLKIO_PROP_class:
 			blkio_read_policy_node_files(cft, blkcg, m);
 			return 0;
 		default:
@@ -1243,6 +1296,14 @@ struct cftype blkio_files[] = {
 		.max_write_len = 256,
 	},
 	{
+		.name = "class",
+		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
+				BLKIO_PROP_class),
+		.read_seq_string = blkiocg_file_read,
+		.write_string = blkiocg_file_write,
+		.max_write_len = 256,
+	},
+	{
 		.name = "weight",
 		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
 				BLKIO_PROP_weight),
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index ea4861b..0e9cd32 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -73,10 +73,18 @@ enum blkg_state_flags {
 	BLKG_empty,
 };
 
+enum blkcg_class {
+	BLKIO_RT_CLASS = 1,
+	BLKIO_BE_CLASS = 2,
+};
+
+#define BLKIO_DEFAULT_CLASS BLKIO_BE_CLASS
+
 /* cgroup files owned by proportional weight policy */
 enum blkcg_file_name_prop {
 	BLKIO_PROP_weight = 1,
 	BLKIO_PROP_weight_device,
+	BLKIO_PROP_class,
 	BLKIO_PROP_io_service_bytes,
 	BLKIO_PROP_io_serviced,
 	BLKIO_PROP_time,
@@ -157,6 +165,21 @@ struct blkio_group {
 	struct blkio_group_stats stats;
 };
 
+struct proportion_policy {
+	unsigned int weight;
+	unsigned int class;
+};
+
+union throttle_policy {
+	/*
+	 * Rate read/write in terms of byptes per second
+	 * Whether this rate represents read or write is determined
+	 * by file type "fileid".
+	 */
+	u64 bps;
+	unsigned int iops;
+};
+
 struct blkio_policy_node {
 	struct list_head node;
 	dev_t dev;
@@ -166,14 +189,8 @@ struct blkio_policy_node {
 	int fileid;
 
 	union {
-		unsigned int weight;
-		/*
-		 * Rate read/write in terms of byptes per second
-		 * Whether this rate represents read or write is determined
-		 * by file type "fileid".
-		 */
-		u64 bps;
-		unsigned int iops;
+		struct proportion_policy prop;
+		union throttle_policy throtl;
 	} val;
 };
 
@@ -192,6 +209,8 @@ typedef void (blkio_unlink_group_fn) (void *key, struct blkio_group *blkg);
 
 typedef void (blkio_update_group_weight_fn) (void *key,
 			struct blkio_group *blkg, unsigned int weight);
+typedef void (blkio_update_group_class_fn) (void *key,
+			struct blkio_group *blkg, unsigned int class);
 typedef void (blkio_update_group_read_bps_fn) (void * key,
 			struct blkio_group *blkg, u64 read_bps);
 typedef void (blkio_update_group_write_bps_fn) (void *key,
@@ -204,6 +223,7 @@ typedef void (blkio_update_group_write_iops_fn) (void *key,
 struct blkio_policy_ops {
 	blkio_unlink_group_fn *blkio_unlink_group_fn;
 	blkio_update_group_weight_fn *blkio_update_group_weight_fn;
+	blkio_update_group_class_fn *blkio_update_group_class_fn;
 	blkio_update_group_read_bps_fn *blkio_update_group_read_bps_fn;
 	blkio_update_group_write_bps_fn *blkio_update_group_write_bps_fn;
 	blkio_update_group_read_iops_fn *blkio_update_group_read_iops_fn;
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index ea83a4f..55e78b7 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -179,6 +179,7 @@ struct cfq_group {
 	/* group service_tree key */
 	u64 vdisktime;
 	unsigned int weight;
+	enum blkcg_class class;
 
 	/* number of cfqq currently on this group */
 	int nr_cfqq;
@@ -988,6 +989,12 @@ void cfq_update_blkio_group_weight(void *key, struct blkio_group *blkg,
 	cfqg_of_blkg(blkg)->weight = weight;
 }
 
+void cfq_update_blkio_group_class(void *key, struct blkio_group *blkg,
+					unsigned int class)
+{
+	cfqg_of_blkg(blkg)->class = class;
+}
+
 static struct cfq_group *
 cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
 {
@@ -4115,8 +4122,9 @@ static struct elevator_type iosched_cfq = {
 #ifdef CONFIG_CFQ_GROUP_IOSCHED
 static struct blkio_policy_type blkio_policy_cfq = {
 	.ops = {
-		.blkio_unlink_group_fn =	cfq_unlink_blkio_group,
-		.blkio_update_group_weight_fn =	cfq_update_blkio_group_weight,
+		.blkio_unlink_group_fn =	       cfq_unlink_blkio_group,
+		.blkio_update_group_weight_fn = cfq_update_blkio_group_weight,
+		.blkio_update_group_class_fn =  cfq_update_blkio_group_class,
 	},
 	.plid = BLKIO_POLICY_PROP,
 };
-- 
1.7.3.1

[PATCH 2/3] cfq-iosched: Fair cross-group preemption (implementation)

[Chad Talbott]

Add a new function cfq_group_should_preempt() which uses the new
service classes to decide if a task in one cgroup should preempt a
task in another.  We only allow it if the preempting task's vdisktime
is "behind" the preempted task.

Signed-off-by: Chad Talbott <ctalbott@google.com>
---
 block/cfq-iosched.c |   41 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 55e78b7..dfcce80 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1157,6 +1157,32 @@ void cfq_unlink_blkio_group(void *key, struct blkio_group *blkg)
 	spin_unlock_irqrestore(cfqd->queue->queue_lock, flags);
 }
 
+/*
+ * Return true if new_cfqg should preempt cfqg.  A return value of
+ * false means "don't care."  In that case, cfq has other heuristics
+ * to decide
+ */
+static bool
+cfq_group_should_preempt(struct cfq_queue *new_cfqq, struct cfq_queue *cfqq,
+			 struct request *rq)
+{
+	struct cfq_group *cfqg = cfqq->cfqg;
+	struct cfq_group *new_cfqg = new_cfqq->cfqg;
+	u64 grace_period;
+
+	/* in-group preemption is handled elsewhere */
+	if (new_cfqg == cfqg)
+		return false;
+
+	if (!(new_cfqg->class == BLKIO_RT_CLASS &&
+	      cfqg->class == BLKIO_BE_CLASS))
+		return false;
+
+	grace_period = cfq_scale_slice(cfqq->allocated_slice, cfqg);
+	return time_before64(new_cfqg->vdisktime,
+			     cfqg->vdisktime + grace_period);
+}
+
 #else /* GROUP_IOSCHED */
 static struct cfq_group *cfq_get_cfqg(struct cfq_data *cfqd, int create)
 {
@@ -1176,6 +1202,12 @@ cfq_link_cfqq_cfqg(struct cfq_queue *cfqq, struct cfq_group *cfqg) {
 static void cfq_release_cfq_groups(struct cfq_data *cfqd) {}
 static inline void cfq_put_cfqg(struct cfq_group *cfqg) {}
 
+static bool
+cfq_group_should_preempt(struct cfq_queue *new_cfqq, struct cfq_queue *cfqq,
+			 struct request *rq)
+{
+	return false;
+}
 #endif /* GROUP_IOSCHED */
 
 /*
@@ -3234,6 +3266,12 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
 	if (!cfqq)
 		return false;
 
+	if (cfq_group_should_preempt(new_cfqq, cfqq, rq))
+		return true;
+
+	if (new_cfqq->cfqg != cfqq->cfqg)
+		return false;
+
 	if (cfq_class_idle(new_cfqq))
 		return false;
 
@@ -3253,9 +3291,6 @@ 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)
-		return false;
-
 	if (cfq_slice_used(cfqq))
 		return true;
 
-- 
1.7.3.1

[PATCH 3/3] cfq-iosched: Fair cross-group preemption (stats)

[Chad Talbott]

Add three counters to monitor the cross-group preemption behavior.

preempt_count:
  counts the number of times this group preempted another

preempt_active:
  counts the number of times this group issued an IO but
  was already the active group

preempt_throttle:
  number of times this group would have preempted another if it hadn't
  already used up its assigned weight

Signed-off-by: Chad Talbott <ctalbott@google.com>
---
 block/blk-cgroup.c  |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 block/blk-cgroup.h  |   18 ++++++++++++++++++
 block/cfq-iosched.c |   20 +++++++++++++++++---
 3 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index f2c553e..41ee61c 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -347,6 +347,25 @@ void blkiocg_set_start_empty_time(struct blkio_group *blkg)
 }
 EXPORT_SYMBOL_GPL(blkiocg_set_start_empty_time);
 
+void blkiocg_update_preempt_stats(struct blkio_group *blkg,
+			enum stat_type which, bool direction, bool sync)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&blkg->stats_lock, flags);
+	switch (which) {
+	case BLKIO_STAT_PREEMPT_COUNT:
+	case BLKIO_STAT_PREEMPT_ACTIVE:
+	case BLKIO_STAT_PREEMPT_THROTTLE:
+		blkio_add_stat(blkg->stats.stat_arr[which], 1, direction, sync);
+		break;
+	default:
+		BUG();
+	}
+	spin_unlock_irqrestore(&blkg->stats_lock, flags);
+}
+EXPORT_SYMBOL_GPL(blkiocg_update_preempt_stats);
+
 void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
 			unsigned long dequeue)
 {
@@ -1193,6 +1212,15 @@ static int blkiocg_file_read_map(struct cgroup *cgrp, struct cftype *cft,
 		case BLKIO_PROP_empty_time:
 			return blkio_read_blkg_stats(blkcg, cft, cb,
 						BLKIO_STAT_EMPTY_TIME, 0);
+		case BLKIO_PROP_preempt_count:
+			return blkio_read_blkg_stats(blkcg, cft, cb,
+						BLKIO_STAT_PREEMPT_COUNT, 0);
+		case BLKIO_PROP_preempt_active:
+			return blkio_read_blkg_stats(blkcg, cft, cb,
+						BLKIO_STAT_PREEMPT_ACTIVE, 0);
+		case BLKIO_PROP_preempt_throttle:
+			return blkio_read_blkg_stats(blkcg, cft, cb,
+						BLKIO_STAT_PREEMPT_THROTTLE, 0);
 #endif
 		default:
 			BUG();
@@ -1443,6 +1471,24 @@ struct cftype blkio_files[] = {
 				BLKIO_PROP_dequeue),
 		.read_map = blkiocg_file_read_map,
 	},
+	{
+		.name = "preempt_count",
+		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
+				BLKIO_PROP_preempt_count),
+		.read_map = blkiocg_file_read_map,
+	},
+	{
+		.name = "preempt_active",
+		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
+				BLKIO_PROP_preempt_active),
+		.read_map = blkiocg_file_read_map,
+	},
+	{
+		.name = "preempt_throttle",
+		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
+				BLKIO_PROP_preempt_throttle),
+		.read_map = blkiocg_file_read_map,
+	},
 #endif
 };
 
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 0e9cd32..d2cbdcd 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -44,6 +44,14 @@ enum stat_type {
 	BLKIO_STAT_WAIT_TIME,
 	/* Number of IOs merged */
 	BLKIO_STAT_MERGED,
+#ifdef CONFIG_DEBUG_BLK_CGROUP
+	/* Number of IOs Added to active group */
+	BLKIO_STAT_PREEMPT_ACTIVE,
+	/* Number of cross-group preemptions */
+	BLKIO_STAT_PREEMPT_COUNT,
+	/* Number of cross-group preemptions disallowed by weight */
+	BLKIO_STAT_PREEMPT_THROTTLE,
+#endif
 	/* Number of IOs queued up */
 	BLKIO_STAT_QUEUED,
 	/* All the single valued stats go below this */
@@ -98,6 +106,9 @@ enum blkcg_file_name_prop {
 	BLKIO_PROP_idle_time,
 	BLKIO_PROP_empty_time,
 	BLKIO_PROP_dequeue,
+	BLKIO_PROP_preempt_count,
+	BLKIO_PROP_preempt_active,
+	BLKIO_PROP_preempt_throttle,
 };
 
 /* cgroup files owned by throttle policy */
@@ -271,6 +282,10 @@ void blkiocg_update_dequeue_stats(struct blkio_group *blkg,
 void blkiocg_update_set_idle_time_stats(struct blkio_group *blkg);
 void blkiocg_update_idle_time_stats(struct blkio_group *blkg);
 void blkiocg_set_start_empty_time(struct blkio_group *blkg);
+void blkiocg_update_preempt_stats(struct blkio_group *blkg,
+				enum stat_type which,
+				bool direction,
+				bool sync);
 
 #define BLKG_FLAG_FNS(name)						\
 static inline void blkio_mark_blkg_##name(				\
@@ -301,6 +316,9 @@ static inline void blkiocg_update_set_idle_time_stats(struct blkio_group *blkg)
 {}
 static inline void blkiocg_update_idle_time_stats(struct blkio_group *blkg) {}
 static inline void blkiocg_set_start_empty_time(struct blkio_group *blkg) {}
+static inline void blkiocg_update_preempt_stats(
+		struct blkio_group *blkg, enum stat_type which, bool direction,
+		bool sync) {}
 #endif
 
 #if defined(CONFIG_BLK_CGROUP) || defined(CONFIG_BLK_CGROUP_MODULE)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index dfcce80..0d55be3 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1171,16 +1171,30 @@ cfq_group_should_preempt(struct cfq_queue *new_cfqq, struct cfq_queue *cfqq,
 	u64 grace_period;
 
 	/* in-group preemption is handled elsewhere */
-	if (new_cfqg == cfqg)
+	if (new_cfqg == cfqg) {
+		blkiocg_update_preempt_stats(&new_cfqg->blkg,
+			BLKIO_STAT_PREEMPT_ACTIVE,
+			rq_data_dir(rq), rq_is_sync(rq));
 		return false;
+	}
 
 	if (!(new_cfqg->class == BLKIO_RT_CLASS &&
 	      cfqg->class == BLKIO_BE_CLASS))
 		return false;
 
 	grace_period = cfq_scale_slice(cfqq->allocated_slice, cfqg);
-	return time_before64(new_cfqg->vdisktime,
-			     cfqg->vdisktime + grace_period);
+	if (time_before64(new_cfqg->vdisktime,
+			  cfqg->vdisktime + grace_period)) {
+		blkiocg_update_preempt_stats(&new_cfqg->blkg,
+			BLKIO_STAT_PREEMPT_COUNT,
+			rq_data_dir(rq), rq_is_sync(rq));
+		return true;
+	}
+
+	blkiocg_update_preempt_stats(&new_cfqg->blkg,
+		BLKIO_STAT_PREEMPT_THROTTLE,
+		rq_data_dir(rq), rq_is_sync(rq));
+	return false;
 }
 
 #else /* GROUP_IOSCHED */
-- 
1.7.3.1

Re: [PATCH 1/3] cfq-iosched: Fair cross-group preemption (interface and documentation)

[Gui Jianfeng]

Chad Talbott wrote:
> blkio.class is a new interface that allows you to specify a cgroup as
> requiring low-latency service on a specific block device.  This patch
> includes only the interface and documentation.
> 
> Signed-off-by: Chad Talbott <ctalbott@google.com>
> ---
>  Documentation/cgroups/blkio-controller.txt |   16 ++++
>  block/blk-cgroup.c                         |  121 +++++++++++++++++++++-------
>  block/blk-cgroup.h                         |   36 +++++++--
>  block/cfq-iosched.c                        |   12 +++-
>  4 files changed, 145 insertions(+), 40 deletions(-)
> 
> diff --git a/Documentation/cgroups/blkio-controller.txt b/Documentation/cgroups/blkio-controller.txt
> index 4ed7b5c..7acf9d2 100644
> --- a/Documentation/cgroups/blkio-controller.txt
> +++ b/Documentation/cgroups/blkio-controller.txt
> @@ -169,6 +169,22 @@ Proportional weight policy files
>  	  dev     weight
>  	  8:16    300
>  
> +- blkio.class
> +	- Specifies the per-cgroup, per-device service class.  There

Hi Chad,

Do we need to implement a globle blkio.class interface as well as
a device specific one something like "blkio.class_device"? So a user
doesn't need to check every device number if he want to specify a
global value.

Thanks,
Gui

> +	  are currently two service classes: real-time (1) and
> +	  best-effort (2).  By default, all groups start as
> +	  best-effort.
> +
> +	  #echo dev_maj:dev_minor class > /path/to/cgroup/blkio.class
> +	  Configure class=real-time on /dev/sdb (8:16) in this cgroup
> +	  # echo '8:16 1' > blkio.class
> +	  # cat blkio.class
> +	  8:16    1
> +
> +	  A real-time group can preempt a best-effort group, only as
> +	  long as it's within its assigned weight; i.e. fairness is
> +	  preserved.
> +
>  - blkio.time
>  	- disk time allocated to cgroup per device in milliseconds. First
>  	  two fields specify the major and minor number of the device and
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 455768a..f2c553e 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -129,6 +129,21 @@ blkio_update_group_weight(struct blkio_group *blkg, unsigned int weight)
>  	}
>  }
>  
> +static inline void
> +blkio_update_group_class(struct blkio_group *blkg, unsigned int class)
> +{
> +	struct blkio_policy_type *blkiop;
> +
> +	list_for_each_entry(blkiop, &blkio_list, list) {
> +		/* If this policy does not own the blkg, do not send updates */
> +		if (blkiop->plid != blkg->plid)
> +			continue;
> +		if (blkiop->ops.blkio_update_group_class_fn)
> +			blkiop->ops.blkio_update_group_class_fn(blkg->key,
> +								blkg, class);
> +	}
> +}
> +
>  static inline void blkio_update_group_bps(struct blkio_group *blkg, u64 bps,
>  				int fileid)
>  {
> @@ -708,15 +723,29 @@ static int blkio_policy_parse_and_set(char *buf,
>  
>  	switch (plid) {
>  	case BLKIO_POLICY_PROP:
> -		ret = strict_strtoul(s[1], 10, &temp);
> -		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> -			temp > BLKIO_WEIGHT_MAX)
> -			return -EINVAL;
> -
> -		newpn->plid = plid;
> -		newpn->fileid = fileid;
> -		newpn->val.weight = temp;
> -		break;
> +		switch (fileid) {
> +		case BLKIO_PROP_weight_device:
> +			ret = strict_strtoul(s[1], 10, &temp);
> +			if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> +			    temp > BLKIO_WEIGHT_MAX)
> +				return -EINVAL;
> +
> +			newpn->plid = plid;
> +			newpn->fileid = fileid;
> +			newpn->val.prop.weight = temp;
> +			break;
> +		case BLKIO_PROP_class:
> +			ret = strict_strtoul(s[1], 10, &temp);
> +			if (ret ||
> +			    temp < BLKIO_RT_CLASS ||
> +			    temp > BLKIO_BE_CLASS)
> +				return -EINVAL;
> +
> +			newpn->plid = plid;
> +			newpn->fileid = fileid;
> +			newpn->val.prop.class = temp;
> +			break;
> +		}
>  	case BLKIO_POLICY_THROTL:
>  		switch(fileid) {
>  		case BLKIO_THROTL_read_bps_device:
> @@ -727,7 +756,7 @@ static int blkio_policy_parse_and_set(char *buf,
>  
>  			newpn->plid = plid;
>  			newpn->fileid = fileid;
> -			newpn->val.bps = bps;
> +			newpn->val.throtl.bps = bps;
>  			break;
>  		case BLKIO_THROTL_read_iops_device:
>  		case BLKIO_THROTL_write_iops_device:
> @@ -740,7 +769,7 @@ static int blkio_policy_parse_and_set(char *buf,
>  
>  			newpn->plid = plid;
>  			newpn->fileid = fileid;
> -			newpn->val.iops = (unsigned int)iops;
> +			newpn->val.throtl.iops = (unsigned int)iops;
>  			break;
>  		}
>  		break;
> @@ -759,20 +788,34 @@ unsigned int blkcg_get_weight(struct blkio_cgroup *blkcg,
>  	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_PROP,
>  				BLKIO_PROP_weight_device);
>  	if (pn)
> -		return pn->val.weight;
> +		return pn->val.prop.weight;
>  	else
>  		return blkcg->weight;
>  }
>  EXPORT_SYMBOL_GPL(blkcg_get_weight);
>  
> -uint64_t blkcg_get_read_bps(struct blkio_cgroup *blkcg, dev_t dev)
> +enum blkcg_class blkcg_get_class(struct blkio_cgroup *blkcg,
> +			     dev_t dev)
>  {
>  	struct blkio_policy_node *pn;
> +	enum blkcg_class class = BLKIO_DEFAULT_CLASS;
> +
> +	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_PROP,
> +				BLKIO_PROP_class);
> +	if (pn)
> +		class = pn->val.prop.class;
>  
> +	return class;
> +}
> +EXPORT_SYMBOL_GPL(blkcg_get_class);
> +
> +uint64_t blkcg_get_read_bps(struct blkio_cgroup *blkcg, dev_t dev)
> +{
> +	struct blkio_policy_node *pn;
>  	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_THROTL,
>  				BLKIO_THROTL_read_bps_device);
>  	if (pn)
> -		return pn->val.bps;
> +		return pn->val.throtl.bps;
>  	else
>  		return -1;
>  }
> @@ -783,7 +826,7 @@ uint64_t blkcg_get_write_bps(struct blkio_cgroup *blkcg, dev_t dev)
>  	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_THROTL,
>  				BLKIO_THROTL_write_bps_device);
>  	if (pn)
> -		return pn->val.bps;
> +		return pn->val.throtl.bps;
>  	else
>  		return -1;
>  }
> @@ -795,7 +838,7 @@ unsigned int blkcg_get_read_iops(struct blkio_cgroup *blkcg, dev_t dev)
>  	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_THROTL,
>  				BLKIO_THROTL_read_iops_device);
>  	if (pn)
> -		return pn->val.iops;
> +		return pn->val.throtl.iops;
>  	else
>  		return -1;
>  }
> @@ -806,7 +849,7 @@ unsigned int blkcg_get_write_iops(struct blkio_cgroup *blkcg, dev_t dev)
>  	pn = blkio_policy_search_node(blkcg, dev, BLKIO_POLICY_THROTL,
>  				BLKIO_THROTL_write_iops_device);
>  	if (pn)
> -		return pn->val.iops;
> +		return pn->val.throtl.iops;
>  	else
>  		return -1;
>  }
> @@ -816,19 +859,19 @@ static bool blkio_delete_rule_command(struct blkio_policy_node *pn)
>  {
>  	switch(pn->plid) {
>  	case BLKIO_POLICY_PROP:
> -		if (pn->val.weight == 0)
> +		if (pn->val.prop.weight == 0)
>  			return 1;
>  		break;
>  	case BLKIO_POLICY_THROTL:
>  		switch(pn->fileid) {
>  		case BLKIO_THROTL_read_bps_device:
>  		case BLKIO_THROTL_write_bps_device:
> -			if (pn->val.bps == 0)
> +			if (pn->val.throtl.bps == 0)
>  				return 1;
>  			break;
>  		case BLKIO_THROTL_read_iops_device:
>  		case BLKIO_THROTL_write_iops_device:
> -			if (pn->val.iops == 0)
> +			if (pn->val.throtl.iops == 0)
>  				return 1;
>  		}
>  		break;
> @@ -844,17 +887,17 @@ static void blkio_update_policy_rule(struct blkio_policy_node *oldpn,
>  {
>  	switch(oldpn->plid) {
>  	case BLKIO_POLICY_PROP:
> -		oldpn->val.weight = newpn->val.weight;
> +		oldpn->val.prop.weight = newpn->val.prop.weight;
>  		break;
>  	case BLKIO_POLICY_THROTL:
>  		switch(newpn->fileid) {
>  		case BLKIO_THROTL_read_bps_device:
>  		case BLKIO_THROTL_write_bps_device:
> -			oldpn->val.bps = newpn->val.bps;
> +			oldpn->val.throtl.bps = newpn->val.throtl.bps;
>  			break;
>  		case BLKIO_THROTL_read_iops_device:
>  		case BLKIO_THROTL_write_iops_device:
> -			oldpn->val.iops = newpn->val.iops;
> +			oldpn->val.throtl.iops = newpn->val.throtl.iops;
>  		}
>  		break;
>  	default:
> @@ -872,22 +915,24 @@ static void blkio_update_blkg_policy(struct blkio_cgroup *blkcg,
>  	unsigned int weight, iops;
>  	u64 bps;
>  
> +
>  	switch(pn->plid) {
>  	case BLKIO_POLICY_PROP:
> -		weight = pn->val.weight ? pn->val.weight :
> +		weight = pn->val.prop.weight ? pn->val.prop.weight :
>  				blkcg->weight;
>  		blkio_update_group_weight(blkg, weight);
> +		blkio_update_group_class(blkg, pn->val.prop.class);
>  		break;
>  	case BLKIO_POLICY_THROTL:
>  		switch(pn->fileid) {
>  		case BLKIO_THROTL_read_bps_device:
>  		case BLKIO_THROTL_write_bps_device:
> -			bps = pn->val.bps ? pn->val.bps : (-1);
> +			bps = pn->val.throtl.bps ? pn->val.throtl.bps : (-1);
>  			blkio_update_group_bps(blkg, bps, pn->fileid);
>  			break;
>  		case BLKIO_THROTL_read_iops_device:
>  		case BLKIO_THROTL_write_iops_device:
> -			iops = pn->val.iops ? pn->val.iops : (-1);
> +			iops = pn->val.throtl.iops ? pn->val.throtl.iops : (-1);
>  			blkio_update_group_iops(blkg, iops, pn->fileid);
>  			break;
>  		}
> @@ -984,21 +1029,28 @@ blkio_print_policy_node(struct seq_file *m, struct blkio_policy_node *pn)
>  {
>  	switch(pn->plid) {
>  		case BLKIO_POLICY_PROP:
> -			if (pn->fileid == BLKIO_PROP_weight_device)
> +			switch(pn->fileid) {
> +			case BLKIO_PROP_weight_device:
> +				seq_printf(m, "%u:%u\t%u\n", MAJOR(pn->dev),
> +					   MINOR(pn->dev), pn->val.prop.weight);
> +				break;
> +			case BLKIO_PROP_class:
>  				seq_printf(m, "%u:%u\t%u\n", MAJOR(pn->dev),
> -					MINOR(pn->dev), pn->val.weight);
> +					   MINOR(pn->dev), pn->val.prop.class);
> +				break;
> +			};
>  			break;
>  		case BLKIO_POLICY_THROTL:
>  			switch(pn->fileid) {
>  			case BLKIO_THROTL_read_bps_device:
>  			case BLKIO_THROTL_write_bps_device:
>  				seq_printf(m, "%u:%u\t%llu\n", MAJOR(pn->dev),
> -					MINOR(pn->dev), pn->val.bps);
> +					   MINOR(pn->dev), pn->val.throtl.bps);
>  				break;
>  			case BLKIO_THROTL_read_iops_device:
>  			case BLKIO_THROTL_write_iops_device:
>  				seq_printf(m, "%u:%u\t%u\n", MAJOR(pn->dev),
> -					MINOR(pn->dev), pn->val.iops);
> +					   MINOR(pn->dev), pn->val.throtl.iops);
>  				break;
>  			}
>  			break;
> @@ -1037,6 +1089,7 @@ static int blkiocg_file_read(struct cgroup *cgrp, struct cftype *cft,
>  	case BLKIO_POLICY_PROP:
>  		switch(name) {
>  		case BLKIO_PROP_weight_device:
> +		case BLKIO_PROP_class:
>  			blkio_read_policy_node_files(cft, blkcg, m);
>  			return 0;
>  		default:
> @@ -1243,6 +1296,14 @@ struct cftype blkio_files[] = {
>  		.max_write_len = 256,
>  	},
>  	{
> +		.name = "class",
> +		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
> +				BLKIO_PROP_class),
> +		.read_seq_string = blkiocg_file_read,
> +		.write_string = blkiocg_file_write,
> +		.max_write_len = 256,
> +	},
> +	{
>  		.name = "weight",
>  		.private = BLKIOFILE_PRIVATE(BLKIO_POLICY_PROP,
>  				BLKIO_PROP_weight),
> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
> index ea4861b..0e9cd32 100644
> --- a/block/blk-cgroup.h
> +++ b/block/blk-cgroup.h
> @@ -73,10 +73,18 @@ enum blkg_state_flags {
>  	BLKG_empty,
>  };
>  
> +enum blkcg_class {
> +	BLKIO_RT_CLASS = 1,
> +	BLKIO_BE_CLASS = 2,
> +};
> +
> +#define BLKIO_DEFAULT_CLASS BLKIO_BE_CLASS
> +
>  /* cgroup files owned by proportional weight policy */
>  enum blkcg_file_name_prop {
>  	BLKIO_PROP_weight = 1,
>  	BLKIO_PROP_weight_device,
> +	BLKIO_PROP_class,
>  	BLKIO_PROP_io_service_bytes,
>  	BLKIO_PROP_io_serviced,
>  	BLKIO_PROP_time,
> @@ -157,6 +165,21 @@ struct blkio_group {
>  	struct blkio_group_stats stats;
>  };
>  
> +struct proportion_policy {
> +	unsigned int weight;
> +	unsigned int class;
> +};
> +
> +union throttle_policy {
> +	/*
> +	 * Rate read/write in terms of byptes per second
> +	 * Whether this rate represents read or write is determined
> +	 * by file type "fileid".
> +	 */
> +	u64 bps;
> +	unsigned int iops;
> +};
> +
>  struct blkio_policy_node {
>  	struct list_head node;
>  	dev_t dev;
> @@ -166,14 +189,8 @@ struct blkio_policy_node {
>  	int fileid;
>  
>  	union {
> -		unsigned int weight;
> -		/*
> -		 * Rate read/write in terms of byptes per second
> -		 * Whether this rate represents read or write is determined
> -		 * by file type "fileid".
> -		 */
> -		u64 bps;
> -		unsigned int iops;
> +		struct proportion_policy prop;
> +		union throttle_policy throtl;
>  	} val;
>  };
>  
> @@ -192,6 +209,8 @@ typedef void (blkio_unlink_group_fn) (void *key, struct blkio_group *blkg);
>  
>  typedef void (blkio_update_group_weight_fn) (void *key,
>  			struct blkio_group *blkg, unsigned int weight);
> +typedef void (blkio_update_group_class_fn) (void *key,
> +			struct blkio_group *blkg, unsigned int class);
>  typedef void (blkio_update_group_read_bps_fn) (void * key,
>  			struct blkio_group *blkg, u64 read_bps);
>  typedef void (blkio_update_group_write_bps_fn) (void *key,
> @@ -204,6 +223,7 @@ typedef void (blkio_update_group_write_iops_fn) (void *key,
>  struct blkio_policy_ops {
>  	blkio_unlink_group_fn *blkio_unlink_group_fn;
>  	blkio_update_group_weight_fn *blkio_update_group_weight_fn;
> +	blkio_update_group_class_fn *blkio_update_group_class_fn;
>  	blkio_update_group_read_bps_fn *blkio_update_group_read_bps_fn;
>  	blkio_update_group_write_bps_fn *blkio_update_group_write_bps_fn;
>  	blkio_update_group_read_iops_fn *blkio_update_group_read_iops_fn;
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index ea83a4f..55e78b7 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -179,6 +179,7 @@ struct cfq_group {
>  	/* group service_tree key */
>  	u64 vdisktime;
>  	unsigned int weight;
> +	enum blkcg_class class;
>  
>  	/* number of cfqq currently on this group */
>  	int nr_cfqq;
> @@ -988,6 +989,12 @@ void cfq_update_blkio_group_weight(void *key, struct blkio_group *blkg,
>  	cfqg_of_blkg(blkg)->weight = weight;
>  }
>  
> +void cfq_update_blkio_group_class(void *key, struct blkio_group *blkg,
> +					unsigned int class)
> +{
> +	cfqg_of_blkg(blkg)->class = class;
> +}
> +
>  static struct cfq_group *
>  cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
>  {
> @@ -4115,8 +4122,9 @@ static struct elevator_type iosched_cfq = {
>  #ifdef CONFIG_CFQ_GROUP_IOSCHED
>  static struct blkio_policy_type blkio_policy_cfq = {
>  	.ops = {
> -		.blkio_unlink_group_fn =	cfq_unlink_blkio_group,
> -		.blkio_update_group_weight_fn =	cfq_update_blkio_group_weight,
> +		.blkio_unlink_group_fn =	       cfq_unlink_blkio_group,
> +		.blkio_update_group_weight_fn = cfq_update_blkio_group_weight,
> +		.blkio_update_group_class_fn =  cfq_update_blkio_group_class,
>  	},
>  	.plid = BLKIO_POLICY_PROP,
>  };

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Vivek Goyal]

On Mon, Mar 21, 2011 at 06:10:42PM -0700, Chad Talbott wrote:
> This patchset introduces fair cross-group preemption.  Right now, we
> don't have strict isolation between processes in different cgroups.
> For example: currently an RT ioprio thread in one group can preempt a
> BE ioprio thread in another group.  We would like to have an
> application specify the relative priorities of its threads, but still
> allow strict isolation between applications.
> 
> With this patch series, we can configure an entire cgroup as needing
> low-latency service.  Then that group will be able to preempt another
> group.  To prevent a runaway application from starving other
> applications, we allow this preemption only until it has exceeded its
> fair share (as specified by its relative weight).  So a rate-limited,
> but latency sensative application (like streaming audio or video) can
> get front-of-the-line service without fear of it hogging a whole
> disk's IO.

Chad,

Why not just implement simply RT class groups and always allow an RT
group to preempt an BE class. Same thing we do for cfq queues. I will
not worry too much about a run away application consuming all the 
bandwidth. If that's a concern we could use blkio controller to limit
the IO rate of a latency sensitive applicaiton to make sure it does
not starve BE applications.

If RT starving BE is an issue, then it is an issue with plain cfq queue
also. First we shall have to fix it there.

This definition that a latency sensitive task get prioritized only
till it is consuming its fair share and if task starts using more than
fair share then CFQ automatically stops prioritizing it sounds little
odd to me. If you are looking for predictability, then we lost it. We
shall have to very well know that task is not eating more than its
fair share before we can gurantee any kind of latencies to that task. And
if we know that task is not hogging the disk, there is anyway no risk
of it starving other groups/tasks completely.

Or we could implement something where once in a while we do allow BE
class tasks to dispatch some IO so that RT tasks do not completely
starve BE class tasks. I guess this will be similar to what cpu scheduler
was trying where an RT tasks got 95% of cpu and 5% bandwidth was reserved
for admin to kill RT tasks if some RT tasks goes wild and locks up the
cpus.

Thanks
Vivek

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Chad Talbott]

On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Why not just implement simply RT class groups and always allow an RT
> group to preempt an BE class. Same thing we do for cfq queues. I will
> not worry too much about a run away application consuming all the
> bandwidth. If that's a concern we could use blkio controller to limit
> the IO rate of a latency sensitive applicaiton to make sure it does
> not starve BE applications.

That is not quite the same semantics.  This limited preemption patch
is still work-conserving.  If the RT task in the only task on the
system with IO, it will be able to use all available disk time.

> If RT starving BE is an issue, then it is an issue with plain cfq queue
> also. First we shall have to fix it there.
>
> This definition that a latency sensitive task get prioritized only
> till it is consuming its fair share and if task starts using more than
> fair share then CFQ automatically stops prioritizing it sounds little
> odd to me. If you are looking for predictability, then we lost it. We
> shall have to very well know that task is not eating more than its
> fair share before we can gurantee any kind of latencies to that task. And
> if we know that task is not hogging the disk, there is anyway no risk
> of it starving other groups/tasks completely.

In a shared environment, we have to be a little bit defensive.  We
hope that a latency sensitive task is well characterized and won't
exceed its share of the disk, and that we haven't over-committed the
disk.  If the app does do more IO than expected, then we'd like them
to bear the burden.  We have a choice of two outcomes.  A single job
sometimes failing to achieve low disk latency when it's very busy.  Or
all jobs on a disk sometimes being very slow when another (unrelated)
job is very busy.  The first is easier to understand and debug.

Chad

Re: [PATCH 1/3] cfq-iosched: Fair cross-group preemption (interface and documentation)

[Chad Talbott]

On Tue, Mar 22, 2011 at 3:03 AM, Gui Jianfeng
<guijianfeng@cn.fujitsu.com> wrote:
> Do we need to implement a globle blkio.class interface as well as
> a device specific one something like "blkio.class_device"? So a user
> doesn't need to check every device number if he want to specify a
> global value.

I think you're right.  It's more symmetrical with the weight
interface, so will be less confusing.  I'll add it.

Chad

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Vivek Goyal]

On Tue, Mar 22, 2011 at 10:39:36AM -0700, Chad Talbott wrote:
> On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Why not just implement simply RT class groups and always allow an RT
> > group to preempt an BE class. Same thing we do for cfq queues. I will
> > not worry too much about a run away application consuming all the
> > bandwidth. If that's a concern we could use blkio controller to limit
> > the IO rate of a latency sensitive applicaiton to make sure it does
> > not starve BE applications.
> 
> That is not quite the same semantics.  This limited preemption patch
> is still work-conserving.  If the RT task in the only task on the
> system with IO, it will be able to use all available disk time.
> 

It is not same semantics but it feels like too much of special casing
for a single use case.

You are using the generic notion of a RT thread (which in general means
that it gets all the cpu or all the disk ahead of BE task). But you have
changed the definition of RT for this special use case. And also now
group RT is different from queue RT definition.

Why not have similar mechanism for cpu scheduler also then. This
application first should be able to get cpu bandwidth in same predictable
manner before it gets the disk bandwidth.

And I think your generation number patch should address this issue up
to great extent. Isn't it? If a latency sensitive task is not using
its fair quota, it will get a lower vdisktime and get to dispatch soon?

If that soon is not enough, then we could operate with reduce base slice
length so that we allocate smaller slices to groups and get better IO
latencies at the cost of total throughput. 

> > If RT starving BE is an issue, then it is an issue with plain cfq queue
> > also. First we shall have to fix it there.
> >
> > This definition that a latency sensitive task get prioritized only
> > till it is consuming its fair share and if task starts using more than
> > fair share then CFQ automatically stops prioritizing it sounds little
> > odd to me. If you are looking for predictability, then we lost it. We
> > shall have to very well know that task is not eating more than its
> > fair share before we can gurantee any kind of latencies to that task. And
> > if we know that task is not hogging the disk, there is anyway no risk
> > of it starving other groups/tasks completely.
> 
> In a shared environment, we have to be a little bit defensive.  We
> hope that a latency sensitive task is well characterized and won't
> exceed its share of the disk, and that we haven't over-committed the
> disk.  If the app does do more IO than expected, then we'd like them
> to bear the burden.  We have a choice of two outcomes.  A single job
> sometimes failing to achieve low disk latency when it's very busy.  Or
> all jobs on a disk sometimes being very slow when another (unrelated)
> job is very busy.  The first is easier to understand and debug.

To me you are trying to come up with a new scheduling class which is
not RT and you are trying to overload the meaning of RT for your use
case and that's the issue I have.

Coming up with a new scheduling class is also not desirable as that
will demand another service tree and we already have too many. Also
it should probably be also done for task and not just group otherwise
extending this concept to hierarchical setup will get complicated. Queues
and groups will just not gel well.

Frankly speaking, the problem you are having should be solved by your
generation number patch and by having smaller base slices. 

Or You could put latency sensitive applications in an RT class and 
then throttle them using blkio controller. That way you get good
latencies as well as you don't starve other tasks.

But I don't think overloading the meaning for RT or this specific use
case is a good idea.

Thanks
Vivek

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Chad Talbott]

Would it be a better approach to avoid calling the feature real-time?
Or perhaps to use another service tree to allow strict preemption (as
is done at the task level today)?

I don't like the approach of tuning tiny slices and taking a
throughput hit all the time - even if there is no latency sensitive
group on the system.

Chad

On Tue, Mar 22, 2011 at 11:12 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Mar 22, 2011 at 10:39:36AM -0700, Chad Talbott wrote:
>> On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Why not just implement simply RT class groups and always allow an RT
>> > group to preempt an BE class. Same thing we do for cfq queues. I will
>> > not worry too much about a run away application consuming all the
>> > bandwidth. If that's a concern we could use blkio controller to limit
>> > the IO rate of a latency sensitive applicaiton to make sure it does
>> > not starve BE applications.
>>
>> That is not quite the same semantics.  This limited preemption patch
>> is still work-conserving.  If the RT task in the only task on the
>> system with IO, it will be able to use all available disk time.
>>
>
> It is not same semantics but it feels like too much of special casing
> for a single use case.
>
> You are using the generic notion of a RT thread (which in general means
> that it gets all the cpu or all the disk ahead of BE task). But you have
> changed the definition of RT for this special use case. And also now
> group RT is different from queue RT definition.
>
> Why not have similar mechanism for cpu scheduler also then. This
> application first should be able to get cpu bandwidth in same predictable
> manner before it gets the disk bandwidth.
>
> And I think your generation number patch should address this issue up
> to great extent. Isn't it? If a latency sensitive task is not using
> its fair quota, it will get a lower vdisktime and get to dispatch soon?
>
> If that soon is not enough, then we could operate with reduce base slice
> length so that we allocate smaller slices to groups and get better IO
> latencies at the cost of total throughput.
>
>> > If RT starving BE is an issue, then it is an issue with plain cfq queue
>> > also. First we shall have to fix it there.
>> >
>> > This definition that a latency sensitive task get prioritized only
>> > till it is consuming its fair share and if task starts using more than
>> > fair share then CFQ automatically stops prioritizing it sounds little
>> > odd to me. If you are looking for predictability, then we lost it. We
>> > shall have to very well know that task is not eating more than its
>> > fair share before we can gurantee any kind of latencies to that task. And
>> > if we know that task is not hogging the disk, there is anyway no risk
>> > of it starving other groups/tasks completely.
>>
>> In a shared environment, we have to be a little bit defensive.  We
>> hope that a latency sensitive task is well characterized and won't
>> exceed its share of the disk, and that we haven't over-committed the
>> disk.  If the app does do more IO than expected, then we'd like them
>> to bear the burden.  We have a choice of two outcomes.  A single job
>> sometimes failing to achieve low disk latency when it's very busy.  Or
>> all jobs on a disk sometimes being very slow when another (unrelated)
>> job is very busy.  The first is easier to understand and debug.
>
> To me you are trying to come up with a new scheduling class which is
> not RT and you are trying to overload the meaning of RT for your use
> case and that's the issue I have.
>
> Coming up with a new scheduling class is also not desirable as that
> will demand another service tree and we already have too many. Also
> it should probably be also done for task and not just group otherwise
> extending this concept to hierarchical setup will get complicated. Queues
> and groups will just not gel well.
>
> Frankly speaking, the problem you are having should be solved by your
> generation number patch and by having smaller base slices.
>
> Or You could put latency sensitive applications in an RT class and
> then throttle them using blkio controller. That way you get good
> latencies as well as you don't starve other tasks.
>
> But I don't think overloading the meaning for RT or this specific use
> case is a good idea.
>
> Thanks
> Vivek
>

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Vivek Goyal]

On Tue, Mar 22, 2011 at 04:46:59PM -0700, Chad Talbott wrote:
> Would it be a better approach to avoid calling the feature real-time?
> Or perhaps to use another service tree to allow strict preemption (as
> is done at the task level today)?
> 
> I don't like the approach of tuning tiny slices and taking a
> throughput hit all the time - even if there is no latency sensitive
> group on the system.

I would appreciate if you could respond to all the points raised in
previous mail, otherwise I will just end up asking these again. 

- Why your cfq generation number patch is not sufficient? If you got a 
  workload which consumes little bandwidth only once in a while, it 
  would be placed at the beginning of service tree and you should
  experience smaller latencies.

- Why not mark the group RT group and to handle the worst case where
  user does not run away all the bandwidth, use blkio throttle to 
  set the upper limit on bandwidth. 

Coming up with another prio class or service tree just makes things 
even more complicated for this specialized case. In fact notion of
fairness or disk share is only for BE class. So coming up with another
class will not help anyway.

If you come up with another service tree, then comes a big question
how do you decide the fairness between two service trees.

IMHO, this is too special casing for a particular workload and really
will make CFQ even more complex. 

Thanks
Vivek

> 
> Chad
> 
> On Tue, Mar 22, 2011 at 11:12 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Tue, Mar 22, 2011 at 10:39:36AM -0700, Chad Talbott wrote:
> >> On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > Why not just implement simply RT class groups and always allow an RT
> >> > group to preempt an BE class. Same thing we do for cfq queues. I will
> >> > not worry too much about a run away application consuming all the
> >> > bandwidth. If that's a concern we could use blkio controller to limit
> >> > the IO rate of a latency sensitive applicaiton to make sure it does
> >> > not starve BE applications.
> >>
> >> That is not quite the same semantics.  This limited preemption patch
> >> is still work-conserving.  If the RT task in the only task on the
> >> system with IO, it will be able to use all available disk time.
> >>
> >
> > It is not same semantics but it feels like too much of special casing
> > for a single use case.
> >
> > You are using the generic notion of a RT thread (which in general means
> > that it gets all the cpu or all the disk ahead of BE task). But you have
> > changed the definition of RT for this special use case. And also now
> > group RT is different from queue RT definition.
> >
> > Why not have similar mechanism for cpu scheduler also then. This
> > application first should be able to get cpu bandwidth in same predictable
> > manner before it gets the disk bandwidth.
> >
> > And I think your generation number patch should address this issue up
> > to great extent. Isn't it? If a latency sensitive task is not using
> > its fair quota, it will get a lower vdisktime and get to dispatch soon?
> >
> > If that soon is not enough, then we could operate with reduce base slice
> > length so that we allocate smaller slices to groups and get better IO
> > latencies at the cost of total throughput.
> >
> >> > If RT starving BE is an issue, then it is an issue with plain cfq queue
> >> > also. First we shall have to fix it there.
> >> >
> >> > This definition that a latency sensitive task get prioritized only
> >> > till it is consuming its fair share and if task starts using more than
> >> > fair share then CFQ automatically stops prioritizing it sounds little
> >> > odd to me. If you are looking for predictability, then we lost it. We
> >> > shall have to very well know that task is not eating more than its
> >> > fair share before we can gurantee any kind of latencies to that task. And
> >> > if we know that task is not hogging the disk, there is anyway no risk
> >> > of it starving other groups/tasks completely.
> >>
> >> In a shared environment, we have to be a little bit defensive.  We
> >> hope that a latency sensitive task is well characterized and won't
> >> exceed its share of the disk, and that we haven't over-committed the
> >> disk.  If the app does do more IO than expected, then we'd like them
> >> to bear the burden.  We have a choice of two outcomes.  A single job
> >> sometimes failing to achieve low disk latency when it's very busy.  Or
> >> all jobs on a disk sometimes being very slow when another (unrelated)
> >> job is very busy.  The first is easier to understand and debug.
> >
> > To me you are trying to come up with a new scheduling class which is
> > not RT and you are trying to overload the meaning of RT for your use
> > case and that's the issue I have.
> >
> > Coming up with a new scheduling class is also not desirable as that
> > will demand another service tree and we already have too many. Also
> > it should probably be also done for task and not just group otherwise
> > extending this concept to hierarchical setup will get complicated. Queues
> > and groups will just not gel well.
> >
> > Frankly speaking, the problem you are having should be solved by your
> > generation number patch and by having smaller base slices.
> >
> > Or You could put latency sensitive applications in an RT class and
> > then throttle them using blkio controller. That way you get good
> > latencies as well as you don't starve other tasks.
> >
> > But I don't think overloading the meaning for RT or this specific use
> > case is a good idea.
> >
> > Thanks
> > Vivek
> >

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Chad Talbott]

On Tue, Mar 22, 2011 at 11:12 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Mar 22, 2011 at 10:39:36AM -0700, Chad Talbott wrote:
>> On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Why not just implement simply RT class groups and always allow an RT
>> > group to preempt an BE class. Same thing we do for cfq queues. I will
>> > not worry too much about a run away application consuming all the
>> > bandwidth. If that's a concern we could use blkio controller to limit
>> > the IO rate of a latency sensitive applicaiton to make sure it does
>> > not starve BE applications.
>>
>> That is not quite the same semantics.  This limited preemption patch
>> is still work-conserving.  If the RT task in the only task on the
>> system with IO, it will be able to use all available disk time.
>>
>
> It is not same semantics but it feels like too much of special casing
> for a single use case.

How are you counting use cases?

> You are using the generic notion of a RT thread (which in general means
> that it gets all the cpu or all the disk ahead of BE task). But you have
> changed the definition of RT for this special use case. And also now
> group RT is different from queue RT definition.

Perhaps the name RT has too much of a "this group should be able to
starve all other groups" connotation.  Is there a better name?  Maybe
latency sensitive?

> Why not have similar mechanism for cpu scheduler also then. This
> application first should be able to get cpu bandwidth in same predictable
> manner before it gets the disk bandwidth.

Perhaps this is a good idea.  If the CPU scheduler folks like it, I'll
be happy to support that.

> And I think your generation number patch should address this issue up
> to great extent. Isn't it? If a latency sensitive task is not using
> its fair quota, it will get a lower vdisktime and get to dispatch soon?

It will get to dispatch as soon as the current task's timeslice
expires.  This could be a long time, depending on the number of other
tasks and groups on the system.  We'd like to provide a latency
guarantee that's dependent only on the behavior of the low-latency
application.

> If that soon is not enough, then we could operate with reduce base slice
> length so that we allocate smaller slices to groups and get better IO
> latencies at the cost of total throughput.

With the limited preemption patch, I can still achieve good throughput
for many tasks, as long as the low-latency task is "quiet" or when
there is no low-latency task on the system.  If I use very small
timeslices, then I always pay a throughput price, even when there is
no low-latency task on the system or that task isn't doing any IO.

>> > If RT starving BE is an issue, then it is an issue with plain cfq queue
>> > also. First we shall have to fix it there.
>> >
>> > This definition that a latency sensitive task get prioritized only
>> > till it is consuming its fair share and if task starts using more than
>> > fair share then CFQ automatically stops prioritizing it sounds little
>> > odd to me. If you are looking for predictability, then we lost it. We
>> > shall have to very well know that task is not eating more than its
>> > fair share before we can gurantee any kind of latencies to that task. And
>> > if we know that task is not hogging the disk, there is anyway no risk
>> > of it starving other groups/tasks completely.
>>
>> In a shared environment, we have to be a little bit defensive.  We
>> hope that a latency sensitive task is well characterized and won't
>> exceed its share of the disk, and that we haven't over-committed the
>> disk.  If the app does do more IO than expected, then we'd like them
>> to bear the burden.  We have a choice of two outcomes.  A single job
>> sometimes failing to achieve low disk latency when it's very busy.  Or
>> all jobs on a disk sometimes being very slow when another (unrelated)
>> job is very busy.  The first is easier to understand and debug.
>
> To me you are trying to come up with a new scheduling class which is
> not RT and you are trying to overload the meaning of RT for your use
> case and that's the issue I have.

Can we come up with a better name?  I've used low-latency and
latency-sensitive in this email, and it's not too cumbersome.

> Coming up with a new scheduling class is also not desirable as that
> will demand another service tree and we already have too many. Also
> it should probably be also done for task and not just group otherwise
> extending this concept to hierarchical setup will get complicated. Queues
> and groups will just not gel well.

Is there a plan to provide RT class for groups in the hierarchical
future to allow full symmetry with RT tasks?

> Frankly speaking, the problem you are having should be solved by your
> generation number patch and by having smaller base slices.

Again, the throughput price is quite high to pay for all disks - even
when they have no latency sensitive groups, or those groups are not
issuing IO.

> Or You could put latency sensitive applications in an RT class and
> then throttle them using blkio controller. That way you get good
> latencies as well as you don't starve other tasks.

This is closer to the semantics offered by this patchset, but requires
debugging the complex interactions between two scheduling policies to
understand the resulting behavior.

> But I don't think overloading the meaning for RT or this specific use
> case is a good idea.

I hear you loud and clear, but I disagree.

Chad

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Vivek Goyal]

On Wed, Mar 23, 2011 at 01:10:32PM -0700, Chad Talbott wrote:
> On Tue, Mar 22, 2011 at 11:12 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Tue, Mar 22, 2011 at 10:39:36AM -0700, Chad Talbott wrote:
> >> On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > Why not just implement simply RT class groups and always allow an RT
> >> > group to preempt an BE class. Same thing we do for cfq queues. I will
> >> > not worry too much about a run away application consuming all the
> >> > bandwidth. If that's a concern we could use blkio controller to limit
> >> > the IO rate of a latency sensitive applicaiton to make sure it does
> >> > not starve BE applications.
> >>
> >> That is not quite the same semantics.  This limited preemption patch
> >> is still work-conserving.  If the RT task in the only task on the
> >> system with IO, it will be able to use all available disk time.
> >>
> >
> > It is not same semantics but it feels like too much of special casing
> > for a single use case.
> 
> How are you counting use cases?

This is the first time I have heard this requirement. So if 2-3 different
folks come up with similar concern, then I have idea an idea that this
is a generic need.

You also have not explained what is the workload and what are the
acceptable latencies etc.

> 
> > You are using the generic notion of a RT thread (which in general means
> > that it gets all the cpu or all the disk ahead of BE task). But you have
> > changed the definition of RT for this special use case. And also now
> > group RT is different from queue RT definition.
> 
> Perhaps the name RT has too much of a "this group should be able to
> starve all other groups" connotation.  Is there a better name?  Maybe
> latency sensitive?

I think what you are trying to achieve is that you want to define an
additional task and group property, say latency sensitive. This is
third property apart from ioclass and ioprio. To me you still want
the task/group to be BE class so that it shares the disk in a
proportional weight manner but this additional property will make sure
that task can preempt the non latency sensitive task/group.

We can't do this additional property for group alone because once we
move to hierarhical setup and everything is entity (be it task or queue)
and then we need to decide whether one entity can preempt another
entity or not. By not definining this property for tasks, latency 
sensitive group will always preempt a task on same tree. (May be
that's what you want for your use case). But it is still odd to add
additional properties only for groups and not tasks.

This is the new paradigm (atleast to me). It introduces additional 
complextiy in a already complicated system. So it makes sense to make
sure that there are more than 1 users of this functionality.

> 
> > Why not have similar mechanism for cpu scheduler also then. This
> > application first should be able to get cpu bandwidth in same predictable
> > manner before it gets the disk bandwidth.
> 
> Perhaps this is a good idea.  If the CPU scheduler folks like it, I'll
> be happy to support that.

> 
> > And I think your generation number patch should address this issue up
> > to great extent. Isn't it? If a latency sensitive task is not using
> > its fair quota, it will get a lower vdisktime and get to dispatch soon?
> 
> It will get to dispatch as soon as the current task's timeslice
> expires.  This could be a long time, depending on the number of other
> tasks and groups on the system.  We'd like to provide a latency
> guarantee that's dependent only on the behavior of the low-latency
> application.

What are your latency requirements? I believe maximum slice length can
be 180ms in default settings. You can change base slice to 50 and that
will make maximum slice length to 90. For default case it will be 50ms.

So question is what workload it is which can not tolerate these latencies.

> 
> > If that soon is not enough, then we could operate with reduce base slice
> > length so that we allocate smaller slices to groups and get better IO
> > latencies at the cost of total throughput.
> 
> With the limited preemption patch, I can still achieve good throughput
> for many tasks, as long as the low-latency task is "quiet" or when
> there is no low-latency task on the system.  If I use very small
> timeslices, then I always pay a throughput price, even when there is
> no low-latency task on the system or that task isn't doing any IO.

Ok, that's fine. So with-in BE class you are trying to define another
type of groups that is "low latency". That's why I think this is third
propety apart from ioprio and ioclass.

> 
> >> > If RT starving BE is an issue, then it is an issue with plain cfq queue
> >> > also. First we shall have to fix it there.
> >> >
> >> > This definition that a latency sensitive task get prioritized only
> >> > till it is consuming its fair share and if task starts using more than
> >> > fair share then CFQ automatically stops prioritizing it sounds little
> >> > odd to me. If you are looking for predictability, then we lost it. We
> >> > shall have to very well know that task is not eating more than its
> >> > fair share before we can gurantee any kind of latencies to that task. And
> >> > if we know that task is not hogging the disk, there is anyway no risk
> >> > of it starving other groups/tasks completely.
> >>
> >> In a shared environment, we have to be a little bit defensive.  We
> >> hope that a latency sensitive task is well characterized and won't
> >> exceed its share of the disk, and that we haven't over-committed the
> >> disk.  If the app does do more IO than expected, then we'd like them
> >> to bear the burden.  We have a choice of two outcomes.  A single job
> >> sometimes failing to achieve low disk latency when it's very busy.  Or
> >> all jobs on a disk sometimes being very slow when another (unrelated)
> >> job is very busy.  The first is easier to understand and debug.
> >
> > To me you are trying to come up with a new scheduling class which is
> > not RT and you are trying to overload the meaning of RT for your use
> > case and that's the issue I have.
> 
> Can we come up with a better name?  I've used low-latency and
> latency-sensitive in this email, and it's not too cumbersome.
> 
> > Coming up with a new scheduling class is also not desirable as that
> > will demand another service tree and we already have too many. Also
> > it should probably be also done for task and not just group otherwise
> > extending this concept to hierarchical setup will get complicated. Queues
> > and groups will just not gel well.
> 
> Is there a plan to provide RT class for groups in the hierarchical
> future to allow full symmetry with RT tasks?
> 
> > Frankly speaking, the problem you are having should be solved by your
> > generation number patch and by having smaller base slices.
> 
> Again, the throughput price is quite high to pay for all disks - even
> when they have no latency sensitive groups, or those groups are not
> issuing IO.
> 
> > Or You could put latency sensitive applications in an RT class and
> > then throttle them using blkio controller. That way you get good
> > latencies as well as you don't starve other tasks.
> 
> This is closer to the semantics offered by this patchset, but requires
> debugging the complex interactions between two scheduling policies to
> understand the resulting behavior.

Can you explain that a bit more? throttling behavior is very clear that
a group is allowed dispatch as long as it does not cross the limit.
Otherwise bio is put in a queue and later submitted to underlying devices.

So as long as latency sensitive task is with-in rate limit, it will get
the latency you want. The moment it tries to do lot of IO, it will get
throttled and practically becomes a oridinary BE task. I believe that's
what your patchset does. latency sensitive gets priority only if it
is consuming its fair share of disk. The only difference here is that
defination of fair share is abosolute (specified interms of bps or iops)
instead of it being dynamic depending on how many groups are doing IO.
 
blktrace results show the throttle as well cfq logs in same file so
correlating two policies is really easy. So I really don't think that
understanding the resulting is behavior is hard. I will be happy to be
proven wrong though.

> 
> > But I don't think overloading the meaning for RT or this specific use
> > case is a good idea.
> 
> I hear you loud and clear, but I disagree.

You disagree with what? Changing the definition of RT is fine. ioclass RT
means one thing for tasks and other thing for group, is it fine?

If we really end up doing it, I think we shall have to define an 
additional group file say, blkio.preempt_fair_share. This will mean
that this is a BE group but has additional property which allows it to
preempt existing entity on service tree as long as it does not exceed
it fair share. That way we don't have to define a new class or don't
have to come up with additional service tree.

But I would prefer that you seriously consider implementing RT group class
and rate limit it with throttling logic. Because I believe it should solve
your issue. Only question would be what should be upper limit and I think
that will depend on type of storage your are using and what's your
workload.

Also if you can give a better example where this kind of latency matters,
it will help to understand the problem better.

Thanks
Vivek

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Chad Talbott]

On Wed, Mar 23, 2011 at 1:41 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 23, 2011 at 01:10:32PM -0700, Chad Talbott wrote:
>> On Tue, Mar 22, 2011 at 11:12 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Tue, Mar 22, 2011 at 10:39:36AM -0700, Chad Talbott wrote:
>> >> On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > Why not just implement simply RT class groups and always allow an RT
>> >> > group to preempt an BE class. Same thing we do for cfq queues. I will
>> >> > not worry too much about a run away application consuming all the
>> >> > bandwidth. If that's a concern we could use blkio controller to limit
>> >> > the IO rate of a latency sensitive applicaiton to make sure it does
>> >> > not starve BE applications.
>> >>
>> >> That is not quite the same semantics. �This limited preemption patch
>> >> is still work-conserving. �If the RT task in the only task on the
>> >> system with IO, it will be able to use all available disk time.
>> >>
>> >
>> > It is not same semantics but it feels like too much of special casing
>> > for a single use case.
>>
>> How are you counting use cases?
>
> This is the first time I have heard this requirement. So if 2-3 different
> folks come up with similar concern, then I have idea an idea that this
> is a generic need.
>
> You also have not explained what is the workload and what are the
> acceptable latencies etc.
>
>>
>> > You are using the generic notion of a RT thread (which in general means
>> > that it gets all the cpu or all the disk ahead of BE task). But you have
>> > changed the definition of RT for this special use case. And also now
>> > group RT is different from queue RT definition.
>>
>> Perhaps the name RT has too much of a "this group should be able to
>> starve all other groups" connotation. �Is there a better name? �Maybe
>> latency sensitive?
>
> I think what you are trying to achieve is that you want to define an
> additional task and group property, say latency sensitive. This is
> third property apart from ioclass and ioprio. To me you still want
> the task/group to be BE class so that it shares the disk in a
> proportional weight manner but this additional property will make sure
> that task can preempt the non latency sensitive task/group.
>
> We can't do this additional property for group alone because once we
> move to hierarhical setup and everything is entity (be it task or queue)
> and then we need to decide whether one entity can preempt another
> entity or not. By not definining this property for tasks, latency
> sensitive group will always preempt a task on same tree. (May be
> that's what you want for your use case). But it is still odd to add
> additional properties only for groups and not tasks.

You raise a good point about hierarchy.  We'd like to use Gui's
hierarchy patches or similar functionality.  As you point out there is
currently an asymmetry between groups and tasks.  Tasks can be RT, but
groups cannot.  This complicates the hierarchy implementation.

How about adding a blkio.class and blkio.class_device interface to a
truly RT service class?  This class would be able to starve a BE class
(thus be more like the traditional RT/BE divide), and could be
implemented similarly to RT/BE cfqqs today.  This way groups and
queues could easily be scheduled as peers.

> This is the new paradigm (atleast to me). It introduces additional
> complextiy in a already complicated system. So it makes sense to make
> sure that there are more than 1 users of this functionality.
>
>>
>> > Why not have similar mechanism for cpu scheduler also then. This
>> > application first should be able to get cpu bandwidth in same predictable
>> > manner before it gets the disk bandwidth.
>>
>> Perhaps this is a good idea. �If the CPU scheduler folks like it, I'll
>> be happy to support that.
>
>>
>> > And I think your generation number patch should address this issue up
>> > to great extent. Isn't it? If a latency sensitive task is not using
>> > its fair quota, it will get a lower vdisktime and get to dispatch soon?
>>
>> It will get to dispatch as soon as the current task's timeslice
>> expires. �This could be a long time, depending on the number of other
>> tasks and groups on the system. �We'd like to provide a latency
>> guarantee that's dependent only on the behavior of the low-latency
>> application.
>
> What are your latency requirements? I believe maximum slice length can
> be 180ms in default settings. You can change base slice to 50 and that
> will make maximum slice length to 90. For default case it will be 50ms.
>
> So question is what workload it is which can not tolerate these latencies.
>
>>
>> > If that soon is not enough, then we could operate with reduce base slice
>> > length so that we allocate smaller slices to groups and get better IO
>> > latencies at the cost of total throughput.
>>
>> With the limited preemption patch, I can still achieve good throughput
>> for many tasks, as long as the low-latency task is "quiet" or when
>> there is no low-latency task on the system. �If I use very small
>> timeslices, then I always pay a throughput price, even when there is
>> no low-latency task on the system or that task isn't doing any IO.
>
> Ok, that's fine. So with-in BE class you are trying to define another
> type of groups that is "low latency". That's why I think this is third
> propety apart from ioprio and ioclass.
>
>>
>> >> > If RT starving BE is an issue, then it is an issue with plain cfq queue
>> >> > also. First we shall have to fix it there.
>> >> >
>> >> > This definition that a latency sensitive task get prioritized only
>> >> > till it is consuming its fair share and if task starts using more than
>> >> > fair share then CFQ automatically stops prioritizing it sounds little
>> >> > odd to me. If you are looking for predictability, then we lost it. We
>> >> > shall have to very well know that task is not eating more than its
>> >> > fair share before we can gurantee any kind of latencies to that task. And
>> >> > if we know that task is not hogging the disk, there is anyway no risk
>> >> > of it starving other groups/tasks completely.
>> >>
>> >> In a shared environment, we have to be a little bit defensive. �We
>> >> hope that a latency sensitive task is well characterized and won't
>> >> exceed its share of the disk, and that we haven't over-committed the
>> >> disk. �If the app does do more IO than expected, then we'd like them
>> >> to bear the burden. �We have a choice of two outcomes. �A single job
>> >> sometimes failing to achieve low disk latency when it's very busy. �Or
>> >> all jobs on a disk sometimes being very slow when another (unrelated)
>> >> job is very busy. �The first is easier to understand and debug.
>> >
>> > To me you are trying to come up with a new scheduling class which is
>> > not RT and you are trying to overload the meaning of RT for your use
>> > case and that's the issue I have.
>>
>> Can we come up with a better name? �I've used low-latency and
>> latency-sensitive in this email, and it's not too cumbersome.
>>
>> > Coming up with a new scheduling class is also not desirable as that
>> > will demand another service tree and we already have too many. Also
>> > it should probably be also done for task and not just group otherwise
>> > extending this concept to hierarchical setup will get complicated. Queues
>> > and groups will just not gel well.
>>
>> Is there a plan to provide RT class for groups in the hierarchical
>> future to allow full symmetry with RT tasks?

I'm still interested in the answer to this question.  If there's
currently no plan, is there at least an interest in seeing an
implementation?

>> > Or You could put latency sensitive applications in an RT class and
>> > then throttle them using blkio controller. That way you get good
>> > latencies as well as you don't starve other tasks.
>>
>> This is closer to the semantics offered by this patchset, but requires
>> debugging the complex interactions between two scheduling policies to
>> understand the resulting behavior.
>
> Can you explain that a bit more? throttling behavior is very clear that
> a group is allowed dispatch as long as it does not cross the limit.
> Otherwise bio is put in a queue and later submitted to underlying devices.
>
> So as long as latency sensitive task is with-in rate limit, it will get
> the latency you want. The moment it tries to do lot of IO, it will get
> throttled and practically becomes a oridinary BE task. I believe that's
> what your patchset does. latency sensitive gets priority only if it
> is consuming its fair share of disk. The only difference here is that
> defination of fair share is abosolute (specified interms of bps or iops)
> instead of it being dynamic depending on how many groups are doing IO.
>
> blktrace results show the throttle as well cfq logs in same file so
> correlating two policies is really easy. So I really don't think that
> understanding the resulting is behavior is hard. I will be happy to be
> proven wrong though.

Once we have a true-RT class implementation, I'll give it a shot.

>> > But I don't think overloading the meaning for RT or this specific use
>> > case is a good idea.
>>
>> I hear you loud and clear, but I disagree.
>
> You disagree with what? Changing the definition of RT is fine. ioclass RT
> means one thing for tasks and other thing for group, is it fine?

I read your comments so far as "I think this implementation for this
specific use case is a bad idea."  This is what I disagree with.  This
implementation nicely provides the needed behavior.

I'd like to provide the lowest possible latency to a single privileged
group per disk.  At the same time, I need to be able to ensure that
the privileged group isn't able to completely consume the throughput
on the disk.  It will likely share that disk with system daemons and
other "critical" functionality.  It's not important that those daemons
get the same latency guarantees, but they must be guaranteed some disk
time.

> If we really end up doing it, I think we shall have to define an
> additional group file say, blkio.preempt_fair_share. This will mean
> that this is a BE group but has additional property which allows it to
> preempt existing entity on service tree as long as it does not exceed
> it fair share. That way we don't have to define a new class or don't
> have to come up with additional service tree.

I think I hear you objecting more to the name RT.  And that if we had
this "limited preemption" functionality, it should be called by a
different name.

> But I would prefer that you seriously consider implementing RT group class
> and rate limit it with throttling logic. Because I believe it should solve
> your issue. Only question would be what should be upper limit and I think
> that will depend on type of storage your are using and what's your
> workload.
>
> Also if you can give a better example where this kind of latency matters,
> it will help to understand the problem better.

The general problem is that a distributed system is generally made up
of multiple machines, and that any significant operation against that
system will involved multiple machines.  The response to any external
request will likely be determined by the sum of the latencies of the
components.  So I want to reduce the latency on a single drive as much
as possible.

This thread is getting tangled.  I see a few options:

  a) Pursue the functionality in my original patchset with a different name.
  b) Build a true RT class for groups and try with blk-throttle.

You seem pretty unenthusiastic about a).  How do you feel about b)?

Chad

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Gui Jianfeng]

Chad Talbott wrote:
> On Wed, Mar 23, 2011 at 1:41 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Wed, Mar 23, 2011 at 01:10:32PM -0700, Chad Talbott wrote:
>>> On Tue, Mar 22, 2011 at 11:12 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>> On Tue, Mar 22, 2011 at 10:39:36AM -0700, Chad Talbott wrote:
>>>>> On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>>>> Why not just implement simply RT class groups and always allow an RT
>>>>>> group to preempt an BE class. Same thing we do for cfq queues. I will
>>>>>> not worry too much about a run away application consuming all the
>>>>>> bandwidth. If that's a concern we could use blkio controller to limit
>>>>>> the IO rate of a latency sensitive applicaiton to make sure it does
>>>>>> not starve BE applications.
>>>>> That is not quite the same semantics. �This limited preemption patch
>>>>> is still work-conserving. �If the RT task in the only task on the
>>>>> system with IO, it will be able to use all available disk time.
>>>>>
>>>> It is not same semantics but it feels like too much of special casing
>>>> for a single use case.
>>> How are you counting use cases?
>> This is the first time I have heard this requirement. So if 2-3 different
>> folks come up with similar concern, then I have idea an idea that this
>> is a generic need.
>>
>> You also have not explained what is the workload and what are the
>> acceptable latencies etc.
>>
>>>> You are using the generic notion of a RT thread (which in general means
>>>> that it gets all the cpu or all the disk ahead of BE task). But you have
>>>> changed the definition of RT for this special use case. And also now
>>>> group RT is different from queue RT definition.
>>> Perhaps the name RT has too much of a "this group should be able to
>>> starve all other groups" connotation. �Is there a better name? �Maybe
>>> latency sensitive?
>> I think what you are trying to achieve is that you want to define an
>> additional task and group property, say latency sensitive. This is
>> third property apart from ioclass and ioprio. To me you still want
>> the task/group to be BE class so that it shares the disk in a
>> proportional weight manner but this additional property will make sure
>> that task can preempt the non latency sensitive task/group.
>>
>> We can't do this additional property for group alone because once we
>> move to hierarhical setup and everything is entity (be it task or queue)
>> and then we need to decide whether one entity can preempt another
>> entity or not. By not definining this property for tasks, latency
>> sensitive group will always preempt a task on same tree. (May be
>> that's what you want for your use case). But it is still odd to add
>> additional properties only for groups and not tasks.
> 
> You raise a good point about hierarchy.  We'd like to use Gui's
> hierarchy patches or similar functionality.  As you point out there is
> currently an asymmetry between groups and tasks.  Tasks can be RT, but
> groups cannot.  This complicates the hierarchy implementation.
> 
> How about adding a blkio.class and blkio.class_device interface to a
> truly RT service class?  This class would be able to starve a BE class
> (thus be more like the traditional RT/BE divide), and could be
> implemented similarly to RT/BE cfqqs today.  This way groups and
> queues could easily be scheduled as peers.

For the current "cfq group hierarchy" implementation, I just put cfqg on
the "BE:SYNC" workload tree for the sake of simplicity. I think we need
to implement ioclass for cfq group for supporting *fully* hierarchical
scheduling.

Gui

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Vivek Goyal]

On Thu, Mar 24, 2011 at 02:47:54PM -0700, Chad Talbott wrote:
> On Wed, Mar 23, 2011 at 1:41 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Mar 23, 2011 at 01:10:32PM -0700, Chad Talbott wrote:
> >> On Tue, Mar 22, 2011 at 11:12 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Tue, Mar 22, 2011 at 10:39:36AM -0700, Chad Talbott wrote:
> >> >> On Tue, Mar 22, 2011 at 8:09 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> >> > Why not just implement simply RT class groups and always allow an RT
> >> >> > group to preempt an BE class. Same thing we do for cfq queues. I will
> >> >> > not worry too much about a run away application consuming all the
> >> >> > bandwidth. If that's a concern we could use blkio controller to limit
> >> >> > the IO rate of a latency sensitive applicaiton to make sure it does
> >> >> > not starve BE applications.
> >> >>
> >> >> That is not quite the same semantics. �This limited preemption patch
> >> >> is still work-conserving. �If the RT task in the only task on the
> >> >> system with IO, it will be able to use all available disk time.
> >> >>
> >> >
> >> > It is not same semantics but it feels like too much of special casing
> >> > for a single use case.
> >>
> >> How are you counting use cases?
> >
> > This is the first time I have heard this requirement. So if 2-3 different
> > folks come up with similar concern, then I have idea an idea that this
> > is a generic need.
> >
> > You also have not explained what is the workload and what are the
> > acceptable latencies etc.
> >
> >>
> >> > You are using the generic notion of a RT thread (which in general means
> >> > that it gets all the cpu or all the disk ahead of BE task). But you have
> >> > changed the definition of RT for this special use case. And also now
> >> > group RT is different from queue RT definition.
> >>
> >> Perhaps the name RT has too much of a "this group should be able to
> >> starve all other groups" connotation. �Is there a better name? �Maybe
> >> latency sensitive?

This is jut not RT name. It is also the using the term blkio.class. At
some point of time, it will be a good idea to be able to define ioclass
for group also (like taskss). So we can use blkio.class to define the
ioclass of the group.

> >
> > I think what you are trying to achieve is that you want to define an
> > additional task and group property, say latency sensitive. This is
> > third property apart from ioclass and ioprio. To me you still want
> > the task/group to be BE class so that it shares the disk in a
> > proportional weight manner but this additional property will make sure
> > that task can preempt the non latency sensitive task/group.
> >
> > We can't do this additional property for group alone because once we
> > move to hierarhical setup and everything is entity (be it task or queue)
> > and then we need to decide whether one entity can preempt another
> > entity or not. By not definining this property for tasks, latency
> > sensitive group will always preempt a task on same tree. (May be
> > that's what you want for your use case). But it is still odd to add
> > additional properties only for groups and not tasks.
> 
> You raise a good point about hierarchy.  We'd like to use Gui's
> hierarchy patches or similar functionality.  As you point out there is
> currently an asymmetry between groups and tasks.  Tasks can be RT, but
> groups cannot.  This complicates the hierarchy implementation.
> 
> How about adding a blkio.class and blkio.class_device interface to a
> truly RT service class?  This class would be able to starve a BE class
> (thus be more like the traditional RT/BE divide), and could be
> implemented similarly to RT/BE cfqqs today.  This way groups and
> queues could easily be scheduled as peers.

I think defining blkio.class and coming up with RT and IDLE (if needed)
groups separately makes sense to me. So in cfqd we can define a news
service tree where RT groups get queued up. Once hierarchical
implementation happens, RT queue and RT group entities will go on a
single tree.

[..]
> >> Is there a plan to provide RT class for groups in the hierarchical
> >> future to allow full symmetry with RT tasks?
> 
> I'm still interested in the answer to this question.  If there's
> currently no plan, is there at least an interest in seeing an
> implementation?

This is one of the possible expansions I have in mind. Just that I did
not plan to do it immediaely as I did not have any immediate need. So
feel free to implement the notion of RT cfq groups.

[..]
> I'd like to provide the lowest possible latency to a single privileged
> group per disk.  At the same time, I need to be able to ensure that
> the privileged group isn't able to completely consume the throughput
> on the disk.  It will likely share that disk with system daemons and
> other "critical" functionality.  It's not important that those daemons
> get the same latency guarantees, but they must be guaranteed some disk
> time.
> 
> > If we really end up doing it, I think we shall have to define an
> > additional group file say, blkio.preempt_fair_share. This will mean
> > that this is a BE group but has additional property which allows it to
> > preempt existing entity on service tree as long as it does not exceed
> > it fair share. That way we don't have to define a new class or don't
> > have to come up with additional service tree.
> 
> I think I hear you objecting more to the name RT.  And that if we had
> this "limited preemption" functionality, it should be called by a
> different name.
> 

I think, it was combination of few things.

- Use of RT namespace.
- usage of blkio.class namespace
- Assymetry between task and group properties

So 1 and 2 can be easily fixed by using a different name.
say blkio_preempt_fair_share. The only remaining issue will be  3rd piece.

> > But I would prefer that you seriously consider implementing RT group class
> > and rate limit it with throttling logic. Because I believe it should solve
> > your issue. Only question would be what should be upper limit and I think
> > that will depend on type of storage your are using and what's your
> > workload.
> >
> > Also if you can give a better example where this kind of latency matters,
> > it will help to understand the problem better.
> 
> The general problem is that a distributed system is generally made up
> of multiple machines, and that any significant operation against that
> system will involved multiple machines.  The response to any external
> request will likely be determined by the sum of the latencies of the
> components.  So I want to reduce the latency on a single drive as much
> as possible.
> 
> This thread is getting tangled.  I see a few options:
> 
>   a) Pursue the functionality in my original patchset with a different name.
>   b) Build a true RT class for groups and try with blk-throttle.
> 
> You seem pretty unenthusiastic about a).  How do you feel about b)?

IMHO, Using RT group with throttling avoids introducing asymmetry between
task and group attributes. So I will prefer that approch. Though it means
more code as we will be introducing RT groups but that might be useful
in general for something else too. (I am assuming that somebody makes
use of RT class for cfqq).

The one more down side of trying to use throttling is that one needs to
come up with absolute limit. So one shall have to know disk capacity
and if there are no BE tasks running then latency sensitive task will
be unnecessarily throttled (until and unless some management software
can monitor it and change limit dynamically).

So if you are worried about setting the absolute limit part, then I guess
I am fine with option a). But if you think that setting absolute limit
is not a problem, then option b) is preferred.

Thanks
Vivek

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Chad Talbott]

On Fri, Mar 25, 2011 at 2:32 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> You seem pretty unenthusiastic about a).  How do you feel about b)?
>
> IMHO, Using RT group with throttling avoids introducing asymmetry between
> task and group attributes. So I will prefer that approch. Though it means
> more code as we will be introducing RT groups but that might be useful
> in general for something else too. (I am assuming that somebody makes
> use of RT class for cfqq).
>
> The one more down side of trying to use throttling is that one needs to
> come up with absolute limit. So one shall have to know disk capacity
> and if there are no BE tasks running then latency sensitive task will
> be unnecessarily throttled (until and unless some management software
> can monitor it and change limit dynamically).
>
> So if you are worried about setting the absolute limit part, then I guess
> I am fine with option a). But if you think that setting absolute limit
> is not a problem, then option b) is preferred.

I prefer option a) - so much so that even with the older CFQ group
implementation we did work to merge the RT and BE service trees to
achieve that behavior.  But I see that using blkio.class is a poor
choice of interface name.  I will rename the interface and resubmit
the patch series (also with Gui's suggestion to keep the "_device"
suffix for consistency).

Also, we'll soon be working to adopt the hierarchy series, and we'll
likely revisit using an RT service tree there.  It's difficult to
justify introducing RT service tree before those patches arrive.

Thanks,
Chad

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Vivek Goyal]

On Fri, Mar 25, 2011 at 04:53:13PM -0700, Chad Talbott wrote:
> On Fri, Mar 25, 2011 at 2:32 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> You seem pretty unenthusiastic about a).  How do you feel about b)?
> >
> > IMHO, Using RT group with throttling avoids introducing asymmetry between
> > task and group attributes. So I will prefer that approch. Though it means
> > more code as we will be introducing RT groups but that might be useful
> > in general for something else too. (I am assuming that somebody makes
> > use of RT class for cfqq).
> >
> > The one more down side of trying to use throttling is that one needs to
> > come up with absolute limit. So one shall have to know disk capacity
> > and if there are no BE tasks running then latency sensitive task will
> > be unnecessarily throttled (until and unless some management software
> > can monitor it and change limit dynamically).
> >
> > So if you are worried about setting the absolute limit part, then I guess
> > I am fine with option a). But if you think that setting absolute limit
> > is not a problem, then option b) is preferred.
> 
> I prefer option a) - so much so that even with the older CFQ group
> implementation we did work to merge the RT and BE service trees to
> achieve that behavior.  But I see that using blkio.class is a poor
> choice of interface name.  I will rename the interface and resubmit
> the patch series (also with Gui's suggestion to keep the "_device"
> suffix for consistency).
> 
> Also, we'll soon be working to adopt the hierarchy series, and we'll
> likely revisit using an RT service tree there.  It's difficult to
> justify introducing RT service tree before those patches arrive.

So once you move to hierachicy series, will be you using RT class and
use throttling and abandon this functionality?

Thanks
Vivek

> 
> Thanks,
> Chad

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Vivek Goyal]

On Fri, Mar 25, 2011 at 04:53:13PM -0700, Chad Talbott wrote:
> On Fri, Mar 25, 2011 at 2:32 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> You seem pretty unenthusiastic about a).  How do you feel about b)?
> >
> > IMHO, Using RT group with throttling avoids introducing asymmetry between
> > task and group attributes. So I will prefer that approch. Though it means
> > more code as we will be introducing RT groups but that might be useful
> > in general for something else too. (I am assuming that somebody makes
> > use of RT class for cfqq).
> >
> > The one more down side of trying to use throttling is that one needs to
> > come up with absolute limit. So one shall have to know disk capacity
> > and if there are no BE tasks running then latency sensitive task will
> > be unnecessarily throttled (until and unless some management software
> > can monitor it and change limit dynamically).
> >
> > So if you are worried about setting the absolute limit part, then I guess
> > I am fine with option a). But if you think that setting absolute limit
> > is not a problem, then option b) is preferred.
> 
> I prefer option a) - so much so that even with the older CFQ group
> implementation we did work to merge the RT and BE service trees to
> achieve that behavior.  But I see that using blkio.class is a poor
> choice of interface name.  I will rename the interface and resubmit
> the patch series (also with Gui's suggestion to keep the "_device"
> suffix for consistency).

Do you need this feature to be global or per device or both?

Thanks
Vivek

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Chad Talbott]

On Mon, Mar 28, 2011 at 6:15 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Mar 25, 2011 at 04:53:13PM -0700, Chad Talbott wrote:
>> Also, we'll soon be working to adopt the hierarchy series, and we'll
>> likely revisit using an RT service tree there.  It's difficult to
>> justify introducing RT service tree before those patches arrive.
>
> So once you move to hierachicy series, will be you using RT class and
> use throttling and abandon this functionality?

I'll certainly investigate using RT class and throttling, but it's too
early to know.  It's a known change in behavior, so it will need
testing (as well as the user-space work to handle the different
interface).

Chad

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Chad Talbott]

On Mon, Mar 28, 2011 at 6:17 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Mar 25, 2011 at 04:53:13PM -0700, Chad Talbott wrote:
>> I prefer option a) - so much so that even with the older CFQ group
>> implementation we did work to merge the RT and BE service trees to
>> achieve that behavior.  But I see that using blkio.class is a poor
>> choice of interface name.  I will rename the interface and resubmit
>> the patch series (also with Gui's suggestion to keep the "_device"
>> suffix for consistency).
>
> Do you need this feature to be global or per device or both?

Per-device.

Chad

Re: [PATCH 0/3] cfq-iosched: Fair cross-group preemption

[Vivek Goyal]

On Mon, Mar 28, 2011 at 09:59:54AM -0700, Chad Talbott wrote:
> On Mon, Mar 28, 2011 at 6:15 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Mar 25, 2011 at 04:53:13PM -0700, Chad Talbott wrote:
> >> Also, we'll soon be working to adopt the hierarchy series, and we'll
> >> likely revisit using an RT service tree there.  It's difficult to
> >> justify introducing RT service tree before those patches arrive.
> >
> > So once you move to hierachicy series, will be you using RT class and
> > use throttling and abandon this functionality?
> 
> I'll certainly investigate using RT class and throttling, but it's too
> early to know.  It's a known change in behavior, so it will need
> testing (as well as the user-space work to handle the different
> interface).

So why not assess that now and then go for the one which meets your
requirements in long term. If we introduce this patch now, this 
introduces a new user visible interface and we can't get rid of it
later. If two releases down the line you implement RT group functionality
and move to that, then we have this dead interface not being used.

So why not implement RT groups internally (It should be a huge patch) now
and test and see how well is it working with throttling logic and whether
it meets your needs or not.

Thanks
Vivek