Re: [PATCH] oom_pardon, aka don't kill my xlock

Tonnerre
2004-09-23T04:47:44Z

[-- Attachment #1: Type: text/plain, Size: 3889 bytes --]

Salut,

On Thu, Sep 23, 2004 at 01:23:08AM +0200, Thomas Habets wrote:
> diff -Nur linux-2.6.7.orig/CREDITS linux-2.6.7/CREDITS
> --- linux-2.6.7.orig/CREDITS 2004-06-16 07:19:43.000000000 +0200
> +++ linux-2.6.7/CREDITS 2004-09-23 00:02:44.000000000 +0200
> @@ -1210,6 +1210,14 @@
> W: http://www.inf.fu-berlin.de/~ehaase
> D: Driver for the Commodore A2232 serial board
>
> +N: Thomas Habets
> +E: thomas@habets.pp.se
> +D: random Linux hacker
> +P: 1024D/AD48E854 A8A3 D1DD 4AE0 8467 7FDE 0945 286A E90A AD48 E854
> +S: Tunnlandsvägen 40
> +S: 168 36 Bromma
> +S: Sweden
> +
> N: Bruno Haible
> E: haible@ma2s2.mathematik.uni-karlsruhe.de
> D: SysV FS, shm swapping, memory management fixes

That should be D: OOM killer exceptions or whatever, I suppose.

> diff -Nur linux-2.6.7.orig/kernel/sysctl.c linux-2.6.7/kernel/sysctl.c
> --- linux-2.6.7.orig/kernel/sysctl.c 2004-06-16 07:18:58.000000000 +0200
> +++ linux-2.6.7/kernel/sysctl.c 2004-09-23 00:28:51.000000000 +0200
> @@ -795,6 +795,15 @@
> .strategy = &sysctl_intvec,
> .extra1 = &zero,
> },
> + {
> + .ctl_name = VM_OOM_PARDON,
> + .procname = "oom_pardon",
> + .data = &vm_oom_pardon,
> + .maxlen = sizeof(vm_oom_pardon),
> + .mode = 0644,
> + .proc_handler = &proc_doutsstring,
> + .strategy = &sysctl_string,
> + },
> { .ctl_name = 0 }
> };
>

A sysctl is a bad implementation since you can only store one single
string in it.

> diff -Nur linux-2.6.7.orig/mm/oom_kill.c linux-2.6.7/mm/oom_kill.c
> --- linux-2.6.7.orig/mm/oom_kill.c 2004-06-16 07:19:29.000000000 +0200
> +++ linux-2.6.7/mm/oom_kill.c 2004-09-23 00:31:12.000000000 +0200
> @@ -16,14 +16,56 @@
> */
>
> #include <linux/mm.h>
> +#include <linux/utsname.h>
> #include <linux/sched.h>
> #include <linux/swap.h>
> #include <linux/timex.h>
> #include <linux/jiffies.h>
>
> +char vm_oom_pardon[VM_OOM_PARDON_LEN];
> /* #define DEBUG */
>
> /**
> + * For the love of kbaek, don't kill processes in /proc/sys/vm/oom_pardon
> + */
> +static int pardon(struct task_struct *task)
> +{
> + static char buf[256];

That 256 should be VM_OOM_PARDON_LEN ?

> + const struct qstr *exe;
> + const char *p;
> + int len;
> +
> + exe = &task->proc_dentry->d_name;
> + len = min((int)exe->len, (int)(sizeof(buf) - 2));

Dito.

> + memcpy(buf, exe->name, len);
> + buf[len] = 0;
> + buf[len+1] = 0;
> +
> + if (strchr(buf, ' ')) {
> + return 0;
> + }
> +
> + down_read(&uts_sem);

We're under the task lock, and you want us to sleep here? There's a
little problem: we'd want to switch the task, and since the task lock
is taken, we'll wait an infinite amount of time (yes, literally!) for
it to become free.

> + p = vm_oom_pardon;
> + do {
> + buf[len] = ' ';
> + if (!strncmp(p, buf, len)) {
> + return 1;
> + }
> +
> + buf[len] = 0;
> + if (!strcmp(p, buf)) {
> + return 1;
> + }
> + p = strchr(p, ' ');
> + } while(p++);

What about programs with spaces in its names?

Actually, I'd really use a different interface to register and
unregister processes to protect. And maybe not (just) by the binary
name. Make a real filter list, or track them by pid.

> + up_read(&uts_sem);
> +
> + return 0;
> +}
> +
> +/**
> * oom_badness - calculate a numeric value for how bad this task has been
> * @p: task struct of which task we should calculate
> *

Tonnerre

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]