[PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

Hi,
here is the V2 of this patch series. Let me rephrase the description
of the series, in view of the fact that CFQ will be gone with legacy
block.

The current implementation of cgroups doesn't allow two or more
entities, e.g., I/O schedulers, to share the same files.  Thus, to
enable people to set group weights with BFQ, I resorted to making BFQ
create its own version of the same interface files used by CFQ (before
going away with legacy block), by prepending a bfq prefix.

Actually, no legacy code uses these different names, or is likely to
do so.  Having these two sets of names is simply a source of
confusion, as pointed out also, e.g., by Lennart Poettering (CCed
here), and acknowledged by Tejun [2].

In [1] we agreed on a solution that solves this problem, by actually
making it possible to share cgroups files.  Both writing to and
reading from a shared file trigger the appropriate operation for each
of the entities that share the file.  In particular, in case of
reading,
- if all entities produce the same output, the this common output is
 shown only once;
- if the outputs differ, then every per-entity output is shown,
 followed by the name of the entity that produced that output.

With this solution, legacy code that, e.g., sets group weights, just
works, regardless of the I/O scheduler actually implementing
proportional share.

But note that this extension is not restricted to only blkio/io.  The
general group interface now enables files to be shared among multiple
entities of any kind.

(I have also added a patch to fix some clerical errors in bfq doc,
which I found while making the latter consistent with the new
interface.)

CHANGES FROM V1:
- Removed patch that introduced a function to only find kernfs nodes,
  without increasing ref counters
- Changed commit messages and BFQ documentation, to comply with the
  fact that there won't be CFQ any longer

Thanks,
Paolo

Angelo Ruocco (5):
  cgroup: link cftypes of the same subsystem with the same name
  cgroup: add owner name to cftypes
  block, bfq: align min and default weights with the old cfq default
  cgroup: make all functions of all cftypes be invoked
  block, throttle: allow sharing cgroup statistic files

Paolo Valente (5):
  cgroup: add hook seq_show_cft with also the owning cftype as parameter
  block, cgroup: pass cftype to functions that need to use it
  block, bfq: use standard file names for the proportional-share policy
  doc, bfq-iosched: fix a few clerical errors
  doc, bfq-iosched: make it consistent with the new cgroup interface

 Documentation/block/bfq-iosched.txt |  34 ++---
 block/bfq-cgroup.c                  | 148 +++++++++++++-------
 block/bfq-iosched.h                 |   4 +-
 block/blk-cgroup.c                  |  22 +--
 block/blk-throttle.c                |  24 ++--
 include/linux/blk-cgroup.h          |  10 +-
 include/linux/cgroup-defs.h         |  14 +-
 include/linux/cgroup.h              |  13 ++
 kernel/cgroup/cgroup.c              | 262 +++++++++++++++++++++++++++++-------
 9 files changed, 390 insertions(+), 141 deletions(-)

--
2.16.1

[PATCH V2 01/10] cgroup: add hook seq_show_cft with also the owning cftype as parameter

[Paolo Valente]

The current implementation of the seq_show hook in the cftype struct
has only, as parameters, the seq_file to write to and the arguments
passed by the command line. Thus, the only way to retrieve the cftype
that owns an instance of such hook function is by using the accessor
in the seq_file itself.

But in a future scenario where the same file may be shared by multiple
cftypes, this accessor will point only to the first of the cftypes
linked to the seq_file. It will then be impossible to access the
cftype owning the seq_show function within the seq_show itself, unless
such cftype is the first one.

This commit adds an additional seq_show_cft hook that has as a formal
parameter also the cftype that owns the function.

Signed-off-by: Angelo Ruocco <angeloruocco90@gmail.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 include/linux/cgroup-defs.h |  3 ++-
 kernel/cgroup/cgroup.c      | 15 +++++++++------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 5e1694fe035b..7841db6e7fb3 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -543,8 +543,9 @@ struct cftype {
 	 */
 	s64 (*read_s64)(struct cgroup_subsys_state *css, struct cftype *cft);
 
-	/* generic seq_file read interface */
+	/* generic seq_file read interfaces*/
 	int (*seq_show)(struct seq_file *sf, void *v);
+	int (*seq_show_cft)(struct seq_file *sf, struct cftype *cft, void *v);
 
 	/* optional ops, implement all or none */
 	void *(*seq_start)(struct seq_file *sf, loff_t *ppos);
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 6aaf5dd5383b..9d0993dd68fe 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1418,7 +1418,8 @@ static umode_t cgroup_file_mode(const struct cftype *cft)
 {
 	umode_t mode = 0;
 
-	if (cft->read_u64 || cft->read_s64 || cft->seq_show)
+	if (cft->read_u64 || cft->read_s64 || cft->seq_show ||
+	    cft->seq_show_cft)
 		mode |= S_IRUGO;
 
 	if (cft->write_u64 || cft->write_s64 || cft->write) {
@@ -3519,17 +3520,19 @@ static int cgroup_seqfile_show(struct seq_file *m, void *arg)
 {
 	struct cftype *cft = seq_cft(m);
 	struct cgroup_subsys_state *css = seq_css(m);
+	int ret = 0;
 
 	if (cft->seq_show)
-		return cft->seq_show(m, arg);
-
-	if (cft->read_u64)
+		ret = cft->seq_show(m, arg);
+	else if (cft->seq_show_cft)
+		ret = cft->seq_show_cft(m, cft, arg);
+	else if (cft->read_u64)
 		seq_printf(m, "%llu\n", cft->read_u64(css, cft));
 	else if (cft->read_s64)
 		seq_printf(m, "%lld\n", cft->read_s64(css, cft));
 	else
-		return -EINVAL;
-	return 0;
+		ret = -EINVAL;
+	return ret;
 }
 
 static struct kernfs_ops cgroup_kf_single_ops = {
-- 
2.16.1

[PATCH V2 02/10] block, cgroup: pass cftype to functions that need to use it

[Paolo Valente]

Some seq_show functions need to access the cftype they belong to, for
retrieving the data to show. These functions get their cftype by using
the seq_cft accessor for the seq_file. This solution is no longer
viable in case a seq_file is shared among more than one cftype,
because the accessor always returns (only) the first of the cftypes
sharing the seq_file.

This commit enables these seq_show functions to be passed their
cftype, by replacing their prototype with that of the newly defined
seq_show_cft hook, and by invoking these functions through this new
hook.

Signed-off-by: Angelo Ruocco <angeloruocco90@gmail.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-cgroup.c         | 54 ++++++++++++++++++++++++----------------------
 block/blk-cgroup.c         | 22 ++++++++++++-------
 block/blk-throttle.c       |  8 +++----
 include/linux/blk-cgroup.h | 10 +++++----
 4 files changed, 52 insertions(+), 42 deletions(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index a7a1712632b0..038e418fa64f 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -918,17 +918,17 @@ static ssize_t bfq_io_set_weight(struct kernfs_open_file *of,
 }
 
 #ifdef CONFIG_DEBUG_BLK_CGROUP
-static int bfqg_print_stat(struct seq_file *sf, void *v)
+static int bfqg_print_stat(struct seq_file *sf, struct cftype *cft, void *v)
 {
 	blkcg_print_blkgs(sf, css_to_blkcg(seq_css(sf)), blkg_prfill_stat,
-			  &blkcg_policy_bfq, seq_cft(sf)->private, false);
+			  &blkcg_policy_bfq, cft->private, false);
 	return 0;
 }
 
-static int bfqg_print_rwstat(struct seq_file *sf, void *v)
+static int bfqg_print_rwstat(struct seq_file *sf, struct cftype *cft, void *v)
 {
 	blkcg_print_blkgs(sf, css_to_blkcg(seq_css(sf)), blkg_prfill_rwstat,
-			  &blkcg_policy_bfq, seq_cft(sf)->private, true);
+			  &blkcg_policy_bfq, cft->private, true);
 	return 0;
 }
 
@@ -949,19 +949,21 @@ static u64 bfqg_prfill_rwstat_recursive(struct seq_file *sf,
 	return __blkg_prfill_rwstat(sf, pd, &sum);
 }
 
-static int bfqg_print_stat_recursive(struct seq_file *sf, void *v)
+static int bfqg_print_stat_recursive(struct seq_file *sf, struct cftype *cft,
+				     void *v)
 {
 	blkcg_print_blkgs(sf, css_to_blkcg(seq_css(sf)),
 			  bfqg_prfill_stat_recursive, &blkcg_policy_bfq,
-			  seq_cft(sf)->private, false);
+			  cft->private, false);
 	return 0;
 }
 
-static int bfqg_print_rwstat_recursive(struct seq_file *sf, void *v)
+static int bfqg_print_rwstat_recursive(struct seq_file *sf, struct cftype *cft,
+				       void *v)
 {
 	blkcg_print_blkgs(sf, css_to_blkcg(seq_css(sf)),
 			  bfqg_prfill_rwstat_recursive, &blkcg_policy_bfq,
-			  seq_cft(sf)->private, true);
+			  cft->private, true);
 	return 0;
 }
 
@@ -1063,18 +1065,18 @@ struct cftype bfq_blkcg_legacy_files[] = {
 	{
 		.name = "bfq.io_service_bytes",
 		.private = (unsigned long)&blkcg_policy_bfq,
-		.seq_show = blkg_print_stat_bytes,
+		.seq_show_cft = blkg_print_stat_bytes,
 	},
 	{
 		.name = "bfq.io_serviced",
 		.private = (unsigned long)&blkcg_policy_bfq,
-		.seq_show = blkg_print_stat_ios,
+		.seq_show_cft = blkg_print_stat_ios,
 	},
 #ifdef CONFIG_DEBUG_BLK_CGROUP
 	{
 		.name = "bfq.time",
 		.private = offsetof(struct bfq_group, stats.time),
-		.seq_show = bfqg_print_stat,
+		.seq_show_cft = bfqg_print_stat,
 	},
 	{
 		.name = "bfq.sectors",
@@ -1083,22 +1085,22 @@ struct cftype bfq_blkcg_legacy_files[] = {
 	{
 		.name = "bfq.io_service_time",
 		.private = offsetof(struct bfq_group, stats.service_time),
-		.seq_show = bfqg_print_rwstat,
+		.seq_show_cft = bfqg_print_rwstat,
 	},
 	{
 		.name = "bfq.io_wait_time",
 		.private = offsetof(struct bfq_group, stats.wait_time),
-		.seq_show = bfqg_print_rwstat,
+		.seq_show_cft = bfqg_print_rwstat,
 	},
 	{
 		.name = "bfq.io_merged",
 		.private = offsetof(struct bfq_group, stats.merged),
-		.seq_show = bfqg_print_rwstat,
+		.seq_show_cft = bfqg_print_rwstat,
 	},
 	{
 		.name = "bfq.io_queued",
 		.private = offsetof(struct bfq_group, stats.queued),
-		.seq_show = bfqg_print_rwstat,
+		.seq_show_cft = bfqg_print_rwstat,
 	},
 #endif /* CONFIG_DEBUG_BLK_CGROUP */
 
@@ -1106,18 +1108,18 @@ struct cftype bfq_blkcg_legacy_files[] = {
 	{
 		.name = "bfq.io_service_bytes_recursive",
 		.private = (unsigned long)&blkcg_policy_bfq,
-		.seq_show = blkg_print_stat_bytes_recursive,
+		.seq_show_cft = blkg_print_stat_bytes_recursive,
 	},
 	{
 		.name = "bfq.io_serviced_recursive",
 		.private = (unsigned long)&blkcg_policy_bfq,
-		.seq_show = blkg_print_stat_ios_recursive,
+		.seq_show_cft = blkg_print_stat_ios_recursive,
 	},
 #ifdef CONFIG_DEBUG_BLK_CGROUP
 	{
 		.name = "bfq.time_recursive",
 		.private = offsetof(struct bfq_group, stats.time),
-		.seq_show = bfqg_print_stat_recursive,
+		.seq_show_cft = bfqg_print_stat_recursive,
 	},
 	{
 		.name = "bfq.sectors_recursive",
@@ -1126,22 +1128,22 @@ struct cftype bfq_blkcg_legacy_files[] = {
 	{
 		.name = "bfq.io_service_time_recursive",
 		.private = offsetof(struct bfq_group, stats.service_time),
-		.seq_show = bfqg_print_rwstat_recursive,
+		.seq_show_cft = bfqg_print_rwstat_recursive,
 	},
 	{
 		.name = "bfq.io_wait_time_recursive",
 		.private = offsetof(struct bfq_group, stats.wait_time),
-		.seq_show = bfqg_print_rwstat_recursive,
+		.seq_show_cft = bfqg_print_rwstat_recursive,
 	},
 	{
 		.name = "bfq.io_merged_recursive",
 		.private = offsetof(struct bfq_group, stats.merged),
-		.seq_show = bfqg_print_rwstat_recursive,
+		.seq_show_cft = bfqg_print_rwstat_recursive,
 	},
 	{
 		.name = "bfq.io_queued_recursive",
 		.private = offsetof(struct bfq_group, stats.queued),
-		.seq_show = bfqg_print_rwstat_recursive,
+		.seq_show_cft = bfqg_print_rwstat_recursive,
 	},
 	{
 		.name = "bfq.avg_queue_size",
@@ -1150,22 +1152,22 @@ struct cftype bfq_blkcg_legacy_files[] = {
 	{
 		.name = "bfq.group_wait_time",
 		.private = offsetof(struct bfq_group, stats.group_wait_time),
-		.seq_show = bfqg_print_stat,
+		.seq_show_cft = bfqg_print_stat,
 	},
 	{
 		.name = "bfq.idle_time",
 		.private = offsetof(struct bfq_group, stats.idle_time),
-		.seq_show = bfqg_print_stat,
+		.seq_show_cft = bfqg_print_stat,
 	},
 	{
 		.name = "bfq.empty_time",
 		.private = offsetof(struct bfq_group, stats.empty_time),
-		.seq_show = bfqg_print_stat,
+		.seq_show_cft = bfqg_print_stat,
 	},
 	{
 		.name = "bfq.dequeue",
 		.private = offsetof(struct bfq_group, stats.dequeue),
-		.seq_show = bfqg_print_stat,
+		.seq_show_cft = bfqg_print_stat,
 	},
 #endif	/* CONFIG_DEBUG_BLK_CGROUP */
 	{ }	/* terminate */
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 63d226a084cd..772d32bbecae 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -565,15 +565,16 @@ static u64 blkg_prfill_rwstat_field(struct seq_file *sf,
 /**
  * blkg_print_stat_bytes - seq_show callback for blkg->stat_bytes
  * @sf: seq_file to print to
+ * @cft: cftype holding the data
  * @v: unused
  *
  * To be used as cftype->seq_show to print blkg->stat_bytes.
  * cftype->private must be set to the blkcg_policy.
  */
-int blkg_print_stat_bytes(struct seq_file *sf, void *v)
+int blkg_print_stat_bytes(struct seq_file *sf, struct cftype *cft, void *v)
 {
 	blkcg_print_blkgs(sf, css_to_blkcg(seq_css(sf)),
-			  blkg_prfill_rwstat_field, (void *)seq_cft(sf)->private,
+			  blkg_prfill_rwstat_field, (void *)cft->private,
 			  offsetof(struct blkcg_gq, stat_bytes), true);
 	return 0;
 }
@@ -582,15 +583,16 @@ EXPORT_SYMBOL_GPL(blkg_print_stat_bytes);
 /**
  * blkg_print_stat_bytes - seq_show callback for blkg->stat_ios
  * @sf: seq_file to print to
+ * @cft: cftype holding the data
  * @v: unused
  *
  * To be used as cftype->seq_show to print blkg->stat_ios.  cftype->private
  * must be set to the blkcg_policy.
  */
-int blkg_print_stat_ios(struct seq_file *sf, void *v)
+int blkg_print_stat_ios(struct seq_file *sf, struct cftype *cft, void *v)
 {
 	blkcg_print_blkgs(sf, css_to_blkcg(seq_css(sf)),
-			  blkg_prfill_rwstat_field, (void *)seq_cft(sf)->private,
+			  blkg_prfill_rwstat_field, (void *)cft->private,
 			  offsetof(struct blkcg_gq, stat_ios), true);
 	return 0;
 }
@@ -608,13 +610,15 @@ static u64 blkg_prfill_rwstat_field_recursive(struct seq_file *sf,
 /**
  * blkg_print_stat_bytes_recursive - recursive version of blkg_print_stat_bytes
  * @sf: seq_file to print to
+ * @cft: cftype holding the data
  * @v: unused
  */
-int blkg_print_stat_bytes_recursive(struct seq_file *sf, void *v)
+int blkg_print_stat_bytes_recursive(struct seq_file *sf, struct cftype *cft,
+				    void *v)
 {
 	blkcg_print_blkgs(sf, css_to_blkcg(seq_css(sf)),
 			  blkg_prfill_rwstat_field_recursive,
-			  (void *)seq_cft(sf)->private,
+			  (void *)cft->private,
 			  offsetof(struct blkcg_gq, stat_bytes), true);
 	return 0;
 }
@@ -623,13 +627,15 @@ EXPORT_SYMBOL_GPL(blkg_print_stat_bytes_recursive);
 /**
  * blkg_print_stat_ios_recursive - recursive version of blkg_print_stat_ios
  * @sf: seq_file to print to
+ * @cft: cftype holding the data
  * @v: unused
  */
-int blkg_print_stat_ios_recursive(struct seq_file *sf, void *v)
+int blkg_print_stat_ios_recursive(struct seq_file *sf, struct cftype *cft,
+				  void *v)
 {
 	blkcg_print_blkgs(sf, css_to_blkcg(seq_css(sf)),
 			  blkg_prfill_rwstat_field_recursive,
-			  (void *)seq_cft(sf)->private,
+			  (void *)cft->private,
 			  offsetof(struct blkcg_gq, stat_ios), true);
 	return 0;
 }
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 8f0a104770ee..6bfdaac53b6f 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1493,22 +1493,22 @@ static struct cftype throtl_legacy_files[] = {
 	{
 		.name = "throttle.io_service_bytes",
 		.private = (unsigned long)&blkcg_policy_throtl,
-		.seq_show = blkg_print_stat_bytes,
+		.seq_show_cft = blkg_print_stat_bytes,
 	},
 	{
 		.name = "throttle.io_service_bytes_recursive",
 		.private = (unsigned long)&blkcg_policy_throtl,
-		.seq_show = blkg_print_stat_bytes_recursive,
+		.seq_show_cft = blkg_print_stat_bytes_recursive,
 	},
 	{
 		.name = "throttle.io_serviced",
 		.private = (unsigned long)&blkcg_policy_throtl,
-		.seq_show = blkg_print_stat_ios,
+		.seq_show_cft = blkg_print_stat_ios,
 	},
 	{
 		.name = "throttle.io_serviced_recursive",
 		.private = (unsigned long)&blkcg_policy_throtl,
-		.seq_show = blkg_print_stat_ios_recursive,
+		.seq_show_cft = blkg_print_stat_ios_recursive,
 	},
 	{ }	/* terminate */
 };
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index a9e2e2037129..770b59e0d35f 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -207,10 +207,12 @@ u64 __blkg_prfill_rwstat(struct seq_file *sf, struct blkg_policy_data *pd,
 u64 blkg_prfill_stat(struct seq_file *sf, struct blkg_policy_data *pd, int off);
 u64 blkg_prfill_rwstat(struct seq_file *sf, struct blkg_policy_data *pd,
 		       int off);
-int blkg_print_stat_bytes(struct seq_file *sf, void *v);
-int blkg_print_stat_ios(struct seq_file *sf, void *v);
-int blkg_print_stat_bytes_recursive(struct seq_file *sf, void *v);
-int blkg_print_stat_ios_recursive(struct seq_file *sf, void *v);
+int blkg_print_stat_bytes(struct seq_file *sf, struct cftype *cft, void *v);
+int blkg_print_stat_ios(struct seq_file *sf, struct cftype *cft, void *v);
+int blkg_print_stat_bytes_recursive(struct seq_file *sf, struct cftype *cft,
+				    void *v);
+int blkg_print_stat_ios_recursive(struct seq_file *sf, struct cftype *cft,
+				  void *v);
 
 u64 blkg_stat_recursive_sum(struct blkcg_gq *blkg,
 			    struct blkcg_policy *pol, int off);
-- 
2.16.1

[PATCH V2 03/10] cgroup: link cftypes of the same subsystem with the same name

[Paolo Valente]

From: Angelo Ruocco <angeloruocco90@gmail.com>

Two entities, of any kind, are not able to create a cgroup file with
the same name in the same folder: if an entity tries to create a file
that has the same name as a file created by another entity, the cgroup
core stops it, warns the user about the error, and then proceeds to
delete all the files created by the last entity.

However, in some specific situations, it may be useful for two or more
entities to use a common file, e.g., the I/O schedulers bfq and cfq
had the same "weight" attribute, that changed the behavior of the two
schedulers in a similar way.

This commit prepares the interface that allows two entities to share
files. It adds a flag CFTYPE_SHARE_FILE for cftypes, flag that allows
cftypes to be linked together if they are part of the same subsystem
and have the same name.

There is a limitation for a cftype that wants to share a file: it can't
have the hooks seq_start/next/stop. The reason is that there is no
consistent way to show portions of a file once multiple cftypes are
attached to it, and thus more than one seq_show() is invoked: there are
neither an univocal start point, nor univocal "next" and "stop"
operations.

Signed-off-by: Angelo Ruocco <angeloruocco90@gmail.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 include/linux/cgroup-defs.h |  9 ++++++
 kernel/cgroup/cgroup.c      | 78 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 7841db6e7fb3..d659763c7221 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -93,6 +93,8 @@ enum {
 	CFTYPE_NO_PREFIX	= (1 << 3),	/* (DON'T USE FOR NEW FILES) no subsys prefix */
 	CFTYPE_WORLD_WRITABLE	= (1 << 4),	/* (DON'T USE FOR NEW FILES) S_IWUGO */
 
+	CFTYPE_SHARES_FILE	= (1 << 5),	/* shares file w/ other cfts */
+
 	/* internal flags, do not use outside cgroup core proper */
 	__CFTYPE_ONLY_ON_DFL	= (1 << 16),	/* only on default hierarchy */
 	__CFTYPE_NOT_ON_DFL	= (1 << 17),	/* not on default hierarchy */
@@ -528,6 +530,13 @@ struct cftype {
 	 */
 	struct cgroup_subsys *ss;	/* NULL for cgroup core files */
 	struct list_head node;		/* anchored at ss->cfts */
+
+	/*
+	 * List of cftypes that are sharing the same file. It allows the hook
+	 * functions of the cftypes in the list to be called together.
+	 */
+	struct list_head share_node;
+
 	struct kernfs_ops *kf_ops;
 
 	int (*open)(struct kernfs_open_file *of);
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 9d0993dd68fe..61eafd69e2fd 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1549,11 +1549,15 @@ struct cgroup *cgroup_kn_lock_live(struct kernfs_node *kn, bool drain_offline)
 	return NULL;
 }
 
-static void cgroup_rm_file(struct cgroup *cgrp, const struct cftype *cft)
+static void cgroup_rm_file(struct cgroup *cgrp, struct cftype *cft)
 {
 	char name[CGROUP_FILE_NAME_MAX];
+	struct kernfs_node *kn = kernfs_find_and_get(cgrp->kn,
+			cgroup_file_name(cgrp, cft, name));
+	struct cftype *cfts = kn->priv;
 
 	lockdep_assert_held(&cgroup_mutex);
+	kernfs_put(kn);
 
 	if (cft->file_offset) {
 		struct cgroup_subsys_state *css = cgroup_css(cgrp, cft->ss);
@@ -1566,7 +1570,19 @@ static void cgroup_rm_file(struct cgroup *cgrp, const struct cftype *cft)
 		del_timer_sync(&cfile->notify_timer);
 	}
 
-	kernfs_remove_by_name(cgrp->kn, cgroup_file_name(cgrp, cft, name));
+	/* Delete the file only if it's used by one cftype */
+	if (list_empty(&cft->share_node) || atomic_read(&kn->count) == 1) {
+		kernfs_remove(kn);
+	} else {
+		/*
+		 * Update the "priv" pointer of the kernfs_node if the cftype
+		 * that first created the file is removed.
+		 */
+		if (cft == cfts)
+			kn->priv = list_next_entry(cft, share_node);
+
+		kernfs_put(kn);
+	}
 }
 
 /**
@@ -3437,6 +3453,7 @@ static int cgroup_file_open(struct kernfs_open_file *of)
 {
 	struct cftype *cft = of->kn->priv;
 
+
 	if (cft->open)
 		return cft->open(of);
 	return 0;
@@ -3585,6 +3602,22 @@ static int cgroup_add_file(struct cgroup_subsys_state *css, struct cgroup *cgrp,
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	key = &cft->lockdep_key;
 #endif
+
+	if (cft->flags & CFTYPE_SHARES_FILE) {
+		/* kn->count keeps track of how many cftypes share kn */
+		kn = kernfs_find_and_get(cgrp->kn,
+					 cgroup_file_name(cgrp, cft, name));
+		if (kn) {
+			struct cftype *cfts = kn->priv;
+
+			if (cfts->flags & CFTYPE_SHARES_FILE)
+				goto out_set_cfile;
+			else
+				kernfs_put(kn);
+
+		}
+	}
+
 	kn = __kernfs_create_file(cgrp->kn, cgroup_file_name(cgrp, cft, name),
 				  cgroup_file_mode(cft),
 				  GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
@@ -3599,6 +3632,7 @@ static int cgroup_add_file(struct cgroup_subsys_state *css, struct cgroup *cgrp,
 		return ret;
 	}
 
+out_set_cfile:
 	if (cft->file_offset) {
 		struct cgroup_file *cfile = (void *)css + cft->file_offset;
 
@@ -3696,11 +3730,46 @@ static void cgroup_exit_cftypes(struct cftype *cfts)
 		cft->kf_ops = NULL;
 		cft->ss = NULL;
 
+		list_del(&cft->share_node);
+
 		/* revert flags set by cgroup core while adding @cfts */
 		cft->flags &= ~(__CFTYPE_ONLY_ON_DFL | __CFTYPE_NOT_ON_DFL);
 	}
 }
 
+/*
+ * Link a cftype that wants to share a file to the list of cftypes that are
+ * using it.
+ *
+ * The conditions for a cftype to be put in an existing list of cftypes and
+ * thus start to share a file are:
+ *	- to have the flag CFTYPE_SHARES_FILE set;
+ *	- to have all flags coincide with the flags of the other cftypes in the
+ *	  list;
+ *	- to not have a seq_start hook: there is no consistent way to show
+ *	  portions of a file once multiple cftypes are attached to it, and thus
+ *	  more than one seq_show() is invoked.
+ *
+ * Once two or more cftypes are linked together, the file only points
+ * to the first of them.
+ */
+static void cgroup_link_cftype(struct cgroup_subsys *ss, struct cftype *cft)
+{
+	struct cftype *cfts;
+
+	list_for_each_entry(cfts, &ss->cfts, node) {
+		struct cftype *c;
+
+		for (c = cfts; c->name[0] != '\0'; c++)
+			if (c != cft && !(c->flags ^ cft->flags) &&
+			    !(c->seq_start || cft->seq_start) &&
+			    !strcmp(c->name, cft->name)) {
+				list_add(&cft->share_node, &c->share_node);
+				return;
+			}
+	}
+}
+
 static int cgroup_init_cftypes(struct cgroup_subsys *ss, struct cftype *cfts)
 {
 	struct cftype *cft;
@@ -3730,6 +3799,11 @@ static int cgroup_init_cftypes(struct cgroup_subsys *ss, struct cftype *cfts)
 
 		cft->kf_ops = kf_ops;
 		cft->ss = ss;
+
+		INIT_LIST_HEAD(&cft->share_node);
+
+		if (cft->flags & CFTYPE_SHARES_FILE)
+			cgroup_link_cftype(ss, cft);
 	}
 
 	return 0;
-- 
2.16.1

[PATCH V2 04/10] cgroup: add owner name to cftypes

[Paolo Valente]

From: Angelo Ruocco <angeloruocco90@gmail.com>

The piece of information "who created a certain cftype" is not stored
anywhere, thus a cftype is not able to know who is its owner.

This commit addresses this problem by adding a new field in the cftype
structure that enables the name of its owner to be explicitly set.

Signed-off-by: Angelo Ruocco <angeloruocco90@gmail.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 include/linux/cgroup-defs.h |  2 ++
 include/linux/cgroup.h      | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index d659763c7221..6e31f478c6e1 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -36,6 +36,7 @@ struct seq_file;
 #define MAX_CGROUP_TYPE_NAMELEN 32
 #define MAX_CGROUP_ROOT_NAMELEN 64
 #define MAX_CFTYPE_NAME		64
+#define MAX_OWNER_NAME		64
 
 /* define the enumeration of all cgroup subsystems */
 #define SUBSYS(_x) _x ## _cgrp_id,
@@ -505,6 +506,7 @@ struct cftype {
 	 * end of cftype array.
 	 */
 	char name[MAX_CFTYPE_NAME];
+	char owner_name[MAX_OWNER_NAME];
 	unsigned long private;
 
 	/*
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 9d12757a65b0..267153bd898a 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -291,6 +291,19 @@ void css_task_iter_end(struct css_task_iter *it);
 			;						\
 		else
 
+/**
+ * list_for_each_cft - walk circular list of cftypes linked together
+ * @cft: cftype from where to start
+ * @n: cftype used as a temporary storage
+ *
+ * A cftype pointed by a file may be part of a circular list of cftypes, this
+ * macro walks the circular list starting from any given cftype. Unlike the
+ * "list_for_each_entry" macro, the first element is included in the iteration.
+ */
+#define list_for_each_cft(cft, n)					\
+	for (n = NULL; cft != n; n = (n == NULL) ? cft : n,		\
+	     cft = list_next_entry(cft, share_node))
+
 /*
  * Inline functions.
  */
-- 
2.16.1

[PATCH V2 05/10] block, bfq: align min and default weights with the old cfq default

[Paolo Valente]

From: Angelo Ruocco <angeloruocco90@gmail.com>

bfq exposes a cgroup attribute, weight, with the same meaning as that
exposed by cfq.

This commit changes bfq default and min weights to match the ones set
by cfq (before legacy blk was removed).

Signed-off-by: Angelo Ruocco <angeloruocco90@gmail.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index 77651d817ecd..249d8128d3ee 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -22,13 +22,13 @@
 #define BFQ_IOPRIO_CLASSES	3
 #define BFQ_CL_IDLE_TIMEOUT	(HZ/5)
 
-#define BFQ_MIN_WEIGHT			1
+#define BFQ_MIN_WEIGHT			10
 #define BFQ_MAX_WEIGHT			1000
 #define BFQ_WEIGHT_CONVERSION_COEFF	10
 
 #define BFQ_DEFAULT_QUEUE_IOPRIO	4
 
-#define BFQ_WEIGHT_LEGACY_DFL	100
+#define BFQ_WEIGHT_LEGACY_DFL	500
 #define BFQ_DEFAULT_GRP_IOPRIO	0
 #define BFQ_DEFAULT_GRP_CLASS	IOPRIO_CLASS_BE
 
-- 
2.16.1

[PATCH V2 06/10] cgroup: make all functions of all cftypes be invoked

[Paolo Valente]

From: Angelo Ruocco <angeloruocco90@gmail.com>

When two or more entities (of any kind) share a file, their respective
cftypes are linked together. The allowed operations on those files
are: open, release, write and show, mapped to the functions defined in
the cftypes.

This commit makes the cgroup core invoke, whenever one of those
operations is requested, the respective function of all the cftypes
linked together.

Signed-off-by: Angelo Ruocco <angeloruocco90@gmail.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 kernel/cgroup/cgroup.c | 181 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 132 insertions(+), 49 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 61eafd69e2fd..9bf6b0b5a0ca 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3452,66 +3452,107 @@ static int cgroup_cpu_pressure_show(struct seq_file *seq, void *v)
 static int cgroup_file_open(struct kernfs_open_file *of)
 {
 	struct cftype *cft = of->kn->priv;
+	struct cftype *n;
+	int ret = 0;
 
+	list_for_each_cft(cft, n) {
+		if (cft->open)
+			ret = cft->open(of);
+		/*
+		 * If there has been an error with the open function of one of
+		 * the cft associated with the file, we call the release
+		 * function of all the cftype associated to cft whose open
+		 * function succeded.
+		 */
+		if (ret) {
+			struct cftype *c = of->kn->priv;
+			struct cftype *n;
+
+			list_for_each_cft(c, n) {
+				if (cft == c)
+					break;
+				if (c->release)
+					c->release(of);
+			}
+			break;
+		}
+	}
 
-	if (cft->open)
-		return cft->open(of);
-	return 0;
+	return ret;
 }
 
 static void cgroup_file_release(struct kernfs_open_file *of)
 {
 	struct cftype *cft = of->kn->priv;
+	struct cftype *n;
 
-	if (cft->release)
-		cft->release(of);
+	list_for_each_cft(cft, n)
+		if (cft->release)
+			cft->release(of);
 }
 
+/*
+ * Call all the write functions of the cftypes associated with the file.
+ *
+ * When a write fails, don't keep trying to write into the file via the write
+ * functions of the other cftypes associated with it.
+ */
 static ssize_t cgroup_file_write(struct kernfs_open_file *of, char *buf,
 				 size_t nbytes, loff_t off)
 {
 	struct cgroup_namespace *ns = current->nsproxy->cgroup_ns;
 	struct cgroup *cgrp = of->kn->parent->priv;
 	struct cftype *cft = of->kn->priv;
+	struct cftype *n;
 	struct cgroup_subsys_state *css;
-	int ret;
+	int ret = 0;
 
-	/*
-	 * If namespaces are delegation boundaries, disallow writes to
-	 * files in an non-init namespace root from inside the namespace
-	 * except for the files explicitly marked delegatable -
-	 * cgroup.procs and cgroup.subtree_control.
-	 */
-	if ((cgrp->root->flags & CGRP_ROOT_NS_DELEGATE) &&
-	    !(cft->flags & CFTYPE_NS_DELEGATABLE) &&
-	    ns != &init_cgroup_ns && ns->root_cset->dfl_cgrp == cgrp)
-		return -EPERM;
+	list_for_each_cft(cft, n) {
+		/*
+		 * If namespaces are delegation boundaries, disallow writes to
+		 * files in an non-init namespace root from inside the
+		 * namespace except for the files explicitly marked
+		 * delegatable - cgroup.procs and cgroup.subtree_control.
+		 */
+		if ((cgrp->root->flags & CGRP_ROOT_NS_DELEGATE) &&
+		    !(cft->flags & CFTYPE_NS_DELEGATABLE) &&
+		    ns != &init_cgroup_ns && ns->root_cset->dfl_cgrp == cgrp)
+			return -EPERM;
 
-	if (cft->write)
-		return cft->write(of, buf, nbytes, off);
+		if (cft->write) {
+			ret = cft->write(of, buf, nbytes, off);
 
-	/*
-	 * kernfs guarantees that a file isn't deleted with operations in
-	 * flight, which means that the matching css is and stays alive and
-	 * doesn't need to be pinned.  The RCU locking is not necessary
-	 * either.  It's just for the convenience of using cgroup_css().
-	 */
-	rcu_read_lock();
-	css = cgroup_css(cgrp, cft->ss);
-	rcu_read_unlock();
+			if (ret)
+				break;
+			continue;
+		}
 
-	if (cft->write_u64) {
-		unsigned long long v;
-		ret = kstrtoull(buf, 0, &v);
-		if (!ret)
-			ret = cft->write_u64(css, cft, v);
-	} else if (cft->write_s64) {
-		long long v;
-		ret = kstrtoll(buf, 0, &v);
-		if (!ret)
-			ret = cft->write_s64(css, cft, v);
-	} else {
-		ret = -EINVAL;
+		/*
+		 * kernfs guarantees that a file isn't deleted with operations
+		 * in flight, which means that the matching css is and stays
+		 * alive and doesn't need to be pinned.  The RCU locking is not
+		 * necessary either.  It's just for the convenience of using
+		 * cgroup_css().
+		 */
+		rcu_read_lock();
+		css = cgroup_css(cgrp, cft->ss);
+		rcu_read_unlock();
+
+		if (cft->write_u64) {
+			unsigned long long v;
+
+			ret = kstrtoull(buf, 0, &v);
+			if (!ret)
+				ret = cft->write_u64(css, cft, v);
+		} else if (cft->write_s64) {
+			long long v;
+
+			ret = kstrtoll(buf, 0, &v);
+			if (!ret)
+				ret = cft->write_s64(css, cft, v);
+		} else {
+			return -EINVAL;
+		}
 	}
 
 	return ret ?: nbytes;
@@ -3533,22 +3574,64 @@ static void cgroup_seqfile_stop(struct seq_file *seq, void *v)
 		seq_cft(seq)->seq_stop(seq, v);
 }
 
+/*
+ * A file shared by more cftypes may be showing different values. In that case
+ * call all the show functions and print the name of the owner that defined
+ * them.
+ */
 static int cgroup_seqfile_show(struct seq_file *m, void *arg)
 {
 	struct cftype *cft = seq_cft(m);
+	struct cftype *n;
 	struct cgroup_subsys_state *css = seq_css(m);
+	char *first_seqshow_str = NULL;
+	size_t first_str_size = 0;
+	size_t current_str_size = 0;
 	int ret = 0;
 
-	if (cft->seq_show)
-		ret = cft->seq_show(m, arg);
-	else if (cft->seq_show_cft)
-		ret = cft->seq_show_cft(m, cft, arg);
-	else if (cft->read_u64)
-		seq_printf(m, "%llu\n", cft->read_u64(css, cft));
-	else if (cft->read_s64)
-		seq_printf(m, "%lld\n", cft->read_s64(css, cft));
-	else
-		ret = -EINVAL;
+	list_for_each_cft(cft, n) {
+		if (cft->seq_show) {
+			ret = cft->seq_show(m, arg);
+			if (ret)
+				break;
+		} else if (cft->seq_show_cft) {
+			ret = cft->seq_show_cft(m, cft, arg);
+			if (ret)
+				break;
+		} else if (cft->read_u64) {
+			seq_printf(m, "%llu\n", cft->read_u64(css, cft));
+		} else if (cft->read_s64) {
+			seq_printf(m, "%lld\n", cft->read_s64(css, cft));
+		} else {
+			ret = -EINVAL;
+			break;
+		}
+		current_str_size = m->count - current_str_size;
+
+		if (first_seqshow_str == NULL) {
+			first_seqshow_str = kmalloc(m->size, GFP_KERNEL);
+			first_str_size = m->count;
+			strcpy(first_seqshow_str, m->buf);
+			first_str_size = m->count;
+		} else if (strcmp(first_seqshow_str,
+				  m->buf + m->count - current_str_size)) {
+			first_str_size = -1;
+		}
+
+		if (current_str_size) {
+			seq_printf(m, " - %s\n", cft->owner_name);
+			current_str_size = m->count;
+		}
+	}
+
+	/*
+	 * If all the cft->seqfile_show/read are equal, truncate the
+	 * output of the seqfile to the length of the first string.
+	 */
+	if (first_str_size != -1)
+		m->count = first_str_size;
+
+	kfree(first_seqshow_str);
 	return ret;
 }
 
-- 
2.16.1

[PATCH V2 07/10] block, bfq: use standard file names for the proportional-share policy

[Paolo Valente]

Some of the files exposed in a cgroup by bfq, for the proportional
share policy, have the same meaning as the files owned by cfq (before
legacy blk was removed).

The old implementation of the cgroup interface didn't allow different
entities to create cgroup files with the same name (in the same
subsystem). So, for bfq, we had to add the prefix "bfq" to the names
of its cgroup files.

This commit renames the cgroup files of the bfq scheduler as those
exposed by cfq, and makes bfq willing to share these files with any
other future policy.

Signed-off-by: Angelo Ruocco <angeloruocco90@gmail.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-cgroup.c | 94 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 25 deletions(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 038e418fa64f..0643147b2cbc 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -1055,50 +1055,67 @@ struct blkcg_policy blkcg_policy_bfq = {
 
 struct cftype bfq_blkcg_legacy_files[] = {
 	{
-		.name = "bfq.weight",
-		.flags = CFTYPE_NOT_ON_ROOT,
+		.name = "weight",
+		.owner_name = "bfq",
+		.flags = CFTYPE_NOT_ON_ROOT | CFTYPE_SHARES_FILE,
 		.seq_show = bfq_io_show_weight,
 		.write_u64 = bfq_io_set_weight_legacy,
 	},
 
 	/* statistics, covers only the tasks in the bfqg */
 	{
-		.name = "bfq.io_service_bytes",
+		.name = "io_service_bytes",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = (unsigned long)&blkcg_policy_bfq,
 		.seq_show_cft = blkg_print_stat_bytes,
 	},
 	{
-		.name = "bfq.io_serviced",
+		.name = "io_serviced",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = (unsigned long)&blkcg_policy_bfq,
 		.seq_show_cft = blkg_print_stat_ios,
 	},
 #ifdef CONFIG_DEBUG_BLK_CGROUP
 	{
-		.name = "bfq.time",
+		.name = "time",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.time),
 		.seq_show_cft = bfqg_print_stat,
 	},
 	{
-		.name = "bfq.sectors",
+		.name = "sectors",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.seq_show = bfqg_print_stat_sectors,
 	},
 	{
-		.name = "bfq.io_service_time",
+		.name = "io_service_time",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.service_time),
 		.seq_show_cft = bfqg_print_rwstat,
 	},
 	{
-		.name = "bfq.io_wait_time",
+		.name = "io_wait_time",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.wait_time),
 		.seq_show_cft = bfqg_print_rwstat,
 	},
 	{
-		.name = "bfq.io_merged",
+		.name = "io_merged",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.merged),
 		.seq_show_cft = bfqg_print_rwstat,
 	},
 	{
-		.name = "bfq.io_queued",
+		.name = "io_queued",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.queued),
 		.seq_show_cft = bfqg_print_rwstat,
 	},
@@ -1106,66 +1123,92 @@ struct cftype bfq_blkcg_legacy_files[] = {
 
 	/* the same statictics which cover the bfqg and its descendants */
 	{
-		.name = "bfq.io_service_bytes_recursive",
+		.name = "io_service_bytes_recursive",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = (unsigned long)&blkcg_policy_bfq,
 		.seq_show_cft = blkg_print_stat_bytes_recursive,
 	},
 	{
-		.name = "bfq.io_serviced_recursive",
+		.name = "io_serviced_recursive",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = (unsigned long)&blkcg_policy_bfq,
 		.seq_show_cft = blkg_print_stat_ios_recursive,
 	},
 #ifdef CONFIG_DEBUG_BLK_CGROUP
 	{
-		.name = "bfq.time_recursive",
+		.name = "time_recursive",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.time),
 		.seq_show_cft = bfqg_print_stat_recursive,
 	},
 	{
-		.name = "bfq.sectors_recursive",
+		.name = "sectors_recursive",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.seq_show = bfqg_print_stat_sectors_recursive,
 	},
 	{
-		.name = "bfq.io_service_time_recursive",
+		.name = "io_service_time_recursive",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.service_time),
 		.seq_show_cft = bfqg_print_rwstat_recursive,
 	},
 	{
-		.name = "bfq.io_wait_time_recursive",
+		.name = "io_wait_time_recursive",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.wait_time),
 		.seq_show_cft = bfqg_print_rwstat_recursive,
 	},
 	{
-		.name = "bfq.io_merged_recursive",
+		.name = "io_merged_recursive",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.merged),
 		.seq_show_cft = bfqg_print_rwstat_recursive,
 	},
 	{
-		.name = "bfq.io_queued_recursive",
+		.name = "io_queued_recursive",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.queued),
 		.seq_show_cft = bfqg_print_rwstat_recursive,
 	},
 	{
-		.name = "bfq.avg_queue_size",
+		.name = "avg_queue_size",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.seq_show = bfqg_print_avg_queue_size,
 	},
 	{
-		.name = "bfq.group_wait_time",
+		.name = "group_wait_time",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.group_wait_time),
 		.seq_show_cft = bfqg_print_stat,
 	},
 	{
-		.name = "bfq.idle_time",
+		.name = "idle_time",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.idle_time),
 		.seq_show_cft = bfqg_print_stat,
 	},
 	{
-		.name = "bfq.empty_time",
+		.name = "empty_time",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.empty_time),
 		.seq_show_cft = bfqg_print_stat,
 	},
 	{
-		.name = "bfq.dequeue",
+		.name = "dequeue",
+		.owner_name = "bfq",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = offsetof(struct bfq_group, stats.dequeue),
 		.seq_show_cft = bfqg_print_stat,
 	},
@@ -1175,8 +1218,9 @@ struct cftype bfq_blkcg_legacy_files[] = {
 
 struct cftype bfq_blkg_files[] = {
 	{
-		.name = "bfq.weight",
-		.flags = CFTYPE_NOT_ON_ROOT,
+		.name = "weight",
+		.owner_name = "bfq",
+		.flags = CFTYPE_NOT_ON_ROOT | CFTYPE_SHARES_FILE,
 		.seq_show = bfq_io_show_weight,
 		.write = bfq_io_set_weight,
 	},
-- 
2.16.1

[PATCH V2 08/10] block, throttle: allow sharing cgroup statistic files

[Paolo Valente]

From: Angelo Ruocco <angeloruocco90@gmail.com>

Some of the cgroup files defined in the throttle policy have the same
meaning as those defined in the proportional share policy.

This commit uses the new file sharing interface in cgroup to share
these files.

Signed-off-by: Angelo Ruocco <angeloruocco90@gmail.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/blk-throttle.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 6bfdaac53b6f..95825448c031 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1491,22 +1491,30 @@ static struct cftype throtl_legacy_files[] = {
 		.write = tg_set_conf_uint,
 	},
 	{
-		.name = "throttle.io_service_bytes",
+		.name = "io_service_bytes",
+		.owner_name = "throttle",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = (unsigned long)&blkcg_policy_throtl,
 		.seq_show_cft = blkg_print_stat_bytes,
 	},
 	{
-		.name = "throttle.io_service_bytes_recursive",
+		.name = "io_service_bytes_recursive",
+		.owner_name = "throttle",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = (unsigned long)&blkcg_policy_throtl,
 		.seq_show_cft = blkg_print_stat_bytes_recursive,
 	},
 	{
-		.name = "throttle.io_serviced",
+		.name = "io_serviced",
+		.owner_name = "throttle",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = (unsigned long)&blkcg_policy_throtl,
 		.seq_show_cft = blkg_print_stat_ios,
 	},
 	{
-		.name = "throttle.io_serviced_recursive",
+		.name = "io_serviced_recursive",
+		.owner_name = "throttle",
+		.flags = CFTYPE_SHARES_FILE,
 		.private = (unsigned long)&blkcg_policy_throtl,
 		.seq_show_cft = blkg_print_stat_ios_recursive,
 	},
-- 
2.16.1

[PATCH V2 09/10] doc, bfq-iosched: fix a few clerical errors

[Paolo Valente]

This commit fixes a few clerical errors in
Documentation/block/bfq-iosched.txt.

Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 Documentation/block/bfq-iosched.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/block/bfq-iosched.txt b/Documentation/block/bfq-iosched.txt
index 8d8d8f06cab2..6d7dd5ab8554 100644
--- a/Documentation/block/bfq-iosched.txt
+++ b/Documentation/block/bfq-iosched.txt
@@ -42,7 +42,7 @@ sustainable throughputs, on the same systems as above:
 
 BFQ works for multi-queue devices too.
 
-The table of contents follow. Impatients can just jump to Section 3.
+The table of contents follows. Impatients can just jump to Section 3.
 
 CONTENTS
 
@@ -51,7 +51,7 @@ CONTENTS
  1-2 Server systems
 2. How does BFQ work?
 3. What are BFQ's tunables and how to properly configure BFQ?
-4. BFQ group scheduling
+4. Group scheduling with BFQ
  4-1 Service guarantees provided
  4-2 Interface
 
@@ -294,7 +294,7 @@ too.
 per-process ioprio and weight
 -----------------------------
 
-Unless the cgroups interface is used (see "4. BFQ group scheduling"),
+Unless the cgroups interface is used (see "4. Group scheduling with BFQ"),
 weights can be assigned to processes only indirectly, through I/O
 priorities, and according to the relation:
 weight = (IOPRIO_BE_NR - ioprio) * 10.
-- 
2.16.1

[PATCH V2 10/10] doc, bfq-iosched: make it consistent with the new cgroup interface

[Paolo Valente]

BFQ now shares interface files with CFQ, for the proportional-share
policy. Make documentation consistent with that.

Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 Documentation/block/bfq-iosched.txt | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/Documentation/block/bfq-iosched.txt b/Documentation/block/bfq-iosched.txt
index 6d7dd5ab8554..3d7ef138ce6a 100644
--- a/Documentation/block/bfq-iosched.txt
+++ b/Documentation/block/bfq-iosched.txt
@@ -508,12 +508,14 @@ process.
 To get proportional sharing of bandwidth with BFQ for a given device,
 BFQ must of course be the active scheduler for that device.
 
-Within each group directory, the names of the files associated with
-BFQ-specific cgroup parameters and stats begin with the "bfq."
-prefix. So, with cgroups-v1 or cgroups-v2, the full prefix for
-BFQ-specific files is "blkio.bfq." or "io.bfq." For example, the group
-parameter to set the weight of a group with BFQ is blkio.bfq.weight
-or io.bfq.weight.
+BFQ uses the standard interface files of the proportional-share
+policy, previously used by CFQ. If one such file is read/written, then
+the operation associated with the file is performed by BFQ for each
+device where BFQ is the active scheduler. In addition, BFQ is
+configured so as to share interface files with other entities (if some
+of these files does happen to be shared, then its associated operation
+is performed also by any of the other entities that is using the
+file).
 
 As for cgroups-v1 (blkio controller), the exact set of stat files
 created, and kept up-to-date by bfq, depends on whether
@@ -521,13 +523,13 @@ CONFIG_DEBUG_BLK_CGROUP is set. If it is set, then bfq creates all
 the stat files documented in
 Documentation/cgroup-v1/blkio-controller.txt. If, instead,
 CONFIG_DEBUG_BLK_CGROUP is not set, then bfq creates only the files
-blkio.bfq.io_service_bytes
-blkio.bfq.io_service_bytes_recursive
-blkio.bfq.io_serviced
-blkio.bfq.io_serviced_recursive
+blkio.io_service_bytes
+blkio.io_service_bytes_recursive
+blkio.io_serviced
+blkio.io_serviced_recursive
 
 The value of CONFIG_DEBUG_BLK_CGROUP greatly influences the maximum
-throughput sustainable with bfq, because updating the blkio.bfq.*
+throughput sustainable with BFQ, because updating the blkio.*
 stats is rather costly, especially for some of the stats enabled by
 CONFIG_DEBUG_BLK_CGROUP.
 
@@ -536,8 +538,8 @@ Parameters to set
 
 For each group, there is only the following parameter to set.
 
-weight (namely blkio.bfq.weight or io.bfq-weight): the weight of the
-group inside its parent. Available values: 1..10000 (default 100). The
+weight (namely blkio.weight or io.weight): the weight of the group
+inside its parent. Available values: 1..10000 (default 100). The
 linear mapping between ioprio and weights, described at the beginning
 of the tunable section, is still valid, but all weights higher than
 IOPRIO_BE_NR*10 are mapped to ioprio 0.
-- 
2.16.1

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Tejun Heo]

Hello, Paolo.

On Mon, Nov 19, 2018 at 11:34:14AM +0100, Paolo Valente wrote:
> - if all entities produce the same output, the this common output is
>  shown only once;
> - if the outputs differ, then every per-entity output is shown,
>  followed by the name of the entity that produced that output.

So, this doesn't make sense to me.  One set of numbers is meaningful,
the other not and the user doesn't have a way to tell which one is.
It makes no sense to present both numbers.

Thanks.

-- 
tejun

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

> Il giorno 20 nov 2018, alle ore 17:28, Tejun Heo <tj@kernel.org> ha scritto:
> 
> Hello, Paolo.
> 
> On Mon, Nov 19, 2018 at 11:34:14AM +0100, Paolo Valente wrote:
>> - if all entities produce the same output, the this common output is
>> shown only once;
>> - if the outputs differ, then every per-entity output is shown,
>> followed by the name of the entity that produced that output.
> 
> So, this doesn't make sense to me.  One set of numbers is meaningful,
> the other not and the user doesn't have a way to tell which one is.
> It makes no sense to present both numbers.
> 

I do agree that these numbers may confuse.  Before discussing how to
do this better, let me tell you why we are showing them.

In the first place, the need for a diversified output showed up in the
following case.  Given a group using the blkio/io controller, and two
drives
- one with legacy block and cfq
- one with blk-mq and bfq
there will be different statistics for each scheduler, for the same
interface files.

Then we understood that exactly the same happens with throttling, in
case the latter is activated on different devices w.r.t. bfq.

In addition, the same may happen, in the near future, with the
bandwidth controller Josef is working on.  If the controller can be
configured per device, as with throttling, then statistics may differ,
for the same interface files, between bfq, throttling and that
controller.

More general examples could be made considering that this extension is
for the generic cgroup interface.

Of course, suggestions for a clearer way to show these numbers are
more than welcome!  Maybe involved device identifiers can be somehow
gathered by the entities providing these numbers, and then shown?  In
this respect, consider that, even without this extension, one still
has the fundamental problem of not knowing to which devices numbers
apply (unless I'm missing something else).

Thanks,
Paolo

> Thanks.
> 
> -- 
> tejun

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

> Il giorno 20 nov 2018, alle ore 17:50, Paolo Valente <paolo.valente@linaro.org> ha scritto:
> 
> 
> 
>> Il giorno 20 nov 2018, alle ore 17:28, Tejun Heo <tj@kernel.org> ha scritto:
>> 
>> Hello, Paolo.
>> 
>> On Mon, Nov 19, 2018 at 11:34:14AM +0100, Paolo Valente wrote:
>>> - if all entities produce the same output, the this common output is
>>> shown only once;
>>> - if the outputs differ, then every per-entity output is shown,
>>> followed by the name of the entity that produced that output.
>> 
>> So, this doesn't make sense to me.  One set of numbers is meaningful,
>> the other not and the user doesn't have a way to tell which one is.
>> It makes no sense to present both numbers.
>> 
> 
> I do agree that these numbers may confuse.  Before discussing how to
> do this better, let me tell you why we are showing them.
> 
> In the first place, the need for a diversified output showed up in the
> following case.  Given a group using the blkio/io controller, and two
> drives
> - one with legacy block and cfq
> - one with blk-mq and bfq
> there will be different statistics for each scheduler, for the same
> interface files.
> 
> Then we understood that exactly the same happens with throttling, in
> case the latter is activated on different devices w.r.t. bfq.
> 
> In addition, the same may happen, in the near future, with the
> bandwidth controller Josef is working on.  If the controller can be
> configured per device, as with throttling, then statistics may differ,
> for the same interface files, between bfq, throttling and that
> controller.
> 
> More general examples could be made considering that this extension is
> for the generic cgroup interface.
> 
> Of course, suggestions for a clearer way to show these numbers are
> more than welcome!  Maybe involved device identifiers can be somehow
> gathered by the entities providing these numbers, and then shown?  In
> this respect, consider that, even without this extension, one still
> has the fundamental problem of not knowing to which devices numbers
> apply (unless I'm missing something else).
> 

Hi Tejun,
have you had time to look into this?  Any improvement to this
interface is ok for us. We are only interested in finally solving this
interface issue, as, for what concerns us directly, it has been
preventing legacy code to use bfq for years.

Thanks,
Paolo



> Thanks,
> Paolo
> 
>> Thanks.
>> 
>> -- 
>> tejun
> 
> -- 
> You received this message because you are subscribed to the Google Groups "bfq-iosched" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to bfq-iosched+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Tejun Heo]

Hello, Paolo.

On Fri, Nov 30, 2018 at 07:23:24PM +0100, Paolo Valente wrote:
> > Then we understood that exactly the same happens with throttling, in
> > case the latter is activated on different devices w.r.t. bfq.
> > 
> > In addition, the same may happen, in the near future, with the
> > bandwidth controller Josef is working on.  If the controller can be
> > configured per device, as with throttling, then statistics may differ,
> > for the same interface files, between bfq, throttling and that
> > controller.

So, regardless of how all these are implemented, what's presented to
user should be consistent and clear.  There's no other way around it.
Only what's relevant should be visible to userspace.

> have you had time to look into this?  Any improvement to this
> interface is ok for us. We are only interested in finally solving this
> interface issue, as, for what concerns us directly, it has been
> preventing legacy code to use bfq for years.

Unfortunately, I don't have any implementation proposal, but we can't
show things this way to userspace.

Thanks.

-- 
tejun

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

> Il giorno 30 nov 2018, alle ore 19:42, Tejun Heo <tj@kernel.org> ha scritto:
> 
> Hello, Paolo.
> 
> On Fri, Nov 30, 2018 at 07:23:24PM +0100, Paolo Valente wrote:
>>> Then we understood that exactly the same happens with throttling, in
>>> case the latter is activated on different devices w.r.t. bfq.
>>> 
>>> In addition, the same may happen, in the near future, with the
>>> bandwidth controller Josef is working on.  If the controller can be
>>> configured per device, as with throttling, then statistics may differ,
>>> for the same interface files, between bfq, throttling and that
>>> controller.
> 
> So, regardless of how all these are implemented, what's presented to
> user should be consistent and clear.  There's no other way around it.
> Only what's relevant should be visible to userspace.
> 
>> have you had time to look into this?  Any improvement to this
>> interface is ok for us. We are only interested in finally solving this
>> interface issue, as, for what concerns us directly, it has been
>> preventing legacy code to use bfq for years.
> 
> Unfortunately, I don't have any implementation proposal, but we can't
> show things this way to userspace.
> 

Well, this is not very helpful to move forward :)

Let me try to repeat the problem, to try to help you help us unblock
the situation.

If we have multiple entities attached to the same interface output
file, you don't find it clear that each entity shows the number it
wants to show.  But you have no idea either of how that differentiated
information should be shown.  Is this the situation, or is the problem
somewhere 'above' this level?

If the problem is as I described it, here are some proposal attempts:
1) Do you want file sharing to be allowed only if all entities will
output the same number?  (this seems excessive, but maybe it makes
sense)
2) Do you want only one number to be shown, equal to the sum of the
numbers of each entity?  (in some cases, this may make sense)
3) Do you prefer an average?
4) Do you have any other idea, even if just germinal?

Looking forward to your feedback,
Paolo 


> Thanks.
> 
> -- 
> tejun
> 
> -- 
> You received this message because you are subscribed to the Google Groups "bfq-iosched" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to bfq-iosched+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Angelo Ruocco]

2018-11-30 19:53 GMT+01:00, Paolo Valente <paolo.valente@linaro.org>:
>
>
>> Il giorno 30 nov 2018, alle ore 19:42, Tejun Heo <tj@kernel.org> ha
>> scritto:
>>
>> Hello, Paolo.
>>
>> On Fri, Nov 30, 2018 at 07:23:24PM +0100, Paolo Valente wrote:
>>>> Then we understood that exactly the same happens with throttling, in
>>>> case the latter is activated on different devices w.r.t. bfq.
>>>>
>>>> In addition, the same may happen, in the near future, with the
>>>> bandwidth controller Josef is working on.  If the controller can be
>>>> configured per device, as with throttling, then statistics may differ,
>>>> for the same interface files, between bfq, throttling and that
>>>> controller.
>>
>> So, regardless of how all these are implemented, what's presented to
>> user should be consistent and clear.  There's no other way around it.
>> Only what's relevant should be visible to userspace.
>>
>>> have you had time to look into this?  Any improvement to this
>>> interface is ok for us. We are only interested in finally solving this
>>> interface issue, as, for what concerns us directly, it has been
>>> preventing legacy code to use bfq for years.
>>
>> Unfortunately, I don't have any implementation proposal, but we can't
>> show things this way to userspace.
>>
>
> Well, this is not very helpful to move forward :)
>
> Let me try to repeat the problem, to try to help you help us unblock
> the situation.
>
> If we have multiple entities attached to the same interface output
> file, you don't find it clear that each entity shows the number it
> wants to show.  But you have no idea either of how that differentiated
> information should be shown.  Is this the situation, or is the problem
> somewhere 'above' this level?
>
> If the problem is as I described it, here are some proposal attempts:
> 1) Do you want file sharing to be allowed only if all entities will
> output the same number?  (this seems excessive, but maybe it makes
> sense)
> 2) Do you want only one number to be shown, equal to the sum of the
> numbers of each entity?  (in some cases, this may make sense)
> 3) Do you prefer an average?
> 4) Do you have any other idea, even if just germinal?

