| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 12 May 2009 17:43:46 -0700
From: Arve Hjønnevåg <arve@...roid.com>
To: Mel Gorman <mel@....ul.ie>
Cc: David Rientjes <rientjes@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...e.de>,
Nick Piggin <npiggin@...e.de>,
Peter Ziljstra <a.p.ziljstra@...llo.nl>,
Christoph Lameter <cl@...ux-foundation.org>,
Dave Hansen <dave@...ux.vnet.ibm.com>,
San Mehat <san@...roid.com>, linux-kernel@...r.kernel.org
Subject: Re: [patch 01/11 -mmotm] lowmemorykiller: Only iterate over process
list when needed.
On Tue, May 12, 2009 at 2:09 AM, Mel Gorman <mel@....ul.ie> wrote:
> On Sun, May 10, 2009 at 03:07:05PM -0700, David Rientjes wrote:
>> From: Arve Hjønnevåg <arve@...roid.com>
>>
>> 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@...roid.com>
>> ---
>> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists