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

Groups > linux.kernel > #1467504 > unrolled thread

OOM detection regressions since 4.7

Back to article view | Back to linux.kernel

Contents

  OOM detection regressions since 4.7 Michal Hocko <mhocko@kernel.org> - 2016-08-22 11:40 +0200
    Re: OOM detection regressions since 4.7 Markus Trippelsdorf <markus@trippelsdorf.de> - 2016-08-22 12:30 +0200
      Re: OOM detection regressions since 4.7 Michal Hocko <mhocko@kernel.org> - 2016-08-22 13:00 +0200
        Re: OOM detection regressions since 4.7 Markus Trippelsdorf <markus@trippelsdorf.de> - 2016-08-22 13:10 +0200
          Re: OOM detection regressions since 4.7 Michal Hocko <mhocko@kernel.org> - 2016-08-22 13:20 +0200
            Re: OOM detection regressions since 4.7 Markus Trippelsdorf <markus@trippelsdorf.de> - 2016-08-22 13:30 +0200
    Re: OOM detection regressions since 4.7 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-23 06:50 +0200
      Re: OOM detection regressions since 4.7 Michal Hocko <mhocko@kernel.org> - 2016-08-23 09:40 +0200
        Re: OOM detection regressions since 4.7 Michal Hocko <mhocko@kernel.org> - 2016-08-23 09:50 +0200
        Re: OOM detection regressions since 4.7 Markus Trippelsdorf <markus@trippelsdorf.de> - 2016-08-23 09:50 +0200
        Re: OOM detection regressions since 4.7 Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-23 21:20 +0200
          Re: OOM detection regressions since 4.7 Michal Hocko <mhocko@kernel.org> - 2016-08-24 08:40 +0200
        Re: OOM detection regressions since 4.7 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-24 09:10 +0200
          Re: OOM detection regressions since 4.7 Michal Hocko <mhocko@kernel.org> - 2016-08-24 09:10 +0200
            Re: OOM detection regressions since 4.7 Joonsoo Kim <js1304@gmail.com> - 2016-08-24 09:30 +0200

#1467504 — OOM detection regressions since 4.7

Hi, 
there have been multiple reports [1][2][3][4][5] about pre-mature OOM
killer invocations since 4.7 which contains oom detection rework. All of
them were for order-2 (kernel stack) alloaction requests failing because
of a high fragmentation and compaction failing to make any forward
progress. While investigating this we have found out that the compaction
just gives up too early. Vlastimil has been working on compaction
improvement for quite some time and his series [6] is already sitting
in mmotm tree. This already helps a lot because it drops some heuristics
which are more aimed at lower latencies for high orders rather than
reliability. Joonsoo has then identified further problem with too many
blocks being marked as unmovable [7] and Vlastimil has prepared a patch
on top of his series [8] which is also in the mmotm tree now.

That being said, the regression is real and should be fixed for 4.7
stable users. [6][8] was reported to help and ooms are no longer
reproducible. I know we are quite late (rc3) in 4.8 but I would vote
for mergeing those patches and have them in 4.8. For 4.7 I would go
with a partial revert of the detection rework for high order requests
(see patch below). This patch is really trivial. If those compaction
improvements are just too large for 4.8 then we can use the same patch
as for 4.7 stable for now and revert it in 4.9 after compaction changes
are merged.

Thoughts?

[1] http://lkml.kernel.org/r/20160731051121.GB307@x4
[2] http://lkml.kernel.org/r/201608120901.41463.a.miskiewicz@gmail.com
[3] http://lkml.kernel.org/r/20160801192620.GD31957@dhcp22.suse.cz
[4] https://lists.opensuse.org/opensuse-kernel/2016-08/msg00021.html
[5] https://bugzilla.opensuse.org/show_bug.cgi?id=994066
[6] http://lkml.kernel.org/r/20160810091226.6709-1-vbabka@suse.cz
[7] http://lkml.kernel.org/r/20160816031222.GC16913@js1304-P5Q-DELUXE
[8] http://lkml.kernel.org/r/f7a9ea9d-bb88-bfd6-e340-3a933559305a@suse.cz

---
From 899b738538de41295839dca2090a774bdd17acd2 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 22 Aug 2016 10:52:06 +0200
Subject: [PATCH] mm, oom: prevent pre-mature OOM killer invocation for high
 order request

There have been several reports about pre-mature OOM killer invocation
in 4.7 kernel when order-2 allocation request (for the kernel stack)
invoked OOM killer even during basic workloads (light IO or even kernel
compile on some filesystems). In all reported cases the memory is
fragmented and there are no order-2+ pages available. There is usually
a large amount of slab memory (usually dentries/inodes) and further
debugging has shown that there are way too many unmovable blocks which
are skipped during the compaction. Multiple reporters have confirmed that
the current linux-next which includes [1] and [2] helped and OOMs are
not reproducible anymore. A simpler fix for the stable is to simply
ignore the compaction feedback and retry as long as there is a reclaim
progress for high order requests which we used to do before. We already
do that for CONFING_COMPACTION=n so let's reuse the same code when
compaction is enabled as well.

[1] http://lkml.kernel.org/r/20160810091226.6709-1-vbabka@suse.cz
[2] http://lkml.kernel.org/r/f7a9ea9d-bb88-bfd6-e340-3a933559305a@suse.cz

Fixes: 0a0337e0d1d1 ("mm, oom: rework oom detection")
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 50 ++------------------------------------------------
 1 file changed, 2 insertions(+), 48 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 8b3e1341b754..6e354199151b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3254,53 +3254,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	return NULL;
 }
 
-static inline bool
-should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
-		     enum compact_result compact_result, enum migrate_mode *migrate_mode,
-		     int compaction_retries)
-{
-	int max_retries = MAX_COMPACT_RETRIES;
-
-	if (!order)
-		return false;
-
-	/*
-	 * compaction considers all the zone as desperately out of memory
-	 * so it doesn't really make much sense to retry except when the
-	 * failure could be caused by weak migration mode.
-	 */
-	if (compaction_failed(compact_result)) {
-		if (*migrate_mode == MIGRATE_ASYNC) {
-			*migrate_mode = MIGRATE_SYNC_LIGHT;
-			return true;
-		}
-		return false;
-	}
-
-	/*
-	 * make sure the compaction wasn't deferred or didn't bail out early
-	 * due to locks contention before we declare that we should give up.
-	 * But do not retry if the given zonelist is not suitable for
-	 * compaction.
-	 */
-	if (compaction_withdrawn(compact_result))
-		return compaction_zonelist_suitable(ac, order, alloc_flags);
-
-	/*
-	 * !costly requests are much more important than __GFP_REPEAT
-	 * costly ones because they are de facto nofail and invoke OOM
-	 * killer to move on while costly can fail and users are ready
-	 * to cope with that. 1/4 retries is rather arbitrary but we
-	 * would need much more detailed feedback from compaction to
-	 * make a better decision.
-	 */
-	if (order > PAGE_ALLOC_COSTLY_ORDER)
-		max_retries /= 4;
-	if (compaction_retries <= max_retries)
-		return true;
-
-	return false;
-}
 #else
 static inline struct page *
 __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
