[PATCH 0/7] Some cleanups and improvements for blk-iocost

[PATCH 0/7] Some cleanups and improvements for blk-iocost

[Baolin Wang]

Hi,

This patch set did some cleanups and improvements for blk-iocost, and
no big functional changes. Please help to review. Thanks.

Baolin Wang (7):
  blk-iocost: Fix some typos in comments
  blk-iocost: Remove unnecessary advance declaration
  blk-iocost: Just open code the q_name()
  blk-iocost: Add a flag to indicate if need update hwi
  blk-iocost: Move the usage ratio calculation to the correct place
  blk-iocost: Factor out the active iocgs' state check into a separate  
      function
  blk-iocost: Factor out the base vrate change into a separate function

 block/blk-iocost.c | 238 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 126 insertions(+), 112 deletions(-)

-- 
1.8.3.1

[PATCH 1/7] blk-iocost: Fix some typos in comments

[Baolin Wang]

Fix some typos in comments.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 block/blk-iocost.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index bbe86d1..4ffde36 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -39,7 +39,7 @@
  * On top of that, a size cost proportional to the length of the IO is
  * added.  While simple, this model captures the operational
  * characteristics of a wide varienty of devices well enough.  Default
- * paramters for several different classes of devices are provided and the
+ * parameters for several different classes of devices are provided and the
  * parameters can be configured from userspace via
  * /sys/fs/cgroup/io.cost.model.
  *
@@ -77,7 +77,7 @@
  *
  * This constitutes the basis of IO capacity distribution.  Each cgroup's
  * vtime is running at a rate determined by its hweight.  A cgroup tracks
- * the vtime consumed by past IOs and can issue a new IO iff doing so
+ * the vtime consumed by past IOs and can issue a new IO if doing so
  * wouldn't outrun the current device vtime.  Otherwise, the IO is
  * suspended until the vtime has progressed enough to cover it.
  *
@@ -155,7 +155,7 @@
  * Instead of debugfs or other clumsy monitoring mechanisms, this
  * controller uses a drgn based monitoring script -
  * tools/cgroup/iocost_monitor.py.  For details on drgn, please see
- * https://github.com/osandov/drgn.  The ouput looks like the following.
+ * https://github.com/osandov/drgn.  The output looks like the following.
  *
  *  sdb RUN   per=300ms cur_per=234.218:v203.695 busy= +1 vrate= 62.12%
  *                 active      weight      hweight% inflt% dbt  delay usages%