To further add to what Paolo said and better expose the problem, I'd like to
say that all those proposals have issues.
If we only allow "same output" cftypes to be shared then we lose all the
flexibility of this solution, and we need a way for an entity to know other
entities internal variables beforehand, which sounds at least very hard, and
maybe is not even an acceptable thing to do.
To put the average, sum or some other mathematical function in the file only
makes sense for certain cftypes, so also doesn't sound like a good idea. In
fact I can think of scenarios where only seeing the different values of the
entities makes sense for a user.

I understand that the problem is inconsistency: having a file that behaves
differently depending on the situation, and the only way to prevent this I can
think of is to *always* show the entity owner of a certain file (or part of the
output), even when the output would be the same among entities or when the
file is not currently shared but could be. Can this be an acceptable solution?

Angelo

>
> Looking forward to your feedback,
> Paolo
>
>
>> Thanks.
>>
>> --
>> tejun
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "bfq-iosched" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to bfq-iosched+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>
>

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

[RESENDING BECAUSE BOUNCED]

> Il giorno 10 dic 2018, alle ore 14:45, Angelo Ruocco <angelo.ruocco.90@gmail.com> ha scritto:
> 
> 2018-11-30 19:53 GMT+01:00, Paolo Valente <paolo.valente@linaro.org>:
>> 
>> 
>>> Il giorno 30 nov 2018, alle ore 19:42, Tejun Heo <tj@kernel.org> ha
>>> scritto:
>>> 
>>> Hello, Paolo.
>>> 
>>> On Fri, Nov 30, 2018 at 07:23:24PM +0100, Paolo Valente wrote:
>>>>> Then we understood that exactly the same happens with throttling, in
>>>>> case the latter is activated on different devices w.r.t. bfq.
>>>>> 
>>>>> In addition, the same may happen, in the near future, with the
>>>>> bandwidth controller Josef is working on.  If the controller can be
>>>>> configured per device, as with throttling, then statistics may differ,
>>>>> for the same interface files, between bfq, throttling and that
>>>>> controller.
>>> 
>>> So, regardless of how all these are implemented, what's presented to
>>> user should be consistent and clear.  There's no other way around it.
>>> Only what's relevant should be visible to userspace.
>>> 
>>>> have you had time to look into this?  Any improvement to this
>>>> interface is ok for us. We are only interested in finally solving this
>>>> interface issue, as, for what concerns us directly, it has been
>>>> preventing legacy code to use bfq for years.
>>> 
>>> Unfortunately, I don't have any implementation proposal, but we can't
>>> show things this way to userspace.
>>> 
>> 
>> Well, this is not very helpful to move forward :)
>> 
>> Let me try to repeat the problem, to try to help you help us unblock
>> the situation.
>> 
>> If we have multiple entities attached to the same interface output
>> file, you don't find it clear that each entity shows the number it
>> wants to show.  But you have no idea either of how that differentiated
>> information should be shown.  Is this the situation, or is the problem
>> somewhere 'above' this level?
>> 
>> If the problem is as I described it, here are some proposal attempts:
>> 1) Do you want file sharing to be allowed only if all entities will
>> output the same number?  (this seems excessive, but maybe it makes
>> sense)
>> 2) Do you want only one number to be shown, equal to the sum of the
>> numbers of each entity?  (in some cases, this may make sense)
>> 3) Do you prefer an average?
>> 4) Do you have any other idea, even if just germinal?
> 
> To further add to what Paolo said and better expose the problem, I'd like to
> say that all those proposals have issues.
> If we only allow "same output" cftypes to be shared then we lose all the
> flexibility of this solution, and we need a way for an entity to know other
> entities internal variables beforehand, which sounds at least very hard, and
> maybe is not even an acceptable thing to do.
> To put the average, sum or some other mathematical function in the file only
> makes sense for certain cftypes, so also doesn't sound like a good idea. In
> fact I can think of scenarios where only seeing the different values of the
> entities makes sense for a user.
> 
> I understand that the problem is inconsistency: having a file that behaves
> differently depending on the situation, and the only way to prevent this I can
> think of is to *always* show the entity owner of a certain file (or part of the
> output), even when the output would be the same among entities or when the
> file is not currently shared but could be. Can this be an acceptable solution?
> 
> Angelo
> 

