Some improvements for blk-throttle

PATCH In review Baolin Wang 2 participants 10 messages Sep 7, 2020 – Sep 15, 2020

Subsystems (3)

Block layer cgroup cgroup blkio

lore

Share & tools

lore

Baolin Wang

author

2020-09-07 08:10 UTC #0 lore raw

cover Some improvements for blk-throttle

Hi All,

This patch set did some clean-ups, as well as removing some unnecessary bps/iops limitation calculation when checking if can dispatch a bio or not for a tg. Please help to review. Thanks.

Baolin Wang (5):
blk-throttle: Fix some comments' typos
blk-throttle: Use readable READ/WRITE macros
blk-throttle: Define readable macros instead of static variables
blk-throttle: Avoid calculating bps/iops limitation repeatedly
blk-throttle: Avoid checking bps/iops limitation if bps or iops is
unlimited

 block/blk-throttle.c | 59 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 23 deletions(-)

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-09-07 08:10 UTC #1 lore raw

PATCH 1/5 blk-throttle: Fix some comments' typos

Fix some comments' typos.

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

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

block/blk-throttle.c +7 -7

index fee3325..2fc6c3e 100644
@@ -150,7 +150,7 @@ struct throtl_grp {
 	/* user configured IOPS limits */
 	unsigned int iops_conf[2][LIMIT_CNT];
 
-	/* Number of bytes disptached in current slice */
+	/* Number of bytes dispatched in current slice */
 	uint64_t bytes_disp[2];
 	/* Number of bio's dispatched in current slice */
 	unsigned int io_disp[2];

@@ -852,7 +852,7 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
 	/*
 	 * A bio has been dispatched. Also adjust slice_end. It might happen
 	 * that initially cgroup limit was very low resulting in high
-	 * slice_end, but later limit was bumped up and bio was dispached
+	 * slice_end, but later limit was bumped up and bio was dispatched
 	 * sooner, then we need to reduce slice_end. A high bogus slice_end
 	 * is bad because it does not allow new slice to start.
 	 */

@@ -1082,7 +1082,7 @@ static void throtl_add_bio_tg(struct bio *bio, struct throtl_qnode *qn,
 	 * If @tg doesn't currently have any bios queued in the same
 	 * direction, queueing @bio can change when @tg should be
 	 * dispatched.  Mark that @tg was empty.  This is automatically
-	 * cleaered on the next tg_update_disptime().
+	 * cleared on the next tg_update_disptime().
 	 */
 	if (!sq->nr_queued[rw])
 		tg->flags |= THROTL_TG_WAS_EMPTY;

@@ -1303,7 +1303,7 @@ static void throtl_pending_timer_fn(struct timer_list *t)
 			}
 		}
 	} else {
-		/* reached the top-level, queue issueing */
+		/* reached the top-level, queue issuing */
 		queue_work(kthrotld_workqueue, &td->dispatch_work);
 	}
 out_unlock:

@@ -1314,8 +1314,8 @@ static void throtl_pending_timer_fn(struct timer_list *t)
  * blk_throtl_dispatch_work_fn - work function for throtl_data->dispatch_work
  * @work: work item being executed
  *
- * This function is queued for execution when bio's reach the bio_lists[]
- * of throtl_data->service_queue.  Those bio's are ready and issued by this
+ * This function is queued for execution when bios reach the bio_lists[]
+ * of throtl_data->service_queue.  Those bios are ready and issued by this
  * function.
  */
 static void blk_throtl_dispatch_work_fn(struct work_struct *work)

@@ -2230,7 +2230,7 @@ bool blk_throtl_bio(struct bio *bio)
 
 		/*
 		 * @bio passed through this layer without being throttled.
-		 * Climb up the ladder.  If we''re already at the top, it
+		 * Climb up the ladder.  If we're already at the top, it
 		 * can be executed directly.
 		 */
 		qn = &tg->qnode_on_parent[rw];

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-09-07 08:10 UTC #2 lore raw

PATCH 2/5 blk-throttle: Use readable READ/WRITE macros