@@ -3311,6 +3264,8 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	return NULL;
 }
 
+#endif /* CONFIG_COMPACTION */
+
 static inline bool
 should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
 		     enum compact_result compact_result,
@@ -3337,7 +3292,6 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
 	}
 	return false;
 }
-#endif /* CONFIG_COMPACTION */
 
 /* Perform direct synchronous page reclaim */
 static int
-- 
2.8.1

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1467533

On 2016.08.22 at 11:32 +0200, Michal Hocko wrote:
> there have been multiple reports [1][2][3][4][5] about pre-mature OOM
> killer invocations since 4.7 which contains oom detection rework. All of
> them were for order-2 (kernel stack) alloaction requests failing because
> of a high fragmentation and compaction failing to make any forward
> progress. While investigating this we have found out that the compaction
> just gives up too early. Vlastimil has been working on compaction
> improvement for quite some time and his series [6] is already sitting
> in mmotm tree. This already helps a lot because it drops some heuristics
> which are more aimed at lower latencies for high orders rather than
> reliability. Joonsoo has then identified further problem with too many
> blocks being marked as unmovable [7] and Vlastimil has prepared a patch
> on top of his series [8] which is also in the mmotm tree now.
> 
> That being said, the regression is real and should be fixed for 4.7
> stable users. [6][8] was reported to help and ooms are no longer
> reproducible. I know we are quite late (rc3) in 4.8 but I would vote
> for mergeing those patches and have them in 4.8. For 4.7 I would go
> with a partial revert of the detection rework for high order requests
> (see patch below). This patch is really trivial. If those compaction
> improvements are just too large for 4.8 then we can use the same patch
> as for 4.7 stable for now and revert it in 4.9 after compaction changes
> are merged.
> 
> Thoughts?
> 
> [1] http://lkml.kernel.org/r/20160731051121.GB307@x4

For the report [1] above:

markus@x4 linux % cat .config | grep CONFIG_COMPACTION
# CONFIG_COMPACTION is not set

-- 
Markus

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

#1467548

On Mon 22-08-16 12:16:14, Markus Trippelsdorf wrote:
> On 2016.08.22 at 11:32 +0200, Michal Hocko wrote:
> > there have been multiple reports [1][2][3][4][5] about pre-mature OOM
> > killer invocations since 4.7 which contains oom detection rework. All of
> > them were for order-2 (kernel stack) alloaction requests failing because
> > of a high fragmentation and compaction failing to make any forward
> > progress. While investigating this we have found out that the compaction
> > just gives up too early. Vlastimil has been working on compaction
> > improvement for quite some time and his series [6] is already sitting
> > in mmotm tree. This already helps a lot because it drops some heuristics
> > which are more aimed at lower latencies for high orders rather than
> > reliability. Joonsoo has then identified further problem with too many
> > blocks being marked as unmovable [7] and Vlastimil has prepared a patch
> > on top of his series [8] which is also in the mmotm tree now.
> > 
> > That being said, the regression is real and should be fixed for 4.7
> > stable users. [6][8] was reported to help and ooms are no longer
> > reproducible. I know we are quite late (rc3) in 4.8 but I would vote
> > for mergeing those patches and have them in 4.8. For 4.7 I would go
> > with a partial revert of the detection rework for high order requests
> > (see patch below). This patch is really trivial. If those compaction
> > improvements are just too large for 4.8 then we can use the same patch
> > as for 4.7 stable for now and revert it in 4.9 after compaction changes
> > are merged.
> > 
> > Thoughts?
> > 
> > [1] http://lkml.kernel.org/r/20160731051121.GB307@x4
> 
> For the report [1] above:
> 
> markus@x4 linux % cat .config | grep CONFIG_COMPACTION
> # CONFIG_COMPACTION is not set

Hmm, without compaction and a heavy fragmentation then I am afraid we
cannot really do much. What is the reason to disable compaction in the
first place?
-- 
Michal Hocko
SUSE Labs

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

#1467552

On 2016.08.22 at 12:56 +0200, Michal Hocko wrote:
> On Mon 22-08-16 12:16:14, Markus Trippelsdorf wrote:
> > On 2016.08.22 at 11:32 +0200, Michal Hocko wrote:
> > > [1] http://lkml.kernel.org/r/20160731051121.GB307@x4
> > 
> > For the report [1] above:
> > 
> > markus@x4 linux % cat .config | grep CONFIG_COMPACTION
> > # CONFIG_COMPACTION is not set
> 
> Hmm, without compaction and a heavy fragmentation then I am afraid we
> cannot really do much. What is the reason to disable compaction in the
> first place?

I don't recall. Must have been some issue in the past. I will re-enable
the option.

-- 
Markus

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

#1467553

On Mon 22-08-16 13:01:13, Markus Trippelsdorf wrote:
> On 2016.08.22 at 12:56 +0200, Michal Hocko wrote:
> > On Mon 22-08-16 12:16:14, Markus Trippelsdorf wrote:
> > > On 2016.08.22 at 11:32 +0200, Michal Hocko wrote:
> > > > [1] http://lkml.kernel.org/r/20160731051121.GB307@x4
> > > 
> > > For the report [1] above:
> > > 
> > > markus@x4 linux % cat .config | grep CONFIG_COMPACTION
> > > # CONFIG_COMPACTION is not set
> > 
> > Hmm, without compaction and a heavy fragmentation then I am afraid we
> > cannot really do much. What is the reason to disable compaction in the
> > first place?
> 
> I don't recall. Must have been some issue in the past. I will re-enable
> the option.

Well, without the compaction there is no source of high order pages at
all. You can only reclaim and hope that some of the reclaimed pages will
find its buddy on the list and form the higher order page. This can take
for ever. We used to have the lumpy reclaim and that could help but this
is long gone.

