Re: [patch 01/11 -mmotm] lowmemorykiller: Only iterate over process list when needed.

[Arve Hjønnevåg]

On Tue, May 12, 2009 at 2:09 AM, Mel Gorman <mel@xxxxxxxxx> wrote:
> On Sun, May 10, 2009 at 03:07:05PM -0700, David Rientjes wrote:
>> From: Arve Hjønnevåg <arve@xxxxxxxxxxx>
>>
>> Use NR_ACTIVE plus NR_INACTIVE as a size estimate for our fake cache
>> instead the sum of rss. Neither method is accurate.
>>
>
> If neither estimate is accurate (and I agree), then explain why this is
> better and why better decisions are made as a result.
>
It should make similar decisions without iterating over the process
list as often.

> I would assume it's because pages can be double accounted when using RSS
> due to shared pages and you might over-estimate the amount of memory
> really in use. If that reasoning is correct, say that.
>
>> Also skip the process scan, if the amount of memory available is above
>> the largest threshold set.
>>
>
> Patches should do one thing, please split if possible. This patch aspect
> seems to be the if check early on so the splitting should not be
> difficult.
>

This is the whole point of the patch.

> In that statement, you check nr_to_scan <= 0, how can you scan a
> negative number of things? Say in the description that the process scan
> is set if min_adj is such a high value that it cannot be used to
> discriminate between processes for suitability to kill.
>
>> Signed-off-by: Arve Hjønnevåg <arve@xxxxxxxxxxx>
>> ---
>>  drivers/staging/android/lowmemorykiller.c |   35 +++++++++++++++++-----------
>>  1 files changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
>> --- a/drivers/staging/android/lowmemorykiller.c
>> +++ b/drivers/staging/android/lowmemorykiller.c
>> @@ -71,23 +71,30 @@ static int lowmem_shrink(int nr_to_scan, gfp_t gfp_mask)
>>       }
>>       if(nr_to_scan > 0)
>>               lowmem_print(3, "lowmem_shrink %d, %x, ofree %d, ma %d\n", nr_to_scan, gfp_mask, other_free, min_adj);
>> +     rem = global_page_state(NR_ACTIVE) + global_page_state(NR_INACTIVE);
>> +     if (nr_to_scan <= 0 || min_adj == OOM_ADJUST_MAX + 1) {
>> +             lowmem_print(5, "lowmem_shrink %d, %x, return %d\n", nr_to_scan, gfp_mask, rem);
>> +             return rem;
>> +     }
>> +
>>       read_lock(&tasklist_lock);
>>       for_each_process(p) {
>> -             if(p->oomkilladj >= 0 && p->mm) {
>> -                     tasksize = get_mm_rss(p->mm);
>> -                     if(nr_to_scan > 0 && tasksize > 0 && p->oomkilladj >= min_adj) {
>> -                             if(selected == NULL ||
>> -                                p->oomkilladj > selected->oomkilladj ||
>> -                                (p->oomkilladj == selected->oomkilladj &&
>> -                                 tasksize > selected_tasksize)) {
>> -                                     selected = p;
>> -                                     selected_tasksize = tasksize;
>> -                                     lowmem_print(2, "select %d (%s), adj %d, size %d, to kill\n",
>> -                                                  p->pid, p->comm, p->oomkilladj, tasksize);
>> -                             }
>> -                     }
>> -                     rem += tasksize;
>> +             if (p->oomkilladj < min_adj || !p->mm)
>> +                     continue;
>> +             tasksize = get_mm_rss(p->mm);
>> +             if (tasksize <= 0)
>> +                     continue;
>
> It's also odd to consider the possibility of a negative tasksize.

Yes, using an unsigned tasksize would be better, but this patch did
not change any of this.

>
> That aside, RSS is the sum of two longs (or atomic_long_t depending) and
> tasksize here is a signed integer. Are you not at the risk of
> overflowing and this check being unable to do anything useful if all the
> processes of interest are using large amounts of memory?

Yes (The systems we use this on do not have enough memory for this to
be possible though).

>
>> +             if (selected) {
>> +                     if (p->oomkilladj < selected->oomkilladj)
>> +                             continue;
>> +                     if (p->oomkilladj == selected->oomkilladj &&
>> +                         tasksize <= selected_tasksize)
>> +                             continue;
>>               }
>> +             selected = p;
>> +             selected_tasksize = tasksize;
>> +             lowmem_print(2, "select %d (%s), adj %d, size %d, to kill\n",
>> +                          p->pid, p->comm, p->oomkilladj, tasksize);
>>       }
>>       if(selected != NULL) {
>>               lowmem_print(1, "send sigkill to %d (%s), adj %d, size %d\n",
>
> I'm hoping it has been discussed elsewhere why the normal OOM killer is not
> being used instead of this driver thing.
>
> --
> Mel Gorman
> Part-time Phd Student                          Linux Technology Center
> University of Limerick                         IBM Dublin Software Lab
>



-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/