Hi Jens, all,
let me push for this interface to be fixed too.  If we don't fix it in
some way, then from 4.21 we well end up with a ridiculous paradox: the
proportional share policy (weights) will of course be available, but
unusable in practice.  In fact, as Lennart--and not only Lennart--can
confirm, no piece of code uses bfq.weight to set weights, or will do
it.

A trivial solution would be to throw away all our work to fix this
issue by extending the interface, and just let bfq use the former cfq
names.  But then the same mess will happen as, e.g., Josef will
propose his proportional-share controller.

Before making this solution, we proposed it and waited for it to be
approved several months ago, so I hope that Tejun concern can be
addressed somehow.

If Tejun cannot see any solution to his concern, then can we just
switch to this extension, considering that
- for non-shared names the interface is *identical* to the current
  one;
- by using this new interface, and getting feedback we could
  understand how to better handle Tejun's concern?

A lot of systems do use weights, and people don't even know that these
systems don't work correctly in blk-mq.  And they won't work correctly
in any available configuration from 4.21, if we don't fix this problem.

Thanks.
Paolo

>> 
>> Looking forward to your feedback,
>> Paolo
>> 
>> 
>>> Thanks.
>>> 
>>> --
>>> tejun
>>> 
>>> --
>>> You received this message because you are subscribed to the Google Groups
>>> "bfq-iosched" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an
>>> email to bfq-iosched+unsubscribe@googlegroups.com.
>>> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Tejun Heo]

