Hi, this series contain a few patches to fix problem when on the default
hierarchy, corrent comment and so on. More detail can be found in
respective changelogs. Thanks.
Kemeng Shi (11):
blk-throttle: Limit whole system if root group is configured when on
the default hierarchy
blk-throttle: Fix that bps of child could exceed bps limited in parent
blk-throttle: ignore cgroup without io queued in
blk_throtl_cancel_bios
blk-throttle: correct calculation of wait time in tg_may_dispatch
blk-throttle: simpfy low limit reached check in throtl_tg_can_upgrade
blk-throttle: fix typo in comment of throtl_adjusted_limit
blk-throttle: remove incorrect comment for tg_last_low_overflow_time
blk-throttle: remove repeat check of elapsed time from last upgrade in
throtl_hierarchy_can_downgrade
blk-throttle: remove unnecessary check for validation of limit index
blk-throttle: remove unused variable td in tg_update_has_rules
blk-throttle: Use more siutable time_after check for update
slice_start
block/blk-throttle.c | 98 ++++++++++++++++++--------------------------
1 file changed, 39 insertions(+), 59 deletions(-)
--
2.30.0
Consider situation as following (on the default hierarchy):
HDD
|
root (bps limit: 4k)
|
child (bps limit :8k)
|
fio bs=8k
Rate of fio is supposed to be 4k, but result is 8k. Reason is as
following:
Size of single IO from fio is larger than bytes allowed in one
throtl_slice in child, so IOs are always queued in child group first.
When queued IOs in child are dispatched to parent group, BIO_BPS_THROTTLED
is set and these IOs will not be limited by tg_within_bps_limit anymore.
Fix this by:
1. Limit IO with BIO_BPS_THROTTLED flag in tg_within_bps_limit.
2. Ignore BIO_BPS_THROTTLED flag when accouting in throtl_charge_bio.
There changes have no influence on situation which is not on the default
hierarchy as each group is a single root group without parent.
Signed-off-by: Kemeng Shi <[email protected]>
---
block/blk-throttle.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 96aa53e30e28..b33bcf53b36e 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -856,8 +856,7 @@ static bool tg_within_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);
- /* no need to throttle if this bio's bytes have been accounted */
- if (bps_limit == U64_MAX || bio_flagged(bio, BIO_BPS_THROTTLED)) {
+ if (bps_limit == U64_MAX) {
if (wait)
*wait = 0;
return true;
@@ -964,10 +963,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
unsigned int bio_size = throtl_bio_data_size(bio);
/* Charge the bio to the group */
- if (!bio_flagged(bio, BIO_BPS_THROTTLED)) {
- tg->bytes_disp[rw] += bio_size;
- tg->last_bytes_disp[rw] += bio_size;
- }
+ tg->bytes_disp[rw] += bio_size;
+ tg->last_bytes_disp[rw] += bio_size;
tg->io_disp[rw]++;
tg->last_io_disp[rw]++;
--
2.30.0
There is no need to check elapsed time from last upgrade for each node in
hierarchy. Move this check before traversing as throtl_can_upgrade do
to remove repeat check.
Signed-off-by: Kemeng Shi <[email protected]>
---
block/blk-throttle.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index bd07f562c799..3eccc7af4368 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1932,8 +1932,7 @@ static bool throtl_tg_can_downgrade(struct throtl_grp *tg)
* If cgroup is below low limit, consider downgrade and throttle other
* cgroups
*/
- if (time_after_eq(now, td->low_upgrade_time + td->throtl_slice) &&
- time_after_eq(now, tg_last_low_overflow_time(tg) +
+ if (time_after_eq(now, tg_last_low_overflow_time(tg) +
td->throtl_slice) &&
(!throtl_tg_is_idle(tg) ||
!list_empty(&tg_to_blkg(tg)->blkcg->css.children)))
@@ -1943,6 +1942,9 @@ static bool throtl_tg_can_downgrade(struct throtl_grp *tg)
static bool throtl_hierarchy_can_downgrade(struct throtl_grp *tg)
{
+ if (time_before(now, tg->td->low_upgrade_time + td->throtl_slice))
+ return false;
+
while (true) {
if (!throtl_tg_can_downgrade(tg))
return false;
--
2.30.0
Use more siutable time_after check for update slice_start
Signed-off-by: Kemeng Shi <[email protected]>
---
block/blk-throttle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 82fe23e79b4b..69eeff764dee 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -635,7 +635,7 @@ static inline void throtl_start_new_slice_with_credit(struct throtl_grp *tg,
* that bandwidth. Do try to make use of that bandwidth while giving
* credit.
*/
- if (time_after_eq(start, tg->slice_start[rw]))
+ if (time_after(start, tg->slice_start[rw]))
tg->slice_start[rw] = start;
tg->slice_end[rw] = jiffies + tg->td->throtl_slice;
--
2.30.0
Quoted from comment in throtl_pd_init: "If on the default hierarchy, we
switch to properly hierarchical behavior where limits on a given
throtl_grp are applied to the whole subtree rather than just the group
itself. e.g. If 16M read_bps limit is set on the root group, the whole
system can' exceed 16M for the device."
Commit b22c417c885ea9 ("blk-throttle: configure bps/iops limit for
cgroup in low limit") broke this rule and did not explain why.
Restore the ability to limit the whole system by root group.
Signed-off-by: Kemeng Shi <[email protected]>
---
block/blk-throttle.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 847721dc2b2b..96aa53e30e28 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -150,9 +150,6 @@ static uint64_t tg_bps_limit(struct throtl_grp *tg, int rw)
struct throtl_data *td;
uint64_t ret;
- if (cgroup_subsys_on_dfl(io_cgrp_subsys) && !blkg->parent)
- return U64_MAX;
-
td = tg->td;
ret = tg->bps[rw][td->limit_index];
if (ret == 0 && td->limit_index == LIMIT_LOW) {
@@ -180,9 +177,6 @@ static unsigned int tg_iops_limit(struct throtl_grp *tg, int rw)
struct throtl_data *td;
unsigned int ret;
- if (cgroup_subsys_on_dfl(io_cgrp_subsys) && !blkg->parent)
- return UINT_MAX;
-
td = tg->td;
ret = tg->iops[rw][td->limit_index];
if (ret == 0 && tg->td->limit_index == LIMIT_LOW) {
--
2.30.0
lapsed time -> elapsed time
Signed-off-by: Kemeng Shi <[email protected]>
---
block/blk-throttle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 322a6ee38fb6..0aa21832cb96 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -129,7 +129,7 @@ static struct throtl_data *sq_to_td(struct throtl_service_queue *sq)
/*
* cgroup's limit in LIMIT_MAX is scaled if low limit is set. This scale is to
* make the IO dispatch more smooth.
- * Scale up: linearly scale up according to lapsed time since upgrade. For
+ * Scale up: linearly scale up according to elapsed time since upgrade. For
* every throtl_slice, the limit scales up 1/2 .low limit till the
* limit hits .max limit
* Scale down: exponentially scale down if a cgroup doesn't hit its .low limit
--
2.30.0
Cgroup reaches low limit if limit is zero or io is already queued. Cgroup
will pass upgrade check if low limits of READ and WRITE are both reached.
Simpfy the check code described above to removce repeat check and improve
readability. There is no functional change.
Signed-off-by: Kemeng Shi <[email protected]>
---
block/blk-throttle.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 01e30380c19b..322a6ee38fb6 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1800,24 +1800,22 @@ static bool throtl_tg_is_idle(struct throtl_grp *tg)
return ret;
}
-static bool throtl_tg_can_upgrade(struct throtl_grp *tg)
+static bool throtl_tg_reach_low_limit(struct throtl_grp *tg, int rw)
{
struct throtl_service_queue *sq = &tg->service_queue;
- bool read_limit, write_limit;
+ bool limit = tg->bps[rw][LIMIT_LOW] || tg->iops[rw][LIMIT_LOW];
/*
* if cgroup reaches low limit (if low limit is 0, the cgroup always
* reaches), it's ok to upgrade to next limit
*/
- read_limit = tg->bps[READ][LIMIT_LOW] || tg->iops[READ][LIMIT_LOW];
- write_limit = tg->bps[WRITE][LIMIT_LOW] || tg->iops[WRITE][LIMIT_LOW];
- if (!read_limit && !write_limit)
- return true;
- if (read_limit && sq->nr_queued[READ] &&
- (!write_limit || sq->nr_queued[WRITE]))
- return true;
- if (write_limit && sq->nr_queued[WRITE] &&
- (!read_limit || sq->nr_queued[READ]))
+ return !limit || sq->nr_queued[rw].
+}
+
+static bool throtl_tg_can_upgrade(struct throtl_grp *tg)
+{
+ if (throtl_tg_reach_low_limit(tg, READ) &&
+ throtl_tg_reach_low_limit(tg, WRITE))
return true;
if (time_after_eq(jiffies,
--
2.30.0
Function tg_last_low_overflow_time is called with intermediate node as
following:
throtl_hierarchy_can_downgrade
throtl_tg_can_downgrade
tg_last_low_overflow_time
throtl_hierarchy_can_upgrade
throtl_tg_can_upgrade
tg_last_low_overflow_time
throtl_hierarchy_can_downgrade/throtl_hierarchy_can_upgrade will traverse
from leaf node to sub-root node and pass traversed intermediate node
to tg_last_low_overflow_time.
No such limit could be found from context and implementation of
tg_last_low_overflow_time, so remove this limit in comment.
Signed-off-by: Kemeng Shi <[email protected]>
---
block/blk-throttle.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 0aa21832cb96..bd07f562c799 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1746,7 +1746,6 @@ static unsigned long __tg_last_low_overflow_time(struct throtl_grp *tg)
return min(rtime, wtime);
}
-/* tg should not be an intermediate node */
static unsigned long tg_last_low_overflow_time(struct throtl_grp *tg)
{
struct throtl_service_queue *parent_sq;
--
2.30.0
remove unused variable td in tg_update_has_rules
Signed-off-by: Kemeng Shi <[email protected]>
---
block/blk-throttle.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 6f509cadd92b..82fe23e79b4b 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -412,7 +412,6 @@ static void throtl_pd_init(struct blkg_policy_data *pd)
static void tg_update_has_rules(struct throtl_grp *tg)
{
struct throtl_grp *parent_tg = sq_to_tg(tg->service_queue.parent_sq);
- struct throtl_data *td = tg->td;
int rw;
for (rw = READ; rw <= WRITE; rw++) {
--
2.30.0
On Wed, Nov 23, 2022 at 02:03:51PM +0800, Kemeng Shi wrote:
> Quoted from comment in throtl_pd_init: "If on the default hierarchy, we
> switch to properly hierarchical behavior where limits on a given
> throtl_grp are applied to the whole subtree rather than just the group
> itself. e.g. If 16M read_bps limit is set on the root group, the whole
> system can' exceed 16M for the device."
On the default hierarchy (cgroup2), the throttle interface files don't exist
in the root cgroup. In general, cgroup doesn't wanna be in the business of
restricting resources at the system level.
Thanks.
--
tejun
On Wed, Nov 23, 2022 at 02:03:52PM +0800, Kemeng Shi wrote:
> @@ -964,10 +963,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
> unsigned int bio_size = throtl_bio_data_size(bio);
>
> /* Charge the bio to the group */
> - if (!bio_flagged(bio, BIO_BPS_THROTTLED)) {
> - tg->bytes_disp[rw] += bio_size;
> - tg->last_bytes_disp[rw] += bio_size;
> - }
> + tg->bytes_disp[rw] += bio_size;
> + tg->last_bytes_disp[rw] += bio_size;
Are you sure this isn't gonna lead to double accounting? IIRC, the primary
purpose of this flag is avoiding duplicate accounting of bios which end up
going through the throttling path multiple times for whatever reason and
we've had numerous breakages around it.
To address the problem you're describing in this patch, wouldn't it make
more sense to set the flag only when the bio traversed the entire tree
rather than after each tg?
Thanks.
--
tejun
On Wed, Nov 23, 2022 at 02:03:56PM +0800, Kemeng Shi wrote:
> lapsed time -> elapsed time
>
> Signed-off-by: Kemeng Shi <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
On Wed, Nov 23, 2022 at 02:04:01PM +0800, Kemeng Shi wrote:
> Use more siutable time_after check for update slice_start
Why is it more suitable?
--
tejun
On Wed, Nov 23, 2022 at 02:04:00PM +0800, Kemeng Shi wrote:
> remove unused variable td in tg_update_has_rules
Please merge this into the prev patch.
--
tejun
On Wed, Nov 23, 2022 at 02:03:57PM +0800, Kemeng Shi wrote:
> Function tg_last_low_overflow_time is called with intermediate node as
> following:
> throtl_hierarchy_can_downgrade
> throtl_tg_can_downgrade
> tg_last_low_overflow_time
>
> throtl_hierarchy_can_upgrade
> throtl_tg_can_upgrade
> tg_last_low_overflow_time
>
> throtl_hierarchy_can_downgrade/throtl_hierarchy_can_upgrade will traverse
> from leaf node to sub-root node and pass traversed intermediate node
> to tg_last_low_overflow_time.
>
> No such limit could be found from context and implementation of
> tg_last_low_overflow_time, so remove this limit in comment.
>
> Signed-off-by: Kemeng Shi <[email protected]>
Acked-by: Tejun Heo <[email protected]>
--
tejun
On Wed, Nov 23, 2022 at 02:03:55PM +0800, Kemeng Shi wrote:
> -static bool throtl_tg_can_upgrade(struct throtl_grp *tg)
> +static bool throtl_tg_reach_low_limit(struct throtl_grp *tg, int rw)
> {
> struct throtl_service_queue *sq = &tg->service_queue;
> - bool read_limit, write_limit;
> + bool limit = tg->bps[rw][LIMIT_LOW] || tg->iops[rw][LIMIT_LOW];
>
> /*
> * if cgroup reaches low limit (if low limit is 0, the cgroup always
> * reaches), it's ok to upgrade to next limit
> */
> - read_limit = tg->bps[READ][LIMIT_LOW] || tg->iops[READ][LIMIT_LOW];
> - write_limit = tg->bps[WRITE][LIMIT_LOW] || tg->iops[WRITE][LIMIT_LOW];
> - if (!read_limit && !write_limit)
> - return true;
> - if (read_limit && sq->nr_queued[READ] &&
> - (!write_limit || sq->nr_queued[WRITE]))
> - return true;
> - if (write_limit && sq->nr_queued[WRITE] &&
> - (!read_limit || sq->nr_queued[READ]))
> + return !limit || sq->nr_queued[rw].
> +}
> +
> +static bool throtl_tg_can_upgrade(struct throtl_grp *tg)
> +{
> + if (throtl_tg_reach_low_limit(tg, READ) &&
> + throtl_tg_reach_low_limit(tg, WRITE))
Are the conditions being checked actually equivalent? If so, can you
explicitly explain that these are equivalent conditions? If not, what are we
changing exactly?
Thanks.
--
tejun
On Wed, Nov 23, 2022 at 02:03:58PM +0800, Kemeng Shi wrote:
> static bool throtl_hierarchy_can_downgrade(struct throtl_grp *tg)
> {
> + if (time_before(now, tg->td->low_upgrade_time + td->throtl_slice))
> + return false;
Does this even build? Where is td defined?
--
tejun
on 11/24/2022 1:11 AM, Tejun Heo wrote:
> On Wed, Nov 23, 2022 at 02:03:51PM +0800, Kemeng Shi wrote:
>> Quoted from comment in throtl_pd_init: "If on the default hierarchy, we
>> switch to properly hierarchical behavior where limits on a given
>> throtl_grp are applied to the whole subtree rather than just the group
>> itself. e.g. If 16M read_bps limit is set on the root group, the whole
>> system can' exceed 16M for the device."
>
> On the default hierarchy (cgroup2), the throttle interface files don't exist
> in the root cgroup. In general, cgroup doesn't wanna be in the business of
> restricting resources at the system level.
Hi, Tejun. Thanks for review. If restricting is not needed anymore, the stale
comment "e.g. If 16M read_bps limit is set on the root group, the whole system
can' exceed 16M for the device." may better be remove. I will remove this
in next version.
--
Best wishes
Kemeng Shi
on 11/24/2022 2:09 AM, Tejun Heo wrote:
> On Wed, Nov 23, 2022 at 02:03:52PM +0800, Kemeng Shi wrote:
>> @@ -964,10 +963,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
>> unsigned int bio_size = throtl_bio_data_size(bio);
>>
>> /* Charge the bio to the group */
>> - if (!bio_flagged(bio, BIO_BPS_THROTTLED)) {
>> - tg->bytes_disp[rw] += bio_size;
>> - tg->last_bytes_disp[rw] += bio_size;
>> - }
>> + tg->bytes_disp[rw] += bio_size;
>> + tg->last_bytes_disp[rw] += bio_size;
>
> Are you sure this isn't gonna lead to double accounting? IIRC, the primary
> purpose of this flag is avoiding duplicate accounting of bios which end up
> going through the throttling path multiple times for whatever reason and
> we've had numerous breakages around it.
Sorry for the mistake, this change does lead to double accounting.
> To address the problem you're describing in this patch, wouldn't it make
> more sense to set the flag only when the bio traversed the entire tree
> rather than after each tg?
I will address the problem in this way in next version. Thanks for the
advice.
--
Best wishes
Kemeng Shi
on 11/24/2022 2:26 AM, Tejun Heo wrote:
> On Wed, Nov 23, 2022 at 02:03:55PM +0800, Kemeng Shi wrote:
>> -static bool throtl_tg_can_upgrade(struct throtl_grp *tg)
>> +static bool throtl_tg_reach_low_limit(struct throtl_grp *tg, int rw)
>> {
>> struct throtl_service_queue *sq = &tg->service_queue;
>> - bool read_limit, write_limit;
>> + bool limit = tg->bps[rw][LIMIT_LOW] || tg->iops[rw][LIMIT_LOW];
>>
>> /*
>> * if cgroup reaches low limit (if low limit is 0, the cgroup always
>> * reaches), it's ok to upgrade to next limit
>> */
>> - read_limit = tg->bps[READ][LIMIT_LOW] || tg->iops[READ][LIMIT_LOW];
>> - write_limit = tg->bps[WRITE][LIMIT_LOW] || tg->iops[WRITE][LIMIT_LOW];
>> - if (!read_limit && !write_limit)
>> - return true;
>> - if (read_limit && sq->nr_queued[READ] &&
>> - (!write_limit || sq->nr_queued[WRITE]))
>> - return true;
>> - if (write_limit && sq->nr_queued[WRITE] &&
>> - (!read_limit || sq->nr_queued[READ]))
>> + return !limit || sq->nr_queued[rw].
>> +}
>> +
>> +static bool throtl_tg_can_upgrade(struct throtl_grp *tg)
>> +{
>> + if (throtl_tg_reach_low_limit(tg, READ) &&
>> + throtl_tg_reach_low_limit(tg, WRITE))
>
> Are the conditions being checked actually equivalent? If so, can you
> explicitly explain that these are equivalent conditions? If not, what are we
> changing exactly?All replaced conditions to return true are as following:
condition 1
(!read_limit && !write_limit)
condition 2
read_limit && sq->nr_queued[READ] && (!write_limit || sq->nr_queued[WRITE])
condition 3
write_limit && sq->nr_queued[WRITE] && (!read_limit || sq->nr_queued[READ])
Transfering condition 2 as following:
read_limit && sq->nr_queued[READ] && (!write_limit || sq->nr_queued[WRITE])
is equivalent to
(read_limit && sq->nr_queued[READ]) &&
(!write_limit || (write_limit && sq->nr_queued[WRITE]))
is equivalent to
(read_limit && sq->nr_queued[READ] && !write_limit) ||
((read_limit && sq->nr_queued[READ] && (write_limit && sq->nr_queued[WRITE]))
Transfering condition 3 as following:
write_limit && sq->nr_queued[WRITE] && (!read_limit || sq->nr_queued[READ])
is equivalent to
(write_limit && sq->nr_queued[WRITE]) &&
(!read_limit || (read_limit && sq->nr_queued[READ]))
is equivalent to
((write_limit && sq->nr_queued[WRITE]) && !read_limit) ||
((write_limit && sq->nr_queued[WRITE]) && (read_limit && sq->nr_queued[READ]))
All replaced conditions to return true are collected as folloing:
condition 1.1
(!read_limit && !write_limit)
condition 1.2
(read_limit && sq->nr_queued[READ] && !write_limit)
condition 1.3
((read_limit && sq->nr_queued[READ] && (write_limit && sq->nr_queued[WRITE]))
condition 1.4
(write_limit && sq->nr_queued[WRITE]) && !read_limit)
condition 1.5 (the same as 1.3, can be ingored)
(write_limit && sq->nr_queued[WRITE]) && (read_limit && sq->nr_queued[READ]))
Condtions to return true in this patch is:
(!read_limit || (read_limit && sq->nr_queued[READ])) &&
(!write_limit || (write_limit && sq->nr_queued[WRITE]))
As "(a || b) && (c || d)" can be extracted to
(a && c) or (a && d) or (b && c) or (b && d ). So we can extract condtions to
condition 2.1
!read_limit && !write_limit
condition 2.2
!read_limit && (write_limit && sq->nr_queued[WRITE])
condition 2.3
(read_limit && sq->nr_queued[READ]) && !write_limit
condition 2.4
(read_limit && sq->nr_queued[READ]) && (write_limit && sq->nr_queued[WRITE])
Conditions match as following:
condition 1.1 = condition 2.1
condition 1.2 = condition 2.3
condition 1.3 = condition 2.4
condition 1.4 = condition 2.2
--
Best wishes
Kemeng Shi
on 11/24/2022 2:29 AM, Tejun Heo wrote:
> On Wed, Nov 23, 2022 at 02:04:01PM +0800, Kemeng Shi wrote:
>> Use more siutable time_after check for update slice_start
>
> Why is it more suitable?
There is no need to assign tg->slice_start[rw] to start when they are
equal already. So time_after seems more suitable here.
--
Best wishes
Kemeng Shi
on 11/24/2022 2:28 AM, Tejun Heo wrote:
> On Wed, Nov 23, 2022 at 02:03:58PM +0800, Kemeng Shi wrote:
>> static bool throtl_hierarchy_can_downgrade(struct throtl_grp *tg)
>> {
>> + if (time_before(now, tg->td->low_upgrade_time + td->throtl_slice))
>> + return false;
>
> Does this even build? Where is td defined?
Sorry for this mistake, CONFIG_BLK_DEV_THROTTLING_LOW seems not be set on
default. I will fix this and build with CONFIG_BLK_DEV_THROTTLING_LOW
set.
--
Best wishes
Kemeng Shi
Hi Kemeng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on linus/master v6.1-rc6 next-20221125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kemeng-Shi/A-few-bugfix-and-cleanup-patches-for-blk-throttle/20221123-140704
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link: https://lore.kernel.org/r/20221123060401.20392-6-shikemeng%40huawei.com
patch subject: [PATCH 05/11] blk-throttle: simpfy low limit reached check in throtl_tg_can_upgrade
config: x86_64-randconfig-a005
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/79d5eb2da90b359d735d434c745a5d6f9c1a690c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kemeng-Shi/A-few-bugfix-and-cleanup-patches-for-blk-throttle/20221123-140704
git checkout 79d5eb2da90b359d735d434c745a5d6f9c1a690c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> block/blk-throttle.c:1813:1: error: expected identifier
}
^
1 error generated.
vim +1813 block/blk-throttle.c
1802
1803 static bool throtl_tg_reach_low_limit(struct throtl_grp *tg, int rw)
1804 {
1805 struct throtl_service_queue *sq = &tg->service_queue;
1806 bool limit = tg->bps[rw][LIMIT_LOW] || tg->iops[rw][LIMIT_LOW];
1807
1808 /*
1809 * if cgroup reaches low limit (if low limit is 0, the cgroup always
1810 * reaches), it's ok to upgrade to next limit
1811 */
1812 return !limit || sq->nr_queued[rw].
> 1813 }
1814
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Kemeng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on linus/master v6.1-rc6 next-20221125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kemeng-Shi/A-few-bugfix-and-cleanup-patches-for-blk-throttle/20221123-140704
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link: https://lore.kernel.org/r/20221123060401.20392-9-shikemeng%40huawei.com
patch subject: [PATCH 08/11] blk-throttle: remove repeat check of elapsed time from last upgrade in throtl_hierarchy_can_downgrade
config: x86_64-randconfig-a005
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f01c7d5bc75515a315dff091b1b1aa1ea1ef12fc
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kemeng-Shi/A-few-bugfix-and-cleanup-patches-for-blk-throttle/20221123-140704
git checkout f01c7d5bc75515a315dff091b1b1aa1ea1ef12fc
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
block/blk-throttle.c:1812:1: error: expected identifier
}
^
>> block/blk-throttle.c:1945:50: error: use of undeclared identifier 'td'
if (time_before(now, tg->td->low_upgrade_time + td->throtl_slice))
^
>> block/blk-throttle.c:1945:18: error: use of undeclared identifier 'now'
if (time_before(now, tg->td->low_upgrade_time + td->throtl_slice))
^
>> block/blk-throttle.c:1945:18: error: use of undeclared identifier 'now'
>> block/blk-throttle.c:1945:50: error: use of undeclared identifier 'td'
if (time_before(now, tg->td->low_upgrade_time + td->throtl_slice))
^
5 errors generated.
vim +/td +1945 block/blk-throttle.c
1942
1943 static bool throtl_hierarchy_can_downgrade(struct throtl_grp *tg)
1944 {
> 1945 if (time_before(now, tg->td->low_upgrade_time + td->throtl_slice))
1946 return false;
1947
1948 while (true) {
1949 if (!throtl_tg_can_downgrade(tg))
1950 return false;
1951 tg = sq_to_tg(tg->service_queue.parent_sq);
1952 if (!tg || !tg_to_blkg(tg)->parent)
1953 break;
1954 }
1955 return true;
1956 }
1957
--
0-DAY CI Kernel Test Service
https://01.org/lkp