Dave Peterson <[email protected]> wrote:
>
> Another thing I noticed: oom_kill_task() calls mmput() while holding
> tasklist_lock.
Yes, that'll make my new might_sleep() get upset.
oom_kill_task() doesn't _have_ to run mmput() there - we could propagate
the mm back to the top-level and do the mmput() there.
> Here the calls to get_task_mm() and mmput() appear to
> be unnecessary. We shouldn't need to use any kind of locking or
> reference counting since oom_kill_task() doesn't dereference into the
> mm_struct or require the value of p->mm to stay constant. I believe
> the following (untested) code changes should fix the problem (and
> simplify some other parts of the code). Does this look correct?
But yes, this looks better.
>
> diff -urNp -X /home/dsp/dontdiff linux-2.6.17-rc1/mm/oom_kill.c linux-2.6.17-rc1-fix/mm/oom_kill.c
> --- linux-2.6.17-rc1/mm/oom_kill.c 2006-03-19 21:53:29.000000000 -0800
> +++ linux-2.6.17-rc1-fix/mm/oom_kill.c 2006-04-14 13:22:15.000000000 -0700
> @@ -244,17 +244,15 @@ static void __oom_kill_task(task_t *p, c
> force_sig(SIGKILL, p);
> }
>
> -static struct mm_struct *oom_kill_task(task_t *p, const char *message)
> +static int oom_kill_task(task_t *p, const char *message)
> {
> - struct mm_struct *mm = get_task_mm(p);
> + struct mm_struct *mm;
> task_t * g, * q;
Please put a loud comment in here explaining that `mm' may not be dereferenced.
> - if (!mm)
> - return NULL;
> - if (mm == &init_mm) {
> - mmput(mm);
> - return NULL;
> - }
> + mm = p->mm;
> +
> + if ((mm == NULL) || (mm == &init_mm))
I think the parenthesisation here is going a bit far ;)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