In reply to Baolin Wang

Use readable READ/WRITE macros instead of magic numbers.

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

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

block/blk-throttle.c +2 -2

index 2fc6c3e..06e73ed 100644
@@ -1428,8 +1428,8 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 	 * that a group's limit are dropped suddenly and we don't want to
 	 * account recently dispatched IO with new low rate.
 	 */
-	throtl_start_new_slice(tg, 0);
-	throtl_start_new_slice(tg, 1);
+	throtl_start_new_slice(tg, READ);
+	throtl_start_new_slice(tg, WRITE);
 
 	if (tg->flags & THROTL_TG_PENDING) {
 		tg_update_disptime(tg);

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-09-07 08:10 UTC #3 lore raw

PATCH 3/5 blk-throttle: Define readable macros instead of static variables

In reply to Baolin Wang

The 'throtl_grp_quantum' and 'throtl_quantum' are both read-only variables, thus better to use readable macros instead of static variables, which can also save some spaces for .bss area.

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

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

block/blk-throttle.c +5 -5

index 06e73ed..ca9002d 100644
@@ -15,10 +15,10 @@
 #include "blk-cgroup-rwstat.h"
 
 /* Max dispatch from a group in 1 round */
-static int throtl_grp_quantum = 8;
+#define THROTL_GRP_QUANTUM 8
 
 /* Total max dispatch from all groups in one round */
-static int throtl_quantum = 32;
+#define THROTL_QUANTUM 32
 
 /* Throttling is performed over a slice and after that slice is renewed */
 #define DFL_THROTL_SLICE_HD (HZ / 10)

@@ -1175,8 +1175,8 @@ static int throtl_dispatch_tg(struct throtl_grp *tg)
 {
 	struct throtl_service_queue *sq = &tg->service_queue;
 	unsigned int nr_reads = 0, nr_writes = 0;
-	unsigned int max_nr_reads = throtl_grp_quantum*3/4;
-	unsigned int max_nr_writes = throtl_grp_quantum - max_nr_reads;
+	unsigned int max_nr_reads = THROTL_GRP_QUANTUM * 3 / 4;
+	unsigned int max_nr_writes = THROTL_GRP_QUANTUM - max_nr_reads;
 	struct bio *bio;
 
 	/* Try to dispatch 75% READS and 25% WRITES */

@@ -1226,7 +1226,7 @@ static int throtl_select_dispatch(struct throtl_service_queue *parent_sq)
 		if (sq->nr_queued[0] || sq->nr_queued[1])
 			tg_update_disptime(tg);
 
-		if (nr_disp >= throtl_quantum)
+		if (nr_disp >= THROTL_QUANTUM)
 			break;
 	}

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-09-07 08:10 UTC #4 lore raw

PATCH 4/5 blk-throttle: Avoid calculating bps/iops limitation repeatedly

In reply to Baolin Wang

The tg_may_dispatch() will call tg_with_in_bps_limit() and tg_with_in_iops_limit() to check if we can dispatch a bio or not, which will calculate bps/iops limitation multiple times. But tg_may_dispatch() is always called under queue lock, which means the bps/iops limitation will not change in tg_may_dispatch().

So we can calculate the bps/iops limitation only once, and pass them to tg_with_in_bps_limit() and tg_with_in_iops_limit() to avoid calculating bps/iops limitation repeatedly.

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

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

block/blk-throttle.c +10 -9

index ca9002d..8719e37 100644
@@ -894,7 +894,7 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
 }
 
 static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
-				  unsigned long *wait)
+				  u32 iops_limit, unsigned long *wait)
 {
 	bool rw = bio_data_dir(bio);
 	unsigned int io_allowed;

@@ -913,7 +913,7 @@ static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 	 * have been trimmed.
 	 */
 
-	tmp = (u64)tg_iops_limit(tg, rw) * jiffy_elapsed_rnd;
+	tmp = (u64)iops_limit * jiffy_elapsed_rnd;
 	do_div(tmp, HZ);
 
 	if (tmp > UINT_MAX)

@@ -936,7 +936,7 @@ static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 }
 
 static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
