[PATCH 0/8] A few cleanup and bugfix patches for blk-iocost

[PATCH 0/8] A few cleanup and bugfix patches for blk-iocost

[Kemeng Shi]

This series contain a few patch to correct comment, correct trace of
vtime_rate and so on. More detail can be found in the respective
changelogs.
Thanks!

Kemeng Shi (8):
  blk-iocost: Fix typo in comment
  blk-iocost: Reset vtime_base_rate in ioc_refresh_params
  blk-iocost: Trace vtime_base_rate instead of vtime_rate
  blk-iocost: Remove vrate member in struct ioc_now
  blk-iocost: Correct comment in blk_iocost_init
  blk-iocost: Avoid to call current_hweight_max if iocg->inuse ==
    iocg->active
  blk-iocost: Remove redundant initialization of struct ioc_gq
  blk-iocost: Get ioc_now inside weight_updated

 block/blk-iocost.c            | 45 +++++++++++++++++------------------
 include/trace/events/iocost.h |  4 ++--
 2 files changed, 24 insertions(+), 25 deletions(-)

-- 
2.30.0

[PATCH 1/8] blk-iocost: Fix typo in comment

[Kemeng Shi]

soley -> solely

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-iocost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 495396425bad..be4bc38821e2 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -111,7 +111,7 @@
  * busy signal.
  *
  * As devices can have deep queues and be unfair in how the queued commands
- * are executed, soley depending on rq wait may not result in satisfactory
+ * are executed, solely depending on rq wait may not result in satisfactory
  * control quality.  For a better control quality, completion latency QoS
  * parameters can be configured so that the device is considered saturated
  * if N'th percentile completion latency rises above the set point.
-- 
2.30.0

[PATCH 2/8] blk-iocost: Reset vtime_base_rate in ioc_refresh_params

[Kemeng Shi]

Since commit ac33e91e2daca("blk-iocost: implement vtime loss compensation")
split vtime_rate into vtime_rate and vtime_base_rate, we need reset both
vtime_base_rate and vtime_rate when device parameters are refreshed.
If vtime_base_rate is no reset here, vtime_rate will be overwritten with
old vtime_base_rate soon in ioc_refresh_vrate.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-iocost.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index be4bc38821e2..9214733bbc14 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -906,8 +906,10 @@ static bool ioc_refresh_params(struct ioc *ioc, bool force)
 	if (idx == ioc->autop_idx && !force)
 		return false;
 
-	if (idx != ioc->autop_idx)
+	if (idx != ioc->autop_idx) {
 		atomic64_set(&ioc->vtime_rate, VTIME_PER_USEC);
+		ioc->vtime_base_rate = VTIME_PER_USEC;
+	}
 
 	ioc->autop_idx = idx;
 	ioc->autop_too_fast_at = 0;
-- 
2.30.0

[PATCH 3/8] blk-iocost: Trace vtime_base_rate instead of vtime_rate

[Kemeng Shi]