I do not think we can really sanely optimize for high-order heavy loads
without COMPACTION sanely. At least not without reintroducing lumpy
reclaim or something similar. To be honest I am even not sure which
configurations should disable compaction - except for really highly
controlled !mmu or other one purpose systems.

-- 
Michal Hocko
SUSE Labs

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

#1467557

On 2016.08.22 at 13:13 +0200, Michal Hocko wrote:
> On Mon 22-08-16 13:01:13, Markus Trippelsdorf wrote:
> > On 2016.08.22 at 12:56 +0200, Michal Hocko wrote:
> > > On Mon 22-08-16 12:16:14, Markus Trippelsdorf wrote:
> > > > On 2016.08.22 at 11:32 +0200, Michal Hocko wrote:
> > > > > [1] http://lkml.kernel.org/r/20160731051121.GB307@x4
> > > > 
> > > > For the report [1] above:
> > > > 
> > > > markus@x4 linux % cat .config | grep CONFIG_COMPACTION
> > > > # CONFIG_COMPACTION is not set
> > > 
> > > Hmm, without compaction and a heavy fragmentation then I am afraid we
> > > cannot really do much. What is the reason to disable compaction in the
> > > first place?
> > 
> > I don't recall. Must have been some issue in the past. I will re-enable
> > the option.
> 
> Well, without the compaction there is no source of high order pages at
> all. You can only reclaim and hope that some of the reclaimed pages will
> find its buddy on the list and form the higher order page. This can take
> for ever. We used to have the lumpy reclaim and that could help but this
> is long gone.
> 
> I do not think we can really sanely optimize for high-order heavy loads
> without COMPACTION sanely. At least not without reintroducing lumpy
> reclaim or something similar. To be honest I am even not sure which
> configurations should disable compaction - except for really highly
> controlled !mmu or other one purpose systems.

I now recall. It was an issue with CONFIG_TRANSPARENT_HUGEPAGE, so I
disabled that option. This then de-selected CONFIG_COMPACTION...

-- 
Markus

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

#1468277

On Mon, Aug 22, 2016 at 11:32:49AM +0200, Michal Hocko wrote:
> Hi, 
> there have been multiple reports [1][2][3][4][5] about pre-mature OOM
> killer invocations since 4.7 which contains oom detection rework. All of
> them were for order-2 (kernel stack) alloaction requests failing because
> of a high fragmentation and compaction failing to make any forward
> progress. While investigating this we have found out that the compaction
> just gives up too early. Vlastimil has been working on compaction
> improvement for quite some time and his series [6] is already sitting
> in mmotm tree. This already helps a lot because it drops some heuristics
> which are more aimed at lower latencies for high orders rather than
> reliability. Joonsoo has then identified further problem with too many
> blocks being marked as unmovable [7] and Vlastimil has prepared a patch
> on top of his series [8] which is also in the mmotm tree now.
> 
> That being said, the regression is real and should be fixed for 4.7
> stable users. [6][8] was reported to help and ooms are no longer
> reproducible. I know we are quite late (rc3) in 4.8 but I would vote
> for mergeing those patches and have them in 4.8. For 4.7 I would go
> with a partial revert of the detection rework for high order requests
> (see patch below). This patch is really trivial. If those compaction
> improvements are just too large for 4.8 then we can use the same patch
> as for 4.7 stable for now and revert it in 4.9 after compaction changes
> are merged.
> 
> Thoughts?
> 
> [1] http://lkml.kernel.org/r/20160731051121.GB307@x4
> [2] http://lkml.kernel.org/r/201608120901.41463.a.miskiewicz@gmail.com
> [3] http://lkml.kernel.org/r/20160801192620.GD31957@dhcp22.suse.cz
> [4] https://lists.opensuse.org/opensuse-kernel/2016-08/msg00021.html
> [5] https://bugzilla.opensuse.org/show_bug.cgi?id=994066
> [6] http://lkml.kernel.org/r/20160810091226.6709-1-vbabka@suse.cz
> [7] http://lkml.kernel.org/r/20160816031222.GC16913@js1304-P5Q-DELUXE
> [8] http://lkml.kernel.org/r/f7a9ea9d-bb88-bfd6-e340-3a933559305a@suse.cz
> 
> ---
> >From 899b738538de41295839dca2090a774bdd17acd2 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 22 Aug 2016 10:52:06 +0200
> Subject: [PATCH] mm, oom: prevent pre-mature OOM killer invocation for high
>  order request
> 
> There have been several reports about pre-mature OOM killer invocation
> in 4.7 kernel when order-2 allocation request (for the kernel stack)
> invoked OOM killer even during basic workloads (light IO or even kernel
> compile on some filesystems). In all reported cases the memory is
> fragmented and there are no order-2+ pages available. There is usually
> a large amount of slab memory (usually dentries/inodes) and further
> debugging has shown that there are way too many unmovable blocks which
> are skipped during the compaction. Multiple reporters have confirmed that
> the current linux-next which includes [1] and [2] helped and OOMs are
> not reproducible anymore. A simpler fix for the stable is to simply
> ignore the compaction feedback and retry as long as there is a reclaim
> progress for high order requests which we used to do before. We already
> do that for CONFING_COMPACTION=n so let's reuse the same code when
> compaction is enabled as well.

Hello, Michal.

I agree with partial revert but revert should be a different form.
Below change try to reuse should_compact_retry() version for
!CONFIG_COMPACTION but it turned out that it also causes regression in
Markus report [1].

Theoretical reason for this regression is that it would stop retry
even if there are enough lru pages. It only checks if freepage
excesses min watermark or not for retry decision. To prevent
pre-mature OOM killer, we need to keep allocation loop when there are
enough lru pages. So, logic should be something like that.

should_compact_retry()
{
        for_each_zone_zonelist_nodemask {
                available = zone_reclaimable_pages(zone);
                available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
                if (__zone_watermark_ok(zone, *0*, min_wmark_pages(zone),
                        ac_classzone_idx(ac), alloc_flags, available))
                        return true;

        }
}

I suggested it before and current situation looks like it is indeed
needed.

And, I still think that your OOM detection rework has some flaws.

1) It doesn't consider freeable objects that can be freed by shrink_slab().
There are many subsystems that cache many objects and they will be
freed by shrink_slab() interface. But, you don't account them when
making the OOM decision.

