| Message ID | 20220516101909.99768-1-zhouchengming@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-iocos: fix inuse clamp when iocg deactivate or free | expand |
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;
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.
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.
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;
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(-)