-				 unsigned long *wait)
+				 u64 bps_limit, unsigned long *wait)
 {
 	bool rw = bio_data_dir(bio);
 	u64 bytes_allowed, extra_bytes, tmp;

@@ -951,7 +951,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice);
 
-	tmp = tg_bps_limit(tg, rw) * jiffy_elapsed_rnd;
+	tmp = bps_limit * jiffy_elapsed_rnd;
 	do_div(tmp, HZ);
 	bytes_allowed = tmp;

@@ -963,7 +963,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 
 	/* Calc approx time to dispatch */
 	extra_bytes = tg->bytes_disp[rw] + bio_size - bytes_allowed;
-	jiffy_wait = div64_u64(extra_bytes * HZ, tg_bps_limit(tg, rw));
+	jiffy_wait = div64_u64(extra_bytes * HZ, bps_limit);
 
 	if (!jiffy_wait)
 		jiffy_wait = 1;

@@ -987,6 +987,8 @@ static bool tg_may_dispatch(struct throtl_grp *tg, struct bio *bio,
 {
 	bool rw = bio_data_dir(bio);
 	unsigned long bps_wait = 0, iops_wait = 0, max_wait = 0;
+	u64 bps_limit = tg_bps_limit(tg, rw);
+	u32 iops_limit = tg_iops_limit(tg, rw);
 
 	/*
  	 * Currently whole state machine of group depends on first bio

@@ -998,8 +1000,7 @@ static bool tg_may_dispatch(struct throtl_grp *tg, struct bio *bio,
 	       bio != throtl_peek_queued(&tg->service_queue.queued[rw]));
 
 	/* If tg->bps = -1, then BW is unlimited */
-	if (tg_bps_limit(tg, rw) == U64_MAX &&
-	    tg_iops_limit(tg, rw) == UINT_MAX) {
+	if (bps_limit == U64_MAX && iops_limit == UINT_MAX) {
 		if (wait)
 			*wait = 0;
 		return true;

@@ -1021,8 +1022,8 @@ static bool tg_may_dispatch(struct throtl_grp *tg, struct bio *bio,
 				jiffies + tg->td->throtl_slice);
 	}
 
-	if (tg_with_in_bps_limit(tg, bio, &bps_wait) &&
-	    tg_with_in_iops_limit(tg, bio, &iops_wait)) {
+	if (tg_with_in_bps_limit(tg, bio, bps_limit, &bps_wait) &&
+	    tg_with_in_iops_limit(tg, bio, iops_limit, &iops_wait)) {
 		if (wait)
 			*wait = 0;
 		return true;

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-09-07 08:10 UTC #5 lore raw

PATCH 5/5 blk-throttle: Avoid checking bps/iops limitation if bps or iops is unlimited

In reply to Baolin Wang

Do not need check the bps or iops limitation if bps or iops is unlimited.

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

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

block/blk-throttle.c +12 -0

index 8719e37..36ba61c 100644
@@ -901,6 +901,12 @@ static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 	unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
 	u64 tmp;
 
+	if (iops_limit == UINT_MAX) {
+		if (wait)
+			*wait = 0;
+		return true;
+	}
+
 	jiffy_elapsed = jiffies - tg->slice_start[rw];
 
 	/* Round up to the next throttle slice, wait time must be nonzero */

@@ -943,6 +949,12 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 	unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
 	unsigned int bio_size = throtl_bio_data_size(bio);
 
+	if (bps_limit == U64_MAX) {
+		if (wait)
+			*wait = 0;
+		return true;
+	}
+
 	jiffy_elapsed = jiffy_elapsed_rnd = jiffies - tg->slice_start[rw];
 
 	/* Slice has just started. Consider one slice interval */

Signature · 1 line

1.8.3.1

Baolin Wang

author

2020-09-14 05:32 UTC #6 lore raw

In reply to Baolin Wang

Hi Tejun and Jens,

Quoted 5 lines · Baolin Wang

Hi All,
This patch set did some clean-ups, as well as removing some unnecessary bps/iops limitation calculation when checking if can dispatch a bio or not for a tg. Please help to review. Thanks.

Any comments for this patch set?

Quoted 14 lines

Baolin Wang (5):
blk-throttle: Fix some comments' typos
blk-throttle: Use readable READ/WRITE macros
blk-throttle: Define readable macros instead of static variables
blk-throttle: Avoid calculating bps/iops limitation repeatedly
blk-throttle: Avoid checking bps/iops limitation if bps or iops is
unlimited
 block/blk-throttle.c | 59 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 23 deletions(-)
Signature · 1 line
1.8.3.1

Jens Axboe

maintainer

2020-09-15 01:37 UTC #7 lore raw

In reply to Baolin Wang

Quoted 14 lines · Baolin Wang

Hi All,
This patch set did some clean-ups, as well as removing some unnecessary bps/iops limitation calculation when checking if can dispatch a bio or not for a tg. Please help to review. Thanks.
Baolin Wang (5):
blk-throttle: Fix some comments' typos
blk-throttle: Use readable READ/WRITE macros
blk-throttle: Define readable macros instead of static variables
blk-throttle: Avoid calculating bps/iops limitation repeatedly
blk-throttle: Avoid checking bps/iops limitation if bps or iops is
unlimited

Quoted 2 lines

 block/blk-throttle.c | 59 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 23 deletions(-)

Looks reasonable to me, I've applied it.

Out of curiosity, are you using blk-throttle in production, or are these just fixes/cleanups that you came across?

Signature · 1 line

Jens Axboe

Baolin Wang

author

2020-09-15 08:59 UTC #8 lore raw

Hi Jens,

Quoted 15 lines · Jens Axboe

Quoted 14 lines · Baolin Wang
Hi All,
This patch set did some clean-ups, as well as removing some unnecessary bps/iops limitation calculation when checking if can dispatch a bio or not for a tg. Please help to review. Thanks.
Baolin Wang (5):
blk-throttle: Fix some comments' typos
blk-throttle: Use readable READ/WRITE macros
blk-throttle: Define readable macros instead of static variables
blk-throttle: Avoid calculating bps/iops limitation repeatedly
blk-throttle: Avoid checking bps/iops limitation if bps or iops is
unlimited

Quoted 4 lines

Quoted 2 lines

 block/blk-throttle.c | 59 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 23 deletions(-)

Looks reasonable to me, I've applied it.

Thanks.

Quoted 3 lines

Out of curiosity, are you using blk-throttle in production, or are these just fixes/cleanups that you came across?

Yes, we're using it in some old products, and I am trying to do some cleanups and optimizaiton when testing it.

Jens Axboe

maintainer

2020-09-15 18:17 UTC #9 lore raw

Quoted 23 lines · Baolin Wang

Hi Jens,
Quoted 15 lines · Jens Axboe
Quoted 14 lines · Baolin Wang
Hi All,
This patch set did some clean-ups, as well as removing some unnecessary bps/iops limitation calculation when checking if can dispatch a bio or not for a tg. Please help to review. Thanks.
Baolin Wang (5):
blk-throttle: Fix some comments' typos
blk-throttle: Use readable READ/WRITE macros
blk-throttle: Define readable macros instead of static variables
blk-throttle: Avoid calculating bps/iops limitation repeatedly
blk-throttle: Avoid checking bps/iops limitation if bps or iops is
unlimited
Quoted 4 lines
Quoted 2 lines
 block/blk-throttle.c | 59 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 23 deletions(-)
Looks reasonable to me, I've applied it.

Quoted 8 lines

Thanks.
Quoted 3 lines
Out of curiosity, are you using blk-throttle in production, or are these just fixes/cleanups that you came across?
Yes, we're using it in some old products, and I am trying to do some cleanups and optimizaiton when testing it.

Gotcha. Reason I ask is I've been considering deprecating it, but when fixes come in for something, that always makes me think that some folks are actually using it.

Signature · 1 line

Jens Axboe