Hello, Paolo.

On Tue, Dec 18, 2018 at 08:48:10AM +0100, Paolo Valente wrote:
> If Tejun cannot see any solution to his concern, then can we just
> switch to this extension, considering that
> - for non-shared names the interface is *identical* to the current
>   one;
> - by using this new interface, and getting feedback we could
>   understand how to better handle Tejun's concern?
> A lot of systems do use weights, and people don't even know that these
> systems don't work correctly in blk-mq.  And they won't work correctly
> in any available configuration from 4.21, if we don't fix this problem.

So, when seen from userland, how it should behave isn't vague or
complicated.  For a given device and policy type, there can be only
one implementation active.  It doesn't make sense to have two weight
mechanisms active on one device, right?  So, the interface should only
present what makes sense to the user for both configuration knobs and
statistics, and that'd be a hard requirement because we don't want to
present confusing spurious information to userspace.

There seemd to have been significant misunderstandings as to what the
requirements are when this was discussed way back, so idk what the
good path forward is at this point.  Just keep the current names?

Thanks.

-- 
tejun

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

> Il giorno 18 dic 2018, alle ore 17:41, Tejun Heo <tj@kernel.org> ha scritto:
> 
> Hello, Paolo.
> 
> On Tue, Dec 18, 2018 at 08:48:10AM +0100, Paolo Valente wrote:
>> If Tejun cannot see any solution to his concern, then can we just
>> switch to this extension, considering that
>> - for non-shared names the interface is *identical* to the current
>>  one;
>> - by using this new interface, and getting feedback we could
>>  understand how to better handle Tejun's concern?
>> A lot of systems do use weights, and people don't even know that these
>> systems don't work correctly in blk-mq.  And they won't work correctly
>> in any available configuration from 4.21, if we don't fix this problem.
> 
> So, when seen from userland, how it should behave isn't vague or
> complicated.  For a given device and policy type, there can be only
> one implementation active.

