Michal, I am not really arguing with this patch, but since you are going
(iiuc) to resend it anyway let me ask a couple of questions.
> This, however, still keeps
> a window open when a killed task didn't manage to die by the time
> freeze_processes finishes.
Sure,
> Fix this race by checking all tasks after OOM killer has been disabled.
But this doesn't close the race entirely? please see below.
> int freeze_processes(void)
> {
> int error;
> + int oom_kills_saved;
>
> error = __usermodehelper_disable(UMH_FREEZING);
> if (error)
> @@ -132,12 +133,40 @@ int freeze_processes(void)
> pm_wakeup_clear();
> printk("Freezing user space processes ... ");
> pm_freezing = true;
> + oom_kills_saved = oom_kills_count();
> error = try_to_freeze_tasks(true);
> if (!error) {
> - printk("done.");
> __usermodehelper_set_disable_depth(UMH_DISABLED);
> oom_killer_disable();
> +
> + /*
> + * There was a OOM kill while we were freezing tasks
> + * and the killed task might be still on the way out
> + * so we have to double check for race.
> + */
> + if (oom_kills_count() != oom_kills_saved) {
OK, I agree, this makes the things better, but perhaps we should document
(at least in the changelog) that this is still racy. oom_killer_disable()
obviously can stop the already called out_of_memory(), it can kill a frozen
task right after this check or even after the loop before.
> + struct task_struct *g, *p;
> +
> + read_lock(&tasklist_lock);
> + do_each_thread(g, p) {
> + if (p == current || freezer_should_skip(p) ||
> + frozen(p))
> + continue;
> + error = -EBUSY;
> + break;
> + } while_each_thread(g, p);
Please use for_each_process_thread(), do/while_each_thread is deprecated.
> +/*
> + * Number of OOM killer invocations (including memcg OOM killer).
> + * Primarily used by PM freezer to check for potential races with
> + * OOM killed frozen task.
> + */
> +static atomic_t oom_kills = ATOMIC_INIT(0);
> +
> +int oom_kills_count(void)
> +{
> + return atomic_read(&oom_kills);
> +}
> +
> #define K(x) ((x) << (PAGE_SHIFT-10))
> /*
> * Must be called while holding a reference to p, which will be released upon
> @@ -504,11 +516,13 @@ void oom_kill_process(struct task_struct
> pr_err("Kill process %d (%s) sharing same memory\n",
> task_pid_nr(p), p->comm);
> task_unlock(p);
> + atomic_inc(&oom_kills);
Do we really need this? Can't freeze_processes() (ab)use oom_notify_list?
Yes, we can have more false positives this way, but probably this doesn't
matter? This is unlikely case anyway.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