@@ -492,7 +492,7 @@ struct ioc_gq {
 	/*
 	 * `vtime` is this iocg's vtime cursor which progresses as IOs are
 	 * issued.  If lagging behind device vtime, the delta represents
-	 * the currently available IO budget.  If runnning ahead, the
+	 * the currently available IO budget.  If running ahead, the
 	 * overage.
 	 *
 	 * `vtime_done` is the same but progressed on completion rather
@@ -1046,7 +1046,7 @@ static void __propagate_weights(struct ioc_gq *iocg, u32 active, u32 inuse,
 
 		/*
 		 * The delta between inuse and active sums indicates that
-		 * that much of weight is being given away.  Parent's inuse
+		 * much of weight is being given away.  Parent's inuse
 		 * and active should reflect the ratio.
 		 */
 		if (parent->child_active_sum) {
@@ -2400,7 +2400,7 @@ static u64 adjust_inuse_and_calc_cost(struct ioc_gq *iocg, u64 vtime,
 		return cost;
 
 	/*
-	 * We only increase inuse during period and do so iff the margin has
+	 * We only increase inuse during period and do so if the margin has
 	 * deteriorated since the previous adjustment.
 	 */
 	if (margin >= iocg->saved_margin || margin >= margins->low ||
-- 
1.8.3.1

[PATCH 2/7] blk-iocost: Remove unnecessary advance declaration

[Baolin Wang]

Remove unnecessary advance declaration of struct ioc_gq.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 block/blk-iocost.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 4ffde36..103ccbd 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -370,8 +370,6 @@ enum {
 	AUTOP_SSD_FAST,
 };
 
-struct ioc_gq;
-
 struct ioc_params {
 	u32				qos[NR_QOS_PARAMS];
 	u64				i_lcoefs[NR_I_LCOEFS];
-- 
1.8.3.1

[PATCH 3/7] blk-iocost: Just open code the q_name()

[Baolin Wang]

The simple q_name() function is only called from ioc_name(),
just open code it to make code more readable.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 block/blk-iocost.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 103ccbd..089f3fe 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -665,17 +665,14 @@ static struct ioc *q_to_ioc(struct request_queue *q)
 	return rqos_to_ioc(rq_qos_id(q, RQ_QOS_COST));
 }
 
-static const char *q_name(struct request_queue *q)
+static const char __maybe_unused *ioc_name(struct ioc *ioc)
 {
+	struct request_queue *q = ioc->rqos.q;
+
 	if (blk_queue_registered(q))
 		return kobject_name(q->kobj.parent);
-	else
-		return "<unknown>";
-}
 
-static const char __maybe_unused *ioc_name(struct ioc *ioc)
-{
-	return q_name(ioc->rqos.q);
+	return "<unknown>";
 }
 
 static struct ioc_gq *pd_to_iocg(struct blkg_policy_data *pd)
-- 
1.8.3.1

[PATCH 4/7] blk-iocost: Add a flag to indicate if need update hwi

[Baolin Wang]

We can get the hwa and hwi at one time if no debt need to pay off,
thus add a flag to indicate if the hw_inuse has been changed and
need to update, which can avoid calling current_hweight() twice
for no debt iocgs.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 block/blk-iocost.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 089f3fe..5305afd 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1405,10 +1405,11 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt,
 	u64 vshortage, expires, oexpires;
 	s64 vbudget;
 	u32 hwa;
+	bool need_update_hwi = false;
 
 	lockdep_assert_held(&iocg->waitq.lock);
 
-	current_hweight(iocg, &hwa, NULL);
+	current_hweight(iocg, &hwa, &ctx.hw_inuse);
 	vbudget = now->vnow - atomic64_read(&iocg->vtime);
 
 	/* pay off debt */
@@ -1423,6 +1424,7 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt,
 		atomic64_add(vpay, &iocg->done_vtime);
 		iocg_pay_debt(iocg, abs_vpay, now);
 		vbudget -= vpay;
+		need_update_hwi = true;
 	}
 
 	if (iocg->abs_vdebt || iocg->delay)
@@ -1445,7 +1447,8 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt,
 	 * after the above debt payment.
 	 */
 	ctx.vbudget = vbudget;
-	current_hweight(iocg, NULL, &ctx.hw_inuse);
+	if (need_update_hwi)
+		current_hweight(iocg, NULL, &ctx.hw_inuse);
 
 	__wake_up_locked_key(&iocg->waitq, TASK_NORMAL, &ctx);
 
-- 
1.8.3.1

[PATCH 5/7] blk-iocost: Move the usage ratio calculation to the correct place

[Baolin Wang]

We only use the hweight based usage ratio to calculate the new
hweight_inuse of the iocg to decide if this iocg can donate some
surplus vtime.

Thus move the usage ratio calculation to the correct place to
avoid unnecessary calculation for some vtime shortage iocgs.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 block/blk-iocost.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 5305afd..e36cd8e 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2200,24 +2200,6 @@ static void ioc_timer_fn(struct timer_list *timer)
 		usage_us = iocg->usage_delta_us;
 		usage_us_sum += usage_us;
 
-		if (vdone != vtime) {
-			u64 inflight_us = DIV64_U64_ROUND_UP(
-				cost_to_abs_cost(vtime - vdone, hw_inuse),
-				ioc->vtime_base_rate);
-			usage_us = max(usage_us, inflight_us);
-		}
-
-		/* convert to hweight based usage ratio */
-		if (time_after64(iocg->activated_at, ioc->period_at))
-			usage_dur = max_t(u64, now.now - iocg->activated_at, 1);
-		else
-			usage_dur = max_t(u64, now.now - ioc->period_at, 1);
-
-		usage = clamp_t(u32,
-				DIV64_U64_ROUND_UP(usage_us * WEIGHT_ONE,
-						   usage_dur),
-				1, WEIGHT_ONE);
-
 		/* see whether there's surplus vtime */
 		WARN_ON_ONCE(!list_empty(&iocg->surplus_list));
 		if (hw_inuse < hw_active ||
@@ -2225,6 +2207,25 @@ static void ioc_timer_fn(struct timer_list *timer)
 		     time_before64(vtime, now.vnow - ioc->margins.low))) {
 			u32 hwa, old_hwi, hwm, new_hwi;
 
+			if (vdone != vtime) {
+				u64 inflight_us = DIV64_U64_ROUND_UP(
+					cost_to_abs_cost(vtime - vdone, hw_inuse),
+					ioc->vtime_base_rate);
+
+				usage_us = max(usage_us, inflight_us);
+			}
+
+			/* convert to hweight based usage ratio */
+			if (time_after64(iocg->activated_at, ioc->period_at))
+				usage_dur = max_t(u64, now.now - iocg->activated_at, 1);
+			else
+				usage_dur = max_t(u64, now.now - ioc->period_at, 1);
+
+			usage = clamp_t(u32,
+				DIV64_U64_ROUND_UP(usage_us * WEIGHT_ONE,
+						   usage_dur),
+				1, WEIGHT_ONE);
+
 			/*
 			 * Already donating or accumulated enough to start.
 			 * Determine the donation amount.
-- 
1.8.3.1

[PATCH 6/7] blk-iocost: Factor out the active iocgs' state check into a separate function

[Baolin Wang]

Factor out the iocgs' state check into a separate function to
simplify the ioc_timer_fn().

No functional change.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 block/blk-iocost.c | 91 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 40 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index e36cd8e..db4f894 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2069,40 +2069,17 @@ static void ioc_forgive_debts(struct ioc *ioc, u64 usage_us_sum, int nr_debtors,
 	}
 }
 
-static void ioc_timer_fn(struct timer_list *timer)
+/*
+ * Waiters determine the sleep durations based on the vrate they
+ * saw at the time of sleep.  If vrate has increased, some waiters
+ * could be sleeping for too long.  Wake up tardy waiters which
+ * should have woken up in the last period and expire idle iocgs.
+ */
+static int ioc_check_iocg_state(struct ioc *ioc, struct ioc_now *now)
 {
-	struct ioc *ioc = container_of(timer, struct ioc, timer);
+	int nr_debtors = 0;
 	struct ioc_gq *iocg, *tiocg;
-	struct ioc_now now;
-	LIST_HEAD(surpluses);
-	int nr_debtors = 0, nr_shortages = 0, nr_lagging = 0;
-	u64 usage_us_sum = 0;
-	u32 ppm_rthr = MILLION - ioc->params.qos[QOS_RPPM];
-	u32 ppm_wthr = MILLION - ioc->params.qos[QOS_WPPM];
-	u32 missed_ppm[2], rq_wait_pct;
-	u64 period_vtime;
-	int prev_busy_level;
 
-	/* how were the latencies during the period? */
-	ioc_lat_stat(ioc, missed_ppm, &rq_wait_pct);
-
-	/* take care of active iocgs */
-	spin_lock_irq(&ioc->lock);
-
-	ioc_now(ioc, &now);
-
-	period_vtime = now.vnow - ioc->period_at_vtime;
-	if (WARN_ON_ONCE(!period_vtime)) {
-		spin_unlock_irq(&ioc->lock);
-		return;
-	}
-
-	/*
-	 * Waiters determine the sleep durations based on the vrate they
-	 * saw at the time of sleep.  If vrate has increased, some waiters
-	 * could be sleeping for too long.  Wake up tardy waiters which
-	 * should have woken up in the last period and expire idle iocgs.
-	 */
 	list_for_each_entry_safe(iocg, tiocg, &ioc->active_iocgs, active_list) {
 		if (!waitqueue_active(&iocg->waitq) && !iocg->abs_vdebt &&
 		    !iocg->delay && !iocg_is_idle(iocg))
@@ -2112,24 +2089,24 @@ static void ioc_timer_fn(struct timer_list *timer)
 
 		/* flush wait and indebt stat deltas */
 		if (iocg->wait_since) {
-			iocg->local_stat.wait_us += now.now - iocg->wait_since;
-			iocg->wait_since = now.now;
+			iocg->local_stat.wait_us += now->now - iocg->wait_since;
+			iocg->wait_since = now->now;
 		}
 		if (iocg->indebt_since) {
 			iocg->local_stat.indebt_us +=
-				now.now - iocg->indebt_since;
-			iocg->indebt_since = now.now;
+				now->now - iocg->indebt_since;
+			iocg->indebt_since = now->now;
 		}
 		if (iocg->indelay_since) {
 			iocg->local_stat.indelay_us +=
-				now.now - iocg->indelay_since;
-			iocg->indelay_since = now.now;
+				now->now - iocg->indelay_since;
+			iocg->indelay_since = now->now;
 		}
 
 		if (waitqueue_active(&iocg->waitq) || iocg->abs_vdebt ||
 		    iocg->delay) {
 			/* might be oversleeping vtime / hweight changes, kick */
-			iocg_kick_waitq(iocg, true, &now);
+			iocg_kick_waitq(iocg, true, now);
 			if (iocg->abs_vdebt || iocg->delay)
 				nr_debtors++;
 		} else if (iocg_is_idle(iocg)) {
@@ -2143,7 +2120,7 @@ static void ioc_timer_fn(struct timer_list *timer)
 			 * error and throw away. On reactivation, it'll start
 			 * with the target budget.
 			 */
-			excess = now.vnow - vtime - ioc->margins.target;
+			excess = now->vnow - vtime - ioc->margins.target;
 			if (excess > 0) {
 				u32 old_hwi;
 
@@ -2152,12 +2129,46 @@ static void ioc_timer_fn(struct timer_list *timer)
 							    WEIGHT_ONE);
 			}
 
-			__propagate_weights(iocg, 0, 0, false, &now);
+			__propagate_weights(iocg, 0, 0, false, now);
 			list_del_init(&iocg->active_list);
 		}
 
 		spin_unlock(&iocg->waitq.lock);
 	}
+
+	return nr_debtors;
+}
+
+static void ioc_timer_fn(struct timer_list *timer)
+{
+	struct ioc *ioc = container_of(timer, struct ioc, timer);
+	struct ioc_gq *iocg, *tiocg;
+	struct ioc_now now;
+	LIST_HEAD(surpluses);
+	int nr_debtors, nr_shortages = 0, nr_lagging = 0;
+	u64 usage_us_sum = 0;
+	u32 ppm_rthr = MILLION - ioc->params.qos[QOS_RPPM];
+	u32 ppm_wthr = MILLION - ioc->params.qos[QOS_WPPM];
+	u32 missed_ppm[2], rq_wait_pct;
+	u64 period_vtime;
+	int prev_busy_level;
+
+	/* how were the latencies during the period? */
+	ioc_lat_stat(ioc, missed_ppm, &rq_wait_pct);
+
+	/* take care of active iocgs */
+	spin_lock_irq(&ioc->lock);
+
+	ioc_now(ioc, &now);
+
+	period_vtime = now.vnow - ioc->period_at_vtime;
+	if (WARN_ON_ONCE(!period_vtime)) {
+		spin_unlock_irq(&ioc->lock);
+		return;
+	}
+
+	nr_debtors = ioc_check_iocg_state(ioc, &now);
+
 	commit_weights(ioc);
 
 	/*
-- 
1.8.3.1

[PATCH 7/7] blk-iocost: Factor out the base vrate change into a separate function

[Baolin Wang]

Factor out the base vrate change code into a separate function
to fimplify the ioc_timer_fn().

No functional change.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 block/blk-iocost.c | 78 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 37 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index db4f894..739c8d4 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -968,6 +968,44 @@ static void ioc_refresh_vrate(struct ioc *ioc, struct ioc_now *now)
 	ioc->vtime_err = clamp(ioc->vtime_err, -vperiod, vperiod);
 }
 
+static void ioc_refresh_base_vrate(struct ioc *ioc, u32 rq_wait_pct)
+{
+	u64 vrate = ioc->vtime_base_rate;
+	u64 vrate_min = ioc->vrate_min, vrate_max = ioc->vrate_max;
+
+	/* rq_wait signal is always reliable, ignore user vrate_min */
+	if (rq_wait_pct > RQ_WAIT_BUSY_PCT)
+		vrate_min = VRATE_MIN;
+
+	/*
+	 * If vrate is out of bounds, apply clamp gradually as the
+	 * bounds can change abruptly.  Otherwise, apply busy_level
+	 * based adjustment.
+	 */
+	if (vrate < vrate_min) {
+		vrate = div64_u64(vrate * (100 + VRATE_CLAMP_ADJ_PCT), 100);
+		vrate = min(vrate, vrate_min);
+	} else if (vrate > vrate_max) {
+		vrate = div64_u64(vrate * (100 - VRATE_CLAMP_ADJ_PCT), 100);
+		vrate = max(vrate, vrate_max);
+	} else {
+		int idx = min_t(int, abs(ioc->busy_level),
+				ARRAY_SIZE(vrate_adj_pct) - 1);
+		u32 adj_pct = vrate_adj_pct[idx];
+
+		if (ioc->busy_level > 0)
+			adj_pct = 100 - adj_pct;
+		else
+			adj_pct = 100 + adj_pct;
+
+		vrate = clamp(DIV64_U64_ROUND_UP(vrate * adj_pct, 100),
+			      vrate_min, vrate_max);
+	}
+
+	ioc->vtime_base_rate = vrate;
+	ioc_refresh_margins(ioc);
+}
+
 /* take a snapshot of the current [v]time and vrate */
 static void ioc_now(struct ioc *ioc, struct ioc_now *now)
 {
@@ -2320,45 +2358,11 @@ static void ioc_timer_fn(struct timer_list *timer)
 	ioc->busy_level = clamp(ioc->busy_level, -1000, 1000);
 
 	if (ioc->busy_level > 0 || (ioc->busy_level < 0 && !nr_lagging)) {
-		u64 vrate = ioc->vtime_base_rate;
-		u64 vrate_min = ioc->vrate_min, vrate_max = ioc->vrate_max;
-
-		/* rq_wait signal is always reliable, ignore user vrate_min */
-		if (rq_wait_pct > RQ_WAIT_BUSY_PCT)
-			vrate_min = VRATE_MIN;
-
-		/*
-		 * If vrate is out of bounds, apply clamp gradually as the
-		 * bounds can change abruptly.  Otherwise, apply busy_level
-		 * based adjustment.
-		 */
-		if (vrate < vrate_min) {
-			vrate = div64_u64(vrate * (100 + VRATE_CLAMP_ADJ_PCT),
-					  100);
-			vrate = min(vrate, vrate_min);
-		} else if (vrate > vrate_max) {
-			vrate = div64_u64(vrate * (100 - VRATE_CLAMP_ADJ_PCT),
-					  100);
-			vrate = max(vrate, vrate_max);
-		} else {
-			int idx = min_t(int, abs(ioc->busy_level),
-					ARRAY_SIZE(vrate_adj_pct) - 1);
-			u32 adj_pct = vrate_adj_pct[idx];
-
-			if (ioc->busy_level > 0)
-				adj_pct = 100 - adj_pct;
-			else
-				adj_pct = 100 + adj_pct;
+		ioc_refresh_base_vrate(ioc, rq_wait_pct);
 
-			vrate = clamp(DIV64_U64_ROUND_UP(vrate * adj_pct, 100),
-				      vrate_min, vrate_max);
-		}
-
-		trace_iocost_ioc_vrate_adj(ioc, vrate, missed_ppm, rq_wait_pct,
+		trace_iocost_ioc_vrate_adj(ioc, ioc->vtime_base_rate,
+					   missed_ppm, rq_wait_pct,
 					   nr_lagging, nr_shortages);
-
-		ioc->vtime_base_rate = vrate;
-		ioc_refresh_margins(ioc);
 	} else if (ioc->busy_level != prev_busy_level || nr_lagging) {
 		trace_iocost_ioc_vrate_adj(ioc, atomic64_read(&ioc->vtime_rate),
 					   missed_ppm, rq_wait_pct, nr_lagging,
-- 
1.8.3.1

Re: [PATCH 1/7] blk-iocost: Fix some typos in comments

[Tejun Heo]

On Tue, Nov 24, 2020 at 11:33:30AM +0800, Baolin Wang wrote:
> Fix some typos in comments.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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

-- 
tejun

Re: [PATCH 2/7] blk-iocost: Remove unnecessary advance declaration

[Tejun Heo]

On Tue, Nov 24, 2020 at 11:33:31AM +0800, Baolin Wang wrote:
> Remove unnecessary advance declaration of struct ioc_gq.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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

-- 
tejun

Re: [PATCH 3/7] blk-iocost: Just open code the q_name()

[Tejun Heo]

On Tue, Nov 24, 2020 at 11:33:32AM +0800, Baolin Wang wrote:
> The simple q_name() function is only called from ioc_name(),
> just open code it to make code more readable.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

I'm not sure this is an improvement. Either way seems fine to me. So, why
change?

Thanks.

-- 
tejun

Re: [PATCH 4/7] blk-iocost: Add a flag to indicate if need update hwi

[Tejun Heo]

Hello,

On Tue, Nov 24, 2020 at 11:33:33AM +0800, Baolin Wang wrote:
> @@ -1445,7 +1447,8 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt,
>  	 * after the above debt payment.
>  	 */
>  	ctx.vbudget = vbudget;
> -	current_hweight(iocg, NULL, &ctx.hw_inuse);
> +	if (need_update_hwi)
> +		current_hweight(iocg, NULL, &ctx.hw_inuse);

So, if you look at the implementation of current_hweight(), it's

1. If nothing has changed, read out the cached values.
2. If something has changed, recalculate.

and the "something changed" test is single memory read (most likely L1 hot
at this point) and testing for equality. IOW, the change you're suggesting
isn't much of an optimization. Maybe the compiler can do a somewhat better
job of arranging the code and it's a register load than memory load but
given that it's already a relatively cold wait path, this is unlikely to
make any actual difference. And that's how current_hweight() is meant to be
used.

So, I'm not sure this is an improvement. It increases complication without
actually gaining anything.

Thanks.

-- 
tejun

Re: [PATCH 5/7] blk-iocost: Move the usage ratio calculation to the correct place

[Tejun Heo]

Hello,
> @@ -2225,6 +2207,25 @@ static void ioc_timer_fn(struct timer_list *timer)
>  		     time_before64(vtime, now.vnow - ioc->margins.low))) {
>  			u32 hwa, old_hwi, hwm, new_hwi;
>  
> +			if (vdone != vtime) {
> +				u64 inflight_us = DIV64_U64_ROUND_UP(
> +					cost_to_abs_cost(vtime - vdone, hw_inuse),
> +					ioc->vtime_base_rate);
> +
> +				usage_us = max(usage_us, inflight_us);
> +			}
> +
> +			/* convert to hweight based usage ratio */
> +			if (time_after64(iocg->activated_at, ioc->period_at))
> +				usage_dur = max_t(u64, now.now - iocg->activated_at, 1);
> +			else
> +				usage_dur = max_t(u64, now.now - ioc->period_at, 1);
> +
> +			usage = clamp_t(u32,
> +				DIV64_U64_ROUND_UP(usage_us * WEIGHT_ONE,
> +						   usage_dur),
> +				1, WEIGHT_ONE);

Can you please move the variable declarations inside the block together with
the code?

Thanks.

-- 
tejun

Re: [PATCH 6/7] blk-iocost: Factor out the active iocgs' state check into a separate function

[Tejun Heo]

Hello,

On Tue, Nov 24, 2020 at 11:33:35AM +0800, Baolin Wang wrote:
> -static void ioc_timer_fn(struct timer_list *timer)
> +/*
> + * Waiters determine the sleep durations based on the vrate they
> + * saw at the time of sleep.  If vrate has increased, some waiters
> + * could be sleeping for too long.  Wake up tardy waiters which
> + * should have woken up in the last period and expire idle iocgs.
> + */

Please reflow the comment.

...
> +	nr_debtors = ioc_check_iocg_state(ioc, &now);

How about ioc_check_iocgs()?

> +
>  	commit_weights(ioc);

I think it'd make more sense to move commit_weights() inside the new
function.

Thanks.

-- 
tejun

Re: [PATCH 7/7] blk-iocost: Factor out the base vrate change into a separate function

[Tejun Heo]

Hello,

On Tue, Nov 24, 2020 at 11:33:36AM +0800, Baolin Wang wrote:
> @@ -2320,45 +2358,11 @@ static void ioc_timer_fn(struct timer_list *timer)
>  	ioc->busy_level = clamp(ioc->busy_level, -1000, 1000);
>  
>  	if (ioc->busy_level > 0 || (ioc->busy_level < 0 && !nr_lagging)) {
> -		u64 vrate = ioc->vtime_base_rate;
> -		u64 vrate_min = ioc->vrate_min, vrate_max = ioc->vrate_max;
...
> +		trace_iocost_ioc_vrate_adj(ioc, ioc->vtime_base_rate,
> +					   missed_ppm, rq_wait_pct,
>  					   nr_lagging, nr_shortages);
> -
> -		ioc->vtime_base_rate = vrate;
> -		ioc_refresh_margins(ioc);
>  	} else if (ioc->busy_level != prev_busy_level || nr_lagging) {
>  		trace_iocost_ioc_vrate_adj(ioc, atomic64_read(&ioc->vtime_rate),
>  					   missed_ppm, rq_wait_pct, nr_lagging,

I think it'd be better to factor out the surrounding if/else block together
(as early exit if blocks). Also, how about ioc_adjust_base_vrate()?

Thanks.

-- 
tejun

Re: [PATCH 3/7] blk-iocost: Just open code the q_name()

[Baolin Wang]

> On Tue, Nov 24, 2020 at 11:33:32AM +0800, Baolin Wang wrote:
>> The simple q_name() function is only called from ioc_name(),
>> just open code it to make code more readable.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> 
> I'm not sure this is an improvement. Either way seems fine to me. So, why
> change?

Yes, this may be not called an 'improvement'. Just from my taste of 
reading code, there is no need to factor out a separate function only 
including 4 lines code and called by only one place. Anyway I can drop
this patch if you think this is unnecessary. Thanks.

Re: [PATCH 5/7] blk-iocost: Move the usage ratio calculation to the correct place

[Baolin Wang]

在 2020/11/25 20:19, Tejun Heo 写道:
> Hello,
>> @@ -2225,6 +2207,25 @@ static void ioc_timer_fn(struct timer_list *timer)
>>   		     time_before64(vtime, now.vnow - ioc->margins.low))) {
>>   			u32 hwa, old_hwi, hwm, new_hwi;
>>   
>> +			if (vdone != vtime) {
>> +				u64 inflight_us = DIV64_U64_ROUND_UP(
>> +					cost_to_abs_cost(vtime - vdone, hw_inuse),
>> +					ioc->vtime_base_rate);
>> +
>> +				usage_us = max(usage_us, inflight_us);
>> +			}
>> +
>> +			/* convert to hweight based usage ratio */
>> +			if (time_after64(iocg->activated_at, ioc->period_at))
>> +				usage_dur = max_t(u64, now.now - iocg->activated_at, 1);
>> +			else
>> +				usage_dur = max_t(u64, now.now - ioc->period_at, 1);
>> +
>> +			usage = clamp_t(u32,
>> +				DIV64_U64_ROUND_UP(usage_us * WEIGHT_ONE,
>> +						   usage_dur),
>> +				1, WEIGHT_ONE);
> 
> Can you please move the variable declarations inside the block together with
> the code?

Yes, sure. Will do in next version. Thanks.

Re: [PATCH 6/7] blk-iocost: Factor out the active iocgs' state check into a separate function

[Baolin Wang]

> Hello,
> 
> On Tue, Nov 24, 2020 at 11:33:35AM +0800, Baolin Wang wrote:
>> -static void ioc_timer_fn(struct timer_list *timer)
>> +/*
>> + * Waiters determine the sleep durations based on the vrate they
>> + * saw at the time of sleep.  If vrate has increased, some waiters
>> + * could be sleeping for too long.  Wake up tardy waiters which
>> + * should have woken up in the last period and expire idle iocgs.
>> + */
> 
> Please reflow the comment.

Sure.

> 
> ...
>> +	nr_debtors = ioc_check_iocg_state(ioc, &now);
> 
> How about ioc_check_iocgs()?

Yes, sounds reasonable to me.

> 
>> +
>>   	commit_weights(ioc);
> 
> I think it'd make more sense to move commit_weights() inside the new
> function.

OK. Thanks.

Re: [PATCH 7/7] blk-iocost: Factor out the base vrate change into a separate function

[Baolin Wang]

> Hello,
> 
> On Tue, Nov 24, 2020 at 11:33:36AM +0800, Baolin Wang wrote:
>> @@ -2320,45 +2358,11 @@ static void ioc_timer_fn(struct timer_list *timer)
>>   	ioc->busy_level = clamp(ioc->busy_level, -1000, 1000);
>>   
>>   	if (ioc->busy_level > 0 || (ioc->busy_level < 0 && !nr_lagging)) {
>> -		u64 vrate = ioc->vtime_base_rate;
>> -		u64 vrate_min = ioc->vrate_min, vrate_max = ioc->vrate_max;
> ...
>> +		trace_iocost_ioc_vrate_adj(ioc, ioc->vtime_base_rate,
>> +					   missed_ppm, rq_wait_pct,
>>   					   nr_lagging, nr_shortages);
>> -
>> -		ioc->vtime_base_rate = vrate;
>> -		ioc_refresh_margins(ioc);
>>   	} else if (ioc->busy_level != prev_busy_level || nr_lagging) {
>>   		trace_iocost_ioc_vrate_adj(ioc, atomic64_read(&ioc->vtime_rate),
>>   					   missed_ppm, rq_wait_pct, nr_lagging,
> 
> I think it'd be better to factor out the surrounding if/else block together

OK.

> (as early exit if blocks). Also, how about ioc_adjust_base_vrate()?

Sure, will rename it in next version. Thanks.

Re: [PATCH 4/7] blk-iocost: Add a flag to indicate if need update hwi

[Baolin Wang]

> Hello,
> 
> On Tue, Nov 24, 2020 at 11:33:33AM +0800, Baolin Wang wrote:
>> @@ -1445,7 +1447,8 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt,
>>   	 * after the above debt payment.
>>   	 */
>>   	ctx.vbudget = vbudget;
>> -	current_hweight(iocg, NULL, &ctx.hw_inuse);
>> +	if (need_update_hwi)
>> +		current_hweight(iocg, NULL, &ctx.hw_inuse);
> 
> So, if you look at the implementation of current_hweight(), it's
> 
> 1. If nothing has changed, read out the cached values.
> 2. If something has changed, recalculate.

Yes, correct.

> 
> and the "something changed" test is single memory read (most likely L1 hot
> at this point) and testing for equality. IOW, the change you're suggesting
> isn't much of an optimization. Maybe the compiler can do a somewhat better
> job of arranging the code and it's a register load than memory load but
> given that it's already a relatively cold wait path, this is unlikely to
> make any actual difference. And that's how current_hweight() is meant to be
> used.

What I want to avoid is the 'atomic_read(&ioc->hweight_gen)' in 
current_hweight(), cause this is not a register load and is always a 
memory load. But introducing a flag can be cached and more light than a 
memory load.

But after thinking more, I think we can just move the 
"current_hweight(iocg, NULL, &ctx.hw_inuse);" to the correct place 
without introducing new flag to optimize the code. How do you think the 
below code?

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index bbe86d1..db29200 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1413,7 +1413,7 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, 
bool pay_debt,

         lockdep_assert_held(&iocg->waitq.lock);

-       current_hweight(iocg, &hwa, NULL);
+       current_hweight(iocg, &hwa, &ctx.hw_inuse);
         vbudget = now->vnow - atomic64_read(&iocg->vtime);

         /* pay off debt */
@@ -1428,6 +1428,11 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, 
bool pay_debt,
                 atomic64_add(vpay, &iocg->done_vtime);
                 iocg_pay_debt(iocg, abs_vpay, now);
                 vbudget -= vpay;
+               /*
+                * As paying off debt restores hw_inuse, it must be read 
after
+                * the above debt payment.
+                */
+               current_hweight(iocg, NULL, &ctx.hw_inuse);
         }

         if (iocg->abs_vdebt || iocg->delay)
@@ -1446,11 +1451,9 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, 
bool pay_debt,

         /*
          * Wake up the ones which are due and see how much vtime we'll 
need for
-        * the next one. As paying off debt restores hw_inuse, it must 
be read
-        * after the above debt payment.
+        * the next one.
          */
         ctx.vbudget = vbudget;
-       current_hweight(iocg, NULL, &ctx.hw_inuse);

         __wake_up_locked_key(&iocg->waitq, TASK_NORMAL, &ctx);

> 
> So, I'm not sure this is an improvement. It increases complication without
> actually gaining anything.
> 
> Thanks.
> 

Re: [PATCH 4/7] blk-iocost: Add a flag to indicate if need update hwi

[Tejun Heo]

On Wed, Nov 25, 2020 at 10:15:38PM +0800, Baolin Wang wrote:
> 
> > Hello,
> > 
> > On Tue, Nov 24, 2020 at 11:33:33AM +0800, Baolin Wang wrote:
> > > @@ -1445,7 +1447,8 @@ static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt,
> > >   	 * after the above debt payment.
> > >   	 */
> > >   	ctx.vbudget = vbudget;
> > > -	current_hweight(iocg, NULL, &ctx.hw_inuse);
> > > +	if (need_update_hwi)
> > > +		current_hweight(iocg, NULL, &ctx.hw_inuse);
> > 
> > So, if you look at the implementation of current_hweight(), it's
> > 
> > 1. If nothing has changed, read out the cached values.
> > 2. If something has changed, recalculate.
> 
> Yes, correct.
> 
> > 
> > and the "something changed" test is single memory read (most likely L1 hot
> > at this point) and testing for equality. IOW, the change you're suggesting
> > isn't much of an optimization. Maybe the compiler can do a somewhat better
> > job of arranging the code and it's a register load than memory load but
> > given that it's already a relatively cold wait path, this is unlikely to
> > make any actual difference. And that's how current_hweight() is meant to be
> > used.
> 
> What I want to avoid is the 'atomic_read(&ioc->hweight_gen)' in
> current_hweight(), cause this is not a register load and is always a memory
> load. But introducing a flag can be cached and more light than a memory
> load.
> 
> But after thinking more, I think we can just move the "current_hweight(iocg,
> NULL, &ctx.hw_inuse);" to the correct place without introducing new flag to
> optimize the code. How do you think the below code?

I don't find this discussion very meaningful. We're talking about
theoretical one memory load optimization in a path which likely isn't hot
enough for such difference to make any difference. If you can show that this
matters, please do. Otherwise, what are we doing?

Thanks.

-- 
tejun