Re: mmotm 2019-07-24-21-39 uploaded (mm/memcontrol)

Stephen Rothwell
2019-07-28T23:51:30Z

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Hi all,

On Sat, 27 Jul 2019 11:16:08 +0100 Chris Down <chris@chrisdown.name> wrote:
>
> u64 division: truly the gift that keeps on giving. Thanks Andrew for following
> up on these.
>
> Andrew Morton writes:
> >Ah.
> >
> >It's rather unclear why that u64 cast is there anyway. We're dealing
> >with ulongs all over this code. The below will suffice.
>
> This place in particular uses u64 to make sure we don't overflow when left
> shifting, since the numbers can get pretty big (and that's somewhat needed due
> to the need for high precision when calculating the penalty jiffies). It's ok
> if the output after division is an unsigned long, just the intermediate steps
> need to have enough precision.
>
> >Chris, please take a look?
> >
> >--- a/mm/memcontrol.c~mm-throttle-allocators-when-failing-reclaim-over-memoryhigh-fix-fix-fix
> >+++ a/mm/memcontrol.c
> >@@ -2415,7 +2415,7 @@ void mem_cgroup_handle_over_high(void)
> > clamped_high = max(high, 1UL);
> >
> > overage = (u64)(usage - high) << MEMCG_DELAY_PRECISION_SHIFT;
> >- do_div(overage, clamped_high);
> >+ overage /= clamped_high;
>
> I think this isn't going to work because left shifting by
> MEMCG_DELAY_PRECISION_SHIFT can make the number bigger than ULONG_MAX, which
> may cause wraparound -- we need to retain the u64 until we divide.
>
> Maybe div_u64 will satisfy both ARM and i386? ie.
>
> diff --git mm/memcontrol.c mm/memcontrol.c
> index 5c7b9facb0eb..e12a47e96154 100644
> --- mm/memcontrol.c
> +++ mm/memcontrol.c
> @@ -2419,8 +2419,8 @@ void mem_cgroup_handle_over_high(void)
> */
> clamped_high = max(high, 1UL);
>
> - overage = (u64)(usage - high) << MEMCG_DELAY_PRECISION_SHIFT;
> - do_div(overage, clamped_high);
> + overage = div_u64((u64)(usage - high) << MEMCG_DELAY_PRECISION_SHIFT,
> + clamped_high);
>
> penalty_jiffies = ((u64)overage * overage * HZ)
> >> (MEMCG_DELAY_PRECISION_SHIFT + MEMCG_DELAY_SCALING_SHIFT);

I have applied this to the akpm-current tree in linux-next today.

--
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]