Skip to contentSkip to conversation

Some cleanups and improvements for blk-iocost

PATCH Superseded Baolin Wang 2 participants 21 messages Nov 24, 2020 – Nov 25, 2020

Subsystems (2)
Block layer cgroup blkio

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Baolin Wang

author
#0 lore raw

cover Some cleanups and improvements for blk-iocost

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(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

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

Fix some typos in comments.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-iocost.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
block/blk-iocost.c +6 -6
index bbe86d1..4ffde36 100644
@@ -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 ||
Signature · 1 line
1.8.3.1
author
#3 lore raw

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

In reply to Baolin Wang

Remove unnecessary advance declaration of struct ioc_gq.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-iocost.c | 2 --
 1 file changed, 2 deletions(-)
block/blk-iocost.c +0 -2
index 4ffde36..103ccbd 100644
@@ -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];
Signature · 1 line
1.8.3.1
author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 block/blk-iocost.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
block/blk-iocost.c +4 -7
index 103ccbd..089f3fe 100644
@@ -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)
Signature · 1 line
1.8.3.1
maintainer
#6 lore raw

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

Quoted 4 lines · 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 <[email protected]>

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

Thanks.

Signature · 1 line
tejun
author
#7 lore raw

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

Quoted 10 lines · Baolin Wang
Quoted 4 lines

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 <[email protected]>

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.

author
#8 lore raw

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

In reply to 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 <[email protected]>
---
 block/blk-iocost.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
block/blk-iocost.c +5 -2
index 089f3fe..5305afd 100644
@@ -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);
Signature · 1 line
1.8.3.1
maintainer
#9 lore raw

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

Hello,

block/blk-iocost.c static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt, · line 1451

Quoted 7 lines · Baolin Wang
block/blk-iocost.c +2 -1
@@ -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.

Signature · 1 line
tejun
author
#10 lore raw

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

Quoted 14 lines

Hello,

Review static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt, · line 1451

Quoted 7 lines · Baolin Wang
(quoted diff) +2 -1
@@ -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

Quoted 2 lines

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

Yes, correct.

Quoted 8 lines

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?

block/blk-iocost.c +6 -1
index bbe86d1..db29200 100644
@@ -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);
Quoted 6 lines

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

Thanks.

maintainer
#11 lore raw

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

Quoted 28 lines · Baolin Wang
Quoted 14 lines

Hello,

Review static void iocg_kick_waitq(struct ioc_gq *iocg, bool pay_debt, · line 1451

Quoted 7 lines · Baolin Wang
(quoted diff) +2 -1
@@ -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

Quoted 2 lines

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

Yes, correct.

Quoted 8 lines

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.

Quoted 8 lines

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.

Signature · 1 line
tejun
author
#12 lore raw

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

In reply to 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 <[email protected]>
---
 block/blk-iocost.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)
block/blk-iocost.c +19 -18
index 5305afd..e36cd8e 100644
@@ -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.
Signature · 1 line
1.8.3.1
maintainer
#13 lore raw

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

Hello,

Quoted 12 lines
block/blk-iocost.c +8 -0
@@ -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);
+			}
+

block/blk-iocost.c static void ioc_timer_fn(struct timer_list *timer) · line 2227

Quoted 10 lines
block/blk-iocost.c +10 -0
+			/* 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.

Signature · 1 line
tejun
author
#14 lore raw

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

Quoted 18 lines · Tejun Heo

Hello,

Quoted 17 lines
block/blk-iocost.c +13 -0
@@ -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);
Quoted 8 lines

block/blk-iocost.c static void ioc_timer_fn(struct timer_list *timer) · line 2227

Quoted 5 lines
block/blk-iocost.c +5 -0
+
+			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.

author
#15 lore raw

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

In reply to 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 <[email protected]>
---
 block/blk-iocost.c | 91 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 40 deletions(-)
block/blk-iocost.c +51 -40
index e36cd8e..db4f894 100644
@@ -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);
 
 	/*
Signature · 1 line
1.8.3.1
maintainer
#16 lore raw

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

Hello,

block/blk-iocost.c static void ioc_forgive_debts(struct ioc *ioc, u64 usage_us_sum, int nr_debtors, · line 2077

Quoted 7 lines · Baolin Wang
block/blk-iocost.c +6 -1
-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.

...

block/blk-iocost.c static void ioc_timer_fn(struct timer_list *timer) · line 2170

Quoted 1 line
block/blk-iocost.c +1 -0
+	nr_debtors = ioc_check_iocg_state(ioc, &now);

How about ioc_check_iocgs()?

Review

Quoted 2 lines
(quoted diff) +1 -0
+
 	commit_weights(ioc);

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

Thanks.

Signature · 1 line
tejun
author
#17 lore raw

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

Quoted 4 lines

Hello,

On Tue, Nov 24, 2020 at 11:33:35AM +0800, Baolin Wang wrote:

Quoted 9 lines

block/blk-iocost.c static void ioc_forgive_debts(struct ioc *ioc, u64 usage_us_sum, int nr_debtors, · line 2077

Quoted 7 lines
block/blk-iocost.c +6 -1
-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.

Quoted 5 lines

...

block/blk-iocost.c static void ioc_timer_fn(struct timer_list *timer) · line 2170

Quoted 1 line
block/blk-iocost.c +1 -0
+	nr_debtors = ioc_check_iocg_state(ioc, &now);

How about ioc_check_iocgs()?

Yes, sounds reasonable to me.

Quoted 6 lines
Quoted 3 lines

+
commit_weights(ioc);

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

OK. Thanks.

author
#18 lore raw

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

In reply to 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 <[email protected]>
---
 block/blk-iocost.c | 78 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 37 deletions(-)
block/blk-iocost.c +41 -37
index db4f894..739c8d4 100644
@@ -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,
Signature · 1 line
1.8.3.1
maintainer
#19 lore raw

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

Hello,

Quoted 6 lines · Baolin Wang
block/blk-iocost.c +0 -2
@@ -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;

...

block/blk-iocost.c static void ioc_timer_fn(struct timer_list *timer) · line 2368

Quoted 9 lines
block/blk-iocost.c +2 -3
+		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.

Signature · 1 line
tejun
author
#20 lore raw

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

Quoted 11 lines

Hello,

Quoted 6 lines · Baolin Wang
block/blk-iocost.c +0 -2
@@ -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;

...

Quoted 11 lines

block/blk-iocost.c static void ioc_timer_fn(struct timer_list *timer) · line 2368

Quoted 9 lines
block/blk-iocost.c +2 -3
+		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.

Quoted 1 line

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

Sure, will rename it in next version. Thanks.