Think about following situation that we are trying to find order-2
freepage and some subsystem has order-2 freepage. It can be freed by
shrink_slab(). Your logic doesn't guarantee that shrink_slab() is
invoked to free this order-2 freepage in that subsystem. OOM would be
triggered when compaction fails even if there is a order-2 freeable
page. I think that if decision is made before whole lru list is
scanned and then shrink_slab() is invoked for whole freeable objects,
it would cause pre-mature OOM.

It seems that you already knows this issue [2].

2) 'OOM detection rework' depends on compaction too much. Compaction
algorithm is racy and has some limitation. It's failure doesn't mean we
are in OOM situation. Even if Vlastimil's patchset and mine is
applied, it is still possible that compaction scanner cannot find enough
freepage due to race condition and return pre-mature failure. To
reduce this race effect, I hope to give more chances to retry even if
full compaction is failed. We can remove this heuristic when we make
sure that compaction is stable enough.

As you know, I said these things several times but isn't accepted.
Please consider them more deeply at this time.

Thanks.

[1] http://lkml.kernel.org/r/20160731051121.GB307@x4
[2] https://bugzilla.opensuse.org/show_bug.cgi?id=994066


> 
> [1] http://lkml.kernel.org/r/20160810091226.6709-1-vbabka@suse.cz
> [2] http://lkml.kernel.org/r/f7a9ea9d-bb88-bfd6-e340-3a933559305a@suse.cz
> 
> Fixes: 0a0337e0d1d1 ("mm, oom: rework oom detection")
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/page_alloc.c | 50 ++------------------------------------------------
>  1 file changed, 2 insertions(+), 48 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 8b3e1341b754..6e354199151b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3254,53 +3254,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  	return NULL;
>  }
>  
> -static inline bool
> -should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> -		     enum compact_result compact_result, enum migrate_mode *migrate_mode,
> -		     int compaction_retries)
> -{
> -	int max_retries = MAX_COMPACT_RETRIES;
> -
> -	if (!order)
> -		return false;
> -
> -	/*
> -	 * compaction considers all the zone as desperately out of memory
> -	 * so it doesn't really make much sense to retry except when the
> -	 * failure could be caused by weak migration mode.
> -	 */
> -	if (compaction_failed(compact_result)) {
> -		if (*migrate_mode == MIGRATE_ASYNC) {
> -			*migrate_mode = MIGRATE_SYNC_LIGHT;
> -			return true;
> -		}
> -		return false;
> -	}
> -
> -	/*
> -	 * make sure the compaction wasn't deferred or didn't bail out early
> -	 * due to locks contention before we declare that we should give up.
> -	 * But do not retry if the given zonelist is not suitable for
> -	 * compaction.
> -	 */
> -	if (compaction_withdrawn(compact_result))
> -		return compaction_zonelist_suitable(ac, order, alloc_flags);
> -
> -	/*
> -	 * !costly requests are much more important than __GFP_REPEAT
> -	 * costly ones because they are de facto nofail and invoke OOM
> -	 * killer to move on while costly can fail and users are ready
> -	 * to cope with that. 1/4 retries is rather arbitrary but we
> -	 * would need much more detailed feedback from compaction to
> -	 * make a better decision.
> -	 */
> -	if (order > PAGE_ALLOC_COSTLY_ORDER)
> -		max_retries /= 4;
> -	if (compaction_retries <= max_retries)
> -		return true;
> -
> -	return false;
> -}
>  #else
>  static inline struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> @@ -3311,6 +3264,8 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  	return NULL;
>  }
>  
> +#endif /* CONFIG_COMPACTION */
> +
>  static inline bool
>  should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
>  		     enum compact_result compact_result,
> @@ -3337,7 +3292,6 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
>  	}
>  	return false;
>  }
> -#endif /* CONFIG_COMPACTION */
>  
>  /* Perform direct synchronous page reclaim */
>  static int
> -- 
> 2.8.1
> 
> -- 
> Michal Hocko
> SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

#1468345

On Tue 23-08-16 13:52:45, Joonsoo Kim wrote:
[...]
> Hello, Michal.
> 
> I agree with partial revert but revert should be a different form.
> Below change try to reuse should_compact_retry() version for
> !CONFIG_COMPACTION but it turned out that it also causes regression in
> Markus report [1].

I would argue that CONFIG_COMPACTION=n behaves so arbitrary for high
order workloads that calling any change in that behavior a regression
is little bit exaggerated. Disabling compaction should have a very
strong reason. I haven't heard any so far. I am even wondering whether
there is a legitimate reason for that these days.

> Theoretical reason for this regression is that it would stop retry
> even if there are enough lru pages. It only checks if freepage
> excesses min watermark or not for retry decision. To prevent
> pre-mature OOM killer, we need to keep allocation loop when there are
> enough lru pages. So, logic should be something like that.
> 
> should_compact_retry()
> {
>         for_each_zone_zonelist_nodemask {
>                 available = zone_reclaimable_pages(zone);
>                 available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
>                 if (__zone_watermark_ok(zone, *0*, min_wmark_pages(zone),
>                         ac_classzone_idx(ac), alloc_flags, available))
>                         return true;
> 
>         }
> }
> 
> I suggested it before and current situation looks like it is indeed
> needed.

this just opens doors for an unbounded reclaim/threshing becacause
you can reclaim as much as you like and there is no guarantee of a
forward progress. The reason why !COMPACTION should_compact_retry only
checks for the min_wmark without the reclaimable bias is that this will
guarantee a retry if we are failing due to high order wmark check rather
than a lack of memory. This condition is guaranteed to converge and the
probability of the unbounded reclaim is much more reduced.

> And, I still think that your OOM detection rework has some flaws.
>
> 1) It doesn't consider freeable objects that can be freed by shrink_slab().
> There are many subsystems that cache many objects and they will be
> freed by shrink_slab() interface. But, you don't account them when
> making the OOM decision.

I fully rely on the reclaim and compaction feedback. And that is the
place where we should strive for improvements. So if we are growing way
too many slab objects we should take care about that in the slab reclaim
which is tightly coupled with the LRU reclaim rather than up the layer
in the page allocator.
 
> Think about following situation that we are trying to find order-2
> freepage and some subsystem has order-2 freepage. It can be freed by
> shrink_slab(). Your logic doesn't guarantee that shrink_slab() is
> invoked to free this order-2 freepage in that subsystem. OOM would be
> triggered when compaction fails even if there is a order-2 freeable
> page. I think that if decision is made before whole lru list is
> scanned and then shrink_slab() is invoked for whole freeable objects,
> it would cause pre-mature OOM.

