[PATCH 1/2] blk-iocost: factor out iocg_deactivate()

[PATCH 1/2] blk-iocost: factor out iocg_deactivate()

[Chengming Zhou]

This patch factor out iocg deactivation into a separate function:
iocg_deactivate(). No functional changes.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-iocost.c | 59 ++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 33a11ba971ea..b1f2305e8032 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1322,6 +1322,37 @@ static bool iocg_activate(struct ioc_gq *iocg, struct ioc_now *now)
 	return false;
 }
 
+static void iocg_deactivate(struct ioc_gq *iocg, struct ioc_now *now)
+{
+	struct ioc *ioc = iocg->ioc;
+	u64 vtime = atomic64_read(&iocg->vtime);
+	s64 excess;
+
+	lockdep_assert_held(&ioc->lock);
+	lockdep_assert_held(&iocg->waitq.lock);
+
+	/*
+	 * @iocg has been inactive for a full duration and will
+	 * have a high budget. Account anything above target as
+	 * error and throw away. On reactivation, it'll start
+	 * with the target budget.
+	 */
+	excess = now->vnow - vtime - ioc->margins.target;
+	if (excess > 0) {
+		u32 old_hwi;
+
+		current_hweight(iocg, NULL, &old_hwi);
+		ioc->vtime_err -= div64_u64(excess * old_hwi,
+					    WEIGHT_ONE);
+	}
+
+	TRACE_IOCG_PATH(iocg_idle, iocg, now,
+			atomic64_read(&iocg->active_period),
+			atomic64_read(&ioc->cur_period), vtime);
+	__propagate_weights(iocg, 0, 0, false, now);
+	list_del_init(&iocg->active_list);
+}
+
 static bool iocg_kick_delay(struct ioc_gq *iocg, struct ioc_now *now)
 {
 	struct ioc *ioc = iocg->ioc;
@@ -2165,32 +2196,8 @@ static int ioc_check_iocgs(struct ioc *ioc, struct ioc_now *now)
 			iocg_kick_waitq(iocg, true, now);
 			if (iocg->abs_vdebt || iocg->delay)
 				nr_debtors++;
-		} else if (iocg_is_idle(iocg)) {
-			/* no waiter and idle, deactivate */
-			u64 vtime = atomic64_read(&iocg->vtime);
-			s64 excess;
-
-			/*
-			 * @iocg has been inactive for a full duration and will
-			 * have a high budget. Account anything above target as
-			 * error and throw away. On reactivation, it'll start
-			 * with the target budget.
-			 */
-			excess = now->vnow - vtime - ioc->margins.target;
-			if (excess > 0) {
-				u32 old_hwi;
-
-				current_hweight(iocg, NULL, &old_hwi);
-				ioc->vtime_err -= div64_u64(excess * old_hwi,
-							    WEIGHT_ONE);
-			}
-
-			TRACE_IOCG_PATH(iocg_idle, iocg, now,
-					atomic64_read(&iocg->active_period),
-					atomic64_read(&ioc->cur_period), vtime);
-			__propagate_weights(iocg, 0, 0, false, now);
-			list_del_init(&iocg->active_list);
-		}
+		} else if (iocg_is_idle(iocg))
+			iocg_deactivate(iocg, now);
 
 		spin_unlock(&iocg->waitq.lock);
 	}
-- 
2.36.1

[PATCH 2/2] blk-iocost: only flush wait and indebt stat deltas when needed

[Chengming Zhou]

We only need to flush wait and indebt stat deltas when the iocg
is in these status.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-iocost.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index b1f2305e8032..502425b44475 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2174,28 +2174,28 @@ static int ioc_check_iocgs(struct ioc *ioc, struct ioc_now *now)
 
 		spin_lock(&iocg->waitq.lock);
 
-		/* flush wait and indebt stat deltas */
-		if (iocg->wait_since) {
-			iocg->stat.wait_us += now->now - iocg->wait_since;
-			iocg->wait_since = now->now;
-		}
-		if (iocg->indebt_since) {
-			iocg->stat.indebt_us +=
-				now->now - iocg->indebt_since;
-			iocg->indebt_since = now->now;
-		}
-		if (iocg->indelay_since) {
-			iocg->stat.indelay_us +=
-				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);
 			if (iocg->abs_vdebt || iocg->delay)
 				nr_debtors++;
+
+			/* flush wait and indebt stat deltas */
+			if (iocg->wait_since) {
+				iocg->stat.wait_us += now->now - iocg->wait_since;
+				iocg->wait_since = now->now;
+			}
+			if (iocg->indebt_since) {
+				iocg->stat.indebt_us +=
+					now->now - iocg->indebt_since;
+				iocg->indebt_since = now->now;
+			}
+			if (iocg->indelay_since) {
+				iocg->stat.indelay_us +=
+					now->now - iocg->indelay_since;
+				iocg->indelay_since = now->now;
+			}
 		} else if (iocg_is_idle(iocg))
 			iocg_deactivate(iocg, now);
 
-- 
2.36.1

Re: [PATCH 2/2] blk-iocost: only flush wait and indebt stat deltas when needed

[Tejun Heo]

On Wed, Jun 01, 2022 at 08:20:07PM +0800, Chengming Zhou wrote:
> We only need to flush wait and indebt stat deltas when the iocg
> is in these status.

Hey, so, I'm not seeing any actual benefits of the suggested patches and
none of them has actual justifications. For the time being, I'm gonna be
ignoring these patches.

Thanks.

-- 
tejun

Re: [PATCH 2/2] blk-iocost: only flush wait and indebt stat deltas when needed

[Chengming Zhou]

On 2022/6/2 00:23, Tejun Heo wrote:
> On Wed, Jun 01, 2022 at 08:20:07PM +0800, Chengming Zhou wrote:
>> We only need to flush wait and indebt stat deltas when the iocg
>> is in these status.
> 
> Hey, so, I'm not seeing any actual benefits of the suggested patches and
> none of them has actual justifications. For the time being, I'm gonna be
> ignoring these patches.

Hi, the current code will flush wait and indebt stat deltas even for idle
iocgs, which seems strange. This patch only do that for iocgs that are in
wait or indebt status, so it's a performance and code improvements, although
it's minor.

Thanks.

> 
> Thanks.
>