Some improvements for blk throttle

PATCH In review Baolin Wang 2 participants 10 messages Oct 8, 2020

Subsystems (3)

Block layer cgroup cgroup blkio

lore

Share & tools

lore

Baolin Wang

author

2020-10-08 03:52 UTC #0 lore raw

v2 cover Some improvements for blk throttle

Hi,

This patch set did some improvements for blk throttle, please help to review. Thanks.

Changes from v1:
- Add another 4 new patches in this patch set.

Baolin Wang (8):
blk-throttle: Remove a meaningless parameter for
throtl_downgrade_state()
blk-throttle: Avoid getting the current time if tg->last_finish_time
is 0
blk-throttle: Avoid tracking latency if low limit is invalid
blk-throttle: Fix IO hang for a corner case
blk-throttle: Move the list operation after list validation
blk-throttle: Move service tree validation out of the
throtl_rb_first()
blk-throttle: Open code __throtl_de/enqueue_tg()
blk-throttle: Re-use the throtl_set_slice_end()

 block/blk-throttle.c | 69 ++++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 34 deletions(-)

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-10-08 03:52 UTC #1 lore raw

PATCH v2 1/8 blk-throttle: Remove a meaningless parameter for throtl_downgrade_state()

The throtl_downgrade_state() is always used to change to LIMIT_LOW limitation, thus remove the latter meaningless parameter which indicates the limitation index.

Signed-off-by Baolin Wang <[email protected]>

---
 block/blk-throttle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

block/blk-throttle.c +3 -3

index 36ba61c..4007b26 100644
@@ -1970,7 +1970,7 @@ static void throtl_upgrade_state(struct throtl_data *td)
 	queue_work(kthrotld_workqueue, &td->dispatch_work);
 }
 
-static void throtl_downgrade_state(struct throtl_data *td, int new)
+static void throtl_downgrade_state(struct throtl_data *td)
 {
 	td->scale /= 2;

@@ -1980,7 +1980,7 @@ static void throtl_downgrade_state(struct throtl_data *td, int new)
 		return;
 	}
 
-	td->limit_index = new;
+	td->limit_index = LIMIT_LOW;
 	td->low_downgrade_time = jiffies;
 }

@@ -2067,7 +2067,7 @@ static void throtl_downgrade_check(struct throtl_grp *tg)
 	 * cgroups
 	 */
 	if (throtl_hierarchy_can_downgrade(tg))
-		throtl_downgrade_state(tg->td, LIMIT_LOW);
+		throtl_downgrade_state(tg->td);
 
 	tg->last_bytes_disp[READ] = 0;
 	tg->last_bytes_disp[WRITE] = 0;

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-10-08 03:52 UTC #2 lore raw

PATCH v2 2/8 blk-throttle: Avoid getting the current time if tg->last_finish_time is 0

In reply to Baolin Wang

We only update the tg->last_finish_time when the low limitaion is enabled, so we can move the tg->last_finish_time validation a little forward to avoid getting the unnecessary current time stamp if the the low limitation is not enabled.

Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>

---
 block/blk-throttle.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

block/blk-throttle.c +6 -2

