[PATCH] blk-iocos: fix inuse clamp when iocg deactivate or free

[PATCH] blk-iocos: fix inuse clamp when iocg deactivate or free

[Chengming Zhou]

For an active leaf node, its inuse shouldn't be zero or exceed
its active, but it's not true when deactivate idle iocg or delete
iocg in ioc_pd_free().

Although inuse of 1 is very small, it could cause noticeable hwi
decrease in the long running server. So we'd better fix it.

And check iocg->child_active_sum is enough for inner iocg, remove
the needless list_empty check by the way.

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

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 2570732b92d1..84374ebcc402 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1073,11 +1073,11 @@ static void __propagate_weights(struct ioc_gq *iocg, u32 active, u32 inuse,
 	 * @active. An active internal node's inuse is solely determined by the
 	 * inuse to active ratio of its children regardless of @inuse.
 	 */
-	if (list_empty(&iocg->active_list) && iocg->child_active_sum) {
+	if (iocg->child_active_sum) {
 		inuse = DIV64_U64_ROUND_UP(active * iocg->child_inuse_sum,
 					   iocg->child_active_sum);
 	} else {
-		inuse = clamp_t(u32, inuse, 1, active);
+		inuse = clamp_t(u32, inuse, 0, active);
 	}
 
 	iocg->last_inuse = iocg->inuse;
-- 
2.36.1

Re: [PATCH] blk-iocos: fix inuse clamp when iocg deactivate or free

[Chengming Zhou]

On 2022/5/16 18:19, Chengming Zhou wrote:
> For an active leaf node, its inuse shouldn't be zero or exceed
> its active, but it's not true when deactivate idle iocg or delete
> iocg in ioc_pd_free().
> 
> Although inuse of 1 is very small, it could cause noticeable hwi
> decrease in the long running server. So we'd better fix it.
> 
> And check iocg->child_active_sum is enough for inner iocg, remove
> the needless list_empty check by the way.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
>  block/blk-iocost.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 2570732b92d1..84374ebcc402 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -1073,11 +1073,11 @@ static void __propagate_weights(struct ioc_gq *iocg, u32 active, u32 inuse,
>  	 * @active. An active internal node's inuse is solely determined by the
>  	 * inuse to active ratio of its children regardless of @inuse.
>  	 */
> -	if (list_empty(&iocg->active_list) && iocg->child_active_sum) {
> +	if (iocg->child_active_sum) {
>  		inuse = DIV64_U64_ROUND_UP(active * iocg->child_inuse_sum,
>  					   iocg->child_active_sum);
>  	} else {
> -		inuse = clamp_t(u32, inuse, 1, active);
> +		inuse = clamp_t(u32, inuse, 0, active);

I found the commit message is wrong after a second look at the test data,
inuse value will be zero when active is zero, since:

#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)

So clamp_t(u32, 0, 1, 0) will return 0, deactivate idle iocg or delete iocg
will set its inuse to zero correctly.

The inuse -> 1 happened in the test data turn out to be iocg_incur_debt(),
which call __propagate_weights() with active = weight, inuse = 0, so
clamp_t(u32, 0, 1, active) return 1.

Then this effect is very small, unlikely to have an impact in practice. Should
I modify the commit message to send v2 or just drop it?

Thanks.

>  	}
>  
>  	iocg->last_inuse = iocg->inuse;

Re: [PATCH] blk-iocos: fix inuse clamp when iocg deactivate or free

[Jens Axboe]

On 5/16/22 6:18 AM, Chengming Zhou wrote:
> Then this effect is very small, unlikely to have an impact in
> practice. Should I modify the commit message to send v2 or just drop
> it?

Send a v2 please.

-- 
Jens Axboe

Re: [PATCH] blk-iocos: fix inuse clamp when iocg deactivate or free

[Tejun Heo]

On Mon, May 16, 2022 at 06:19:09PM +0800, Chengming Zhou wrote:
> For an active leaf node, its inuse shouldn't be zero or exceed
> its active, but it's not true when deactivate idle iocg or delete
> iocg in ioc_pd_free().
> 
> Although inuse of 1 is very small, it could cause noticeable hwi
> decrease in the long running server. So we'd better fix it.
> 
> And check iocg->child_active_sum is enough for inner iocg, remove
> the needless list_empty check by the way.

Hey, so, I'm not a fan of these "I read code a bit and thought this could be
changed here and there" patches. There's no theme, overarching direction, or
comprehensive view of the structure. The suggested changes can often be
really subtle, which is likely why it may not seem immediately intuitive on
the first look and triggered the submitter to write up the patch. There's no
practical gain from these changes while there's substantical risk of subtle
breakages.

Here, setting inuse to 1 would cause divide by one in the donation logic and
there are comments about the in the code too. So, nack on the patch, and
plase reconsider your approach to sending patches. The current approach
costs more than helps.

Thanks.

-- 
tejun