* [PATCH 2/2] memcg, oom: do not wake up memcg_oom_waitq if waitqueue is empty
@ 2023-09-22 7:05 Haifeng Xu
2023-09-22 23:22 ` Roman Gushchin
0 siblings, 1 reply; 3+ messages in thread
From: Haifeng Xu @ 2023-09-22 7:05 UTC (permalink / raw)
To: mhocko; +Cc: hannes, roman.gushchin, shakeelb, cgroups, linux-mm, Haifeng Xu
Only when memcg oom killer is disabled, the task which triggers memecg
oom handling will sleep on a waitqueue. Except this case, the waitqueue
is empty though under_oom is true. There is no need to step into wake
up path when resolve the oom situation. So add a check that whether the
waitqueue is empty.
Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
---
mm/memcontrol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 0b6ed63504ca..2bb98ff5be3d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1918,7 +1918,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
* achieved by invoking mem_cgroup_mark_under_oom() before
* triggering notification.
*/
- if (memcg && memcg->under_oom)
+ if (memcg && memcg->under_oom && !list_empty(&memcg_oom_waitq.head))
__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
}
--
2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] memcg, oom: do not wake up memcg_oom_waitq if waitqueue is empty
2023-09-22 7:05 [PATCH 2/2] memcg, oom: do not wake up memcg_oom_waitq if waitqueue is empty Haifeng Xu
@ 2023-09-22 23:22 ` Roman Gushchin
2023-09-23 8:01 ` Haifeng Xu
0 siblings, 1 reply; 3+ messages in thread
From: Roman Gushchin @ 2023-09-22 23:22 UTC (permalink / raw)
To: Haifeng Xu; +Cc: mhocko, hannes, shakeelb, cgroups, linux-mm
On Fri, Sep 22, 2023 at 07:05:29AM +0000, Haifeng Xu wrote:
> Only when memcg oom killer is disabled, the task which triggers memecg
> oom handling will sleep on a waitqueue. Except this case, the waitqueue
> is empty though under_oom is true. There is no need to step into wake
> up path when resolve the oom situation. So add a check that whether the
> waitqueue is empty.
>
> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
> ---
> mm/memcontrol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 0b6ed63504ca..2bb98ff5be3d 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1918,7 +1918,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
> * achieved by invoking mem_cgroup_mark_under_oom() before
> * triggering notification.
> */
> - if (memcg && memcg->under_oom)
> + if (memcg && memcg->under_oom && !list_empty(&memcg_oom_waitq.head))
> __wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
This change looks questionable to me:
1) it's not obvious that this racy check is fine. can an oom event be
missed because of a race here? why not?
2) is there any measurable impact? it's not a hot path, so I'd keep it
simple.
Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] memcg, oom: do not wake up memcg_oom_waitq if waitqueue is empty
2023-09-22 23:22 ` Roman Gushchin
@ 2023-09-23 8:01 ` Haifeng Xu
0 siblings, 0 replies; 3+ messages in thread
From: Haifeng Xu @ 2023-09-23 8:01 UTC (permalink / raw)
To: Roman Gushchin; +Cc: mhocko, hannes, shakeelb, cgroups, linux-mm
On 2023/9/23 07:22, Roman Gushchin wrote:
> On Fri, Sep 22, 2023 at 07:05:29AM +0000, Haifeng Xu wrote:
>> Only when memcg oom killer is disabled, the task which triggers memecg
>> oom handling will sleep on a waitqueue. Except this case, the waitqueue
>> is empty though under_oom is true. There is no need to step into wake
>> up path when resolve the oom situation. So add a check that whether the
>> waitqueue is empty.
>>
>> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
>> ---
>> mm/memcontrol.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 0b6ed63504ca..2bb98ff5be3d 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1918,7 +1918,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
>> * achieved by invoking mem_cgroup_mark_under_oom() before
>> * triggering notification.
>> */
>> - if (memcg && memcg->under_oom)
>> + if (memcg && memcg->under_oom && !list_empty(&memcg_oom_waitq.head))
>> __wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
>
> This change looks questionable to me:
> 1) it's not obvious that this racy check is fine. can an oom event be
> missed because of a race here? why not?
The oom event can't be missed, because the OOM task has been put on the waitqueue before marking the hierarchy as
under OOM(can be seen in mem_cgroup_oom_synchronize()).
> 2) is there any measurable impact?
No.
it's not a hot path, so I'd keep it
> simple.
ok, thanks.
>
> Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-23 8:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-22 7:05 [PATCH 2/2] memcg, oom: do not wake up memcg_oom_waitq if waitqueue is empty Haifeng Xu
2023-09-22 23:22 ` Roman Gushchin
2023-09-23 8:01 ` Haifeng Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox