| Message ID | 20231219024246.65654-3-jefflexu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: fix arithmetic for bdi min_ratio and | expand |
On Tue, Dec 19, 2023 at 10:42:46AM +0800, Jingbo Xu wrote: > } else { > bdi->max_ratio = max_ratio; > - bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / 100; > + bdi->max_prop_frac = div64_u64(FPROP_FRAC_BASE * max_ratio, > + 100 * BDI_RATIO_SCALE); > } Why use div64_u64 here? FPROP_FRAC_BASE is an unsigned long. max_ratio is an unsigned int, so the numerator is an unsigned long. BDI_RATIO_SCALE is 10,000, so the numerator is an unsigned int. There's no 64-bit arithmetic needed here.
On 12/19/23 12:06 PM, Matthew Wilcox wrote: > On Tue, Dec 19, 2023 at 10:42:46AM +0800, Jingbo Xu wrote: >> } else { >> bdi->max_ratio = max_ratio; >> - bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / 100; >> + bdi->max_prop_frac = div64_u64(FPROP_FRAC_BASE * max_ratio, >> + 100 * BDI_RATIO_SCALE); >> } > > Why use div64_u64 here? > > FPROP_FRAC_BASE is an unsigned long. max_ratio is an unsigned int, so > the numerator is an unsigned long. BDI_RATIO_SCALE is 10,000, so the > numerator is an unsigned int. There's no 64-bit arithmetic needed here. Yes, div64_u64() is actually not needed here. So it seems bdi->max_prop_frac = FPROP_FRAC_BASE * max_ratio / 100 / BDI_RATIO_SCALE; is adequate?
On Tue, Dec 19, 2023 at 01:58:21PM +0800, Jingbo Xu wrote: > On 12/19/23 12:06 PM, Matthew Wilcox wrote: > > On Tue, Dec 19, 2023 at 10:42:46AM +0800, Jingbo Xu wrote: > >> } else { > >> bdi->max_ratio = max_ratio; > >> - bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / 100; > >> + bdi->max_prop_frac = div64_u64(FPROP_FRAC_BASE * max_ratio, > >> + 100 * BDI_RATIO_SCALE); > >> } > > > > Why use div64_u64 here? > > > > FPROP_FRAC_BASE is an unsigned long. max_ratio is an unsigned int, so > > the numerator is an unsigned long. BDI_RATIO_SCALE is 10,000, so the > > numerator is an unsigned int. There's no 64-bit arithmetic needed here. > > Yes, div64_u64() is actually not needed here. So it seems > > bdi->max_prop_frac = FPROP_FRAC_BASE * max_ratio / 100 / BDI_RATIO_SCALE; > > is adequate? I'd rather spell that as: bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / (100 * BDI_RATIO_SCALE); It's closer to how you'd write it out mathematically and so it reads more easily. At least for me.
On 12/19/23 9:01 PM, Matthew Wilcox wrote: > On Tue, Dec 19, 2023 at 01:58:21PM +0800, Jingbo Xu wrote: >> On 12/19/23 12:06 PM, Matthew Wilcox wrote: >>> On Tue, Dec 19, 2023 at 10:42:46AM +0800, Jingbo Xu wrote: >>>> } else { >>>> bdi->max_ratio = max_ratio; >>>> - bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / 100; >>>> + bdi->max_prop_frac = div64_u64(FPROP_FRAC_BASE * max_ratio, >>>> + 100 * BDI_RATIO_SCALE); >>>> } >>> >>> Why use div64_u64 here? >>> >>> FPROP_FRAC_BASE is an unsigned long. max_ratio is an unsigned int, so >>> the numerator is an unsigned long. BDI_RATIO_SCALE is 10,000, so the >>> numerator is an unsigned int. There's no 64-bit arithmetic needed here. >> >> Yes, div64_u64() is actually not needed here. So it seems >> >> bdi->max_prop_frac = FPROP_FRAC_BASE * max_ratio / 100 / BDI_RATIO_SCALE; >> >> is adequate? > > I'd rather spell that as: > > bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / > (100 * BDI_RATIO_SCALE); > > It's closer to how you'd write it out mathematically and so it reads > more easily. At least for me. Thanks, I would send v3 soon.
diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 2140382dd768..dda59b368c01 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -728,7 +728,8 @@ static int __bdi_set_max_ratio(struct backing_dev_info *bdi, unsigned int max_ra ret = -EINVAL; } else { bdi->max_ratio = max_ratio; - bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / 100; + bdi->max_prop_frac = div64_u64(FPROP_FRAC_BASE * max_ratio, + 100 * BDI_RATIO_SCALE); } spin_unlock_bh(&bdi_lock);
Since now bdi->max_ratio is part per million, fix the wrong arithmetic for max_prop_frac when setting max_ratio. Otherwise the miscalculated max_prop_frac will affect the incrementing of writeout completion count when max_ratio is not 100%. Fixes: efc3e6ad53ea ("mm: split off __bdi_set_max_ratio() function") Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com> --- mm/page-writeback.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)