| Message ID | 56180e53-b705-b1be-9b60-75e141c8560c@virtuozzo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memcg: prohibit unconditional exceeding the limit of dying tasks | expand |
On Wed 20-10-21 15:13:46, Vasily Averin wrote: > ToDo: should we keep task_is_dying() in mem_cgroup_out_of_memory() ? > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > --- > mm/memcontrol.c | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 6da5020a8656..74a7379dbac1 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -239,7 +239,7 @@ enum res_type { > iter != NULL; \ > iter = mem_cgroup_iter(NULL, iter, NULL)) > > -static inline bool should_force_charge(void) > +static inline bool task_is_dying(void) > { > return tsk_is_oom_victim(current) || fatal_signal_pending(current) || > (current->flags & PF_EXITING); > @@ -1575,7 +1575,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask, > * A few threads which were not waiting at mutex_lock_killable() can > * fail to bail out. Therefore, check again after holding oom_lock. > */ > - ret = should_force_charge() || out_of_memory(&oc); > + ret = task_is_dying() || out_of_memory(&oc); Why are you keeping the task_is_dying check here? IIRC I have already pointed out that out_of_memory already has some means to do a bypass when needed.
On 20.10.2021 15:41, Michal Hocko wrote: > On Wed 20-10-21 15:13:46, Vasily Averin wrote: >> ToDo: should we keep task_is_dying() in mem_cgroup_out_of_memory() ? >> >> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> >> --- >> mm/memcontrol.c | 20 +++++++------------- >> 1 file changed, 7 insertions(+), 13 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 6da5020a8656..74a7379dbac1 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -239,7 +239,7 @@ enum res_type { >> iter != NULL; \ >> iter = mem_cgroup_iter(NULL, iter, NULL)) >> >> -static inline bool should_force_charge(void) >> +static inline bool task_is_dying(void) >> { >> return tsk_is_oom_victim(current) || fatal_signal_pending(current) || >> (current->flags & PF_EXITING); >> @@ -1575,7 +1575,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask, >> * A few threads which were not waiting at mutex_lock_killable() can >> * fail to bail out. Therefore, check again after holding oom_lock. >> */ >> - ret = should_force_charge() || out_of_memory(&oc); >> + ret = task_is_dying() || out_of_memory(&oc); > > Why are you keeping the task_is_dying check here? IIRC I have already > pointed out that out_of_memory already has some means to do a bypass > when needed. It was a misunderstanding. I've been waiting for your final decision. I have no good arguments "pro" or strong objection "contra". However, I prefer to keep task_is_dying() so as not to touch other tasks unnecessarily. I can't justify why its removal is necessary, but on the other hand, it shouldn't break anything. I can drop it if you think it's necessary. Thank you, Vasily Averin
On Wed 20-10-21 17:21:33, Vasily Averin wrote: > On 20.10.2021 15:41, Michal Hocko wrote: > > On Wed 20-10-21 15:13:46, Vasily Averin wrote: > >> ToDo: should we keep task_is_dying() in mem_cgroup_out_of_memory() ? > >> > >> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > >> --- > >> mm/memcontrol.c | 20 +++++++------------- > >> 1 file changed, 7 insertions(+), 13 deletions(-) > >> > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c > >> index 6da5020a8656..74a7379dbac1 100644 > >> --- a/mm/memcontrol.c > >> +++ b/mm/memcontrol.c > >> @@ -239,7 +239,7 @@ enum res_type { > >> iter != NULL; \ > >> iter = mem_cgroup_iter(NULL, iter, NULL)) > >> > >> -static inline bool should_force_charge(void) > >> +static inline bool task_is_dying(void) > >> { > >> return tsk_is_oom_victim(current) || fatal_signal_pending(current) || > >> (current->flags & PF_EXITING); > >> @@ -1575,7 +1575,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask, > >> * A few threads which were not waiting at mutex_lock_killable() can > >> * fail to bail out. Therefore, check again after holding oom_lock. > >> */ > >> - ret = should_force_charge() || out_of_memory(&oc); > >> + ret = task_is_dying() || out_of_memory(&oc); > > > > Why are you keeping the task_is_dying check here? IIRC I have already > > pointed out that out_of_memory already has some means to do a bypass > > when needed. > > It was a misunderstanding. Sorry if I made you confused. > I've been waiting for your final decision. > > I have no good arguments "pro" or strong objection "contra". > However, I prefer to keep task_is_dying() so as not to touch other tasks unnecessarily. One argument for removing it from here is the maintainability. Now you have a memcg specific check which is not in sync with the oom. E.g. out_of_memory does task_will_free_mem as the very first thing. You are also automatically excluding oom killer for cases where that might make a sense.
On 2021/10/20 23:57, Michal Hocko wrote: > One argument for removing it from here is the maintainability. Now you > have a memcg specific check which is not in sync with the oom. E.g. > out_of_memory does task_will_free_mem as the very first thing. You are > also automatically excluding oom killer for cases where that might make > a sense. What makes it possible to remove this check? This check was added because task_will_free_mem() in out_of_memory() does NOT work. See commit 7775face207922ea ("memcg: killed threads should not invoke memcg OOM killer").
On Thu 21-10-21 00:20:02, Tetsuo Handa wrote: > On 2021/10/20 23:57, Michal Hocko wrote: > > One argument for removing it from here is the maintainability. Now you > > have a memcg specific check which is not in sync with the oom. E.g. > > out_of_memory does task_will_free_mem as the very first thing. You are > > also automatically excluding oom killer for cases where that might make > > a sense. > > What makes it possible to remove this check? > This check was added because task_will_free_mem() in out_of_memory() does NOT work. > See commit 7775face207922ea ("memcg: killed threads should not invoke memcg OOM killer"). You are right. I've forgot about this and should have checked git blame. Thanks for bringing this up!
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 6da5020a8656..74a7379dbac1 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -239,7 +239,7 @@ enum res_type { iter != NULL; \ iter = mem_cgroup_iter(NULL, iter, NULL)) -static inline bool should_force_charge(void) +static inline bool task_is_dying(void) { return tsk_is_oom_victim(current) || fatal_signal_pending(current) || (current->flags & PF_EXITING); @@ -1575,7 +1575,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask, * A few threads which were not waiting at mutex_lock_killable() can * fail to bail out. Therefore, check again after holding oom_lock. */ - ret = should_force_charge() || out_of_memory(&oc); + ret = task_is_dying() || out_of_memory(&oc); unlock: mutex_unlock(&oom_lock); @@ -2530,6 +2530,7 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, struct page_counter *counter; enum oom_status oom_status; unsigned long nr_reclaimed; + bool passed_oom = false; bool may_swap = true; bool drained = false; unsigned long pflags; @@ -2564,15 +2565,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, if (gfp_mask & __GFP_ATOMIC) goto force; - /* - * Unlike in global OOM situations, memcg is not in a physical - * memory shortage. Allow dying and OOM-killed tasks to - * bypass the last charges so that they can exit quickly and - * free their memory. - */ - if (unlikely(should_force_charge())) - goto force; - /* * Prevent unbounded recursion when reclaim operations need to * allocate memory. This might exceed the limits temporarily, @@ -2630,8 +2622,9 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, if (gfp_mask & __GFP_RETRY_MAYFAIL) goto nomem; - if (fatal_signal_pending(current)) - goto force; + /* Avoid endless loop for tasks bypassed by the oom killer */ + if (passed_oom && task_is_dying()) + goto nomem; /* * keep retrying as long as the memcg oom killer is able to make @@ -2642,6 +2635,7 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, get_order(nr_pages * PAGE_SIZE)); switch (oom_status) { case OOM_SUCCESS: + passed_oom = true; nr_retries = MAX_RECLAIM_RETRIES; goto retry; case OOM_FAILED:
ToDo: should we keep task_is_dying() in mem_cgroup_out_of_memory() ? Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- mm/memcontrol.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)