Yes, but the problem is the opposite. You may have
- two different policies, with the same interface parameter, 
- one active on one device
- the other one active on another device

In that case, statistics from one policy necessarily differ from
statistics from the other policy.

In this respect, in a system with more than one drive it already
happens that the same policy is active on different devices.  When
printing a statistics interface file for the policy, the output will
be a list of separate statistics, with a bunch of statistics *for
each* drive (plus a grand total in some cases).

So, our proposal simply extends this scheme in the most natural way:
if, now, also two or more policies share the same statistics file,
then the output will be a list of separate statistics, one for each
policy.  The statistics for each policy will be tagged with the policy
name, and will have the same identical form as above.  It seems the
most natural hierarchical extension of the same scheme.

At any rate, if you don't like it, just tell us how you prefer it
done.  Do you prefer the sharing of statistics file to be simply
forbidden?  (If this can be done.) I think such an incomplete solution
would preserve part of the current mess; but, if this allows us to
exit from this impasse, then it is ok for me.

*Any* feasible option is ok for me. Just pick one.

>  It doesn't make sense to have two weight
> mechanisms active on one device, right?

(Un)fortunately, the problem are not weights.  There won't be two
weights for two policies expiring a weight parameter.  The problems
concerns statistics.  See above.


>  So, the interface should only
> present what makes sense to the user for both configuration knobs and
> statistics, and that'd be a hard requirement because we don't want to
> present confusing spurious information to userspace.
> 
> There seemd to have been significant misunderstandings as to what the
> requirements are when this was discussed way back, so idk what the
> good path forward is at this point.  Just keep the current names?
> 

