| Message ID | 20240514125254.142203-9-shikemeng@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add helper functions to remove repeated code and improve readability of cgroup writeback | expand |
Hello, On Tue, May 14, 2024 at 08:52:54PM +0800, Kemeng Shi wrote: > +static void balance_wb_limits(struct dirty_throttle_control *dtc, > + bool strictlimit) > +{ > + wb_dirty_freerun(dtc, strictlimit); > + if (dtc->freerun) > + return; > + > + wb_dirty_exceeded(dtc, strictlimit); > + wb_position_ratio(dtc); > +} ... > @@ -1869,12 +1880,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb, > * Calculate global domain's pos_ratio and select the > * global dtc by default. > */ > - wb_dirty_freerun(gdtc, strictlimit); > + balance_wb_limits(gdtc, strictlimit); > if (gdtc->freerun) > goto free_running; > - > - wb_dirty_exceeded(gdtc, strictlimit); > - wb_position_ratio(gdtc); > sdtc = gdtc; Isn't this a bit nasty? The helper skips updating states because it knows the caller is not going to use them? I'm not sure the slight code reduction justifies the added subtlety. Thanks.
Hello, on 5/31/2024 2:33 AM, Tejun Heo wrote: > Hello, > > On Tue, May 14, 2024 at 08:52:54PM +0800, Kemeng Shi wrote: >> +static void balance_wb_limits(struct dirty_throttle_control *dtc, >> + bool strictlimit) >> +{ >> + wb_dirty_freerun(dtc, strictlimit); >> + if (dtc->freerun) >> + return; >> + >> + wb_dirty_exceeded(dtc, strictlimit); >> + wb_position_ratio(dtc); >> +} > ... >> @@ -1869,12 +1880,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb, >> * Calculate global domain's pos_ratio and select the >> * global dtc by default. >> */ >> - wb_dirty_freerun(gdtc, strictlimit); >> + balance_wb_limits(gdtc, strictlimit); >> if (gdtc->freerun) >> goto free_running; >> - >> - wb_dirty_exceeded(gdtc, strictlimit); >> - wb_position_ratio(gdtc); >> sdtc = gdtc; > > Isn't this a bit nasty? The helper skips updating states because it knows > the caller is not going to use them? I'm not sure the slight code reduction > justifies the added subtlety. It's a general rule that wb should not be limited if the wb is in freerun state. So I think it's intuitive to obey the rule in both balance_wb_limits and it's caller in which case balance_wb_limits and it's caller should stop to do anything when freerun state of wb is first seen. But no insistant on this... Thanks. > > Thanks. >
Hello, On Mon, Jun 03, 2024 at 02:39:18PM +0800, Kemeng Shi wrote: > > Isn't this a bit nasty? The helper skips updating states because it knows > > the caller is not going to use them? I'm not sure the slight code reduction > > justifies the added subtlety. > > It's a general rule that wb should not be limited if the wb is in freerun state. > So I think it's intuitive to obey the rule in both balance_wb_limits and it's > caller in which case balance_wb_limits and it's caller should stop to do anything > when freerun state of wb is first seen. > But no insistant on this... Hmm... can you at least add comments pointing out that if freerun, the limits fields are invalid? Thanks.
on 6/4/2024 2:09 AM, Tejun Heo wrote: > Hello, > > On Mon, Jun 03, 2024 at 02:39:18PM +0800, Kemeng Shi wrote: >>> Isn't this a bit nasty? The helper skips updating states because it knows >>> the caller is not going to use them? I'm not sure the slight code reduction >>> justifies the added subtlety. >> >> It's a general rule that wb should not be limited if the wb is in freerun state. >> So I think it's intuitive to obey the rule in both balance_wb_limits and it's >> caller in which case balance_wb_limits and it's caller should stop to do anything >> when freerun state of wb is first seen. >> But no insistant on this... > > Hmm... can you at least add comments pointing out that if freerun, the > limits fields are invalid? Sure, will add it in next version. Thanks > > Thanks. >
diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 0f1f3e179be2..d1d385373c5b 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -1783,6 +1783,17 @@ static inline void wb_dirty_exceeded(struct dirty_throttle_control *dtc, ((dtc->dirty > dtc->thresh) || strictlimit); } +static void balance_wb_limits(struct dirty_throttle_control *dtc, + bool strictlimit) +{ + wb_dirty_freerun(dtc, strictlimit); + if (dtc->freerun) + return; + + wb_dirty_exceeded(dtc, strictlimit); + wb_position_ratio(dtc); +} + /* * balance_dirty_pages() must be called by processes which are generating dirty * data. It looks at the number of dirty pages in the machine and will force @@ -1869,12 +1880,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb, * Calculate global domain's pos_ratio and select the * global dtc by default. */ - wb_dirty_freerun(gdtc, strictlimit); + balance_wb_limits(gdtc, strictlimit); if (gdtc->freerun) goto free_running; - - wb_dirty_exceeded(gdtc, strictlimit); - wb_position_ratio(gdtc); sdtc = gdtc; if (mdtc) { @@ -1884,12 +1892,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb, * both global and memcg domains. Choose the one * w/ lower pos_ratio. */ - wb_dirty_freerun(mdtc, strictlimit); + balance_wb_limits(mdtc, strictlimit); if (mdtc->freerun) goto free_running; - - wb_dirty_exceeded(mdtc, strictlimit); - wb_position_ratio(mdtc); if (mdtc->pos_ratio < gdtc->pos_ratio) sdtc = mdtc; }
Factor out balance_wb_limits to remove repeated code Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- mm/page-writeback.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)