linux-kernel - Re: [PATCH V5 10/17] blk-throttle: make bandwidth change smooth

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170109202844.GN12827@mtj.duckdns.org>
Date:   Mon, 9 Jan 2017 15:28:44 -0500
From:   Tejun Heo <tj@...nel.org>
To:     Shaohua Li <shli@...com>
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-team@...com, axboe@...com, vgoyal@...hat.com
Subject: Re: [PATCH V5 10/17] blk-throttle: make bandwidth change smooth

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