Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1701167 > unrolled thread

Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-08-01 17:40 +0200
    Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory  reserves access Michal Hocko <mhocko@kernel.org> - 2017-08-01 19:00 +0200
      Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory  reserves access Michal Hocko <mhocko@kernel.org> - 2017-08-02 08:20 +0200
      Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-08-03 03:50 +0200
        Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory  reserves access Michal Hocko <mhocko@kernel.org> - 2017-08-03 09:10 +0200
          Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-08-03 10:10 +0200
            Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory  reserves access Michal Hocko <mhocko@kernel.org> - 2017-08-03 10:30 +0200

#1701167 — Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access

Michal Hocko wrote:
> CONFIG_MMU=n doesn't have oom reaper so let's stick to the original
> ALLOC_NO_WATERMARKS approach but be careful because they still might
> deplete all the memory reserves so keep the semantic as close to the
> original implementation as possible and give them access to memory
> reserves only up to exit_mm (when tsk->mm is cleared) rather than while
> tsk_is_oom_victim which is until signal struct is gone.

Currently memory allocations from __mmput() can use memory reserves but
this patch changes __mmput() not to use memory reserves. You say "keep
the semantic as close to the original implementation as possible" but
this change is not guaranteed to be safe.

> @@ -2943,10 +2943,19 @@ bool __zone_watermark_ok(struct zone *z, unsigned int order, unsigned long mark,
>  	 * the high-atomic reserves. This will over-estimate the size of the
>  	 * atomic reserve but it avoids a search.
>  	 */
> -	if (likely(!alloc_harder))
> +	if (likely(!alloc_harder)) {
>  		free_pages -= z->nr_reserved_highatomic;
> -	else
> -		min -= min / 4;
> +	} else {
> +		/*
> +		 * OOM victims can try even harder than normal ALLOC_HARDER
> +		 * users
> +		 */
> +		if (alloc_flags & ALLOC_OOM)

ALLOC_OOM is ALLOC_NO_WATERMARKS if CONFIG_MMU=n.
I wonder this test makes sense for ALLOC_NO_WATERMARKS.

> +			min -= min / 2;
> +		else
> +			min -= min / 4;
> +	}
> +
>  
>  #ifdef CONFIG_CMA
>  	/* If allocation can't use CMA areas don't use free CMA pages */
> @@ -3603,6 +3612,22 @@ gfp_to_alloc_flags(gfp_t gfp_mask)
>  	return alloc_flags;
>  }
>  
> +static bool oom_reserves_allowed(struct task_struct *tsk)
> +{
> +	if (!tsk_is_oom_victim(tsk))
> +		return false;
> +
> +	/*
> +	 * !MMU doesn't have oom reaper so we shouldn't risk the memory reserves
> +	 * depletion and shouldn't give access to memory reserves passed the
> +	 * exit_mm
> +	 */
> +	if (!IS_ENABLED(CONFIG_MMU) && !tsk->mm)
> +		return false;

Branching based on CONFIG_MMU is ugly. I suggest timeout based next OOM
victim selection if CONFIG_MMU=n. Then, we no longer need to worry about
memory reserves depletion and we can treat equally.

> +
> +	return true;
> +}
> +
>  bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>  {
>  	if (unlikely(gfp_mask & __GFP_NOMEMALLOC))

> @@ -3770,6 +3795,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	unsigned long alloc_start = jiffies;
>  	unsigned int stall_timeout = 10 * HZ;
>  	unsigned int cpuset_mems_cookie;
> +	bool reserves;
>  
>  	/*
>  	 * In the slowpath, we sanity check order to avoid ever trying to
> @@ -3875,15 +3901,24 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (gfp_mask & __GFP_KSWAPD_RECLAIM)
>  		wake_all_kswapds(order, ac);
>  
> -	if (gfp_pfmemalloc_allowed(gfp_mask))
> -		alloc_flags = ALLOC_NO_WATERMARKS;
> +	/*
> +	 * Distinguish requests which really need access to whole memory
> +	 * reserves from oom victims which can live with their own reserve
> +	 */
> +	reserves = gfp_pfmemalloc_allowed(gfp_mask);
> +	if (reserves) {
> +		if (tsk_is_oom_victim(current))
> +			alloc_flags = ALLOC_OOM;

If reserves == true due to reasons other than tsk_is_oom_victim(current) == true
(e.g. __GFP_MEMALLOC), why dare to reduce it?

> +		else
> +			alloc_flags = ALLOC_NO_WATERMARKS;
> +	}

If CONFIG_MMU=n, doing this test is silly.

if (tsk_is_oom_victim(current))
	alloc_flags = ALLOC_NO_WATERMARKS;
else
	alloc_flags = ALLOC_NO_WATERMARKS;

>  
>  	/*
>  	 * Reset the zonelist iterators if memory policies can be ignored.
>  	 * These allocations are high priority and system rather than user
>  	 * orientated.
>  	 */
> -	if (!(alloc_flags & ALLOC_CPUSET) || (alloc_flags & ALLOC_NO_WATERMARKS)) {
> +	if (!(alloc_flags & ALLOC_CPUSET) || reserves) {
>  		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
>  		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
>  					ac->high_zoneidx, ac->nodemask);
> @@ -3960,7 +3995,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  		goto got_pg;
>  
>  	/* Avoid allocations with no watermarks from looping endlessly */
> -	if (test_thread_flag(TIF_MEMDIE) &&
> +	if (tsk_is_oom_victim(current) &&
>  	    (alloc_flags == ALLOC_NO_WATERMARKS ||
>  	     (gfp_mask & __GFP_NOMEMALLOC)))
>  		goto nopage;

And you are silently changing to "!costly __GFP_DIRECT_RECLAIM allocations never fail
(even selected for OOM victims)" (i.e. updating the too small to fail memory allocation
rule) by doing alloc_flags == ALLOC_NO_WATERMARKS if CONFIG_MMU=y.

Applying this change might disturb memory allocation behavior. I don't like this patch.

[toc] | [next] | [standalone]

#1701248 — Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access

On Wed 02-08-17 00:30:33, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > CONFIG_MMU=n doesn't have oom reaper so let's stick to the original
> > ALLOC_NO_WATERMARKS approach but be careful because they still might
> > deplete all the memory reserves so keep the semantic as close to the
> > original implementation as possible and give them access to memory
> > reserves only up to exit_mm (when tsk->mm is cleared) rather than while
> > tsk_is_oom_victim which is until signal struct is gone.
> 
> Currently memory allocations from __mmput() can use memory reserves but
> this patch changes __mmput() not to use memory reserves. You say "keep
> the semantic as close to the original implementation as possible" but
> this change is not guaranteed to be safe.

Yeah it cannot. That's why I've said as close as possible rather than
equivalent. On the other hand I am wondering whether you have anything
specific in mind or this is just a formalistic nitpicking^Wremark.

> > @@ -2943,10 +2943,19 @@ bool __zone_watermark_ok(struct zone *z, unsigned int order, unsigned long mark,
> >  	 * the high-atomic reserves. This will over-estimate the size of the
> >  	 * atomic reserve but it avoids a search.
> >  	 */
> > -	if (likely(!alloc_harder))
> > +	if (likely(!alloc_harder)) {
> >  		free_pages -= z->nr_reserved_highatomic;
> > -	else
> > -		min -= min / 4;
> > +	} else {
> > +		/*
> > +		 * OOM victims can try even harder than normal ALLOC_HARDER
> > +		 * users
> > +		 */
> > +		if (alloc_flags & ALLOC_OOM)
> 
> ALLOC_OOM is ALLOC_NO_WATERMARKS if CONFIG_MMU=n.
> I wonder this test makes sense for ALLOC_NO_WATERMARKS.

Yeah, it would be pointless because get_page_from_freelist will then
ignore the result of the watermark check for ALLOC_NO_WATERMARKS. It is
not harmfull though. I didn't find much better way without making the
code harder to read.  Do you have any suggestion?

> > +			min -= min / 2;
> > +		else
> > +			min -= min / 4;
> > +	}
> > +
> >  
> >  #ifdef CONFIG_CMA
> >  	/* If allocation can't use CMA areas don't use free CMA pages */
> > @@ -3603,6 +3612,22 @@ gfp_to_alloc_flags(gfp_t gfp_mask)
> >  	return alloc_flags;
> >  }
> >  
> > +static bool oom_reserves_allowed(struct task_struct *tsk)
> > +{
> > +	if (!tsk_is_oom_victim(tsk))
> > +		return false;
> > +
> > +	/*
> > +	 * !MMU doesn't have oom reaper so we shouldn't risk the memory reserves
> > +	 * depletion and shouldn't give access to memory reserves passed the
> > +	 * exit_mm
> > +	 */
> > +	if (!IS_ENABLED(CONFIG_MMU) && !tsk->mm)
> > +		return false;
> 
> Branching based on CONFIG_MMU is ugly. I suggest timeout based next OOM
> victim selection if CONFIG_MMU=n.

I suggest we do not argue about nommu without actually optimizing for or
fixing nommu which we are not here. I am even not sure memory reserves
can ever be depleted for that config.

Anyway I will go with the following instead
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5e5911f40014..3510e06b3bf3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3618,11 +3618,10 @@ static bool oom_reserves_allowed(struct task_struct *tsk)
 		return false;
 
 	/*
-	 * !MMU doesn't have oom reaper so we shouldn't risk the memory reserves
-	 * depletion and shouldn't give access to memory reserves passed the
-	 * exit_mm
+	 * !MMU doesn't have oom reaper so give access to memory reserves
+	 * only to the thread with TIF_MEMDIE set
 	 */
-	if (!IS_ENABLED(CONFIG_MMU) && !tsk->mm)
+	if (!IS_ENABLED(CONFIG_MMU) && !test_thread_flag(TIF_MEMDIE))
 		return false;
 
 	return true;

This should preserve the original semantic. Is that acceptable for you?

> > @@ -3875,15 +3901,24 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  	if (gfp_mask & __GFP_KSWAPD_RECLAIM)
> >  		wake_all_kswapds(order, ac);
> >  
> > -	if (gfp_pfmemalloc_allowed(gfp_mask))
> > -		alloc_flags = ALLOC_NO_WATERMARKS;
> > +	/*
> > +	 * Distinguish requests which really need access to whole memory
> > +	 * reserves from oom victims which can live with their own reserve
> > +	 */
> > +	reserves = gfp_pfmemalloc_allowed(gfp_mask);
> > +	if (reserves) {
> > +		if (tsk_is_oom_victim(current))
> > +			alloc_flags = ALLOC_OOM;
> 
> If reserves == true due to reasons other than tsk_is_oom_victim(current) == true
> (e.g. __GFP_MEMALLOC), why dare to reduce it?

Well the comment above tries to explain. I assume that the oom victim is
special here. a) it is on the way to die and b) we know that something
will be freeing memory on the background so I assume this is acceptable.
 
> > +		else
> > +			alloc_flags = ALLOC_NO_WATERMARKS;
> > +	}
> 
> If CONFIG_MMU=n, doing this test is silly.
> 
> if (tsk_is_oom_victim(current))
> 	alloc_flags = ALLOC_NO_WATERMARKS;
> else
> 	alloc_flags = ALLOC_NO_WATERMARKS;

I am pretty sure any compiler can see the outcome is the same so the
check would be dropped in that case. I primarily wanted to prevent from
an additional ifdefery. I am open to suggestions for a better layout
though.

> > @@ -3960,7 +3995,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  		goto got_pg;
> >  
> >  	/* Avoid allocations with no watermarks from looping endlessly */
> > -	if (test_thread_flag(TIF_MEMDIE) &&
> > +	if (tsk_is_oom_victim(current) &&
> >  	    (alloc_flags == ALLOC_NO_WATERMARKS ||
> >  	     (gfp_mask & __GFP_NOMEMALLOC)))
> >  		goto nopage;
> 
> And you are silently changing to "!costly __GFP_DIRECT_RECLAIM allocations never fail
> (even selected for OOM victims)" (i.e. updating the too small to fail memory allocation
> rule) by doing alloc_flags == ALLOC_NO_WATERMARKS if CONFIG_MMU=y.

Ups that is an oversight during the rebase.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5e5911f40014..6593ff9de1d9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3996,7 +3996,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 
 	/* Avoid allocations with no watermarks from looping endlessly */
 	if (tsk_is_oom_victim(current) &&
-	    (alloc_flags == ALLOC_NO_WATERMARKS ||
+	    (alloc_flags == ALLOC_OOM ||
 	     (gfp_mask & __GFP_NOMEMALLOC)))
 		goto nopage;
 
Does this look better?
 
> Applying this change might disturb memory allocation behavior. I don't
> like this patch.

Do you see anything appart from nommu that would be an unfixable road
block?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1701751 — Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access

On Tue 01-08-17 18:52:42, Michal Hocko wrote:
> On Wed 02-08-17 00:30:33, Tetsuo Handa wrote:
[...]
> > > -	if (gfp_pfmemalloc_allowed(gfp_mask))
> > > -		alloc_flags = ALLOC_NO_WATERMARKS;
> > > +	/*
> > > +	 * Distinguish requests which really need access to whole memory
> > > +	 * reserves from oom victims which can live with their own reserve
> > > +	 */
> > > +	reserves = gfp_pfmemalloc_allowed(gfp_mask);
> > > +	if (reserves) {
> > > +		if (tsk_is_oom_victim(current))
> > > +			alloc_flags = ALLOC_OOM;
> > 
> > If reserves == true due to reasons other than tsk_is_oom_victim(current) == true
> > (e.g. __GFP_MEMALLOC), why dare to reduce it?
> 
> Well the comment above tries to explain. I assume that the oom victim is
> special here. a) it is on the way to die and b) we know that something
> will be freeing memory on the background so I assume this is acceptable.

I was thinking about this some more. It is not that hard to achive the
original semantic. The code is slightly uglier but acceptable I guess
What do you think about the following?
---
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3510e06b3bf3..7ae0f6d45614 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3627,21 +3627,31 @@ static bool oom_reserves_allowed(struct task_struct *tsk)
 	return true;
 }
 
-bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
+/*
+ * Distinguish requests which really need access to full memory
+ * reserves from oom victims which can live with a portion of it
+ */
+static inline int __gfp_pfmemalloc_flags(gfp_t gfp_mask)
 {
 	if (unlikely(gfp_mask & __GFP_NOMEMALLOC))
-		return false;
-
+		return 0;
 	if (gfp_mask & __GFP_MEMALLOC)
-		return true;
+		return ALLOC_NO_WATERMARKS;
 	if (in_serving_softirq() && (current->flags & PF_MEMALLOC))
-		return true;
-	if (!in_interrupt() &&
-			((current->flags & PF_MEMALLOC) ||
-			 oom_reserves_allowed(current)))
-		return true;
+		return ALLOC_NO_WATERMARKS;
+	if (!in_interrupt()) {
+		if (current->flags & PF_MEMALLOC)
+			return ALLOC_NO_WATERMARKS;
+		else if (oom_reserves_allowed(current))
+			return ALLOC_OOM;
+	}
 
-	return false;
+	return 0;
+}
+
+bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
+{
+	return __gfp_pfmemalloc_flags(gfp_mask) > 0;
 }
 
 /*
@@ -3794,7 +3804,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	unsigned long alloc_start = jiffies;
 	unsigned int stall_timeout = 10 * HZ;
 	unsigned int cpuset_mems_cookie;
-	bool reserves;
+	int reserves;
 
 	/*
 	 * In the slowpath, we sanity check order to avoid ever trying to
@@ -3900,17 +3910,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (gfp_mask & __GFP_KSWAPD_RECLAIM)
 		wake_all_kswapds(order, ac);
 
-	/*
-	 * Distinguish requests which really need access to whole memory
-	 * reserves from oom victims which can live with their own reserve
-	 */
-	reserves = gfp_pfmemalloc_allowed(gfp_mask);
-	if (reserves) {
-		if (tsk_is_oom_victim(current))
-			alloc_flags = ALLOC_OOM;
-		else
-			alloc_flags = ALLOC_NO_WATERMARKS;
-	}
+	reserves = __gfp_pfmemalloc_flags(gfp_mask);
+	if (reserves)
+		alloc_flags = reserves;
 
 	/*
 	 * Reset the zonelist iterators if memory policies can be ignored.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1702611

Michal Hocko wrote:
> On Wed 02-08-17 00:30:33, Tetsuo Handa wrote:
> > > @@ -3603,6 +3612,22 @@ gfp_to_alloc_flags(gfp_t gfp_mask)
> > >  	return alloc_flags;
> > >  }
> > >  
> > > +static bool oom_reserves_allowed(struct task_struct *tsk)
> > > +{
> > > +	if (!tsk_is_oom_victim(tsk))
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * !MMU doesn't have oom reaper so we shouldn't risk the memory reserves
> > > +	 * depletion and shouldn't give access to memory reserves passed the
> > > +	 * exit_mm
> > > +	 */
> > > +	if (!IS_ENABLED(CONFIG_MMU) && !tsk->mm)
> > > +		return false;
> > 
> > Branching based on CONFIG_MMU is ugly. I suggest timeout based next OOM
> > victim selection if CONFIG_MMU=n.
> 
> I suggest we do not argue about nommu without actually optimizing for or
> fixing nommu which we are not here. I am even not sure memory reserves
> can ever be depleted for that config.

