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

[Tonnerre]

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@xxxxxxxxxxxx
> +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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>  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: signature.asc
Description: Digital signature