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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 May 2009 10:09:12 +0100
From:	Mel Gorman <mel@....ul.ie>
To:	David Rientjes <rientjes@...gle.com>
Cc:	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>, Arve Hj?nnev?g <arve@...roid.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [patch 01/11 -mmotm] lowmemorykiller: Only iterate over
	process list when needed.

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.

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.

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.

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?

> +		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
--
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/