I don't think memory reserves can deplete for CONFIG_MMU=n environment.
But the reason the OOM reaper was introduced is not limited to handling
depletion of memory reserves. The OOM reaper was introduced because
OOM victims might get stuck indirectly waiting for other threads doing
memory allocation. You said

  > Yes, exit_aio is the only blocking call I know of currently. But I would
  > like this to be as robust as possible and so I do not want to rely on
  > the current implementation. This can change in future and I can
  > guarantee that nobody will think about the oom path when adding
  > something to the final __mmput path.

at http://lkml.kernel.org/r/20170726054533.GA960@dhcp22.suse.cz , but
how can you guarantee that nobody will think about the oom path
when adding something to the final __mmput() path without thinking
about possibility of getting stuck waiting for memory allocation in
CONFIG_MMU=n environment? As long as possibility of getting stuck remains,
you should not assume that something you don't want will not happen.
It's time to make CONFIG_MMU=n kernels treatable like CONFIG_MMU=y kernels.

If it is technically impossible (or is not worthwhile) to implement
the OOM reaper for CONFIG_MMU=n kernels, I'm fine with timeout based
approach like shown below. Then, we no longer need to use branching
based on CONFIG_MMU.

 include/linux/mm_types.h |  3 +++
 mm/oom_kill.c            | 20 +++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 7f384bb..374a2ae 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -504,6 +504,9 @@ struct mm_struct {
 	atomic_long_t hugetlb_usage;
 #endif
 	struct work_struct async_put_work;
+#ifndef CONFIG_MMU
+	unsigned long oom_victim_wait_timer;
+#endif
 } __randomize_layout;
 
 extern struct mm_struct init_mm;
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 9e8b4f0..dd6239d 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -53,6 +53,17 @@
 
 DEFINE_MUTEX(oom_lock);
 