index 4007b26..7e72102 100644
@@ -2077,10 +2077,14 @@ static void throtl_downgrade_check(struct throtl_grp *tg)
 
 static void blk_throtl_update_idletime(struct throtl_grp *tg)
 {
-	unsigned long now = ktime_get_ns() >> 10;
+	unsigned long now;
 	unsigned long last_finish_time = tg->last_finish_time;
 
-	if (now <= last_finish_time || last_finish_time == 0 ||
+	if (last_finish_time == 0)
+		return;
+
+	now = ktime_get_ns() >> 10;
+	if (now <= last_finish_time ||
 	    last_finish_time == tg->checked_last_finish_time)
 		return;

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-10-08 03:52 UTC #3 lore raw

PATCH v2 3/8 blk-throttle: Avoid tracking latency if low limit is invalid

In reply to Baolin Wang

The IO latency tracking is only for LOW limit, so we should add a validation to avoid redundant latency tracking if the LOW limit is not valid.

Signed-off-by Baolin Wang <[email protected]>

---
 block/blk-throttle.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

block/blk-throttle.c +3 -1

index 7e72102..b0d8f7c 100644
@@ -2100,7 +2100,7 @@ static void throtl_update_latency_buckets(struct throtl_data *td)
 	unsigned long last_latency[2] = { 0 };
 	unsigned long latency[2];
 
-	if (!blk_queue_nonrot(td->queue))
+	if (!blk_queue_nonrot(td->queue) || !td->limit_valid[LIMIT_LOW])
 		return;
 	if (time_before(jiffies, td->last_calculate_time + HZ))
 		return;

@@ -2338,6 +2338,8 @@ void blk_throtl_bio_endio(struct bio *bio)
 	if (!blkg)
 		return;
 	tg = blkg_to_tg(blkg);
+	if (!tg->td->limit_valid[LIMIT_LOW])
+		return;
 
 	finish_time_ns = ktime_get_ns();
 	tg->last_finish_time = finish_time_ns >> 10;

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-10-08 03:52 UTC #4 lore raw

PATCH v2 4/8 blk-throttle: Fix IO hang for a corner case

In reply to Baolin Wang

It can not scale up in throtl_adjusted_limit() if we set bps or iops is 1, which will cause IO hang when enable low limit. Thus we should treat 1 as a illegal value to avoid this issue.

Signed-off-by Baolin Wang <[email protected]>

---
 block/blk-throttle.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

block/blk-throttle.c +4 -4

index b0d8f7c..0649bce 100644
@@ -1687,13 +1687,13 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 			goto out_finish;
 
 		ret = -EINVAL;
-		if (!strcmp(tok, "rbps"))
+		if (!strcmp(tok, "rbps") && val > 1)
 			v[0] = val;
-		else if (!strcmp(tok, "wbps"))
+		else if (!strcmp(tok, "wbps") && val > 1)
 			v[1] = val;
-		else if (!strcmp(tok, "riops"))
+		else if (!strcmp(tok, "riops") && val > 1)
 			v[2] = min_t(u64, val, UINT_MAX);
-		else if (!strcmp(tok, "wiops"))
+		else if (!strcmp(tok, "wiops") && val > 1)
 			v[3] = min_t(u64, val, UINT_MAX);
 		else if (off == LIMIT_LOW && !strcmp(tok, "idle"))
 			idle_time = val;

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-10-08 03:52 UTC #5 lore raw

PATCH v2 5/8 blk-throttle: Move the list operation after list validation

In reply to Baolin Wang

We should move the list operation after validation.

Signed-off-by Baolin Wang <[email protected]>

---
 block/blk-throttle.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

block/blk-throttle.c +4 -2

index 0649bce..f1bcb5c 100644
@@ -423,12 +423,13 @@ static void throtl_qnode_add_bio(struct bio *bio, struct throtl_qnode *qn,
  */
 static struct bio *throtl_peek_queued(struct list_head *queued)
 {
-	struct throtl_qnode *qn = list_first_entry(queued, struct throtl_qnode, node);
+	struct throtl_qnode *qn;
 	struct bio *bio;
 
 	if (list_empty(queued))
 		return NULL;
 
+	qn = list_first_entry(queued, struct throtl_qnode, node);
 	bio = bio_list_peek(&qn->bios);
 	WARN_ON_ONCE(!bio);
 	return bio;

@@ -451,12 +452,13 @@ static struct bio *throtl_peek_queued(struct list_head *queued)
 static struct bio *throtl_pop_queued(struct list_head *queued,
 				     struct throtl_grp **tg_to_put)
 {
-	struct throtl_qnode *qn = list_first_entry(queued, struct throtl_qnode, node);
+	struct throtl_qnode *qn;
 	struct bio *bio;
 
 	if (list_empty(queued))
 		return NULL;
 
+	qn = list_first_entry(queued, struct throtl_qnode, node);
 	bio = bio_list_pop(&qn->bios);
 	WARN_ON_ONCE(!bio);

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-10-08 03:52 UTC #6 lore raw

PATCH v2 6/8 blk-throttle: Move service tree validation out of the throtl_rb_first()

In reply to Baolin Wang

The throtl_schedule_next_dispatch() will validate if the service queue is empty before calling update_min_dispatch_time(), and the update_min_dispatch_time() will call throtl_rb_first(), which will validate service queue again.

Thus we can move the service queue validation out of the throtl_rb_first() to remove the redundant validation in the fast path.

Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>

---
 block/blk-throttle.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

block/blk-throttle.c +5 -4

index f1bcb5c..38aed8b 100644
@@ -638,9 +638,6 @@ static void throtl_pd_free(struct blkg_policy_data *pd)
 throtl_rb_first(struct throtl_service_queue *parent_sq)
 {
 	struct rb_node *n;
-	/* Service tree is empty */
-	if (!parent_sq->nr_pending)
-		return NULL;
 
 	n = rb_first_cached(&parent_sq->pending_tree);
 	WARN_ON_ONCE(!n);

@@ -1224,9 +1221,13 @@ static int throtl_select_dispatch(struct throtl_service_queue *parent_sq)
 	unsigned int nr_disp = 0;
 
 	while (1) {
-		struct throtl_grp *tg = throtl_rb_first(parent_sq);
+		struct throtl_grp *tg;
 		struct throtl_service_queue *sq;
 
+		if (!parent_sq->nr_pending)
+			break;
+
+		tg = throtl_rb_first(parent_sq);
 		if (!tg)
 			break;

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-10-08 03:52 UTC #7 lore raw

PATCH v2 7/8 blk-throttle: Open code __throtl_de/enqueue_tg()

In reply to Baolin Wang

The __throtl_de/enqueue_tg() functions are only be called by throtl_de/enqueue_tg(), thus we can just open code them to make code more readable.

Signed-off-by Baolin Wang <[email protected]>

---
 block/blk-throttle.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

block/blk-throttle.c +9 ~~-17~~

index 38aed8b..fc5c14f 100644
@@ -691,29 +691,21 @@ static void tg_service_queue_add(struct throtl_grp *tg)
 			       leftmost);
 }
 
-static void __throtl_enqueue_tg(struct throtl_grp *tg)
-{
-	tg_service_queue_add(tg);
-	tg->flags |= THROTL_TG_PENDING;
-	tg->service_queue.parent_sq->nr_pending++;
-}
-
 static void throtl_enqueue_tg(struct throtl_grp *tg)
 {
-	if (!(tg->flags & THROTL_TG_PENDING))
-		__throtl_enqueue_tg(tg);
-}
-
-static void __throtl_dequeue_tg(struct throtl_grp *tg)
-{
-	throtl_rb_erase(&tg->rb_node, tg->service_queue.parent_sq);
-	tg->flags &= ~THROTL_TG_PENDING;
+	if (!(tg->flags & THROTL_TG_PENDING)) {
+		tg_service_queue_add(tg);
+		tg->flags |= THROTL_TG_PENDING;
+		tg->service_queue.parent_sq->nr_pending++;
+	}
 }
 
 static void throtl_dequeue_tg(struct throtl_grp *tg)
 {
-	if (tg->flags & THROTL_TG_PENDING)
-		__throtl_dequeue_tg(tg);
+	if (tg->flags & THROTL_TG_PENDING) {
+		throtl_rb_erase(&tg->rb_node, tg->service_queue.parent_sq);
+		tg->flags &= ~THROTL_TG_PENDING;
+	}
 }
 
 /* Call with queue lock held */

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-10-08 03:52 UTC #8 lore raw

PATCH v2 8/8 blk-throttle: Re-use the throtl_set_slice_end()

In reply to Baolin Wang

Re-use throtl_set_slice_end() to remove duplicate code.

Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>

---
 block/blk-throttle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block/blk-throttle.c +1 -1

index fc5c14f..b771c42 100644
@@ -808,7 +808,7 @@ static inline void throtl_set_slice_end(struct throtl_grp *tg, bool rw,
 static inline void throtl_extend_slice(struct throtl_grp *tg, bool rw,
 				       unsigned long jiffy_end)
 {
-	tg->slice_end[rw] = roundup(jiffy_end, tg->td->throtl_slice);
+	throtl_set_slice_end(tg, rw, jiffy_end);
 	throtl_log(&tg->service_queue,
 		   "[%c] extend slice start=%lu end=%lu jiffies=%lu",
 		   rw == READ ? 'R' : 'W', tg->slice_start[rw],

Signature · 1 line

1.8.3.1

Jens Axboe

maintainer

2020-10-08 14:02 UTC #9 lore raw

In reply to Baolin Wang

Quoted 21 lines · Baolin Wang

Hi,
This patch set did some improvements for blk throttle, please help to review. Thanks.
Changes from v1:
- Add another 4 new patches in this patch set.
Baolin Wang (8):
blk-throttle: Remove a meaningless parameter for
throtl_downgrade_state()
blk-throttle: Avoid getting the current time if tg->last_finish_time
is 0
blk-throttle: Avoid tracking latency if low limit is invalid
blk-throttle: Fix IO hang for a corner case
blk-throttle: Move the list operation after list validation
blk-throttle: Move service tree validation out of the
throtl_rb_first()
blk-throttle: Open code __throtl_de/enqueue_tg()
blk-throttle: Re-use the throtl_set_slice_end()

Quoted 2 lines

 block/blk-throttle.c | 69 ++++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 34 deletions(-)

LGTM, applied, thanks.

Signature · 1 line

Jens Axboe