Re: mm->oom_disable_count is broken

KOSAKI Motohiro
2011-08-01T11:52:37Z

(2011/07/31 0:22), Oleg Nesterov wrote:
> On 07/30, Oleg Nesterov wrote:
>>
>> So I'd suggest this hack^Wpatch as a quick fix. I still think this
>> code is not right, but the patch tries to keep the current logic.
>
> And this reminds me. mm->oom_disable_count looks absolutely broken.
> IIRC, I already complained but nobody replied.
>
> What does this counter mean?
>
> /* How many tasks sharing this mm are OOM_DISABLE */
> atomic_t oom_disable_count;
>
> tasks? processes or threads?
>
> Lets look at oom_adjust_write(),
>
> if (task->signal->oom_adj == OOM_DISABLE)
> atomic_inc(&task->mm->oom_disable_count);
>
> OK, so it is per-process. No matter how many threads this process
> has, mm->oom_disable_count becomes 1 (ignoring CLONE_VM).
>
>
> However, exit_mm() does:
>
> if (tsk->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> atomic_dec(&mm->oom_disable_count);
>
> but this is per-thread! it becomes zero and then negative after
> pthread_exit().
>
>
> copy_process()->copy_mm() seems to think it is per-thread too. But,
> bad_fork_cleanup_mm:
>
> if (p->mm) {
> task_lock(p);
> if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> atomic_dec(&p->mm->oom_disable_count);
> task_unlock(p);
> mmput(p->mm);
> }
>
> Why do we take task_lock() ? OK, oom_score_adj_write() does task_lock()
> too, but this can't help in the multithreaded case? Why copy_mm() checks
> ->oom_score_adj lockless?

IIRC, I did pointed out this issue. But nobody replied.
I think ->oom_disable_count is currently broken. but now I have no time to
audit this stuff. So, I'd suggest to revert this code if nobody don't fix it.