+static bool should_ignore_this_mm(struct mm_struct *mm)
+{
+#ifndef CONFIG_MMU
+	if (!mm->oom_victim_wait_timer)
+		mm->oom_victim_wait_timer = jiffies;
+	else if (time_after(jiffies, mm->oom_victim_wait_timer + HZ))
+		return true;
+#endif
+	return test_bit(MMF_OOM_SKIP, &mm->flags);
+};
+
 #ifdef CONFIG_NUMA
 /**
  * has_intersects_mems_allowed() - check task eligiblity for kill
@@ -188,9 +199,8 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
 	 * the middle of vfork
 	 */
 	adj = (long)p->signal->oom_score_adj;
-	if (adj == OOM_SCORE_ADJ_MIN ||
-			test_bit(MMF_OOM_SKIP, &p->mm->flags) ||
-			in_vfork(p)) {
+	if (adj == OOM_SCORE_ADJ_MIN || should_ignore_this_mm(p->mm) ||
+	    in_vfork(p)) {
 		task_unlock(p);
 		return 0;
 	}
@@ -303,7 +313,7 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	 * any memory is quite low.
 	 */
 	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
-		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags))
+		if (should_ignore_this_mm(task->signal->oom_mm))
 			goto next;
 		goto abort;
 	}
@@ -783,7 +793,7 @@ static bool task_will_free_mem(struct task_struct *task)
 	 * This task has already been drained by the oom reaper so there are
 	 * only small chances it will free some more
 	 */
