Messages in this thread | | | Date | Fri, 6 Aug 2021 08:40:10 +0200 | From | Michal Hocko <> | Subject | Re: [PATCH v7 1/2] mm: introduce process_mrelease system call |
| |
On Thu 05-08-21 10:08:58, Suren Baghdasaryan wrote: [...] > + /* > + * If the task is dying and in the process of releasing its memory > + * then get its mm. > + */ > + p = find_lock_task_mm(task); > + if (!p) { > + ret = -ESRCH; > + goto put_pid; > + } > + if (task != p) { > + get_task_struct(p); > + put_task_struct(task); > + task = p; > + }
Why do you need to take a reference to the p here? You are under task_lock so this will not go away and you only need p to get your mm.
> + > + /* If the work has been done already, just exit with success */ > + if (test_bit(MMF_OOM_SKIP, &task->mm->flags)) > + goto put_task;
You want to release the task_lock
> + > + if (task_will_free_mem(task) && (task->flags & PF_KTHREAD) == 0) {
you want task_will_free_mem(p) and what is the point of the PF_KTHREAD check?
> + mm = task->mm; > + mmget(mm);
All you need is to make sure mm will not get released under your feet once task_lock is released so mmgrab is the right thing to do here. The address space can be torn down in parallel and that is OK and desirable.
I think you really want something like this:
if (flags) return -EINVAL; pid = pidfd_get_pid(fd, &f_flags); if (IS_ERR(pid)) return PTR_ERR(pid); task = get_pid_task(pid, PIDTYPE_PID); if (!task) { ret = -ESRCH; goto put_pid; }
/* * Make sure to chose a thread which still has a reference to mm * during the group exit */ p = find_lock_task_mm(task); if (!p) { ret = -ESRCH; goto put_task; }
mm = task->mm; mmgrab(mm); reap = true; /* If the work has been done already, just exit with success */ if (test_bit(MMF_OOM_SKIP, &mm->flags)) { reap = false; } else if (!task_will_free_mem(p)) { reap = false; ret = -EINVAL; } task_unlock(p);
if (!reap) goto dropmm;;
/* Do the work*/
dropmm: mmdrop(mm); put_task: put_task(task); put_pid: put_pid(pid);
return ret;
-- Michal Hocko SUSE Labs
| |