On Thu, Jul 22, 2021 at 6:14 PM Suren Baghdasaryan [off-list ref]
wrote:
quoted
[...]
quoted
+
+ mmap_read_lock(mm);
How about mmap_read_trylock(mm) and return -EAGAIN on failure?
That sounds like a good idea. Thanks! I'll add that in the next respin.
Why is that a good idea? Can you do anything meaningful about the
failure other than immediately retry the syscall and hope for the best?
I was thinking if this syscall implements "best effort without blocking"
approach then for a more strict usage user can simply retry.
I do not think we really want to promise non blocking behavior at this
stage unless that is absolutely necessary. The current implementation
goes an extra mile to not block but I wouldn't carve it into stone via
userspace expectations.
However
retrying means issuing another syscall, so additional overhead...
I guess such "best effort" approach would be unusual for a syscall, so
maybe we can keep it as it is now and if such "do not block" mode is needed
we can use flags to implement it later?
Yeah, an explicit opt-in via flags would be an option if that turns out
to be really necessary.
--
Michal Hocko
SUSE Labs
On Fri, Jul 23, 2021 at 1:53 AM Michal Hocko [off-list ref] wrote:
[...]
quoted
However
retrying means issuing another syscall, so additional overhead...
I guess such "best effort" approach would be unusual for a syscall, so
maybe we can keep it as it is now and if such "do not block" mode is needed
we can use flags to implement it later?
Yeah, an explicit opt-in via flags would be an option if that turns out
to be really necessary.
I am fine with keeping it as it is but we do need the non-blocking
option (via flags) to enable userspace to act more aggressively.
On Fri, Jul 23, 2021 at 6:46 AM Shakeel Butt [off-list ref] wrote:
On Fri, Jul 23, 2021 at 1:53 AM Michal Hocko [off-list ref] wrote:
quoted
[...]
quoted
quoted
However
retrying means issuing another syscall, so additional overhead...
I guess such "best effort" approach would be unusual for a syscall, so
maybe we can keep it as it is now and if such "do not block" mode is needed
we can use flags to implement it later?
Yeah, an explicit opt-in via flags would be an option if that turns out
to be really necessary.
I am fine with keeping it as it is but we do need the non-blocking
option (via flags) to enable userspace to act more aggressively.
I think you want to check memory conditions shortly after issuing
kill/reap requests irrespective of mmap_sem contention. The reason is
that even when memory release is not blocked, allocations from other
processes might consume memory faster than we release it. For example,
in Android we issue kill and start waiting on pidfd for its death
notification. As soon as the process is dead we reassess the situation
and possibly kill again. If the process is not dead within a
configurable timeout we check conditions again and might issue more
kill requests (IOW our wait for the process to die has a timeout). If
process_mrelease() is blocked on mmap_sem, we might timeout like this.
I imagine that a non-blocking option for process_mrelease() would not
really change this logic.
Adding such an option is trivial but I would like to make sure it's
indeed useful. Maybe after the syscall is in place you can experiment
with it and see if such an option would really change the way you use
it?
On Fri, Jul 23, 2021 at 9:09 AM Suren Baghdasaryan [off-list ref] wrote:
On Fri, Jul 23, 2021 at 6:46 AM Shakeel Butt [off-list ref] wrote:
quoted
On Fri, Jul 23, 2021 at 1:53 AM Michal Hocko [off-list ref] wrote:
quoted
[...]
quoted
quoted
However
retrying means issuing another syscall, so additional overhead...
I guess such "best effort" approach would be unusual for a syscall, so
maybe we can keep it as it is now and if such "do not block" mode is needed
we can use flags to implement it later?
Yeah, an explicit opt-in via flags would be an option if that turns out
to be really necessary.
I am fine with keeping it as it is but we do need the non-blocking
option (via flags) to enable userspace to act more aggressively.
I think you want to check memory conditions shortly after issuing
kill/reap requests irrespective of mmap_sem contention. The reason is
that even when memory release is not blocked, allocations from other
processes might consume memory faster than we release it. For example,
in Android we issue kill and start waiting on pidfd for its death
notification. As soon as the process is dead we reassess the situation
and possibly kill again. If the process is not dead within a
configurable timeout we check conditions again and might issue more
kill requests (IOW our wait for the process to die has a timeout). If
process_mrelease() is blocked on mmap_sem, we might timeout like this.
I imagine that a non-blocking option for process_mrelease() would not
really change this logic.
On a containerized system, killing a job requires killing multiple
processes and then process_mrelease() them. Now there is cgroup.kill
to kill all the processes in a cgroup tree but we would still need to
process_mrelease() all the processes in that tree. There is a chance
that we get stuck in reaping the early process. Making
process_mrelease() non-blocking will enable the userspace to go to
other processes in the list.
An alternative would be to have a cgroup specific interface for
reaping similar to cgroup.kill.
Adding such an option is trivial but I would like to make sure it's
indeed useful. Maybe after the syscall is in place you can experiment
with it and see if such an option would really change the way you use
it?
From: Michal Hocko <mhocko@suse.com> Date: 2021-07-26 07:27:23
On Fri 23-07-21 10:00:26, Shakeel Butt wrote:
On Fri, Jul 23, 2021 at 9:09 AM Suren Baghdasaryan [off-list ref] wrote:
quoted
On Fri, Jul 23, 2021 at 6:46 AM Shakeel Butt [off-list ref] wrote:
quoted
On Fri, Jul 23, 2021 at 1:53 AM Michal Hocko [off-list ref] wrote:
quoted
[...]
quoted
quoted
However
retrying means issuing another syscall, so additional overhead...
I guess such "best effort" approach would be unusual for a syscall, so
maybe we can keep it as it is now and if such "do not block" mode is needed
we can use flags to implement it later?
Yeah, an explicit opt-in via flags would be an option if that turns out
to be really necessary.
I am fine with keeping it as it is but we do need the non-blocking
option (via flags) to enable userspace to act more aggressively.
I think you want to check memory conditions shortly after issuing
kill/reap requests irrespective of mmap_sem contention. The reason is
that even when memory release is not blocked, allocations from other
processes might consume memory faster than we release it. For example,
in Android we issue kill and start waiting on pidfd for its death
notification. As soon as the process is dead we reassess the situation
and possibly kill again. If the process is not dead within a
configurable timeout we check conditions again and might issue more
kill requests (IOW our wait for the process to die has a timeout). If
process_mrelease() is blocked on mmap_sem, we might timeout like this.
I imagine that a non-blocking option for process_mrelease() would not
really change this logic.
On a containerized system, killing a job requires killing multiple
processes and then process_mrelease() them. Now there is cgroup.kill
to kill all the processes in a cgroup tree but we would still need to
process_mrelease() all the processes in that tree.
Is process_mrelease on all of them really necessary? I thought that the
primary reason for the call is to guarantee a forward progress in cases
where the userspace OOM victim cannot die on SIGKILL. That should be
more an exception than a normal case, no?
There is a chance
that we get stuck in reaping the early process. Making
process_mrelease() non-blocking will enable the userspace to go to
other processes in the list.
I do agree that allowing (guanrateed) non-blocking behavior is nice but
it is also a rather strong promise. There is some memory that cannot be
released by the oom reaper currently because there are locks involved
(e.g. mlocked memory or memory areas backed by blocking notifiers).
I can imagine some users of this api would rather block and make sure to
release the memory rather than skip over it. So if anything this has to
be an opt in with a big fat warning that the behavior of the kernel wrt
to releasable memory can vary due to all sorts of implementation
details.
An alternative would be to have a cgroup specific interface for
reaping similar to cgroup.kill.
On Mon, Jul 26, 2021 at 12:27 AM Michal Hocko [off-list ref] wrote:
[...]
Is process_mrelease on all of them really necessary? I thought that the
primary reason for the call is to guarantee a forward progress in cases
where the userspace OOM victim cannot die on SIGKILL. That should be
more an exception than a normal case, no?
I am thinking of using this API in this way: On user-defined OOM
condition, kill a job/cgroup and unconditionally reap all of its
processes. Keep monitoring the situation and if it does not improve go
for another kill and reap.
I can add additional logic in between kill and reap to see if reap is
necessary but unconditionally reaping is more simple.
quoted
An alternative would be to have a cgroup specific interface for
reaping similar to cgroup.kill.
Could you elaborate?
I mentioned this in [1] where I was thinking if it makes sense to
overload cgroup.kill to also add the SIGKILLed processes in
oom_reaper_list. The downside would be that there will be one thread
doing the reaping and the syscall approach allows userspace to reap in
multiple threads. I think for now, I would go with whatever Suren is
proposing and we can always add more stuff if need arises.
[1] https://lore.kernel.org/containers/CALvZod4jsb6bFzTOS4ZRAJGAzBru0oWanAhezToprjACfGm+ew@mail.gmail.com/
On Mon, Jul 26, 2021 at 6:44 AM Shakeel Butt [off-list ref] wrote:
On Mon, Jul 26, 2021 at 12:27 AM Michal Hocko [off-list ref] wrote:
quoted
[...]
quoted
Is process_mrelease on all of them really necessary? I thought that the
primary reason for the call is to guarantee a forward progress in cases
where the userspace OOM victim cannot die on SIGKILL. That should be
more an exception than a normal case, no?
I am thinking of using this API in this way: On user-defined OOM
condition, kill a job/cgroup and unconditionally reap all of its
processes. Keep monitoring the situation and if it does not improve go
for another kill and reap.
I can add additional logic in between kill and reap to see if reap is
necessary but unconditionally reaping is more simple.
quoted
quoted
An alternative would be to have a cgroup specific interface for
reaping similar to cgroup.kill.
Could you elaborate?
I mentioned this in [1] where I was thinking if it makes sense to
overload cgroup.kill to also add the SIGKILLed processes in
oom_reaper_list. The downside would be that there will be one thread
doing the reaping and the syscall approach allows userspace to reap in
multiple threads. I think for now, I would go with whatever Suren is
proposing and we can always add more stuff if need arises.
[1] https://lore.kernel.org/containers/CALvZod4jsb6bFzTOS4ZRAJGAzBru0oWanAhezToprjACfGm+ew@mail.gmail.com/
Hi Folks,
So far I don't think there was any request for further changes.
Anything else you would want me to address or are we in a good shape
wrt this feature?
If so, would people who had a chance to review this patchset be
willing to endorse it with their Reviewed-by or Acked-by?
Thanks,
Suren.
On Mon, Aug 2, 2021 at 12:54 PM Suren Baghdasaryan [off-list ref] wrote:
On Mon, Jul 26, 2021 at 6:44 AM Shakeel Butt [off-list ref] wrote:
quoted
On Mon, Jul 26, 2021 at 12:27 AM Michal Hocko [off-list ref] wrote:
quoted
[...]
quoted
Is process_mrelease on all of them really necessary? I thought that the
primary reason for the call is to guarantee a forward progress in cases
where the userspace OOM victim cannot die on SIGKILL. That should be
more an exception than a normal case, no?
I am thinking of using this API in this way: On user-defined OOM
condition, kill a job/cgroup and unconditionally reap all of its
processes. Keep monitoring the situation and if it does not improve go
for another kill and reap.
I can add additional logic in between kill and reap to see if reap is
necessary but unconditionally reaping is more simple.
quoted
quoted
An alternative would be to have a cgroup specific interface for
reaping similar to cgroup.kill.
Could you elaborate?
I mentioned this in [1] where I was thinking if it makes sense to
overload cgroup.kill to also add the SIGKILLed processes in
oom_reaper_list. The downside would be that there will be one thread
doing the reaping and the syscall approach allows userspace to reap in
multiple threads. I think for now, I would go with whatever Suren is
proposing and we can always add more stuff if need arises.
[1] https://lore.kernel.org/containers/CALvZod4jsb6bFzTOS4ZRAJGAzBru0oWanAhezToprjACfGm+ew@mail.gmail.com/
Hi Folks,
So far I don't think there was any request for further changes.
Anything else you would want me to address or are we in a good shape
wrt this feature?
If so, would people who had a chance to review this patchset be
willing to endorse it with their Reviewed-by or Acked-by?
I think with Michal's suggestion to use a killable mmap lock, at least
I am good with the patch.
On Mon, Aug 2, 2021 at 1:05 PM Shakeel Butt [off-list ref] wrote:
On Mon, Aug 2, 2021 at 12:54 PM Suren Baghdasaryan [off-list ref] wrote:
quoted
On Mon, Jul 26, 2021 at 6:44 AM Shakeel Butt [off-list ref] wrote:
quoted
On Mon, Jul 26, 2021 at 12:27 AM Michal Hocko [off-list ref] wrote:
quoted
[...]
quoted
Is process_mrelease on all of them really necessary? I thought that the
primary reason for the call is to guarantee a forward progress in cases
where the userspace OOM victim cannot die on SIGKILL. That should be
more an exception than a normal case, no?
I am thinking of using this API in this way: On user-defined OOM
condition, kill a job/cgroup and unconditionally reap all of its
processes. Keep monitoring the situation and if it does not improve go
for another kill and reap.
I can add additional logic in between kill and reap to see if reap is
necessary but unconditionally reaping is more simple.
quoted
quoted
An alternative would be to have a cgroup specific interface for
reaping similar to cgroup.kill.
Could you elaborate?
I mentioned this in [1] where I was thinking if it makes sense to
overload cgroup.kill to also add the SIGKILLed processes in
oom_reaper_list. The downside would be that there will be one thread
doing the reaping and the syscall approach allows userspace to reap in
multiple threads. I think for now, I would go with whatever Suren is
proposing and we can always add more stuff if need arises.
[1] https://lore.kernel.org/containers/CALvZod4jsb6bFzTOS4ZRAJGAzBru0oWanAhezToprjACfGm+ew@mail.gmail.com/
Hi Folks,
So far I don't think there was any request for further changes.
Anything else you would want me to address or are we in a good shape
wrt this feature?
If so, would people who had a chance to review this patchset be
willing to endorse it with their Reviewed-by or Acked-by?
I think with Michal's suggestion to use a killable mmap lock, at least
I am good with the patch.
Ah, yes. Thanks for pointing this out! I'll replace mmap_read_lock()
with mmap_read_lock_killable(). Will post an updated version later
today.
On Mon, Aug 2, 2021 at 1:08 PM Suren Baghdasaryan [off-list ref] wrote:
On Mon, Aug 2, 2021 at 1:05 PM Shakeel Butt [off-list ref] wrote:
quoted
On Mon, Aug 2, 2021 at 12:54 PM Suren Baghdasaryan [off-list ref] wrote:
quoted
On Mon, Jul 26, 2021 at 6:44 AM Shakeel Butt [off-list ref] wrote:
quoted
On Mon, Jul 26, 2021 at 12:27 AM Michal Hocko [off-list ref] wrote:
quoted
[...]
quoted
Is process_mrelease on all of them really necessary? I thought that the
primary reason for the call is to guarantee a forward progress in cases
where the userspace OOM victim cannot die on SIGKILL. That should be
more an exception than a normal case, no?
I am thinking of using this API in this way: On user-defined OOM
condition, kill a job/cgroup and unconditionally reap all of its
processes. Keep monitoring the situation and if it does not improve go
for another kill and reap.
I can add additional logic in between kill and reap to see if reap is
necessary but unconditionally reaping is more simple.
quoted
quoted
An alternative would be to have a cgroup specific interface for
reaping similar to cgroup.kill.
Could you elaborate?
I mentioned this in [1] where I was thinking if it makes sense to
overload cgroup.kill to also add the SIGKILLed processes in
oom_reaper_list. The downside would be that there will be one thread
doing the reaping and the syscall approach allows userspace to reap in
multiple threads. I think for now, I would go with whatever Suren is
proposing and we can always add more stuff if need arises.
[1] https://lore.kernel.org/containers/CALvZod4jsb6bFzTOS4ZRAJGAzBru0oWanAhezToprjACfGm+ew@mail.gmail.com/
Hi Folks,
So far I don't think there was any request for further changes.
Anything else you would want me to address or are we in a good shape
wrt this feature?
If so, would people who had a chance to review this patchset be
willing to endorse it with their Reviewed-by or Acked-by?
I think with Michal's suggestion to use a killable mmap lock, at least
I am good with the patch.
Ah, yes. Thanks for pointing this out! I'll replace mmap_read_lock()
with mmap_read_lock_killable(). Will post an updated version later
today.