-	if (test_bit(MMF_OOM_SKIP, &mm->flags))
+	if (should_ignore_this_mm(mm))
 		return false;
 
 	if (atomic_read(&mm->mm_users) <= 1)
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1702720 — Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access

On Thu 03-08-17 10:39:42, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Wed 02-08-17 00:30:33, Tetsuo Handa wrote:
> > > > @@ -3603,6 +3612,22 @@ gfp_to_alloc_flags(gfp_t gfp_mask)
> > > >  	return alloc_flags;
> > > >  }
> > > >  
> > > > +static bool oom_reserves_allowed(struct task_struct *tsk)
> > > > +{
> > > > +	if (!tsk_is_oom_victim(tsk))
> > > > +		return false;
> > > > +
> > > > +	/*
> > > > +	 * !MMU doesn't have oom reaper so we shouldn't risk the memory reserves
> > > > +	 * depletion and shouldn't give access to memory reserves passed the
> > > > +	 * exit_mm
> > > > +	 */
> > > > +	if (!IS_ENABLED(CONFIG_MMU) && !tsk->mm)
> > > > +		return false;
> > > 
> > > Branching based on CONFIG_MMU is ugly. I suggest timeout based next OOM
> > > victim selection if CONFIG_MMU=n.
> > 
> > I suggest we do not argue about nommu without actually optimizing for or
> > fixing nommu which we are not here. I am even not sure memory reserves
> > can ever be depleted for that config.
> 
> I don't think memory reserves can deplete for CONFIG_MMU=n environment.
> But the reason the OOM reaper was introduced is not limited to handling
> depletion of memory reserves. The OOM reaper was introduced because
> OOM victims might get stuck indirectly waiting for other threads doing
> memory allocation. You said
> 
>   > Yes, exit_aio is the only blocking call I know of currently. But I would
>   > like this to be as robust as possible and so I do not want to rely on
>   > the current implementation. This can change in future and I can
>   > guarantee that nobody will think about the oom path when adding
>   > something to the final __mmput path.
> 
> at http://lkml.kernel.org/r/20170726054533.GA960@dhcp22.suse.cz , but
> how can you guarantee that nobody will think about the oom path
> when adding something to the final __mmput() path without thinking
> about possibility of getting stuck waiting for memory allocation in
> CONFIG_MMU=n environment?

Look, I really appreciate your sentiment for for nommu platform but with
an absolute lack of _any_ oom reports on that platform that I am aware
of nor any reports about lockups during oom I am less than thrilled to
add a code to fix a problem which even might not exist. Nommu is usually
very special with a very specific workload running (e.g. no overcommit)
so I strongly suspect that any OOM theories are highly academic.

All I do care about is to not regress nommu as much as possible. So can
we get back to the proposed patch and updates I have done to address
your review feedback please?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1702753

Michal Hocko wrote:
> Look, I really appreciate your sentiment for for nommu platform but with
> an absolute lack of _any_ oom reports on that platform that I am aware
> of nor any reports about lockups during oom I am less than thrilled to
> add a code to fix a problem which even might not exist. Nommu is usually
> very special with a very specific workload running (e.g. no overcommit)
> so I strongly suspect that any OOM theories are highly academic.

If you believe that there is really no oom report, get rid of the OOM
killer completely.

> 
> All I do care about is to not regress nommu as much as possible. So can
> we get back to the proposed patch and updates I have done to address
> your review feedback please?

No unless we get rid of the OOM killer if CONFIG_MMU=n.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 170db4d..e931969 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3312,7 +3312,8 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 		goto out;
 
 	/* Exhausted what can be done so it's blamo time */
-	if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
+	if ((IS_ENABLED(CONFIG_MMU) && out_of_memory(&oc)) ||
+		WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
 		*did_some_progress = 1;
 
 		/*

[toc] | [prev] | [next] | [standalone]

#1702774 — Re: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access

On Thu 03-08-17 17:03:20, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > Look, I really appreciate your sentiment for for nommu platform but with
> > an absolute lack of _any_ oom reports on that platform that I am aware
> > of nor any reports about lockups during oom I am less than thrilled to
> > add a code to fix a problem which even might not exist. Nommu is usually
> > very special with a very specific workload running (e.g. no overcommit)
> > so I strongly suspect that any OOM theories are highly academic.
> 
> If you believe that there is really no oom report, get rid of the OOM
> killer completely.

I am not an user or even an owner of such a platform. As I've said all I
care about is to not regress for those guys and I believe that the patch
doesn't change nommu behavior in any risky way. If yes, point them out
and I will try to address them.
 
> > All I do care about is to not regress nommu as much as possible. So can
> > we get back to the proposed patch and updates I have done to address
> > your review feedback please?
> 
> No unless we get rid of the OOM killer if CONFIG_MMU=n.

Are you saying that you are going to nack the patch based on this
reasoning? This is just ridiculous.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web