Hello,
On Thu, Dec 15, 2016 at 12:33:01PM -0800, Shaohua Li wrote:
> static uint64_t tg_bps_limit(struct throtl_grp *tg, int rw)
> {
> struct blkcg_gq *blkg = tg_to_blkg(tg);
> + struct throtl_data *td;
> uint64_t ret;
>
> if (cgroup_subsys_on_dfl(io_cgrp_subsys) && !blkg->parent)
> return U64_MAX;
> - return tg->bps[rw][tg->td->limit_index];
> +
> + td = tg->td;
> + ret = tg->bps[rw][td->limit_index];
> + if (td->limit_index == LIMIT_MAX && tg->bps[rw][LIMIT_LOW] !=
> + tg->bps[rw][LIMIT_MAX]) {
> + uint64_t increase;
> +
> + if (td->scale < 4096 && time_after_eq(jiffies,
Hmm... why do we need to limit scale to 4096? As 4096 is a big
number, this is only theoretical but this means that if max is more
then 2048 times low, that will never be reached, right?
> + td->low_upgrade_time + td->scale * td->throtl_slice)) {
> + unsigned int time = jiffies - td->low_upgrade_time;
> +
> + td->scale = time / td->throtl_slice;
> + }
> + increase = (tg->bps[rw][LIMIT_LOW] >> 1) * td->scale;
> + ret = min(tg->bps[rw][LIMIT_MAX],
> + tg->bps[rw][LIMIT_LOW] + increase);
> + }
> + return ret;
> }
I think the code can use some comments.
> static unsigned int tg_iops_limit(struct throtl_grp *tg, int rw)
> {
> struct blkcg_gq *blkg = tg_to_blkg(tg);
> + struct throtl_data *td;
> unsigned int ret;
>
> if (cgroup_subsys_on_dfl(io_cgrp_subsys) && !blkg->parent)
> return UINT_MAX;
> - return tg->iops[rw][tg->td->limit_index];
> +
> + td = tg->td;
> + ret = tg->iops[rw][td->limit_index];
> + if (td->limit_index == LIMIT_MAX && tg->iops[rw][LIMIT_LOW] !=
> + tg->iops[rw][LIMIT_MAX]) {
> + uint64_t increase;
> +
> + if (td->scale < 4096 && time_after_eq(jiffies,
> + td->low_upgrade_time + td->scale * td->throtl_slice)) {
> + unsigned int time = jiffies - td->low_upgrade_time;
> +
> + td->scale = time / td->throtl_slice;
> + }
> +
> + increase = (tg->iops[rw][LIMIT_LOW] >> 1) * td->scale;
> + ret = min(tg->iops[rw][LIMIT_MAX],
> + tg->iops[rw][LIMIT_LOW] + (unsigned int)increase);
Would it be worthwhile to factor the common part into a helper?
> @@ -1662,6 +1702,13 @@ static void throtl_upgrade_state(struct throtl_data *td)
>
> static void throtl_downgrade_state(struct throtl_data *td, int new)
> {
> + td->scale /= 2;
> +
> + if (td->scale) {
> + td->low_upgrade_time = jiffies - td->scale * td->throtl_slice;
> + return;
> + }
Cool, so linear increase and exponential backdown. Yeah, that makes
sense to me but let's please document it.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