I don't clearly understand how "just picking the current names" is a
way forward, but if we do not make this extension, in a way or the
other, then two policies will simply not be allowed to share the same
interface files.  And we will be still at the starting point.

Thanks,
Paolo

> Thanks.
> 
> -- 
> tejun

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

> Il giorno 18 dic 2018, alle ore 18:22, Paolo Valente <paolo.valente@linaro.org> ha scritto:
> 
> 
> 
>> Il giorno 18 dic 2018, alle ore 17:41, Tejun Heo <tj@kernel.org> ha scritto:
>> 
>> Hello, Paolo.
>> 
>> On Tue, Dec 18, 2018 at 08:48:10AM +0100, Paolo Valente wrote:
>>> If Tejun cannot see any solution to his concern, then can we just
>>> switch to this extension, considering that
>>> - for non-shared names the interface is *identical* to the current
>>> one;
>>> - by using this new interface, and getting feedback we could
>>> understand how to better handle Tejun's concern?
>>> A lot of systems do use weights, and people don't even know that these
>>> systems don't work correctly in blk-mq.  And they won't work correctly
>>> in any available configuration from 4.21, if we don't fix this problem.
>> 
>> So, when seen from userland, how it should behave isn't vague or
>> complicated.  For a given device and policy type, there can be only
>> one implementation active.
> 
> Yes, but the problem is the opposite. You may have
> - two different policies, with the same interface parameter, 
> - one active on one device
> - the other one active on another device
> 
> In that case, statistics from one policy necessarily differ from
> statistics from the other policy.
> 
> In this respect, in a system with more than one drive it already
> happens that the same policy is active on different devices.  When
> printing a statistics interface file for the policy, the output will
> be a list of separate statistics, with a bunch of statistics *for
> each* drive (plus a grand total in some cases).
> 
> So, our proposal simply extends this scheme in the most natural way:
> if, now, also two or more policies share the same statistics file,
> then the output will be a list of separate statistics, one for each
> policy.  The statistics for each policy will be tagged with the policy
> name, and will have the same identical form as above.  It seems the
> most natural hierarchical extension of the same scheme.
> 
> At any rate, if you don't like it, just tell us how you prefer it
> done.  Do you prefer the sharing of statistics file to be simply
> forbidden?  (If this can be done.) I think such an incomplete solution
> would preserve part of the current mess; but, if this allows us to
> exit from this impasse, then it is ok for me.
> 
> *Any* feasible option is ok for me. Just pick one.
> 
>> It doesn't make sense to have two weight
>> mechanisms active on one device, right?
> 
> (Un)fortunately, the problem are not weights.  There won't be two
> weights for two policies expiring a weight parameter.  The problems

