Since commit 29ef680ae7c2 ("memcg, oom: move out_of_memory back to
the charge path"), only oom_kill_disable is set, oom killer will
be delayed to page fault path. In the charge patch, even if the
oom_lock in memcg can't be acquired, the oom handing can also be
invoked. In order to keep the behavior consistent with it, remove
the lock check, just leave oom_kill_disable check behind in the
page fault path.
Furthermore, the lock contender won't be scheduled out, this doesn't
fit the sixth description in commit fb2a6fc56be66 ("mm: memcg:
rework and document OOM waiting and wakeup"). So remove the explicit
wakeup for the lock holder.
Fixes: fb2a6fc56be6 ("mm: memcg: rework and document OOM waiting and wakeup")
Signed-off-by: Haifeng Xu <[email protected]>
---
mm/memcontrol.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 5abffe6f8389..360fa7cf7879 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1999,7 +1999,7 @@ bool mem_cgroup_oom_synchronize(bool handle)
if (locked)
mem_cgroup_oom_notify(memcg);
- if (locked && !memcg->oom_kill_disable) {
+ if (!memcg->oom_kill_disable) {
mem_cgroup_unmark_under_oom(memcg);
finish_wait(&memcg_oom_waitq, &owait.wait);
mem_cgroup_out_of_memory(memcg, current->memcg_oom_gfp_mask,
@@ -2010,15 +2010,8 @@ bool mem_cgroup_oom_synchronize(bool handle)
finish_wait(&memcg_oom_waitq, &owait.wait);
}
- if (locked) {
+ if (locked)
mem_cgroup_oom_unlock(memcg);
- /*
- * There is no guarantee that an OOM-lock contender
- * sees the wakeups triggered by the OOM kill
- * uncharges. Wake any sleepers explicitly.
- */
- memcg_oom_recover(memcg);
- }
cleanup:
current->memcg_in_oom = NULL;
css_put(&memcg->css);
--
2.25.1
On Wed 15-03-23 07:03:02, Haifeng Xu wrote:
> Since commit 29ef680ae7c2 ("memcg, oom: move out_of_memory back to
> the charge path"), only oom_kill_disable is set, oom killer will
> be delayed to page fault path. In the charge patch, even if the
> oom_lock in memcg can't be acquired, the oom handing can also be
> invoked. In order to keep the behavior consistent with it, remove
> the lock check, just leave oom_kill_disable check behind in the
> page fault path.
I do not understand the actual problem you are trying to deal with here.
> Furthermore, the lock contender won't be scheduled out, this doesn't
> fit the sixth description in commit fb2a6fc56be66 ("mm: memcg:
> rework and document OOM waiting and wakeup"). So remove the explicit
> wakeup for the lock holder.
>
> Fixes: fb2a6fc56be6 ("mm: memcg: rework and document OOM waiting and wakeup")
The subject mentions a clean up but the fixes tag would indicate an
acutal fix.
> Signed-off-by: Haifeng Xu <[email protected]>
> ---
> mm/memcontrol.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5abffe6f8389..360fa7cf7879 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1999,7 +1999,7 @@ bool mem_cgroup_oom_synchronize(bool handle)
> if (locked)
> mem_cgroup_oom_notify(memcg);
>
> - if (locked && !memcg->oom_kill_disable) {
> + if (!memcg->oom_kill_disable) {
> mem_cgroup_unmark_under_oom(memcg);
> finish_wait(&memcg_oom_waitq, &owait.wait);
> mem_cgroup_out_of_memory(memcg, current->memcg_oom_gfp_mask,
Now looking at the actual code I suspect you in fact want to simplify
the logic here as mem_cgroup_oom_synchronize is only ever triggered whe
oom_kill_disable == true because current->memcg_in_oom is never non NULL
otherwise. So the check is indeed unnecessary. Your patch, however
doesn't really simplify the code much.
Did you want this instead?
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 12559c08d976..a77dc88cfa12 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1999,16 +1999,9 @@ bool mem_cgroup_oom_synchronize(bool handle)
if (locked)
mem_cgroup_oom_notify(memcg);
- if (locked && !READ_ONCE(memcg->oom_kill_disable)) {
- mem_cgroup_unmark_under_oom(memcg);
- finish_wait(&memcg_oom_waitq, &owait.wait);
- mem_cgroup_out_of_memory(memcg, current->memcg_oom_gfp_mask,
- current->memcg_oom_order);
- } else {
- schedule();
- mem_cgroup_unmark_under_oom(memcg);
- finish_wait(&memcg_oom_waitq, &owait.wait);
- }
+ schedule();
+ mem_cgroup_unmark_under_oom(memcg);
+ finish_wait(&memcg_oom_waitq, &owait.wait);
if (locked) {
mem_cgroup_oom_unlock(memcg);
> @@ -2010,15 +2010,8 @@ bool mem_cgroup_oom_synchronize(bool handle)
> finish_wait(&memcg_oom_waitq, &owait.wait);
> }
>
> - if (locked) {
> + if (locked)
> mem_cgroup_oom_unlock(memcg);
> - /*
> - * There is no guarantee that an OOM-lock contender
> - * sees the wakeups triggered by the OOM kill
> - * uncharges. Wake any sleepers explicitly.
> - */
> - memcg_oom_recover(memcg);
> - }
Hmm, so this seems unneded as well for the oom_kill_disable case as
well. Rather than referring to fb2a6fc56be66 it would be better to
why the explicit recovery is not really needed anymore.
> cleanup:
> current->memcg_in_oom = NULL;
> css_put(&memcg->css);
--
Michal Hocko
SUSE Labs
On 2023/3/17 19:47, Michal Hocko wrote:
> On Wed 15-03-23 07:03:02, Haifeng Xu wrote:
>> Since commit 29ef680ae7c2 ("memcg, oom: move out_of_memory back to
>> the charge path"), only oom_kill_disable is set, oom killer will
>> be delayed to page fault path. In the charge patch, even if the
>> oom_lock in memcg can't be acquired, the oom handing can also be
>> invoked. In order to keep the behavior consistent with it, remove
>> the lock check, just leave oom_kill_disable check behind in the
>> page fault path.
>
> I do not understand the actual problem you are trying to deal with here.
>
>> Furthermore, the lock contender won't be scheduled out, this doesn't
>> fit the sixth description in commit fb2a6fc56be66 ("mm: memcg:
>> rework and document OOM waiting and wakeup"). So remove the explicit
>> wakeup for the lock holder.
>>
>> Fixes: fb2a6fc56be6 ("mm: memcg: rework and document OOM waiting and wakeup")
>
> The subject mentions a clean up but the fixes tag would indicate an
> acutal fix.
>
>> Signed-off-by: Haifeng Xu <[email protected]>
>> ---
>> mm/memcontrol.c | 11 ++---------
>> 1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 5abffe6f8389..360fa7cf7879 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1999,7 +1999,7 @@ bool mem_cgroup_oom_synchronize(bool handle)
>> if (locked)
>> mem_cgroup_oom_notify(memcg);
>>
>> - if (locked && !memcg->oom_kill_disable) {
>> + if (!memcg->oom_kill_disable) {
>> mem_cgroup_unmark_under_oom(memcg);
>> finish_wait(&memcg_oom_waitq, &owait.wait);
>> mem_cgroup_out_of_memory(memcg, current->memcg_oom_gfp_mask,
>
> Now looking at the actual code I suspect you in fact want to simplify
> the logic here as mem_cgroup_oom_synchronize is only ever triggered whe
> oom_kill_disable == true because current->memcg_in_oom is never non NULL
> otherwise. So the check is indeed unnecessary. Your patch, however
> doesn't really simplify the code much.
>
> Did you want this instead?
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 12559c08d976..a77dc88cfa12 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1999,16 +1999,9 @@ bool mem_cgroup_oom_synchronize(bool handle)
> if (locked)
> mem_cgroup_oom_notify(memcg);
>
> - if (locked && !READ_ONCE(memcg->oom_kill_disable)) {
> - mem_cgroup_unmark_under_oom(memcg);
> - finish_wait(&memcg_oom_waitq, &owait.wait);
> - mem_cgroup_out_of_memory(memcg, current->memcg_oom_gfp_mask,
> - current->memcg_oom_order);
> - } else {
> - schedule();
> - mem_cgroup_unmark_under_oom(memcg);
> - finish_wait(&memcg_oom_waitq, &owait.wait);
> - }
> + schedule();
> + mem_cgroup_unmark_under_oom(memcg);
> + finish_wait(&memcg_oom_waitq, &owait.wait);
>
> if (locked) {
> mem_cgroup_oom_unlock(memcg);
>
Yes, the chance that someone else disable the oom_kill_disable again in the page fault path
is quite low.
>> @@ -2010,15 +2010,8 @@ bool mem_cgroup_oom_synchronize(bool handle)
>> finish_wait(&memcg_oom_waitq, &owait.wait);
>> }
>>
>> - if (locked) {
>> + if (locked)
>> mem_cgroup_oom_unlock(memcg);
>> - /*
>> - * There is no guarantee that an OOM-lock contender
>> - * sees the wakeups triggered by the OOM kill
>> - * uncharges. Wake any sleepers explicitly.
>> - */
>> - memcg_oom_recover(memcg);
>> - }
>
> Hmm, so this seems unneded as well for the oom_kill_disable case as
> well. Rather than referring to fb2a6fc56be66 it would be better to
> why the explicit recovery is not really needed anymore.
>
>> cleanup:
>> current->memcg_in_oom = NULL;
>> css_put(&memcg->css);
>
Thank you for your suggestion. I'll post an official patch later.