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

[Stephen Rothwell <sfr@canb.auug.org.au>]

[-- 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 --]