s/expiring/sharing sorry

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

> Il giorno 18 dic 2018, alle ore 18:22, Paolo Valente <paolo.valente@linaro.org> ha scritto:
> 
> 
> 
>> Il giorno 18 dic 2018, alle ore 17:41, Tejun Heo <tj@kernel.org> ha scritto:
>> 
>> Hello, Paolo.
>> 
>> On Tue, Dec 18, 2018 at 08:48:10AM +0100, Paolo Valente wrote:
>>> If Tejun cannot see any solution to his concern, then can we just
>>> switch to this extension, considering that
>>> - for non-shared names the interface is *identical* to the current
>>> one;
>>> - by using this new interface, and getting feedback we could
>>> understand how to better handle Tejun's concern?
>>> A lot of systems do use weights, and people don't even know that these
>>> systems don't work correctly in blk-mq.  And they won't work correctly
>>> in any available configuration from 4.21, if we don't fix this problem.
>> 
>> So, when seen from userland, how it should behave isn't vague or
>> complicated.  For a given device and policy type, there can be only
>> one implementation active.
> 
> Yes, but the problem is the opposite. You may have
> - two different policies, with the same interface parameter, 
> - one active on one device
> - the other one active on another device
> 
> In that case, statistics from one policy necessarily differ from
> statistics from the other policy.
> 
> In this respect, in a system with more than one drive it already
> happens that the same policy is active on different devices.  When
> printing a statistics interface file for the policy, the output will
> be a list of separate statistics, with a bunch of statistics *for
> each* drive (plus a grand total in some cases).
> 
> So, our proposal simply extends this scheme in the most natural way:
> if, now, also two or more policies share the same statistics file,
> then the output will be a list of separate statistics, one for each
> policy.  The statistics for each policy will be tagged with the policy
> name, and will have the same identical form as above.  It seems the
> most natural hierarchical extension of the same scheme.
> 

