blkcg: prepare blkcg knobs for default hierarchy

Hello,

Unified hierarchy has finally been posted.

http://thread.gmane.org/gmane.linux.kernel.containers/27601

It took a lot longer than I originally anticipated and over the course quite a few aspects of the initial design have changed, hopefully, for the better. One of the areas which has seen major redirection is internal tasks competing against child cgroups. The original plan was implementing an implicit leaf node for each cgroup so that internal tasks compete as an implicit child against other children. cfq was chosen as the first one to try the strategy and the result was leaf_weight[_device] knobs. This, unfortunately, doesn't seem to be the right direction.

The strategy complicates the controller interface and implementation, and ends up adding an extra layer of nesting at leaves. Also, initially, it was thought that internal tasks are problematic only for weight based controllers; however, it turns out it also is a problem for absolute limit based ones like memory, which means that we'd need to complicate more controllers.

As the problem is something fundamental in most resource controllers, the implemented unified hierarchy now enforces structural constraint which prevents competion between internal tasks and child cgroups from the get-go making leaf_weight[_device] mechanism unnecessary.

In addition, blkio interface is generally a bit messy and all cfq knobs are missing "cfq." prefix. As unified hierarchy involves noticeable changes in usage, this patch takes the chance and make blkio present more consistent and concise interface on unified hierarchy

I'm currently preparing a doc describing the design and rationales of unified hierarchy and most of the above information will be present in the documentation.

Thanks.

------ 8< ------
Some blkcg knobs don't make sense on unified hierarchy. This patch makes the following changes for unified hierarchy.

* reset_stats never made much sense outside developement and debugging. Omit it.

* As unified hierarchy excludes internal tasks competing against child cgroups, cfq's leaf_weight[_device] knobs are no longer necessary. Omit it.

* For cfq, weight[_device] behave inherently differently from the same knobs on !root cgroups as root cgroup is treated as parallel to immediate children. Let's omit it for now.

* cfq's time stats reports the total time slice consumed but its unit is in jiffies and it's not reported per-device like all other stats. Omit it for now. If slice time is actually necessary, let's add it in the form consistent with other stats.

* cfq's io_queued reporting is different from all other stats in that it's a snapshot value rather than accumulated value and is mostly meaningful only for debugging. Omit it.

* As unified hierarchy doesn't allow internal tasks, non-recursive stats are largely irrelevant. Omit them.

* cfq adds a number of stats knobs if CONFIG_DEBUG_BLK_CGROUP. If these are actually for debugging, they shouldn't show up by default (ie. should be gated by kernel param or something). If userland already developed dependency on them, we need to take them out of CONFIG_DEBUG_BLK_CGROUP. Skip them for now. We can add back the relevant ones later.

* Prefix the remaining cfq knobs with "cfq." and make the names consistent.

weight_device -> cfq.weight_device
weight -> cfq.weight
sectors_recursive -> cfq.sectors
io_service_bytes_recursive -> cfq.bytes

io_serviced_recursive -> cfq.serviced

io_merged_recursive -> cfq.merged
io_service_time_recursive -> cfq.io_time
io_wait_time_recursive -> cfq.wait_time

Signed-off-by Tejun Heo <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-cgroup.c  |    1 
 block/cfq-iosched.c |   89 +++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 86 insertions(+), 4 deletions(-)

block/blk-cgroup.c +4 -0