Since commit ac33e91e2daca ("blk-iocost: implement vtime loss
compensation") rename original vtime_base to vtime_base_rate
and current vtime_base is original vtime_base with compensation.
The current rate showed in tracepoint is mixed with vtime_base
and vtime_base_rate:
1) In function ioc_adjust_base_vrate, the first trace_iocost_ioc_vrate_adj
shows vtime_base, the second trace_iocost_ioc_vrate_adj shows
vtime_base_rate.
2) In function iocg_activate shows vtime_base by calling
TRACE_IOCG_PATH(iocg_activate...
3) In function ioc_check_iocgs shows vtime_base by calling
TRACE_IOCG_PATH(iocg_idle...

Trace vtime_base_rate instead of vtime_rate as:
1) Before commit ac33e91e2daca ("blk-iocost: implement vtime loss
compensation"), the traced rate is without compensation, so still
show rate without compensation.
2) The vtime_base_rate is more stable while vtime_rate heavily depends on
excess budeget on current period which may change abruptly in next period.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-iocost.c            | 2 +-
 include/trace/events/iocost.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 9214733bbc14..b0991b52e3dd 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -977,7 +977,7 @@ static void ioc_adjust_base_vrate(struct ioc *ioc, u32 rq_wait_pct,
 
 	if (!ioc->busy_level || (ioc->busy_level < 0 && nr_lagging)) {
 		if (ioc->busy_level != prev_busy_level || nr_lagging)
-			trace_iocost_ioc_vrate_adj(ioc, atomic64_read(&ioc->vtime_rate),
+			trace_iocost_ioc_vrate_adj(ioc, vrate,
 						   missed_ppm, rq_wait_pct,
 						   nr_lagging, nr_shortages);
 
diff --git a/include/trace/events/iocost.h b/include/trace/events/iocost.h
index 6d1626e7a4ce..af8bfed528fc 100644
--- a/include/trace/events/iocost.h
+++ b/include/trace/events/iocost.h
@@ -38,7 +38,7 @@ DECLARE_EVENT_CLASS(iocost_iocg_state,
 		__assign_str(cgroup, path);
 		__entry->now = now->now;
 		__entry->vnow = now->vnow;
-		__entry->vrate = now->vrate;
+		__entry->vrate = iocg->ioc->vtime_base_rate;
 		__entry->last_period = last_period;
 		__entry->cur_period = cur_period;
 		__entry->vtime = vtime;
@@ -160,7 +160,7 @@ TRACE_EVENT(iocost_ioc_vrate_adj,
 
 	TP_fast_assign(
 		__assign_str(devname, ioc_name(ioc));
-		__entry->old_vrate = atomic64_read(&ioc->vtime_rate);
+		__entry->old_vrate = ioc->vtime_base_rate;
 		__entry->new_vrate = new_vrate;
 		__entry->busy_level = ioc->busy_level;
 		__entry->read_missed_ppm = missed_ppm[READ];
-- 
2.30.0

[PATCH 4/8] blk-iocost: Remove vrate member in struct ioc_now

[Kemeng Shi]

If we trace vtime_base_rate instead of vtime_rate, there is nowhere to
access now->vrate except function ioc_now using now->vrate locally.
Just clean it.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-iocost.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index b0991b52e3dd..761295ed9c5a 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -556,7 +556,6 @@ struct ioc_now {
 	u64				now_ns;
 	u64				now;
 	u64				vnow;
-	u64				vrate;
 };
 
 struct iocg_wait {
@@ -1020,10 +1019,11 @@ static void ioc_adjust_base_vrate(struct ioc *ioc, u32 rq_wait_pct,
 static void ioc_now(struct ioc *ioc, struct ioc_now *now)
 {
 	unsigned seq;
+	u64 vrate;
 
 	now->now_ns = ktime_get();
 	now->now = ktime_to_us(now->now_ns);
-	now->vrate = atomic64_read(&ioc->vtime_rate);
+	vrate = atomic64_read(&ioc->vtime_rate);
 
 	/*
 	 * The current vtime is
@@ -1036,7 +1036,7 @@ static void ioc_now(struct ioc *ioc, struct ioc_now *now)
 	do {
 		seq = read_seqcount_begin(&ioc->period_seqcount);
 		now->vnow = ioc->period_at_vtime +
-			(now->now - ioc->period_at) * now->vrate;
+			(now->now - ioc->period_at) * vrate;
 	} while (read_seqcount_retry(&ioc->period_seqcount, seq));
 }
 
-- 
2.30.0

[PATCH 5/8] blk-iocost: Correct comment in blk_iocost_init

[Kemeng Shi]

There is no iocg_pd_init function. The pd_alloc_fn function pointer of
iocost policy is set with ioc_pd_init. Just correct it.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-iocost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 761295ed9c5a..96c1571a8a1d 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2880,7 +2880,7 @@ static int blk_iocost_init(struct gendisk *disk)
 	spin_unlock_irq(&ioc->lock);
 
 	/*
-	 * rqos must be added before activation to allow iocg_pd_init() to
+	 * rqos must be added before activation to allow ioc_pd_init() to
 	 * lookup the ioc from q. This means that the rqos methods may get
 	 * called before policy activation completion, can't assume that the
 	 * target bio has an iocg associated and need to test for NULL iocg.
-- 
2.30.0

[PATCH 6/8] blk-iocost: Avoid to call current_hweight_max if iocg->inuse == iocg->active

[Kemeng Shi]

The old_hwi is already max hweight_inuse if iocg->inuse == iocg->active.
Remove unnecessary calculation.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-iocost.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 96c1571a8a1d..fa90f471dfdc 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2299,7 +2299,10 @@ static void ioc_timer_fn(struct timer_list *timer)
 			 * Determine the donation amount.
 			 */
 			current_hweight(iocg, &hwa, &old_hwi);
-			hwm = current_hweight_max(iocg);
+			if (iocg->inuse == iocg->active)
+				hwm = old_hwi;
+			else
+				hwm = current_hweight_max(iocg);
 			new_hwi = hweight_after_donation(iocg, old_hwi, hwm,
 							 usage, &now);
 			/*
-- 
2.30.0

[PATCH 7/8] blk-iocost: Remove redundant initialization of struct ioc_gq

[Kemeng Shi]

Some member of struct ioc_gq will not be accessed before it's
first activation and will be initialized again in it's first
activation after ioc_pd_init. To be more specific:
1)Member iocg->vtime and iocg->done_vtime will set to target in
activation which only expects vtime is equal to done_vtime in
first activation.
2)Member iocg->active_period will be set with ioc->cur_period
again in first activation.

Remove the redundant initialization to improve ioc_pd_init a
littile bit.

The parameter now of weight_updated will not be used if iocg is
not active, so pass NULL to weight_update here is safe and we
can remove call to ioc_now.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-iocost.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index fa90f471dfdc..4815e676733d 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2946,16 +2946,10 @@ static void ioc_pd_init(struct blkg_policy_data *pd)
 	struct ioc_gq *iocg = pd_to_iocg(pd);
 	struct blkcg_gq *blkg = pd_to_blkg(&iocg->pd);
 	struct ioc *ioc = q_to_ioc(blkg->q);
-	struct ioc_now now;
 	struct blkcg_gq *tblkg;
 	unsigned long flags;
 
-	ioc_now(ioc, &now);
-
 	iocg->ioc = ioc;
-	atomic64_set(&iocg->vtime, now.vnow);
-	atomic64_set(&iocg->done_vtime, now.vnow);
-	atomic64_set(&iocg->active_period, atomic64_read(&ioc->cur_period));
 	INIT_LIST_HEAD(&iocg->active_list);
 	INIT_LIST_HEAD(&iocg->walk_list);
 	INIT_LIST_HEAD(&iocg->surplus_list);
@@ -2974,7 +2968,7 @@ static void ioc_pd_init(struct blkg_policy_data *pd)
 	}
 
 	spin_lock_irqsave(&ioc->lock, flags);
-	weight_updated(iocg, &now);
+	weight_updated(iocg, NULL);
 	spin_unlock_irqrestore(&ioc->lock, flags);
 }
 
-- 
2.30.0

[PATCH 8/8] blk-iocost: Get ioc_now inside weight_updated

[Kemeng Shi]

The ioc_now parameter of weight_updated is only needed if we need call
propagate_weights. Move ioc_now inside weight_updated to remove
unnecessary get of ioc_now from outside of weight_updated.

Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
---
 block/blk-iocost.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 4815e676733d..e2aaf133deb8 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1220,18 +1220,21 @@ static u32 current_hweight_max(struct ioc_gq *iocg)
 	return max_t(u32, hwm, 1);
 }
 
-static void weight_updated(struct ioc_gq *iocg, struct ioc_now *now)
+static void weight_updated(struct ioc_gq *iocg)
 {
 	struct ioc *ioc = iocg->ioc;
 	struct blkcg_gq *blkg = iocg_to_blkg(iocg);
 	struct ioc_cgrp *iocc = blkcg_to_iocc(blkg->blkcg);
+	struct ioc_now now;
 	u32 weight;
 
 	lockdep_assert_held(&ioc->lock);
 
 	weight = iocg->cfg_weight ?: iocc->dfl_weight;
-	if (weight != iocg->weight && iocg->active)
-		propagate_weights(iocg, weight, iocg->inuse, true, now);
+	if (weight != iocg->weight && iocg->active) {
+		ioc_now(iocg->ioc, &now);
+		propagate_weights(iocg, weight, iocg->inuse, true, &now);
+	}
 	iocg->weight = weight;
 }
 
@@ -2968,7 +2971,7 @@ static void ioc_pd_init(struct blkg_policy_data *pd)
 	}
 
 	spin_lock_irqsave(&ioc->lock, flags);
-	weight_updated(iocg, NULL);
+	weight_updated(iocg);
 	spin_unlock_irqrestore(&ioc->lock, flags);
 }
 
@@ -3053,7 +3056,6 @@ static ssize_t ioc_weight_write(struct kernfs_open_file *of, char *buf,
 	struct blkcg *blkcg = css_to_blkcg(of_css(of));
 	struct ioc_cgrp *iocc = blkcg_to_iocc(blkcg);
 	struct blkg_conf_ctx ctx;
-	struct ioc_now now;
 	struct ioc_gq *iocg;
 	u32 v;
 	int ret;
@@ -3074,8 +3076,7 @@ static ssize_t ioc_weight_write(struct kernfs_open_file *of, char *buf,
 
 			if (iocg) {
 				spin_lock(&iocg->ioc->lock);
-				ioc_now(iocg->ioc, &now);
-				weight_updated(iocg, &now);
+				weight_updated(iocg);
 				spin_unlock(&iocg->ioc->lock);
 			}
 		}
@@ -3101,8 +3102,7 @@ static ssize_t ioc_weight_write(struct kernfs_open_file *of, char *buf,
 
 	spin_lock(&iocg->ioc->lock);
 	iocg->cfg_weight = v * WEIGHT_ONE;
-	ioc_now(iocg->ioc, &now);
-	weight_updated(iocg, &now);
+	weight_updated(iocg);
 	spin_unlock(&iocg->ioc->lock);
 
 	blkg_conf_finish(&ctx);
-- 
2.30.0

Re: [PATCH 1/8] blk-iocost: Fix typo in comment

[Tejun Heo]

On Mon, Oct 17, 2022 at 10:00:04AM +0800, Kemeng Shi wrote:
> soley -> solely
> 
> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 2/8] blk-iocost: Reset vtime_base_rate in ioc_refresh_params

[Tejun Heo]

On Mon, Oct 17, 2022 at 10:00:05AM +0800, Kemeng Shi wrote:
> Since commit ac33e91e2daca("blk-iocost: implement vtime loss compensation")
> split vtime_rate into vtime_rate and vtime_base_rate, we need reset both
> vtime_base_rate and vtime_rate when device parameters are refreshed.
> If vtime_base_rate is no reset here, vtime_rate will be overwritten with
> old vtime_base_rate soon in ioc_refresh_vrate.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 3/8] blk-iocost: Trace vtime_base_rate instead of vtime_rate

[Tejun Heo]

On Mon, Oct 17, 2022 at 10:00:06AM +0800, Kemeng Shi wrote:
> Since commit ac33e91e2daca ("blk-iocost: implement vtime loss
> compensation") rename original vtime_base to vtime_base_rate
> and current vtime_base is original vtime_base with compensation.
              ^
              vtime_rate

There are multiple places with the same mistake. Can you please fix the
patch description?

> The current rate showed in tracepoint is mixed with vtime_base
> and vtime_base_rate:
> 1) In function ioc_adjust_base_vrate, the first trace_iocost_ioc_vrate_adj
> shows vtime_base, the second trace_iocost_ioc_vrate_adj shows
> vtime_base_rate.
> 2) In function iocg_activate shows vtime_base by calling
> TRACE_IOCG_PATH(iocg_activate...
> 3) In function ioc_check_iocgs shows vtime_base by calling
> TRACE_IOCG_PATH(iocg_idle...
> 
> Trace vtime_base_rate instead of vtime_rate as:
> 1) Before commit ac33e91e2daca ("blk-iocost: implement vtime loss
> compensation"), the traced rate is without compensation, so still
> show rate without compensation.
> 2) The vtime_base_rate is more stable while vtime_rate heavily depends on
> excess budeget on current period which may change abruptly in next period.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>

Other than that,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 4/8] blk-iocost: Remove vrate member in struct ioc_now

[Tejun Heo]

On Mon, Oct 17, 2022 at 10:00:07AM +0800, Kemeng Shi wrote:
> If we trace vtime_base_rate instead of vtime_rate, there is nowhere to
> access now->vrate except function ioc_now using now->vrate locally.
  ^
  which accesses ("to access" doesn't mean the same thing here)

> Just clean it.
       ^
       remove

> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>

With the above description updates,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 5/8] blk-iocost: Correct comment in blk_iocost_init

[Tejun Heo]

On Mon, Oct 17, 2022 at 10:00:08AM +0800, Kemeng Shi wrote:
> There is no iocg_pd_init function. The pd_alloc_fn function pointer of
> iocost policy is set with ioc_pd_init. Just correct it.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 6/8] blk-iocost: Avoid to call current_hweight_max if iocg->inuse == iocg->active

[Tejun Heo]

On Mon, Oct 17, 2022 at 10:00:09AM +0800, Kemeng Shi wrote:
> The old_hwi is already max hweight_inuse if iocg->inuse == iocg->active.
> Remove unnecessary calculation.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>
> ---
>  block/blk-iocost.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 96c1571a8a1d..fa90f471dfdc 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -2299,7 +2299,10 @@ static void ioc_timer_fn(struct timer_list *timer)
>  			 * Determine the donation amount.
>  			 */
>  			current_hweight(iocg, &hwa, &old_hwi);
> -			hwm = current_hweight_max(iocg);
> +			if (iocg->inuse == iocg->active)
> +				hwm = old_hwi;
> +			else
> +				hwm = current_hweight_max(iocg);

I don't think this is correct. The intermediate nodes might be donating.
This also isn't a meaningful optimization given that it's in the cold
periodic timer path. I'd much rather keep the code simpler unless the
performance benfeit can be clearly demonstrated.

Thanks.

-- 
tejun

Re: [PATCH 7/8] blk-iocost: Remove redundant initialization of struct ioc_gq

[Tejun Heo]

On Mon, Oct 17, 2022 at 10:00:10AM +0800, Kemeng Shi wrote:
> Some member of struct ioc_gq will not be accessed before it's
> first activation and will be initialized again in it's first
> activation after ioc_pd_init. To be more specific:
> 1)Member iocg->vtime and iocg->done_vtime will set to target in
> activation which only expects vtime is equal to done_vtime in
> first activation.
> 2)Member iocg->active_period will be set with ioc->cur_period
> again in first activation.
> 
> Remove the redundant initialization to improve ioc_pd_init a
> littile bit.
> 
> The parameter now of weight_updated will not be used if iocg is
> not active, so pass NULL to weight_update here is safe and we
> can remove call to ioc_now.

This isn't a meaningful optimization and makes the code fragile for no
practical gain. Let's not do this.

Thanks.

-- 
tejun

Re: [PATCH 8/8] blk-iocost: Get ioc_now inside weight_updated

[Tejun Heo]

On Mon, Oct 17, 2022 at 10:00:11AM +0800, Kemeng Shi wrote:
> The ioc_now parameter of weight_updated is only needed if we need call
> propagate_weights. Move ioc_now inside weight_updated to remove
> unnecessary get of ioc_now from outside of weight_updated.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huawei.com>

Ditto. I don't think this improves anything meaningful.

Thanks.

-- 
tejun