Maybe my generic description didn't highlight how plain are.

If you print, e.g., io_serviced with the current interface, you get

--------------------------

8:0 Read 4291168
8:0 Write 2181577
8:0 Sync 5897755
8:0 Async 574990
8:0 Total 6472745
Total 6472745

--------------------------

With the new, interface, you get *the same output*, if only one policy
is attached to this interface file.  In, instead
- two policies share
the the file, because one is active on a device and one on another
device
- these policies are named, e.g., bfq and pol2
then you get (device number and statistics invented):

--------------------------

bfq:
8:0 Read 4291168
8:0 Write 2181577
8:0 Sync 5897755
8:0 Async 574990
8:0 Total 6472745
Total 6472745

pol2:
16:0 Read 238768
16:0 Write 323123
16:0 Sync 43243
16:0 Async 432432
16:0 Total 412435
Total 4341244

--------------------------

So you see the per-device statistics as before, without the problem
of inventing a new set of names for every new policy that has the same
interface files of an existing policy.

Tejun, let's try to converge, to whatever solution you prefer.


4.21 is coming ...  and the legacy proportional share interface will
be gone with cfq.  This will break legacy code using the
proportional-share interface on top of bfq.  This code may just fail
when trying to use interface files that don't exist any longer.

Thanks,
Paolo

> At any rate, if you don't like it, just tell us how you prefer it
> done.  Do you prefer the sharing of statistics file to be simply
> forbidden?  (If this can be done.) I think such an incomplete solution
> would preserve part of the current mess; but, if this allows us to
> exit from this impasse, then it is ok for me.
> 
> *Any* feasible option is ok for me. Just pick one.
> 
>> It doesn't make sense to have two weight
>> mechanisms active on one device, right?
> 
> (Un)fortunately, the problem are not weights.  There won't be two
> weights for two policies expiring a weight parameter.  The problems
> concerns statistics.  See above.
> 
> 
>> So, the interface should only
>> present what makes sense to the user for both configuration knobs and
>> statistics, and that'd be a hard requirement because we don't want to
>> present confusing spurious information to userspace.
>> 
>> There seemd to have been significant misunderstandings as to what the
>> requirements are when this was discussed way back, so idk what the
>> good path forward is at this point.  Just keep the current names?
>> 
> 
> I don't clearly understand how "just picking the current names" is a
> way forward, but if we do not make this extension, in a way or the
> other, then two policies will simply not be allowed to share the same
> interface files.  And we will be still at the starting point.
> 
> Thanks,
> Paolo
> 
>> Thanks.
>> 
>> -- 
>> tejun
> 
> -- 
> You received this message because you are subscribed to the Google Groups "bfq-iosched" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to bfq-iosched+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Tejun Heo]

Hello, Paolo.

On Sun, Dec 23, 2018 at 12:00:14PM +0100, Paolo Valente wrote:
> 4.21 is coming ...  and the legacy proportional share interface will
> be gone with cfq.  This will break legacy code using the
> proportional-share interface on top of bfq.  This code may just fail
> when trying to use interface files that don't exist any longer.

Sounds like inheriting .cfq namespace would be the easiest.  Would
that work?

Thanks.

-- 
tejun

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

> Il giorno 28 dic 2018, alle ore 00:41, Tejun Heo <tj@kernel.org> ha scritto:
> 
> Hello, Paolo.
> 
> On Sun, Dec 23, 2018 at 12:00:14PM +0100, Paolo Valente wrote:
>> 4.21 is coming ...  and the legacy proportional share interface will
>> be gone with cfq.  This will break legacy code using the
>> proportional-share interface on top of bfq.  This code may just fail
>> when trying to use interface files that don't exist any longer.
> 
> Sounds like inheriting .cfq namespace would be the easiest.  Would
> that work?
> 

For bfq, yes, but what will, e.g., Josef do when he adds his new
proportional-share implementation?  Will he add a new set of names not
used by any legacy piece of code?

What's the benefit of throwing away months of work, on which we agreed
before starting it, and that solves a problem already acknowledged by
interested parties?

Thanks,
Paolo

> Thanks.
> 
> -- 
> tejun

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Tejun Heo]

Hello, Paolo.

On Sun, Dec 30, 2018 at 11:25:25AM +0100, Paolo Valente wrote:
> What's the benefit of throwing away months of work, on which we agreed
> before starting it, and that solves a problem already acknowledged by
> interested parties?

Showing multiple conflicting numbers definitely isn't anything which
is agreed upon.

Thanks.

-- 
tejun

Re: [PATCH V2 00/10] unify the interface of the proportional-share policy in blkio/io

[Paolo Valente]

> Il giorno 2 gen 2019, alle ore 17:03, Tejun Heo <tj@kernel.org> ha scritto:
> 
> Hello, Paolo.
> 
> On Sun, Dec 30, 2018 at 11:25:25AM +0100, Paolo Valente wrote:
>> What's the benefit of throwing away months of work, on which we agreed
>> before starting it, and that solves a problem already acknowledged by
>> interested parties?
> 
> Showing multiple conflicting numbers definitely isn't anything which
> is agreed upon.
> 

Sorry, of course you din't realize that sharing interface files had
this consequence, otherwise you'd have protested beforehand.

The problem is that this consequence seems unavoidable: if two
policies have different numbers to convey, through a shared interface
file, then they must be allowed to write their different numbers.  To
me, this doesn't sound like a problem.

The only other natural option is no unification, unless you have a
third way.

What do you prefer, or propose?

Thanks,
Paolo

> Thanks.
> 
> -- 
> tejun