I do not see why we would need to scan through the whole LRU list when
we are under a high order pressure. It is true, though, that slab
shrinkers can and should be more sensitive to the requested order to
help release higher order pages preferably.

> It seems that you already knows this issue [2].
> 
> 2) 'OOM detection rework' depends on compaction too much. Compaction
> algorithm is racy and has some limitation. It's failure doesn't mean we
> are in OOM situation.

As long as this is the only reliable source of higher order pages then
we do not have any other choice in order to have deterministic behavior.

> Even if Vlastimil's patchset and mine is
> applied, it is still possible that compaction scanner cannot find enough
> freepage due to race condition and return pre-mature failure. To
> reduce this race effect, I hope to give more chances to retry even if
> full compaction is failed.

Than we can improve compaction_failed() heuristic and do not call it the
end of the day after a single attempt to get a high order page after
scanning the whole memory. But to me this all sounds like an internal
implementation detail of the compaction and the OOM detection in the
page allocator should be as much independent on it as possible - same as
it is independent on the internal reclaim decisions. That was the whole
point of my rework. To actually melt "do something as long as at least a
single page is reclaimed" into an actual algorithm which can be measured
and reason about.

> We can remove this heuristic when we make sure that compaction is
> stable enough.

How do we know that, though, if we do not rely on it? Artificial tests
do not exhibit those corner cases. I was bashing my testing systems to
cause as much fragmentation as possible, yet I wasn't able to trigger
issues reported recently by real world workloads. Do not take me wrong,
I understand your concerns but OOM detection will never be perfect. We
can easily get to one or other extremes. We should strive to make it
work in most workloads. So far it seems that there were no regressions
for order-0 pressure and we can improve compaction to cover higher
orders. I am willing to reconsider this after we hit a cliff where we
cannot do much more in the compaction proper and still hit pre-mature
oom killer invocations in not-so-insane workloads, though.

I believe that Vlastimil's patches show the path to go longterm. Get rid
of the latency heuristics for allocations where that matters in the
first step. Then try to squeeze as much for reliability for !costly
orders as possible.

I also believe that these issues will be less of the problem once we
switch to vmalloc stacks because this is the primary source of high
order allocations these days. Most others are more an optimization than
a reliability thing.

> As you know, I said these things several times but isn't accepted.
> Please consider them more deeply at this time.
> 
> Thanks.
> 
> [1] http://lkml.kernel.org/r/20160731051121.GB307@x4
> [2] https://bugzilla.opensuse.org/show_bug.cgi?id=994066
-- 
Michal Hocko
SUSE Labs

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

#1468354

On Tue 23-08-16 09:40:14, Markus Trippelsdorf wrote:
> On 2016.08.23 at 09:33 +0200, Michal Hocko wrote:
> > On Tue 23-08-16 13:52:45, Joonsoo Kim wrote:
> > [...]
> > > Hello, Michal.
> > > 
> > > I agree with partial revert but revert should be a different form.
> > > Below change try to reuse should_compact_retry() version for
> > > !CONFIG_COMPACTION but it turned out that it also causes regression in
> > > Markus report [1].
> > 
> > I would argue that CONFIG_COMPACTION=n behaves so arbitrary for high
> > order workloads that calling any change in that behavior a regression
> > is little bit exaggerated. Disabling compaction should have a very
> > strong reason. I haven't heard any so far. I am even wondering whether
> > there is a legitimate reason for that these days.
> 
> BTW, the current config description:
> 
>   CONFIG_COMPACTION:
>   Allows the compaction of memory for the allocation of huge pages. 
> 
> doesn't make it clear to the user that this is an essential feature.

Yes I plan to send a clarification patch.
-- 
Michal Hocko
SUSE Labs

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

#1468358

On 2016.08.23 at 09:33 +0200, Michal Hocko wrote:
> On Tue 23-08-16 13:52:45, Joonsoo Kim wrote:
> [...]
> > Hello, Michal.
> > 
> > I agree with partial revert but revert should be a different form.
> > Below change try to reuse should_compact_retry() version for
> > !CONFIG_COMPACTION but it turned out that it also causes regression in
> > Markus report [1].
> 
> I would argue that CONFIG_COMPACTION=n behaves so arbitrary for high
> order workloads that calling any change in that behavior a regression
> is little bit exaggerated. Disabling compaction should have a very
> strong reason. I haven't heard any so far. I am even wondering whether
> there is a legitimate reason for that these days.

BTW, the current config description:

  CONFIG_COMPACTION:
  Allows the compaction of memory for the allocation of huge pages. 

doesn't make it clear to the user that this is an essential feature.

-- 
Markus

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

#1468787

On Tue, Aug 23, 2016 at 3:33 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> I would argue that CONFIG_COMPACTION=n behaves so arbitrary for high
> order workloads that calling any change in that behavior a regression
> is little bit exaggerated.

Well, the thread info allocations certainly haven't been big problems
before. So regressing those would seem to be a real regression.

What happened? We've done the order-2 allocation for the stack since
May 2014, so that isn't new. Did we cut off retries for low orders?

So I would not say that it's an exaggeration to say that order-2
allocations failing is a regression.

Yes, yes, for 4.9 we may well end up using vmalloc for the kernel
stack, but there are certainly other things that want low-order
(non-hugepage) allocations. Like kmalloc(), which often ends up using
small orders just to pack data more efficiently (allocating a single
page can be hugely wasteful even if the individual allocations are
smaller than that - so allocating a few pages and packing more
allocations into it helps fight internal fragmentation)

So this definitely needs to be fixed for 4.7 (and apparently there's a
few patches still pending even for 4.8)

                 Linus

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

#1469105

On Tue 23-08-16 15:08:05, Linus Torvalds wrote:
> On Tue, Aug 23, 2016 at 3:33 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > I would argue that CONFIG_COMPACTION=n behaves so arbitrary for high
> > order workloads that calling any change in that behavior a regression
> > is little bit exaggerated.
> 
> Well, the thread info allocations certainly haven't been big problems
> before. So regressing those would seem to be a real regression.
> 
> What happened? We've done the order-2 allocation for the stack since
> May 2014, so that isn't new. Did we cut off retries for low orders?

Yes, with the original implementation the number of reclaim retries is
basically unbounded and as long as we have a reclaim progress. This has
changed to be a bounded process. Without the compaction this means that
we were reclaim as long as an order-2 page was formed.

> So I would not say that it's an exaggeration to say that order-2
> allocations failing is a regression.

