(Revision starting from v2 since v1 was used off-list)
Hi Paolo and others,
This adds to BFQ the missing per-device weight interfaces:
blkio.bfq.weight_device on legacy and io.bfq.weight on unified. The
implementation pretty closely resembles what we had in CFQ and the parsing code
is basically reused.
Tests
=====
Using two cgroups and three block devices, having weights setup as:
Cgroup test1 test2
============================================
default 100 500
sda 500 100
sdb default default
sdc 200 200
cgroup v1 runs
--------------
sda.test1.out: READ: bw=913MiB/s
sda.test2.out: READ: bw=183MiB/s
sdb.test1.out: READ: bw=213MiB/s
sdb.test2.out: READ: bw=1054MiB/s
sdc.test1.out: READ: bw=650MiB/s
sdc.test2.out: READ: bw=650MiB/s
cgroup v2 runs
--------------
sda.test1.out: READ: bw=915MiB/s
sda.test2.out: READ: bw=184MiB/s
sdb.test1.out: READ: bw=216MiB/s
sdb.test2.out: READ: bw=1069MiB/s
sdc.test1.out: READ: bw=621MiB/s
sdc.test2.out: READ: bw=622MiB/s
Fam Zheng (3):
bfq: Fix the missing barrier in __bfq_entity_update_weight_prio
bfq: Extract bfq_group_set_weight from bfq_io_set_weight_legacy
bfq: Add per-device weight
block/bfq-cgroup.c | 151 +++++++++++++++++++++++++++++++++++++++-------------
block/bfq-iosched.h | 3 ++
block/bfq-wf2q.c | 2 +
3 files changed, 119 insertions(+), 37 deletions(-)
--
2.11.0
The comment of bfq_group_set_weight says the reading of prio_changed
should happen before the reading of weight, but a memory barrier is
missing here. Add it now, to match the smp_wmb() there.
Signed-off-by: Fam Zheng <[email protected]>
---
block/bfq-wf2q.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index c9ba225081ce..05f0bf4a1144 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -744,6 +744,8 @@ __bfq_entity_update_weight_prio(struct bfq_service_tree *old_st,
}
#endif
+ /* Matches the smp_wmb() in bfq_group_set_weight. */
+ smp_rmb();
old_st->wsum -= entity->weight;
if (entity->new_weight != entity->orig_weight) {
--
2.11.0
Signed-off-by: Fam Zheng <[email protected]>
---
block/bfq-cgroup.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-------
block/bfq-iosched.h | 3 ++
2 files changed, 87 insertions(+), 11 deletions(-)
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 28e5a9241237..de4fd8b725aa 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -904,7 +904,7 @@ void bfq_end_wr_async(struct bfq_data *bfqd)
bfq_end_wr_async_queues(bfqd, bfqd->root_group);
}
-static int bfq_io_show_weight(struct seq_file *sf, void *v)
+static int bfq_io_show_weight_legacy(struct seq_file *sf, void *v)
{
struct blkcg *blkcg = css_to_blkcg(seq_css(sf));
struct bfq_group_data *bfqgd = blkcg_to_bfqgd(blkcg);
@@ -918,8 +918,32 @@ static int bfq_io_show_weight(struct seq_file *sf, void *v)
return 0;
}
-static void bfq_group_set_weight(struct bfq_group *bfqg, u64 weight)
+static u64 bfqg_prfill_weight_device(struct seq_file *sf,
+ struct blkg_policy_data *pd, int off)
+{
+ struct bfq_group *bfqg = pd_to_bfqg(pd);
+
+ if (!bfqg->entity.dev_weight)
+ return 0;
+ return __blkg_prfill_u64(sf, pd, bfqg->entity.dev_weight);
+}
+
+static int bfq_io_show_weight(struct seq_file *sf, void *v)
+{
+ struct blkcg *blkcg = css_to_blkcg(seq_css(sf));
+ struct bfq_group_data *bfqgd = blkcg_to_bfqgd(blkcg);
+
+ seq_printf(sf, "default %u\n", bfqgd->weight);
+ blkcg_print_blkgs(sf, blkcg, bfqg_prfill_weight_device,
+ &blkcg_policy_bfq, 0, false);
+ return 0;
+}
+
+static void bfq_group_set_weight(struct bfq_group *bfqg, u64 weight, u64 dev_weight)
{
+ weight = dev_weight ?: weight;
+
+ bfqg->entity.dev_weight = dev_weight;
/*
* Setting the prio_changed flag of the entity
* to 1 with new_weight == weight would re-set
@@ -967,28 +991,71 @@ static int bfq_io_set_weight_legacy(struct cgroup_subsys_state *css,
struct bfq_group *bfqg = blkg_to_bfqg(blkg);
if (bfqg)
- bfq_group_set_weight(bfqg, val);
+ bfq_group_set_weight(bfqg, val, 0);
}
spin_unlock_irq(&blkcg->lock);
return ret;
}
-static ssize_t bfq_io_set_weight(struct kernfs_open_file *of,
- char *buf, size_t nbytes,
- loff_t off)
+static ssize_t bfq_io_set_device_weight(struct kernfs_open_file *of,
+ char *buf, size_t nbytes,
+ loff_t off)
{
- u64 weight;
- /* First unsigned long found in the file is used */
- int ret = kstrtoull(strim(buf), 0, &weight);
+ int ret;
+ struct blkg_conf_ctx ctx;
+ struct blkcg *blkcg = css_to_blkcg(of_css(of));
+ struct bfq_group *bfqg;
+ u64 v;
+ ret = blkg_conf_prep(blkcg, &blkcg_policy_bfq, buf, &ctx);
if (ret)
return ret;
- ret = bfq_io_set_weight_legacy(of_css(of), NULL, weight);
+ if (sscanf(ctx.body, "%llu", &v) == 1) {
+ /* require "default" on dfl */
+ ret = -ERANGE;
+ if (!v)
+ goto out;
+ } else if (!strcmp(strim(ctx.body), "default")) {
+ v = 0;
+ } else {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ bfqg = blkg_to_bfqg(ctx.blkg);
+
+ ret = -ERANGE;
+ if (!v || (v >= BFQ_MIN_WEIGHT && v <= BFQ_MAX_WEIGHT)) {
+ bfq_group_set_weight(bfqg, bfqg->entity.weight, v);
+ ret = 0;
+ }
+out:
+ blkg_conf_finish(&ctx);
return ret ?: nbytes;
}
+static ssize_t bfq_io_set_weight(struct kernfs_open_file *of,
+ char *buf, size_t nbytes,
+ loff_t off)
+{
+ char *endp;
+ int ret;
+ u64 v;
+
+ buf = strim(buf);
+
+ /* "WEIGHT" or "default WEIGHT" sets the default weight */
+ v = simple_strtoull(buf, &endp, 0);
+ if (*endp == '\0' || sscanf(buf, "default %llu", &v) == 1) {
+ ret = bfq_io_set_weight_legacy(of_css(of), NULL, v);
+ return ret ?: nbytes;
+ }
+
+ return bfq_io_set_device_weight(of, buf, nbytes, off);
+}
+
#ifdef CONFIG_BFQ_CGROUP_DEBUG
static int bfqg_print_stat(struct seq_file *sf, void *v)
{
@@ -1145,9 +1212,15 @@ struct cftype bfq_blkcg_legacy_files[] = {
{
.name = "bfq.weight",
.flags = CFTYPE_NOT_ON_ROOT,
- .seq_show = bfq_io_show_weight,
+ .seq_show = bfq_io_show_weight_legacy,
.write_u64 = bfq_io_set_weight_legacy,
},
+ {
+ .name = "bfq.weight_device",
+ .flags = CFTYPE_NOT_ON_ROOT,
+ .seq_show = bfq_io_show_weight,
+ .write = bfq_io_set_weight,
+ },
/* statistics, covers only the tasks in the bfqg */
{
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index e80adf822bbe..5d1a519640f6 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -168,6 +168,9 @@ struct bfq_entity {
/* budget, used also to calculate F_i: F_i = S_i + @budget / @weight */
int budget;
+ /* device weight, if non-zero, it overrides the default weight of
+ * bfq_group_data */
+ int dev_weight;
/* weight of the queue */
int weight;
/* next weight if a change is in progress */
--
2.11.0
This function will be useful when we update weight from the soon-coming
per-device interface.
Signed-off-by: Fam Zheng <[email protected]>
---
block/bfq-cgroup.c | 60 +++++++++++++++++++++++++++++-------------------------
1 file changed, 32 insertions(+), 28 deletions(-)
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 0f6cd688924f..28e5a9241237 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -918,6 +918,36 @@ static int bfq_io_show_weight(struct seq_file *sf, void *v)
return 0;
}
+static void bfq_group_set_weight(struct bfq_group *bfqg, u64 weight)
+{
+ /*
+ * Setting the prio_changed flag of the entity
+ * to 1 with new_weight == weight would re-set
+ * the value of the weight to its ioprio mapping.
+ * Set the flag only if necessary.
+ */
+ if ((unsigned short)weight != bfqg->entity.new_weight) {
+ bfqg->entity.new_weight = (unsigned short)weight;
+ /*
+ * Make sure that the above new value has been
+ * stored in bfqg->entity.new_weight before
+ * setting the prio_changed flag. In fact,
+ * this flag may be read asynchronously (in
+ * critical sections protected by a different
+ * lock than that held here), and finding this
+ * flag set may cause the execution of the code
+ * for updating parameters whose value may
+ * depend also on bfqg->entity.new_weight (in
+ * __bfq_entity_update_weight_prio).
+ * This barrier makes sure that the new value
+ * of bfqg->entity.new_weight is correctly
+ * seen in that code.
+ */
+ smp_wmb();
+ bfqg->entity.prio_changed = 1;
+ }
+}
+
static int bfq_io_set_weight_legacy(struct cgroup_subsys_state *css,
struct cftype *cftype,
u64 val)
@@ -936,34 +966,8 @@ static int bfq_io_set_weight_legacy(struct cgroup_subsys_state *css,
hlist_for_each_entry(blkg, &blkcg->blkg_list, blkcg_node) {
struct bfq_group *bfqg = blkg_to_bfqg(blkg);
- if (!bfqg)
- continue;
- /*
- * Setting the prio_changed flag of the entity
- * to 1 with new_weight == weight would re-set
- * the value of the weight to its ioprio mapping.
- * Set the flag only if necessary.
- */
- if ((unsigned short)val != bfqg->entity.new_weight) {
- bfqg->entity.new_weight = (unsigned short)val;
- /*
- * Make sure that the above new value has been
- * stored in bfqg->entity.new_weight before
- * setting the prio_changed flag. In fact,
- * this flag may be read asynchronously (in
- * critical sections protected by a different
- * lock than that held here), and finding this
- * flag set may cause the execution of the code
- * for updating parameters whose value may
- * depend also on bfqg->entity.new_weight (in
- * __bfq_entity_update_weight_prio).
- * This barrier makes sure that the new value
- * of bfqg->entity.new_weight is correctly
- * seen in that code.
- */
- smp_wmb();
- bfqg->entity.prio_changed = 1;
- }
+ if (bfqg)
+ bfq_group_set_weight(bfqg, val);
}
spin_unlock_irq(&blkcg->lock);
--
2.11.0
Thank you very much, Fam, for this extension.
Reviewed-by: Paolo Valente <[email protected]>
> Il giorno 5 ago 2019, alle ore 08:38, Fam Zheng <[email protected]> ha scritto:
>
> (Revision starting from v2 since v1 was used off-list)
>
> Hi Paolo and others,
>
> This adds to BFQ the missing per-device weight interfaces:
> blkio.bfq.weight_device on legacy and io.bfq.weight on unified. The
> implementation pretty closely resembles what we had in CFQ and the parsing code
> is basically reused.
>
> Tests
> =====
>
> Using two cgroups and three block devices, having weights setup as:
>
> Cgroup test1 test2
> ============================================
> default 100 500
> sda 500 100
> sdb default default
> sdc 200 200
>
> cgroup v1 runs
> --------------
>
> sda.test1.out: READ: bw=913MiB/s
> sda.test2.out: READ: bw=183MiB/s
>
> sdb.test1.out: READ: bw=213MiB/s
> sdb.test2.out: READ: bw=1054MiB/s
>
> sdc.test1.out: READ: bw=650MiB/s
> sdc.test2.out: READ: bw=650MiB/s
>
> cgroup v2 runs
> --------------
>
> sda.test1.out: READ: bw=915MiB/s
> sda.test2.out: READ: bw=184MiB/s
>
> sdb.test1.out: READ: bw=216MiB/s
> sdb.test2.out: READ: bw=1069MiB/s
>
> sdc.test1.out: READ: bw=621MiB/s
> sdc.test2.out: READ: bw=622MiB/s
>
> Fam Zheng (3):
> bfq: Fix the missing barrier in __bfq_entity_update_weight_prio
> bfq: Extract bfq_group_set_weight from bfq_io_set_weight_legacy
> bfq: Add per-device weight
>
> block/bfq-cgroup.c | 151 +++++++++++++++++++++++++++++++++++++++-------------
> block/bfq-iosched.h | 3 ++
> block/bfq-wf2q.c | 2 +
> 3 files changed, 119 insertions(+), 37 deletions(-)
>
> --
> 2.11.0
>
On Mon, Aug 05, 2019 at 02:38:07PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <[email protected]>
Looks good to me.
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
Hi Jens,
do you think this series could now be queued for 5.4?
Thanks,
Paolo
> Il giorno 21 ago 2019, alle ore 17:44, Tejun Heo <[email protected]> ha scritto:
>
> On Mon, Aug 05, 2019 at 02:38:07PM +0800, Fam Zheng wrote:
>> Signed-off-by: Fam Zheng <[email protected]>
>
> Looks good to me.
>
> Acked-by: Tejun Heo <[email protected]>
>
> Thanks.
>
> --
> tejun
On 8/26/19 12:36 AM, Paolo Valente wrote:
> Hi Jens,
> do you think this series could now be queued for 5.4?
The most glaring oversight in this series, is that the meat of it,
patch #3, doesn't even have a commit message. The cover letter
essentially looks like it should have been the commit message for
that patch.
Please resend with acks/reviews collected, and ensure that all
patches have a reasonable commit message.
--
Jens Axboe
On Mon, 08/26 07:59, Jens Axboe wrote:
> On 8/26/19 12:36 AM, Paolo Valente wrote:
> > Hi Jens,
> > do you think this series could now be queued for 5.4?
>
> The most glaring oversight in this series, is that the meat of it,
> patch #3, doesn't even have a commit message. The cover letter
> essentially looks like it should have been the commit message for
> that patch.
>
> Please resend with acks/reviews collected, and ensure that all
> patches have a reasonable commit message.
Will do. Thanks!
Fam