lkml.org 
[lkml]   [2011]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] oom: fix integer overflow of points in oom_badness
On Mon, Oct 31, 2011 at 04:28:33PM +0100, Oleg Nesterov wrote:
> On 10/31, Frantisek Hrbata wrote:
> >
> > My understanding is that we may just change the type of points variable from int
> > to long and keep the current imho clearer(better readable) computation. There
> > should not be an overflow on 32bit and there is a plenty of space for 64bit.
> > If you like this solution better I will post the patch as v2.
>
> Up to maintainer, but personally I think the simple s/int/long/ looks better.
> Everything like get_mm_*/nr_ptes returns long.

Agreed. I will post v2 with the int => long change.

>
> Anyway good catch. Imho stable needs the fix too.
>
> Cosmetic nit,
>
> > - points = get_mm_rss(p->mm) + p->mm->nr_ptes;
> > - points += get_mm_counter(p->mm, MM_SWAPENTS);
> > + points = (int)((get_mm_rss(p->mm) + get_mm_counter(p->mm, MM_SWAPENTS) +
> ^^^^^
>
> Why do we need the explicit typecast? It buys nothing and looks a bit confusing.

You are right, it's not needed. I just wanted to make the cast more visible, but
some kind of comment would be probably better here.

>
> And, if you prefer "int", perhaps something like
>
> - points *= 1000;
> - points /= totalpages;
> + /* avoid the possible overflow */
> + points = points * 1000L / totalpages;
>
> looks a bit more readable with the same effect. But I won't insist, this is
> up to you and David.

Sure, this looks much better than the one line expression in the patch I sent.
If David or others decide to not go with the int=>long change I think we should
use this.

>
> Oleg.
>

Many thanks Oleg.

--
Frantisek Hrbata


\
 
 \ /
  Last update: 2011-10-31 16:51    [from the cache]
©2003-2011 Jasper Spaans