I would agree with you with COMPACTION enabled but with compaction
disabled which should be really limited to !MMU configurations I think
there is not much we can do. Well, we could simply retry for ever
without invoking OOM killer for higher order request for this config
option and rely on order-0 to hit the OOM. Do we want that though?
I do not remember anybody with !MMU to complain. Markus had COMPACTION
disabled accidentally.
-- 
Michal Hocko
SUSE Labs

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

#1469122

Looks like my mail client eat my reply so I resend.

On Tue, Aug 23, 2016 at 09:33:18AM +0200, Michal Hocko wrote:
> On Tue 23-08-16 13:52:45, Joonsoo Kim wrote:
> [...]
> > Hello, Michal.
> > 
> > I agree with partial revert but revert should be a different form.
> > Below change try to reuse should_compact_retry() version for
> > !CONFIG_COMPACTION but it turned out that it also causes regression in
> > Markus report [1].
> 
> I would argue that CONFIG_COMPACTION=n behaves so arbitrary for high
> order workloads that calling any change in that behavior a regression
> is little bit exaggerated. Disabling compaction should have a very
> strong reason. I haven't heard any so far. I am even wondering whether
> there is a legitimate reason for that these days.
> 
> > Theoretical reason for this regression is that it would stop retry
> > even if there are enough lru pages. It only checks if freepage
> > excesses min watermark or not for retry decision. To prevent
> > pre-mature OOM killer, we need to keep allocation loop when there are
> > enough lru pages. So, logic should be something like that.
> > 
> > should_compact_retry()
> > {
> >         for_each_zone_zonelist_nodemask {
> >                 available = zone_reclaimable_pages(zone);
> >                 available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> >                 if (__zone_watermark_ok(zone, *0*, min_wmark_pages(zone),
> >                         ac_classzone_idx(ac), alloc_flags, available))
> >                         return true;
> > 
> >         }
> > }
> > 
> > I suggested it before and current situation looks like it is indeed
> > needed.
> 
> this just opens doors for an unbounded reclaim/threshing becacause
> you can reclaim as much as you like and there is no guarantee of a
> forward progress. The reason why !COMPACTION should_compact_retry only
> checks for the min_wmark without the reclaimable bias is that this will
> guarantee a retry if we are failing due to high order wmark check rather
> than a lack of memory. This condition is guaranteed to converge and the
> probability of the unbounded reclaim is much more reduced.

In case of a lack of memory with a lot of reclaimable lru pages, why 
do we stop reclaim/compaction?

With your partial reverting patch, allocation logic would be like as
following.

Assume following situation:
o a lot of reclaimable lru pages
o no order-2 freepage
o not enough order-0 freepage for min watermark
o order-2 allocation

1. order-2 allocation failed due to min watermark
2. go to reclaim/compaction
3. reclaim some pages (maybe SWAP_CLUSTER_MAX (32) pages) but still
min watermark isn't met for order-0
4. compaction is skipped due to not enough freepage
5. should_reclaim_retry() returns false because min watermark for
order-2 page isn't met
6. should_compact_retry() returns false because min watermark for
order-0 page isn't met
6. allocation is failed without any retry and OOM is invoked.

Is it what you want?

And, please elaborate more on how your logic guarantee to converge.
After order-0 freepage exceed min watermark, there is no way to stop
reclaim/threshing. Number of freepage just increase monotonically and
retry cannot be stopped until order-2 allocation succeed. Am I missing
something?


> > And, I still think that your OOM detection rework has some flaws.
> >
> > 1) It doesn't consider freeable objects that can be freed by shrink_slab().
> > There are many subsystems that cache many objects and they will be
> > freed by shrink_slab() interface. But, you don't account them when
> > making the OOM decision.
> 
> I fully rely on the reclaim and compaction feedback. And that is the
> place where we should strive for improvements. So if we are growing way
> too many slab objects we should take care about that in the slab reclaim
> which is tightly coupled with the LRU reclaim rather than up the layer
> in the page allocator.

No. slab shrink logic which is tightly coupled with the LRU reclaim
totally makes sense. What doesn't makes sense is the way of using
these functionality and utilizing these freebacks on your OOM
detection rework.

For example, compaction will do it's best with current resource. But,
as I said before, compaction will be more powerful if the system has
more free memory. Your logic just guarantee to give it to minimum
amount of free memory to run so I don't think it's result is
reliable to determine if we are in OOM or not.

And, your logic doesn't consider how many pages can be freed by slab
shrink. As I said before, there would exist high order reclaimable
page or we can make high order freepage by actual free.

Most importantly, I think that it is fundamentally impossible to
anticipate if we can make high order freepage or not by snapshot of
information about number of freeable page. So, your logic rely on
compaction but there are many types of pages that cannot be migrated
by compaction but can be reclaimed. So, fully relying on compaction
result for OOM decision would cause the problem.

I know that there is a trade-off. But, your logic makes me worry that
we lose too much accuracy for deterministic behaviour.

>  
> > Think about following situation that we are trying to find order-2
> > freepage and some subsystem has order-2 freepage. It can be freed by
> > shrink_slab(). Your logic doesn't guarantee that shrink_slab() is
> > invoked to free this order-2 freepage in that subsystem. OOM would be
> > triggered when compaction fails even if there is a order-2 freeable
> > page. I think that if decision is made before whole lru list is
> > scanned and then shrink_slab() is invoked for whole freeable objects,
> > it would cause pre-mature OOM.
> 
> I do not see why we would need to scan through the whole LRU list when
> we are under a high order pressure. It is true, though, that slab
> shrinkers can and should be more sensitive to the requested order to
> help release higher order pages preferably.
> 
> > It seems that you already knows this issue [2].
> > 
> > 2) 'OOM detection rework' depends on compaction too much. Compaction
> > algorithm is racy and has some limitation. It's failure doesn't mean we
> > are in OOM situation.
> 
> As long as this is the only reliable source of higher order pages then
> we do not have any other choice in order to have deterministic behavior.
> 
> > Even if Vlastimil's patchset and mine is
> > applied, it is still possible that compaction scanner cannot find enough
> > freepage due to race condition and return pre-mature failure. To
> > reduce this race effect, I hope to give more chances to retry even if
> > full compaction is failed.
> 
> Than we can improve compaction_failed() heuristic and do not call it the
> end of the day after a single attempt to get a high order page after
> scanning the whole memory. But to me this all sounds like an internal
> implementation detail of the compaction and the OOM detection in the
> page allocator should be as much independent on it as possible - same as
> it is independent on the internal reclaim decisions. That was the whole
> point of my rework. To actually melt "do something as long as at least a
> single page is reclaimed" into an actual algorithm which can be measured
> and reason about.

