2006-02-28 14:17:25

by Kurt Garloff

[permalink] [raw]
Subject: Re: [PATCH] OOM: initialise points variable in out_of_memory()

Hi Carlos,

On Mon, Feb 27, 2006 at 10:36:25PM +0100, Carlos Martin wrote:
> We didn't initialise points, so the value reported was completely
> random. This doesn't affect the behaviour of the funcion.

Did you observe it?

In the original patch, there is
+static struct task_struct * select_bad_process(unsigned long *ppoints)
{
- unsigned long maxpoints = 0;
struct task_struct *g, *p;
struct task_struct *chosen = NULL;
struct timespec uptime;
+ *ppoints = 0;

And this is called from out_of_memory().

But the constrained_alloc stuff seems to use points before
select_bad_process() is called, so initializing to 0 is a
good idea. I don't remember having see the constrained_alloc
stuff, though, so I either did not look carefully enough or a
merge error happened afterwards.

Thanks for noticing and thanks for sending the fix!

Best,
--
Kurt Garloff, Head Architect Linux, Novell Inc.


Attachments:
(No filename) (933.00 B)
(No filename) (189.00 B)
Download all attachments

2006-02-28 15:02:47

by Carlos Martín Nieto

[permalink] [raw]
Subject: Re: [PATCH] OOM: initialise points variable in out_of_memory()

On Tuesday 28 February 2006 13:13, Kurt Garloff wrote:
> Hi Carlos,
>
> On Mon, Feb 27, 2006 at 10:36:25PM +0100, Carlos Martin wrote:
> > We didn't initialise points, so the value reported was completely
> > random. This doesn't affect the behaviour of the funcion.
>
> Did you observe it?

No. GCC complained about it.

>
> In the original patch, there is
> +static struct task_struct * select_bad_process(unsigned long *ppoints)
> {
> - unsigned long maxpoints = 0;
> struct task_struct *g, *p;
> struct task_struct *chosen = NULL;
> struct timespec uptime;
> + *ppoints = 0;
>
> And this is called from out_of_memory().
>
> But the constrained_alloc stuff seems to use points before
> select_bad_process() is called, so initializing to 0 is a
> good idea. I don't remember having see the constrained_alloc
> stuff, though, so I either did not look carefully enough or a
> merge error happened afterwards.

It never used to be a problem because it was initialised when calling
select_bad_process().

I though it was your change, but I've looked again and it was Christoph
Lameter's commit 9b0f8b040acd8dfd23860754c0d09ff4f44e2cbc which introduced
the problem.

cmn
--
Carlos Martín Nieto | http://www.cmartin.tk
Hobbyist programmer |