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(-)
--
1.8.3.1
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(+)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 8719e37..36ba61c 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -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 */
--
1.8.3.1
Use readable READ/WRITE macros instead of magic numbers.
Signed-off-by: Baolin Wang <[email protected]>
---
block/blk-throttle.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 2fc6c3e..06e73ed 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -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);
--
1.8.3.1
Fix some comments' typos.
Signed-off-by: Baolin Wang <[email protected]>
---
block/blk-throttle.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index fee3325..2fc6c3e 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -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];
--
1.8.3.1
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(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 06e73ed..ca9002d 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -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;
}
--
1.8.3.1
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(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index ca9002d..8719e37 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -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;
--
1.8.3.1
Hi Tejun and Jens,
On Mon, Sep 07, 2020 at 04:10:12PM +0800, Baolin Wang wrote:
> 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?
>
> 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(-)
>
> --
> 1.8.3.1
On 9/7/20 2:10 AM, Baolin Wang wrote:
> 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(-)
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?
--
Jens Axboe
Hi Jens,
On Mon, Sep 14, 2020 at 07:37:53PM -0600, Jens Axboe wrote:
> On 9/7/20 2:10 AM, Baolin Wang wrote:
> > 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(-)
>
> Looks reasonable to me, I've applied it.
Thanks.
>
> 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.
On 9/15/20 2:59 AM, Baolin Wang wrote:
> Hi Jens,
>
> On Mon, Sep 14, 2020 at 07:37:53PM -0600, Jens Axboe wrote:
>> On 9/7/20 2:10 AM, Baolin Wang wrote:
>>> 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(-)
>>
>> Looks reasonable to me, I've applied it.
>
> Thanks.
>
>>
>> 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.
--
Jens Axboe