As you said before, your logic cannot be independent of these
feedbacks.

 "I fully rely on the reclaim and compaction feedback"

Your logic need to consider implementation details.

> 
> > We can remove this heuristic when we make sure that compaction is
> > stable enough.
> 
> How do we know that, though, if we do not rely on it? Artificial tests
> do not exhibit those corner cases. I was bashing my testing systems to
> cause as much fragmentation as possible, yet I wasn't able to trigger
> issues reported recently by real world workloads. Do not take me wrong,
> I understand your concerns but OOM detection will never be perfect. We
> can easily get to one or other extremes. We should strive to make it
> work in most workloads. So far it seems that there were no regressions
> for order-0 pressure and we can improve compaction to cover higher
> orders. I am willing to reconsider this after we hit a cliff where we

As I said before, I fully agree that your work will work well for
order-0 pressure.

> cannot do much more in the compaction proper and still hit pre-mature
> oom killer invocations in not-so-insane workloads, though.

If you understand my concners, it would be better to prevent the known
possible problem in advance? You cannot know whole real workload in
the world. Your logic has some limitations at least theoretically and
cause a lot of regressions already. Why do you continue to insist
"let's see other report by real workload?" Bug report can be reported
long time later and it would be not appropriate time to fix the issue.

> 
> I believe that Vlastimil's patches show the path to go longterm. Get rid
> of the latency heuristics for allocations where that matters in the
> first step. Then try to squeeze as much for reliability for !costly
> orders as possible.
> 
> I also believe that these issues will be less of the problem once we
> switch to vmalloc stacks because this is the primary source of high
> order allocations these days. Most others are more an optimization than
> a reliability thing.

Even if vmalloc stacks patches are applied, there are other
core cases. For example, ARM uses order-2 allocation for page table
allocation(pgd). It is just one allocation per process rather per
thread so less aggressive but it caused the problem before in our system.

Thanks.

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

#1469124

On Wed 24-08-16 14:01:57, Joonsoo Kim wrote:
> Looks like my mail client eat my reply so I resend.
> 
> On Tue, Aug 23, 2016 at 09:33:18AM +0200, Michal Hocko wrote:
> > On Tue 23-08-16 13:52:45, Joonsoo Kim wrote:
> > [...]
> > > Hello, Michal.
> > > 
> > > I agree with partial revert but revert should be a different form.
> > > Below change try to reuse should_compact_retry() version for
> > > !CONFIG_COMPACTION but it turned out that it also causes regression in
> > > Markus report [1].
> > 
> > I would argue that CONFIG_COMPACTION=n behaves so arbitrary for high
> > order workloads that calling any change in that behavior a regression
> > is little bit exaggerated. Disabling compaction should have a very
> > strong reason. I haven't heard any so far. I am even wondering whether
> > there is a legitimate reason for that these days.
> > 
> > > Theoretical reason for this regression is that it would stop retry
> > > even if there are enough lru pages. It only checks if freepage
> > > excesses min watermark or not for retry decision. To prevent
> > > pre-mature OOM killer, we need to keep allocation loop when there are
> > > enough lru pages. So, logic should be something like that.
> > > 
> > > should_compact_retry()
> > > {
> > >         for_each_zone_zonelist_nodemask {
> > >                 available = zone_reclaimable_pages(zone);
> > >                 available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> > >                 if (__zone_watermark_ok(zone, *0*, min_wmark_pages(zone),
> > >                         ac_classzone_idx(ac), alloc_flags, available))
> > >                         return true;
> > > 
> > >         }
> > > }
> > > 
> > > I suggested it before and current situation looks like it is indeed
> > > needed.
> > 
> > this just opens doors for an unbounded reclaim/threshing becacause
> > you can reclaim as much as you like and there is no guarantee of a
> > forward progress. The reason why !COMPACTION should_compact_retry only
> > checks for the min_wmark without the reclaimable bias is that this will
> > guarantee a retry if we are failing due to high order wmark check rather
> > than a lack of memory. This condition is guaranteed to converge and the
> > probability of the unbounded reclaim is much more reduced.
> 
> In case of a lack of memory with a lot of reclaimable lru pages, why 
> do we stop reclaim/compaction?
> 
> With your partial reverting patch, allocation logic would be like as
> following.
> 
> Assume following situation:
> o a lot of reclaimable lru pages
> o no order-2 freepage
> o not enough order-0 freepage for min watermark
> o order-2 allocation
> 
> 1. order-2 allocation failed due to min watermark
> 2. go to reclaim/compaction
> 3. reclaim some pages (maybe SWAP_CLUSTER_MAX (32) pages) but still
> min watermark isn't met for order-0
> 4. compaction is skipped due to not enough freepage
> 5. should_reclaim_retry() returns false because min watermark for
> order-2 page isn't met
> 6. should_compact_retry() returns false because min watermark for
> order-0 page isn't met
> 6. allocation is failed without any retry and OOM is invoked.

If the direct reclaim is not able to get us over min wmark for order-0
then we would be likely to hit the oom even for order-0 requests.

> Is it what you want?
> 
> And, please elaborate more on how your logic guarantee to converge.
> After order-0 freepage exceed min watermark, there is no way to stop
> reclaim/threshing. Number of freepage just increase monotonically and
> retry cannot be stopped until order-2 allocation succeed. Am I missing
> something?

My statement was imprecise at best. You are right that there is no
guarantee to fullfil order-2 request. What I meant to say is that we
should converge when we are getting out of memory (aka even order-0
would have hard time to succeed). should_reclaim_retry does that by
the back off scaling of the reclaimable pages. should_compact_retry
would have to do the same thing which would effectively turn it into
should_reclaim_retry.

> > > And, I still think that your OOM detection rework has some flaws.
> > >
> > > 1) It doesn't consider freeable objects that can be freed by shrink_slab().
> > > There are many subsystems that cache many objects and they will be
> > > freed by shrink_slab() interface. But, you don't account them when
> > > making the OOM decision.
> > 
> > I fully rely on the reclaim and compaction feedback. And that is the
> > place where we should strive for improvements. So if we are growing way
> > too many slab objects we should take care about that in the slab reclaim
> > which is tightly coupled with the LRU reclaim rather than up the layer
> > in the page allocator.
> 
> No. slab shrink logic which is tightly coupled with the LRU reclaim
> totally makes sense.