+	/* files for default hierarchy, properly prefixed with cfq */
+	{
+		.name = "cfq.weight_device",
+		.flags = CFTYPE_ONLY_ON_DFL | CFTYPE_NOT_ON_ROOT,

Can you please also update Documentation/block/cfq-iosched.txt and Documentation/cgroup/blkio-controller.txt to reflect these new changes.

So now we have tree modes?

- Orignal multi hierachy mode
- Multi hierarchy with sane flag
- Sane flag modifies behavior of throttling.
- Unified hierarchy
- Changes tunables.

Looks like some of the changes are related to sane flag and others are related to unified hierarchy. Will it make sense to break it down int two patches.

Quoted 3 lines

* For cfq, weight[_device] behave inherently differently from the same knobs on !root cgroups as root cgroup is treated as parallel to immediate children. Let's omit it for now.

So now root cgroup will not have any cfq, weight[_device] knobs but non root cgroups will have one? What's the harm in providing one for root too for the sake of uniformity. That's a differnt thing thet root does not have a competitor so value in that knob does not matter.

Quoted 4 lines

* cfq's time stats reports the total time slice consumed but its unit is in jiffies and it's not reported per-device like all other stats. Omit it for now. If slice time is actually necessary, let's add it in the form consistent with other stats.

To me this knob is also for debugging purposes. CFQ divides disk time proportionally. And if one wants to see how disk time is being divided among cgroups, this knob helps. This is more to verify whether our algorithm is working fine or not and if disk time is being divided in proportion to weights or not.

But again, I am not particular about this knob. Sounds more like debugging stuff.

Google folks wanted all these knobs. And they said that they have lot of more knobs which they carry internally.

Personally I think io_queued might be a useful knob because it can show current backlog on a particular cgroup and some tool might want to dynamically monitor the queue lenghts of various cgroups and adjust weights dynamically.

So this one might be worth retaining.

Quoted 6 lines

* cfq adds a number of stats knobs if CONFIG_DEBUG_BLK_CGROUP. If these are actually for debugging, they shouldn't show up by default (ie. should be gated by kernel param or something). If userland already developed dependency on them, we need to take them out of CONFIG_DEBUG_BLK_CGROUP. Skip them for now. We can add back the relevant ones later.

I am fine with this. Again Google folks intorduced those. Not sure if they still use those. Less stats are better.

Quoted 4 lines

weight_device -> cfq.weight_device
weight -> cfq.weight
sectors_recursive -> cfq.sectors
io_service_bytes_recursive -> cfq.bytes

throttling has correspnding knob as throttle.io_service_bytes. So either we can change the name of throttling knob to throttle.bytes or chnage keep this one as it is (cfq.io_service_bytes) for the sake of consistency.

Quoted 1 line

io_serviced_recursive -> cfq.serviced

throttling layer uses throttle.io_serviced.

Given the fact that throttling layer counts bios and CFQ counts requests may be we can keep throttle.bio_serviced and cfq.rq_serviced as names.

Or may be just throttle.bios and cfq.requests respectively.

block/blk-cgroup.c EXPORT_SYMBOL_GPL(blkg_conf_finish); · line 765

Quoted 6 lines

block/blk-cgroup.c +1 -0

@@ -761,6 +761,7 @@ EXPORT_SYMBOL_GPL(blkg_conf_finish);
 struct cftype blkcg_files[] = {
 	{
 		.name = "reset_stats",
+		.flags = CFTYPE_INSANE,
 		.write_u64 = blkcg_reset_stats,

So this knob will not be user visible with unified hierarchy as well as with sane behavior?

So this looks like a cleanup independent of unified hiearchy.

Quoted 8 lines

block/cfq-iosched.c +8 -0

+	/* files for default hierarchy, properly prefixed with cfq */
+	{
+		.name = "cfq.weight_device",
+		.flags = CFTYPE_ONLY_ON_DFL | CFTYPE_NOT_ON_ROOT,
+		.seq_show = cfqg_print_weight_device,
+		.write_string = cfqg_set_weight_device,
+	},
+	{

block/cfq-iosched.c static struct cftype cfq_blkcg_files[] = · line 2026

Quoted 6 lines

block/cfq-iosched.c +6 -0

+		.name = "cfq.weight",
+		.flags = CFTYPE_ONLY_ON_DFL | CFTYPE_NOT_ON_ROOT,
+		.seq_show = cfq_print_weight,
+		.write_u64 = cfq_set_weight,
+	},
+

What't the harm in providing these knobs for root too?

Quoted 7 lines

So now we have tree modes?

- Orignal multi hierachy mode
- Multi hierarchy with sane flag
- Sane flag modifies behavior of throttling.
- Unified hierarchy
- Changes tunables.

No, we don't. __DEVEL__sane_behavior is a tool to implement unified hierarchy. Once unified hierarchy lands, sane_behavior will be folded into unified hierarchy.

Quoted 4 lines

Looks like some of the changes are related to sane flag and others are related to unified hierarchy. Will it make sense to break it down int two patches.

So, they're one and the same.

Quoted 8 lines

Quoted 3 lines

* For cfq, weight[_device] behave inherently differently from the same knobs on !root cgroups as root cgroup is treated as parallel to immediate children. Let's omit it for now.

So now root cgroup will not have any cfq, weight[_device] knobs but non root cgroups will have one? What's the harm in providing one for root too for the sake of uniformity. That's a differnt thing thet root does not have a competitor so value in that knob does not matter.

Please see at the end of the mail.

Quoted 11 lines

Quoted 4 lines

* cfq's time stats reports the total time slice consumed but its unit is in jiffies and it's not reported per-device like all other stats. Omit it for now. If slice time is actually necessary, let's add it in the form consistent with other stats.

To me this knob is also for debugging purposes. CFQ divides disk time proportionally. And if one wants to see how disk time is being divided among cgroups, this knob helps. This is more to verify whether our algorithm is working fine or not and if disk time is being divided in proportion to weights or not.

Quoted 2 lines

But again, I am not particular about this knob. Sounds more like debugging stuff.

If it is a debug knob, let's please require it to be enabled explicitly - ie. require a boot param which *clearly* indicates that this is a debug option; otherwise, we end up leaking internal details w/o actually intending or designing for it and userland is likely to grow dependency on it.

Quoted 3 lines

Google folks wanted all these knobs. And they said that they have lot of more knobs which they carry internally.

AFAIK, google folks forked cfq a couple years ago, I'm not sure how much this will affect them.

Quoted 5 lines

Personally I think io_queued might be a useful knob because it can show current backlog on a particular cgroup and some tool might want to dynamically monitor the queue lenghts of various cgroups and adjust weights dynamically.

Quoted 1 line

So this one might be worth retaining.

Can we do that once we know for sure that this is actually necessary? I'm quite skeptical that this sort of snapshot stats are actually useful except for debugging.

Quoted 10 lines

Quoted 7 lines

* cfq adds a number of stats knobs if CONFIG_DEBUG_BLK_CGROUP. If these are actually for debugging, they shouldn't show up by default (ie. should be gated by kernel param or something). If userland already developed dependency on them, we need to take them out of CONFIG_DEBUG_BLK_CGROUP. Skip them for now. We can add back the relevant ones later.

I am fine with this. Again Google folks intorduced those. Not sure if they still use those. Less stats are better.

The same thing with above. If these are really for debugging, let's please hide them better.

Quoted 5 lines

Quoted 4 lines

weight_device -> cfq.weight_device
weight -> cfq.weight
sectors_recursive -> cfq.sectors
io_service_bytes_recursive -> cfq.bytes

Quoted 4 lines

throttling has correspnding knob as throttle.io_service_bytes. So either we can change the name of throttling knob to throttle.bytes or chnage keep this one as it is (cfq.io_service_bytes) for the sake of consistency.

Yeah, thought about keeping them consistent but would that really matter? Having internal consistency in cfq seems more important to me.

Quoted 4 lines

Quoted 2 lines

io_serviced_recursive -> cfq.serviced

throttling layer uses throttle.io_serviced.

Ditto.

Quoted 3 lines

Given the fact that throttling layer counts bios and CFQ counts requests may be we can keep throttle.bio_serviced and cfq.rq_serviced as names.

Quoted 1 line

Or may be just throttle.bios and cfq.requests respectively.

I'm not sure whether we want to rename throttle knobs. Renaming cfq knobs makes sense as they need to be renamed for cfq. prefix anyway but throttle knobs are already mostly fine, so wouldn't it make sense to leave them as they are? It makes things a bit incosistent between cfq and throttle but do we really care?

But looks like that you are targetting that one can have multiple hierarchies. One of those will be unified hierarchy with new contstraints. Other hierarchies can be old type hierarchies. Do I understand it right?

So you want all the debug knobs to be enabled by DEBUG_BLK_CGROUP option as well as using a kernel command line? But I thought that in general people are not supposed to set CONFIG_DEBUG_BLK_CGROUP=y. It brings in unnecessary overhead. Those who are doing development and trying to debug things only those can enable above config option.

But anyway, I am fine with hiding these files behind a command line parameter too.

Quoted 3 lines

Yeah, thought about keeping them consistent but would that really matter? Having internal consistency in cfq seems more important to me.

I think from a user's perspective it really matters. If I see two knobs in a cgroup, say throttle.io_service_bytes and cfq.io_service_bytes, it is much more intuitive.

So I feel that we should aim for having consistent names in CFQ and throttling. If one understands one knob at one layer, it is intuitive to figure out what same knob means at other layer.

And, thankfully, we have only two stats at throttling layer.

blkio.throttle.io_serviced
blkio.throttle.io_service_bytes

Though names are little longer, but they look fine to me. They are intuitive. So I can't see a strong reason to change those names at CFQ layer.

blkio.cfq.io_serviced
blkio.cfq.io_service_bytes

But then do we name other stat knobs similarly too?

blkio.cfq.io_service_sectors
blkio.cfq.io_service_bytes
blkio.cfq.io_serviced
blkio.cfq.io_merged

I don't know. The names look outright stupid to me. If we don't do the above, then we have internal inconsistencies among cfq knob names which gotta be worse then cfq / throttl inconsistency. It's not a perfect situation no matter what we do. As long as each knob is clearly documented, I don't think these inconsistencies are big deal, so let's just clean up cfq names as we need to add prefix anyway.

Ok, that's fine. Let us just document the knobs well so that people can find which knob is giving what information and make cfq names better at the expense of inconsistency of names with throttling layer.

Quoted 3 lines · Vivek Goyal

Ok, that's fine. Let us just document the knobs well so that people can find which knob is giving what information and make cfq names better at the expense of inconsistency of names with throttling layer.

I've been thinking about it more. Why do we even have separate stats for common things like bytes transferred? It doesn't serve any purpose to do double accounting and reporting on everything, does it? Shouldn't we just have single set of common stats for things like requests / bytes serviced?

I think we should just require two. One for measuring rate in terms of IOPS and other for measuring rate in terms of [kMG]B/sec.

Hello,

Quoted 2 lines · Vivek Goyal

I think we should just require two. One for measuring rate in terms of IOPS and other for measuring rate in terms of [kMG]B/sec.

I meant between cfq and blk-throttle. Why do we have separate stats for them to present ultimately the same numbers?

Oh, sorry, I had misunderstood your question.

- Number of IOs serviced will be different at throttling layer and CFQ layer as throttling accounts IO in terms of bios and CFQ accounts in terms of number of requests.

- CFQ might not be operational on a device while throttling might be on and one needs bytes stats.

- In a custom kernel throttling might not be on and CFQ is on and one needs the stats.

Hello,

Quoted 2 lines · Vivek Goyal

Oh, sorry, I had misunderstood your question.

Quoted 3 lines

- Number of IOs serviced will be different at throttling layer and CFQ layer as throttling accounts IO in terms of bios and CFQ accounts in terms of number of requests.

But shouldn't it be possible to determine that from merged count? Or we can just expose both bio and request counts at block core layer. The point is that there's nothing controller specific about these stats.

In general this idea makes sense. Exporting both request and bio will solve the problem of io accounting. Also that should allow us to get rid of blkio.io_merged.

What about sync/async differentiation? Throttling layer seems to flag a request sync only if bio->bi_rw flag has REQ_SYNC set. While CFQ seems to consider request sync if bio is either read or bio->bi_rw has REQ_SYNC flag set.

So we need to make this definition uniform. Or I am wondering do we really need to export sync/async data. (Again put in by google folks). How useful this info really is.

What about sync/async differentiation? Throttling layer seems to flag a request sync only if bio->bi_rw flag has REQ_SYNC set. While CFQ seems to consider request sync if bio is either read or bio->bi_rw has REQ_SYNC flag set.

Hello, Vivek.

Quoted 3 lines · Vivek Goyal

What about sync/async differentiation? Throttling layer seems to flag a request sync only if bio->bi_rw flag has REQ_SYNC set. While CFQ seems to consider request sync if bio is either read or bio->bi_rw has REQ_SYNC flag set.

Working on this again, AFAICS, both treat REQ_SYNC the same way as far as stats are concerned. If SYNC is set, it's sync; otherwise, it's accounted as async whether read or write.