Once the number of slab object is much larger than LRU pages (what we
have seen in some oom reports) then the way how they are coupled just
stops making a sense because the current approach no longer scales.  We
might not have cared before because we used to retry blindly.  At least
that is my understanding.

I am sorry to skip large parts of your email but I believe those things
have been discussed and we would just repeat here. I full understand
there are some disagreements between our views but I still maintain that
as long as we can handle not-so-crazy workloads I prefer determinism
over a blind retrying. It is to be expected that there will be some
regressions. It would be just too ideal to change one heuristic by
another and expect nobody will notice. But as long as we are able to fix
those issues without adding hacks on top of hacks then I think it is
worth pursuing this path. And so far the compaction changes which helped
to cover recent regressions are not hacks but rather long term way how
to change it from best effort to reliability behavior. As I've said
before, if this proves to be insufficient then I will definitely not
insist on the current approach and replace the compaction feedback by
something else. I do not have much idea by what because, yet again, this
is a heuristic and there is clearly not right thing to do (tm).
-- 
Michal Hocko
SUSE Labs

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

#1469130

2016-08-24 16:04 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 24-08-16 14:01:57, Joonsoo Kim wrote:
>> Looks like my mail client eat my reply so I resend.
>>
>> On Tue, Aug 23, 2016 at 09:33:18AM +0200, Michal Hocko wrote:
>> > On Tue 23-08-16 13:52:45, Joonsoo Kim wrote:
>> > [...]
>> > > Hello, Michal.
>> > >
>> > > I agree with partial revert but revert should be a different form.
>> > > Below change try to reuse should_compact_retry() version for
>> > > !CONFIG_COMPACTION but it turned out that it also causes regression in
>> > > Markus report [1].
>> >
>> > I would argue that CONFIG_COMPACTION=n behaves so arbitrary for high
>> > order workloads that calling any change in that behavior a regression
>> > is little bit exaggerated. Disabling compaction should have a very
>> > strong reason. I haven't heard any so far. I am even wondering whether
>> > there is a legitimate reason for that these days.
>> >
>> > > Theoretical reason for this regression is that it would stop retry
>> > > even if there are enough lru pages. It only checks if freepage
>> > > excesses min watermark or not for retry decision. To prevent
>> > > pre-mature OOM killer, we need to keep allocation loop when there are
>> > > enough lru pages. So, logic should be something like that.
>> > >
>> > > should_compact_retry()
>> > > {
>> > >         for_each_zone_zonelist_nodemask {
>> > >                 available = zone_reclaimable_pages(zone);
>> > >                 available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
>> > >                 if (__zone_watermark_ok(zone, *0*, min_wmark_pages(zone),
>> > >                         ac_classzone_idx(ac), alloc_flags, available))
>> > >                         return true;
>> > >
>> > >         }
>> > > }
>> > >
>> > > I suggested it before and current situation looks like it is indeed
>> > > needed.
>> >
>> > this just opens doors for an unbounded reclaim/threshing becacause
>> > you can reclaim as much as you like and there is no guarantee of a
>> > forward progress. The reason why !COMPACTION should_compact_retry only
>> > checks for the min_wmark without the reclaimable bias is that this will
>> > guarantee a retry if we are failing due to high order wmark check rather
>> > than a lack of memory. This condition is guaranteed to converge and the
>> > probability of the unbounded reclaim is much more reduced.
>>
>> In case of a lack of memory with a lot of reclaimable lru pages, why
>> do we stop reclaim/compaction?
>>
>> With your partial reverting patch, allocation logic would be like as
>> following.
>>
>> Assume following situation:
>> o a lot of reclaimable lru pages
>> o no order-2 freepage
>> o not enough order-0 freepage for min watermark
>> o order-2 allocation
>>
>> 1. order-2 allocation failed due to min watermark
>> 2. go to reclaim/compaction
>> 3. reclaim some pages (maybe SWAP_CLUSTER_MAX (32) pages) but still
>> min watermark isn't met for order-0
>> 4. compaction is skipped due to not enough freepage
>> 5. should_reclaim_retry() returns false because min watermark for
>> order-2 page isn't met
>> 6. should_compact_retry() returns false because min watermark for
>> order-0 page isn't met
>> 6. allocation is failed without any retry and OOM is invoked.
>
> If the direct reclaim is not able to get us over min wmark for order-0
> then we would be likely to hit the oom even for order-0 requests.

No, this situation is that direct reclaim can get us over min wmark for order-0
but it needs retry. IIUC, direct reclaim would not reclaim enough memory
at once. It tries to reclaim small amount of lru pages and break out to check
watermark.

>> Is it what you want?
>>
>> And, please elaborate more on how your logic guarantee to converge.
>> After order-0 freepage exceed min watermark, there is no way to stop
>> reclaim/threshing. Number of freepage just increase monotonically and
>> retry cannot be stopped until order-2 allocation succeed. Am I missing
>> something?
>
> My statement was imprecise at best. You are right that there is no
> guarantee to fullfil order-2 request. What I meant to say is that we
> should converge when we are getting out of memory (aka even order-0
> would have hard time to succeed). should_reclaim_retry does that by
> the back off scaling of the reclaimable pages. should_compact_retry
> would have to do the same thing which would effectively turn it into
> should_reclaim_retry.

So, I suggested to change should_reclaim_retry() for high order request,
before.

>> > > And, I still think that your OOM detection rework has some flaws.
>> > >
>> > > 1) It doesn't consider freeable objects that can be freed by shrink_slab().
>> > > There are many subsystems that cache many objects and they will be
>> > > freed by shrink_slab() interface. But, you don't account them when
>> > > making the OOM decision.
>> >
>> > I fully rely on the reclaim and compaction feedback. And that is the
>> > place where we should strive for improvements. So if we are growing way
>> > too many slab objects we should take care about that in the slab reclaim
>> > which is tightly coupled with the LRU reclaim rather than up the layer
>> > in the page allocator.
>>
>> No. slab shrink logic which is tightly coupled with the LRU reclaim
>> totally makes sense.
>
> Once the number of slab object is much larger than LRU pages (what we
> have seen in some oom reports) then the way how they are coupled just
> stops making a sense because the current approach no longer scales.  We
> might not have cared before because we used to retry blindly.  At least
> that is my understanding.

If your logic guarantee to retry until number of lru pages are scanned,
it would work well. It's not a problem of slab shrink.

> I am sorry to skip large parts of your email but I believe those things
> have been discussed and we would just repeat here. I full understand

Okay. We discussed it several times and I'm also tired to discuss this topic.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web