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

Groups > linux.kernel > #1393956 > unrolled thread

Re: [PATCH 0.14] oom detection rework v6

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 0.14] oom detection rework v6 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 07:50 +0200
    Re: [PATCH 0.14] oom detection rework v6 Vlastimil Babka <vbabka@suse.cz> - 2016-05-04 10:20 +0200
      Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 10:40 +0200
      Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-04 11:00 +0200
    Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-04 10:50 +0200
      Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <js1304@gmail.com> - 2016-05-04 16:40 +0200
        Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-04 20:20 +0200
          Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <js1304@gmail.com> - 2016-05-10 08:50 +0200
            Re: [PATCH 0.14] oom detection rework v6 Vlastimil Babka <vbabka@suse.cz> - 2016-05-10 09:10 +0200
              Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <js1304@gmail.com> - 2016-05-10 10:10 +0200
                Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-10 11:50 +0200
            Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-10 11:50 +0200
              Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-12 04:30 +0200
                Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-12 07:20 +0200
                Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-12 13:10 +0200

#1393956 — Re: [PATCH 0.14] oom detection rework v6

On Wed, Apr 20, 2016 at 03:47:13PM -0400, Michal Hocko wrote:
> Hi,
> 
> This is v6 of the series. The previous version was posted [1]. The
> code hasn't changed much since then. I have found one old standing
> bug (patch 1) which just got much more severe and visible with this
> series. Other than that I have reorganized the series and put the
> compaction feedback abstraction to the front just in case we find out
> that parts of the series would have to be reverted later on for some
> reason. The premature oom killer invocation reported by Hugh [2] seems
> to be addressed.
> 
> We have discussed this series at LSF/MM summit in Raleigh and there
> didn't seem to be any concerns/objections to go on with the patch set
> and target it for the next merge window. 

I still don't agree with some part of this patchset that deal with
!costly order. As you know, there was two regression reports from Hugh
and Aaron and you fixed them by ensuring to trigger compaction. I
think that these show the problem of this patchset. Previous kernel
doesn't need to ensure to trigger compaction and just works fine in
any case. Your series make compaction necessary for all. OOM handling
is essential part in MM but compaction isn't. OOM handling should not
depend on compaction. I tested my own benchmark without
CONFIG_COMPACTION and found that premature OOM happens.

I hope that you try to test something without CONFIG_COMPACTION.

Thanks.

> 
> Motivation:
> As pointed by Linus [3][4] relying on zone_reclaimable as a way to
> communicate the reclaim progress is rater dubious. I tend to agree,
> not only it is really obscure, it is not hard to imagine cases where a
> single page freed in the loop keeps all the reclaimers looping without
> getting any progress because their gfp_mask wouldn't allow to get that
> page anyway (e.g. single GFP_ATOMIC alloc and free loop). This is rather
> rare so it doesn't happen in the practice but the current logic which we
> have is rather obscure and hard to follow a also non-deterministic.
> 
> This is an attempt to make the OOM detection more deterministic and
> easier to follow because each reclaimer basically tracks its own
> progress which is implemented at the page allocator layer rather spread
> out between the allocator and the reclaim. The more on the implementation
> is described in the first patch.
> 
> I have tested several different scenarios but it should be clear that
> testing OOM killer is quite hard to be representative. There is usually
> a tiny gap between almost OOM and full blown OOM which is often time
> sensitive. Anyway, I have tested the following 2 scenarios and I would
> appreciate if there are more to test.
> 
> Testing environment: a virtual machine with 2G of RAM and 2CPUs without
> any swap to make the OOM more deterministic.
> 
> 1) 2 writers (each doing dd with 4M blocks to an xfs partition with 1G
>    file size, removes the files and starts over again) running in
>    parallel for 10s to build up a lot of dirty pages when 100 parallel
>    mem_eaters (anon private populated mmap which waits until it gets
>    signal) with 80M each.
> 
>    This causes an OOM flood of course and I have compared both patched
>    and unpatched kernels. The test is considered finished after there
>    are no OOM conditions detected. This should tell us whether there are
>    any excessive kills or some of them premature (e.g. due to dirty pages):
> 
> I have performed two runs this time each after a fresh boot.
> 
> * base kernel
> $ grep "Out of memory:" base-oom-run1.log | wc -l
> 78
> $ grep "Out of memory:" base-oom-run2.log | wc -l
> 78
> 
> $ grep "Kill process" base-oom-run1.log | tail -n1
> [   91.391203] Out of memory: Kill process 3061 (mem_eater) score 39 or sacrifice child
> $ grep "Kill process" base-oom-run2.log | tail -n1
> [   82.141919] Out of memory: Kill process 3086 (mem_eater) score 39 or sacrifice child
> 
> $ grep "DMA32 free:" base-oom-run1.log | sed 's@.*free:\([0-9]*\)kB.*@\1@' | calc_min_max.awk 
> min: 5376.00 max: 6776.00 avg: 5530.75 std: 166.50 nr: 61
> $ grep "DMA32 free:" base-oom-run2.log | sed 's@.*free:\([0-9]*\)kB.*@\1@' | calc_min_max.awk 
> min: 5416.00 max: 5608.00 avg: 5514.15 std: 42.94 nr: 52
> 
> $ grep "DMA32.*all_unreclaimable? no" base-oom-run1.log | wc -l
> 1
> $ grep "DMA32.*all_unreclaimable? no" base-oom-run2.log | wc -l
> 3
> 
> * patched kernel
> $ grep "Out of memory:" patched-oom-run1.log | wc -l
> 78
> miso@tiehlicka /mnt/share/devel/miso/kvm $ grep "Out of memory:" patched-oom-run2.log | wc -l
> 77
> 
> e grep "Kill process" patched-oom-run1.log | tail -n1
> [  497.317732] Out of memory: Kill process 3108 (mem_eater) score 39 or sacrifice child
> $ grep "Kill process" patched-oom-run2.log | tail -n1
> [  316.169920] Out of memory: Kill process 3093 (mem_eater) score 39 or sacrifice child
> 
> $ grep "DMA32 free:" patched-oom-run1.log | sed 's@.*free:\([0-9]*\)kB.*@\1@' | calc_min_max.awk 
> min: 5420.00 max: 5808.00 avg: 5513.90 std: 60.45 nr: 78
> $ grep "DMA32 free:" patched-oom-run2.log | sed 's@.*free:\([0-9]*\)kB.*@\1@' | calc_min_max.awk 
> min: 5380.00 max: 6384.00 avg: 5520.94 std: 136.84 nr: 77
> 
> e grep "DMA32.*all_unreclaimable? no" patched-oom-run1.log | wc -l
> 2
> $ grep "DMA32.*all_unreclaimable? no" patched-oom-run2.log | wc -l
> 3
> 
> The patched kernel run noticeably longer while invoking OOM killer same
> number of times. This means that the original implementation is much
> more aggressive and triggers the OOM killer sooner. free pages stats
> show that neither kernels went OOM too early most of the time, though. I
> guess the difference is in the backoff when retries without any progress
> do sleep for a while if there is memory under writeback or dirty which
> is highly likely considering the parallel IO.
> Both kernels have seen races where zone wasn't marked unreclaimable
> and we still hit the OOM killer. This is most likely a race where
> a task managed to exit between the last allocation attempt and the oom
> killer invocation.
> 
> 2) 2 writers again with 10s of run and then 10 mem_eaters to consume as much
>    memory as possible without triggering the OOM killer. This required a lot
>    of tuning but I've considered 3 consecutive runs in three different boots
>    without OOM as a success.
> 
> * base kernel
> size=$(awk '/MemFree/{printf "%dK", ($2/10)-(16*1024)}' /proc/meminfo)
> 
> * patched kernel
> size=$(awk '/MemFree/{printf "%dK", ($2/10)-(12*1024)}' /proc/meminfo)
> 
> That means 40M more memory was usable without triggering OOM killer. The
> base kernel sometimes managed to handle the same as patched but it
> wasn't consistent and failed in at least on of the 3 runs. This seems
> like a minor improvement.
> 
> I was testing also GPF_REPEAT costly requests (hughetlb) with fragmented
> memory and under memory pressure. The results are in patch 11 where the
> logic is implemented. In short I can see huge improvement there.
> 
> I am certainly interested in other usecases as well as well as any
> feedback. Especially those which require higher order requests.
> 
> * Changes since v5
> - added "vmscan: consider classzone_idx in compaction_ready"
> - added "mm, oom, compaction: prevent from should_compact_retry looping
>   for ever for costly orders"
> - acked-bys from Vlastimil
> - integrated feedback from review
> * Changes since v4
> - dropped __GFP_REPEAT for costly allocation as it is now replaced by
>   the compaction based feedback logic
> - !costly high order requests are retried based on the compaction feedback
> - compaction feedback has been tweaked to give us an useful information
>   to make decisions in the page allocator
> - rebased on the current mmotm-2016-04-01-16-24 with the previous version
>   of the rework reverted
> 
> * Changes since v3
> - factor out the new heuristic into its own function as suggested by
>   Johannes (no functional changes)
> 
> * Changes since v2
> - rebased on top of mmotm-2015-11-25-17-08 which includes
>   wait_iff_congested related changes which needed refresh in
>   patch#1 and patch#2
> - use zone_page_state_snapshot for NR_FREE_PAGES per David
> - shrink_zones doesn't need to return anything per David
> - retested because the major kernel version has changed since
>   the last time (4.2 -> 4.3 based kernel + mmotm patches)
> 
> * Changes since v1
> - backoff calculation was de-obfuscated by using DIV_ROUND_UP
> - __GFP_NOFAIL high order migh fail fixed - theoretical bug
> 
> [1] http://lkml.kernel.org/r/1459855533-4600-1-git-send-email-mhocko@kernel.org
> [2] http://lkml.kernel.org/r/alpine.LSU.2.11.1602241832160.15564@eggly.anvils
> [3] http://lkml.kernel.org/r/CA+55aFwapaED7JV6zm-NVkP-jKie+eQ1vDXWrKD=SkbshZSgmw@mail.gmail.com
> [4] http://lkml.kernel.org/r/CA+55aFxwg=vS2nrXsQhAUzPQDGb8aQpZi0M7UUh21ftBo-z46Q@mail.gmail.com
> 
> --
> 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] | [next] | [standalone]

#1394077

On 05/04/2016 07:45 AM, Joonsoo Kim wrote:
> I still don't agree with some part of this patchset that deal with
> !costly order. As you know, there was two regression reports from Hugh
> and Aaron and you fixed them by ensuring to trigger compaction. I
> think that these show the problem of this patchset. Previous kernel
> doesn't need to ensure to trigger compaction and just works fine in
> any case.

IIRC previous kernel somehow subtly never OOM'd for !costly orders. So 
anything that introduces the possibility of OOM may look like regression 
for some corner case workloads. But I don't think that it's OK to not 
OOM for e.g. kernel stack allocations?

> Your series make compaction necessary for all. OOM handling
> is essential part in MM but compaction isn't. OOM handling should not
> depend on compaction. I tested my own benchmark without
> CONFIG_COMPACTION and found that premature OOM happens.
>
> I hope that you try to test something without CONFIG_COMPACTION.

Hmm a valid point, !CONFIG_COMPACTION should be considered. But reclaim 
cannot guarantee forming an order>0 page. But neither does OOM. So would 
you suggest we keep reclaiming without OOM as before, to prevent these 
regressions? Or where to draw the line here?

> Thanks.

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

#1394085

On Wed, May 04, 2016 at 10:12:43AM +0200, Vlastimil Babka wrote:
> On 05/04/2016 07:45 AM, Joonsoo Kim wrote:
> >I still don't agree with some part of this patchset that deal with
> >!costly order. As you know, there was two regression reports from Hugh
> >and Aaron and you fixed them by ensuring to trigger compaction. I
> >think that these show the problem of this patchset. Previous kernel
> >doesn't need to ensure to trigger compaction and just works fine in
> >any case.
> 
> IIRC previous kernel somehow subtly never OOM'd for !costly orders.

IIRC, it would not OOM in thrashing case. But, it could OOM in other
cases.

> So anything that introduces the possibility of OOM may look like
> regression for some corner case workloads. But I don't think that
> it's OK to not OOM for e.g. kernel stack allocations?

Sorry. Double negation makes me hard to understand since I'm not
native. So, you think that it's OK to OOM for kernel stack allocation?
I think so, too. But, I want not to OOM prematurely.

> >Your series make compaction necessary for all. OOM handling
> >is essential part in MM but compaction isn't. OOM handling should not
> >depend on compaction. I tested my own benchmark without
> >CONFIG_COMPACTION and found that premature OOM happens.
> >
> >I hope that you try to test something without CONFIG_COMPACTION.
> 
> Hmm a valid point, !CONFIG_COMPACTION should be considered. But
> reclaim cannot guarantee forming an order>0 page. But neither does
> OOM. So would you suggest we keep reclaiming without OOM as before,
> to prevent these regressions? Or where to draw the line here?

I suggested that memorizing number of reclaimable pages when entering
allocation slowpath and try to reclaim at least that amount. Thrashing
is effectively prevented in this algorithm and we don't trigger OOM
prematurely.

Thanks.

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

#1394098

On Wed 04-05-16 10:12:43, Vlastimil Babka wrote:
> On 05/04/2016 07:45 AM, Joonsoo Kim wrote:
> >I still don't agree with some part of this patchset that deal with
> >!costly order. As you know, there was two regression reports from Hugh
> >and Aaron and you fixed them by ensuring to trigger compaction. I
> >think that these show the problem of this patchset. Previous kernel
> >doesn't need to ensure to trigger compaction and just works fine in
> >any case.
> 
> IIRC previous kernel somehow subtly never OOM'd for !costly orders. So
> anything that introduces the possibility of OOM may look like regression for
> some corner case workloads.

The bug fixed by this series was COMPACTION specific because
compaction_ready is not considered otherwise.
-- 
Michal Hocko
SUSE Labs

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

#1394090

On Wed 04-05-16 14:45:02, Joonsoo Kim wrote:
> On Wed, Apr 20, 2016 at 03:47:13PM -0400, Michal Hocko wrote:
> > Hi,
> > 
> > This is v6 of the series. The previous version was posted [1]. The
> > code hasn't changed much since then. I have found one old standing
> > bug (patch 1) which just got much more severe and visible with this
> > series. Other than that I have reorganized the series and put the
> > compaction feedback abstraction to the front just in case we find out
> > that parts of the series would have to be reverted later on for some
> > reason. The premature oom killer invocation reported by Hugh [2] seems
> > to be addressed.
> > 
> > We have discussed this series at LSF/MM summit in Raleigh and there
> > didn't seem to be any concerns/objections to go on with the patch set
> > and target it for the next merge window. 
> 
> I still don't agree with some part of this patchset that deal with
> !costly order. As you know, there was two regression reports from Hugh
> and Aaron and you fixed them by ensuring to trigger compaction. I
> think that these show the problem of this patchset. Previous kernel
> doesn't need to ensure to trigger compaction and just works fine in
> any case. Your series make compaction necessary for all. OOM handling
> is essential part in MM but compaction isn't. OOM handling should not
> depend on compaction. I tested my own benchmark without
> CONFIG_COMPACTION and found that premature OOM happens.

High order allocations without compaction are basically a lost game. You
can wait unbounded amount of time and still have no guarantee of any
progress. What is the usual reason to disable compaction in the first
place?

Anyway if this is _really_ a big issue then we can do something like the
following to emulate the previous behavior. We are losing the
determinism but if you really thing that the !COMPACTION workloads
already reconcile with it I can live with that.
---
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2e7e26c5d3ba..f48b9e9b1869 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3319,6 +3319,24 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
 		     enum migrate_mode *migrate_mode,
 		     int compaction_retries)
 {
+	struct zone *zone;
+	struct zoneref *z;
+
+	if (order > PAGE_ALLOC_COSTLY_ORDER)
+		return false;
+
+	/*
+	 * There are setups with compaction disabled which would prefer to loop
+	 * inside the allocator rather than hit the oom killer prematurely. Let's
+	 * give them a good hope and keep retrying while the order-0 watermarks
+	 * are OK.
+	 */
+	for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
+					ac->nodemask) {
+		if(zone_watermark_ok(zone, 0, min_wmark_pages(zone),
+					ac->high_zoneidx, alloc_flags))
+			return true;
+	}
 	return false;
 }
 #endif /* CONFIG_COMPACTION */
-- 
Michal Hocko
SUSE Labs

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

#1394364

2016-05-04 17:47 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 04-05-16 14:45:02, Joonsoo Kim wrote:
>> On Wed, Apr 20, 2016 at 03:47:13PM -0400, Michal Hocko wrote:
>> > Hi,
>> >
>> > This is v6 of the series. The previous version was posted [1]. The
>> > code hasn't changed much since then. I have found one old standing
>> > bug (patch 1) which just got much more severe and visible with this
>> > series. Other than that I have reorganized the series and put the
>> > compaction feedback abstraction to the front just in case we find out
>> > that parts of the series would have to be reverted later on for some
>> > reason. The premature oom killer invocation reported by Hugh [2] seems
>> > to be addressed.
>> >
>> > We have discussed this series at LSF/MM summit in Raleigh and there
>> > didn't seem to be any concerns/objections to go on with the patch set
>> > and target it for the next merge window.
>>
>> I still don't agree with some part of this patchset that deal with
>> !costly order. As you know, there was two regression reports from Hugh
>> and Aaron and you fixed them by ensuring to trigger compaction. I
>> think that these show the problem of this patchset. Previous kernel
>> doesn't need to ensure to trigger compaction and just works fine in
>> any case. Your series make compaction necessary for all. OOM handling
>> is essential part in MM but compaction isn't. OOM handling should not
>> depend on compaction. I tested my own benchmark without
>> CONFIG_COMPACTION and found that premature OOM happens.
>
> High order allocations without compaction are basically a lost game. You

I don't think that order 1 or 2 allocation has a big trouble without compaction.
They can be made by buddy algorithm that keeps high order freepages
as long as possible.

> can wait unbounded amount of time and still have no guarantee of any

I know that it has no guarantee. But, it doesn't mean that it's better to
give up early. Since OOM could causes serious problem, if there is
reclaimable memory, we need to reclaim all of them at least once
with praying for high order page before triggering OOM. Optimizing
this situation by incomplete guessing is a dangerous idea.

> progress. What is the usual reason to disable compaction in the first
> place?

I don't disable it. But, who knows who disable compaction? It's been *not*
a long time that CONFIG_COMPACTION is default enable. Maybe, 3 years?

> Anyway if this is _really_ a big issue then we can do something like the
> following to emulate the previous behavior. We are losing the
> determinism but if you really thing that the !COMPACTION workloads
> already reconcile with it I can live with that.
> ---
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 2e7e26c5d3ba..f48b9e9b1869 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3319,6 +3319,24 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
>                      enum migrate_mode *migrate_mode,
>                      int compaction_retries)
>  {
> +       struct zone *zone;
> +       struct zoneref *z;
> +
> +       if (order > PAGE_ALLOC_COSTLY_ORDER)
> +               return false;
> +
> +       /*
> +        * There are setups with compaction disabled which would prefer to loop
> +        * inside the allocator rather than hit the oom killer prematurely. Let's
> +        * give them a good hope and keep retrying while the order-0 watermarks
> +        * are OK.
> +        */
> +       for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> +                                       ac->nodemask) {
> +               if(zone_watermark_ok(zone, 0, min_wmark_pages(zone),
> +                                       ac->high_zoneidx, alloc_flags))
> +                       return true;
> +       }
>         return false;

I hope that this kind of logic is added to should_reclaim_retry() so
that this logic is
applied in any setup. should_compact_retry() should not become a fundamental
criteria to determine OOM. What compaction does can be changed in the future
and it's undesirable that it's change affects OOM condition greatly.

Thanks.

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

#1394597

On Wed 04-05-16 23:32:31, Joonsoo Kim wrote:
> 2016-05-04 17:47 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Wed 04-05-16 14:45:02, Joonsoo Kim wrote:
> >> On Wed, Apr 20, 2016 at 03:47:13PM -0400, Michal Hocko wrote:
> >> > Hi,
> >> >
> >> > This is v6 of the series. The previous version was posted [1]. The
> >> > code hasn't changed much since then. I have found one old standing
> >> > bug (patch 1) which just got much more severe and visible with this
> >> > series. Other than that I have reorganized the series and put the
> >> > compaction feedback abstraction to the front just in case we find out
> >> > that parts of the series would have to be reverted later on for some
> >> > reason. The premature oom killer invocation reported by Hugh [2] seems
> >> > to be addressed.
> >> >
> >> > We have discussed this series at LSF/MM summit in Raleigh and there
> >> > didn't seem to be any concerns/objections to go on with the patch set
> >> > and target it for the next merge window.
> >>
> >> I still don't agree with some part of this patchset that deal with
> >> !costly order. As you know, there was two regression reports from Hugh
> >> and Aaron and you fixed them by ensuring to trigger compaction. I
> >> think that these show the problem of this patchset. Previous kernel
> >> doesn't need to ensure to trigger compaction and just works fine in
> >> any case. Your series make compaction necessary for all. OOM handling
> >> is essential part in MM but compaction isn't. OOM handling should not
> >> depend on compaction. I tested my own benchmark without
> >> CONFIG_COMPACTION and found that premature OOM happens.
> >
> > High order allocations without compaction are basically a lost game. You
> 
> I don't think that order 1 or 2 allocation has a big trouble without compaction.
> They can be made by buddy algorithm that keeps high order freepages
> as long as possible.
> 
> > can wait unbounded amount of time and still have no guarantee of any
> 
> I know that it has no guarantee. But, it doesn't mean that it's better to
> give up early. Since OOM could causes serious problem, if there is
> reclaimable memory, we need to reclaim all of them at least once
> with praying for high order page before triggering OOM. Optimizing
> this situation by incomplete guessing is a dangerous idea.
> 
> > progress. What is the usual reason to disable compaction in the first
> > place?
> 
> I don't disable it. But, who knows who disable compaction? It's been *not*
> a long time that CONFIG_COMPACTION is default enable. Maybe, 3 years?

I would really like to hear about real life usecase before we go and
cripple otherwise deterministic algorithms. It might be very well
possible that those configurations simply do not have problems with high
order allocations because they are too specific.

> > Anyway if this is _really_ a big issue then we can do something like the
> > following to emulate the previous behavior. We are losing the
> > determinism but if you really thing that the !COMPACTION workloads
> > already reconcile with it I can live with that.
> > ---
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 2e7e26c5d3ba..f48b9e9b1869 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3319,6 +3319,24 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
> >                      enum migrate_mode *migrate_mode,
> >                      int compaction_retries)
> >  {
> > +       struct zone *zone;
> > +       struct zoneref *z;
> > +
> > +       if (order > PAGE_ALLOC_COSTLY_ORDER)
> > +               return false;
> > +
> > +       /*
> > +        * There are setups with compaction disabled which would prefer to loop
> > +        * inside the allocator rather than hit the oom killer prematurely. Let's
> > +        * give them a good hope and keep retrying while the order-0 watermarks
> > +        * are OK.
> > +        */
> > +       for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> > +                                       ac->nodemask) {
> > +               if(zone_watermark_ok(zone, 0, min_wmark_pages(zone),
> > +                                       ac->high_zoneidx, alloc_flags))
> > +                       return true;
> > +       }
> >         return false;
> 
> I hope that this kind of logic is added to should_reclaim_retry() so
> that this logic is
> applied in any setup. should_compact_retry() should not become a fundamental
> criteria to determine OOM. What compaction does can be changed in the future
> and it's undesirable that it's change affects OOM condition greatly.

I disagree. High order allocations relying on the reclaim is a bad idea
because there is no guarantee that reclaiming more memory leads to the
success. This is the whole idea of the oom detection rework. So the
whole point of should_reclaim_retry is to get over watermarks while
should_compact_retry is about retrying when high order allocations might
make a progress. I really hate to tweak this for a configuration which
relies on the pure luck. So if we really need to do something
undeterministic then !COMPACTION should_compact_retry is the place where
it should be done.

If you are able to reproduce pre mature OOMs with !COMPACTION then I
would really appreciate if you could test with this patch so that I can
prepare a full patch.
-- 
Michal Hocko
SUSE Labs

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

#1397733

2016-05-05 3:16 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 04-05-16 23:32:31, Joonsoo Kim wrote:
>> 2016-05-04 17:47 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
>> > On Wed 04-05-16 14:45:02, Joonsoo Kim wrote:
>> >> On Wed, Apr 20, 2016 at 03:47:13PM -0400, Michal Hocko wrote:
>> >> > Hi,
>> >> >
>> >> > This is v6 of the series. The previous version was posted [1]. The
>> >> > code hasn't changed much since then. I have found one old standing
>> >> > bug (patch 1) which just got much more severe and visible with this
>> >> > series. Other than that I have reorganized the series and put the
>> >> > compaction feedback abstraction to the front just in case we find out
>> >> > that parts of the series would have to be reverted later on for some
>> >> > reason. The premature oom killer invocation reported by Hugh [2] seems
>> >> > to be addressed.
>> >> >
>> >> > We have discussed this series at LSF/MM summit in Raleigh and there
>> >> > didn't seem to be any concerns/objections to go on with the patch set
>> >> > and target it for the next merge window.
>> >>
>> >> I still don't agree with some part of this patchset that deal with
>> >> !costly order. As you know, there was two regression reports from Hugh
>> >> and Aaron and you fixed them by ensuring to trigger compaction. I
>> >> think that these show the problem of this patchset. Previous kernel
>> >> doesn't need to ensure to trigger compaction and just works fine in
>> >> any case. Your series make compaction necessary for all. OOM handling
>> >> is essential part in MM but compaction isn't. OOM handling should not
>> >> depend on compaction. I tested my own benchmark without
>> >> CONFIG_COMPACTION and found that premature OOM happens.
>> >
>> > High order allocations without compaction are basically a lost game. You
>>
>> I don't think that order 1 or 2 allocation has a big trouble without compaction.
>> They can be made by buddy algorithm that keeps high order freepages
>> as long as possible.
>>
>> > can wait unbounded amount of time and still have no guarantee of any
>>
>> I know that it has no guarantee. But, it doesn't mean that it's better to
>> give up early. Since OOM could causes serious problem, if there is
>> reclaimable memory, we need to reclaim all of them at least once
>> with praying for high order page before triggering OOM. Optimizing
>> this situation by incomplete guessing is a dangerous idea.
>>
>> > progress. What is the usual reason to disable compaction in the first
>> > place?
>>
>> I don't disable it. But, who knows who disable compaction? It's been *not*
>> a long time that CONFIG_COMPACTION is default enable. Maybe, 3 years?
>
> I would really like to hear about real life usecase before we go and
> cripple otherwise deterministic algorithms. It might be very well
> possible that those configurations simply do not have problems with high
> order allocations because they are too specific.
>
>> > Anyway if this is _really_ a big issue then we can do something like the
>> > following to emulate the previous behavior. We are losing the
>> > determinism but if you really thing that the !COMPACTION workloads
>> > already reconcile with it I can live with that.
>> > ---
>> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> > index 2e7e26c5d3ba..f48b9e9b1869 100644
>> > --- a/mm/page_alloc.c
>> > +++ b/mm/page_alloc.c
>> > @@ -3319,6 +3319,24 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
>> >                      enum migrate_mode *migrate_mode,
>> >                      int compaction_retries)
>> >  {
>> > +       struct zone *zone;
>> > +       struct zoneref *z;
>> > +
>> > +       if (order > PAGE_ALLOC_COSTLY_ORDER)
>> > +               return false;
>> > +
>> > +       /*
>> > +        * There are setups with compaction disabled which would prefer to loop
>> > +        * inside the allocator rather than hit the oom killer prematurely. Let's
>> > +        * give them a good hope and keep retrying while the order-0 watermarks
>> > +        * are OK.
>> > +        */
>> > +       for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
>> > +                                       ac->nodemask) {
>> > +               if(zone_watermark_ok(zone, 0, min_wmark_pages(zone),
>> > +                                       ac->high_zoneidx, alloc_flags))
>> > +                       return true;
>> > +       }
>> >         return false;
>>
>> I hope that this kind of logic is added to should_reclaim_retry() so
>> that this logic is
>> applied in any setup. should_compact_retry() should not become a fundamental
>> criteria to determine OOM. What compaction does can be changed in the future
>> and it's undesirable that it's change affects OOM condition greatly.
>
> I disagree. High order allocations relying on the reclaim is a bad idea
> because there is no guarantee that reclaiming more memory leads to the
> success. This is the whole idea of the oom detection rework. So the
> whole point of should_reclaim_retry is to get over watermarks while
> should_compact_retry is about retrying when high order allocations might
> make a progress. I really hate to tweak this for a configuration which
> relies on the pure luck. So if we really need to do something
> undeterministic then !COMPACTION should_compact_retry is the place where
> it should be done.
>
> If you are able to reproduce pre mature OOMs with !COMPACTION then I
> would really appreciate if you could test with this patch so that I can
> prepare a full patch.

My benchmark is too specific so I make another one. It does very
simple things.

1) Run the system with 256 MB memory and 2 GB swap
2) Run memory-hogger which takes (anonymous memory) 256 MB
3) Make 1000 new processes by fork (It will take 16 MB order-2 pages)

You can do it yourself with above instructions.

On current upstream kernel without CONFIG_COMPACTION, OOM doesn't happen.
On next-20160509 kernel without CONFIG_COMPACTION, OOM happens when
roughly *500* processes forked.

With CONFIG_COMPACTION, OOM doesn't happen on any kernel.

Other kernels doesn't trigger OOM even if I make 10000 new processes.

This example is very intuitive and reasonable. I think that it's not artificial.
It has enough swap space so OOM should not happen.

This failure shows that fundamental assumption of your patch is
wrong. You triggers OOM even if there is enough reclaimable memory but
no high order freepage depending on the fact that we can't guarantee
that we can make high order page with reclaiming these reclaimable
memory. Yes, we can't guarantee it but we also doesn't know if it
can be possible or not. We should not stop reclaim until this
estimation is is proved. Otherwise, it would be premature OOM.

You applied band-aid for CONFIG_COMPACTION and fixed some reported
problem but it is also fragile. Assume almost pageblock's skipbit are
set. In this case, compaction easily returns COMPACT_COMPLETE and your
logic will stop retry. Compaction isn't designed to report accurate
fragmentation state of the system so depending on it's return value
for OOM is fragile.

Please fix your fundamental assumption and don't add band-aid using
compaction.

I said same thing again and again and I can't convince you until now.
I'm not sure what I can do more.

Thanks.

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

#1397742

On 05/10/2016 08:41 AM, Joonsoo Kim wrote:
> You applied band-aid for CONFIG_COMPACTION and fixed some reported
> problem but it is also fragile. Assume almost pageblock's skipbit are
> set. In this case, compaction easily returns COMPACT_COMPLETE and your
> logic will stop retry. Compaction isn't designed to report accurate
> fragmentation state of the system so depending on it's return value
> for OOM is fragile.

Guess I'll just post a RFC now, even though it's not much tested...

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

#1397820

2016-05-10 16:09 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> On 05/10/2016 08:41 AM, Joonsoo Kim wrote:
>>
>> You applied band-aid for CONFIG_COMPACTION and fixed some reported
>> problem but it is also fragile. Assume almost pageblock's skipbit are
>> set. In this case, compaction easily returns COMPACT_COMPLETE and your
>> logic will stop retry. Compaction isn't designed to report accurate
>> fragmentation state of the system so depending on it's return value
>> for OOM is fragile.
>
>
> Guess I'll just post a RFC now, even though it's not much tested...

I will look at it later. But, I'd like to say something first.
Even if compaction returns more accurate fragmentation states, it's not a good
idea to depend on compaction's result to decide OOM. We have reclaimable but
not migratable pages. Depending on compaction's result cannot deal
with this case.

For example, please assume that all of the system memory are filled
with THP pages
or reclaimable slab pages. They cannot be migrated but we can reclaim them.

Thanks.

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

#1397909

On Tue 10-05-16 17:00:08, Joonsoo Kim wrote:
> 2016-05-10 16:09 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> > On 05/10/2016 08:41 AM, Joonsoo Kim wrote:
> >>
> >> You applied band-aid for CONFIG_COMPACTION and fixed some reported
> >> problem but it is also fragile. Assume almost pageblock's skipbit are
> >> set. In this case, compaction easily returns COMPACT_COMPLETE and your
> >> logic will stop retry. Compaction isn't designed to report accurate
> >> fragmentation state of the system so depending on it's return value
> >> for OOM is fragile.
> >
> >
> > Guess I'll just post a RFC now, even though it's not much tested...
> 
> I will look at it later. But, I'd like to say something first.
> Even if compaction returns more accurate fragmentation states, it's not a good
> idea to depend on compaction's result to decide OOM. We have reclaimable but
> not migratable pages. Depending on compaction's result cannot deal
> with this case.
> 
> For example, please assume that all of the system memory are filled
> with THP pages
> or reclaimable slab pages. They cannot be migrated but we can reclaim them.

Direct reclaim should break those THP pages or shrink those slabs. And
we make sure to reclaim before we consider final call for fail from
compaction feedback. If this is a vast majority of memory we should hit
it pretty reliably AFAICS.
-- 
Michal Hocko
SUSE Labs

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

#1397907

On Tue 10-05-16 15:41:04, Joonsoo Kim wrote:
> 2016-05-05 3:16 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Wed 04-05-16 23:32:31, Joonsoo Kim wrote:
> >> 2016-05-04 17:47 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
[...]
> >> > progress. What is the usual reason to disable compaction in the first
> >> > place?
> >>
> >> I don't disable it. But, who knows who disable compaction? It's been *not*
> >> a long time that CONFIG_COMPACTION is default enable. Maybe, 3 years?
> >
> > I would really like to hear about real life usecase before we go and
> > cripple otherwise deterministic algorithms. It might be very well
> > possible that those configurations simply do not have problems with high
> > order allocations because they are too specific.

Sorry for insisting but I would really like to hear some answer for
this, please.

[...]
> >> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >> > index 2e7e26c5d3ba..f48b9e9b1869 100644
> >> > --- a/mm/page_alloc.c
> >> > +++ b/mm/page_alloc.c
> >> > @@ -3319,6 +3319,24 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
> >> >                      enum migrate_mode *migrate_mode,
> >> >                      int compaction_retries)
> >> >  {
> >> > +       struct zone *zone;
> >> > +       struct zoneref *z;
> >> > +
> >> > +       if (order > PAGE_ALLOC_COSTLY_ORDER)
> >> > +               return false;
> >> > +
> >> > +       /*
> >> > +        * There are setups with compaction disabled which would prefer to loop
> >> > +        * inside the allocator rather than hit the oom killer prematurely. Let's
> >> > +        * give them a good hope and keep retrying while the order-0 watermarks
> >> > +        * are OK.
> >> > +        */
> >> > +       for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> >> > +                                       ac->nodemask) {
> >> > +               if(zone_watermark_ok(zone, 0, min_wmark_pages(zone),
> >> > +                                       ac->high_zoneidx, alloc_flags))
> >> > +                       return true;
> >> > +       }
> >> >         return false;
[...]
> My benchmark is too specific so I make another one. It does very
> simple things.
> 
> 1) Run the system with 256 MB memory and 2 GB swap
> 2) Run memory-hogger which takes (anonymous memory) 256 MB
> 3) Make 1000 new processes by fork (It will take 16 MB order-2 pages)
> 
> You can do it yourself with above instructions.
> 
> On current upstream kernel without CONFIG_COMPACTION, OOM doesn't happen.
> On next-20160509 kernel without CONFIG_COMPACTION, OOM happens when
> roughly *500* processes forked.
> 
> With CONFIG_COMPACTION, OOM doesn't happen on any kernel.

Does the patch I have posted helped?

> Other kernels doesn't trigger OOM even if I make 10000 new processes.

Is this an usual load on !CONFIG_COMPACTION configurations?

> This example is very intuitive and reasonable. I think that it's not
> artificial.  It has enough swap space so OOM should not happen.

I am not really convinced this is true actually. You can have an
arbitrary amount of the swap space yet it still won't help you
because more reclaimed memory simply doesn't imply a more continuous
memory. This is a fundamental problem. So I think that relying on
!CONFIG_COMPACTION for heavy fork (or other high order) loads simply
never works reliably.

> This failure shows that fundamental assumption of your patch is
> wrong. You triggers OOM even if there is enough reclaimable memory but
> no high order freepage depending on the fact that we can't guarantee
> that we can make high order page with reclaiming these reclaimable
> memory. Yes, we can't guarantee it but we also doesn't know if it
> can be possible or not. We should not stop reclaim until this
> estimation is is proved. Otherwise, it would be premature OOM.

We've been through this before and you keep repeating this argument. 
I have tried to explain that a deterministic behavior is more reasonable
than a random retry loops which pretty much depends on timing and which
can hugely over-reclaim which might be even worse than an OOM killer
invocation which would target a single process.

I do agree that relying solely on the compaction is not the right way
but combining the two (reclaim & compaction) should work reasonably well
in practice. The only regression I have heard so far resulted from the
lack of compaction feedback.

> You applied band-aid for CONFIG_COMPACTION and fixed some reported
> problem but it is also fragile. Assume almost pageblock's skipbit are
> set. In this case, compaction easily returns COMPACT_COMPLETE and your
> logic will stop retry. Compaction isn't designed to report accurate
> fragmentation state of the system so depending on it's return value
> for OOM is fragile.

Which is a deficiency of compaction. And the one which is worked on as
already said by Vlastimil. Even with that deficiency, I am not able
to trigger pre-mature OOM so it sounds more theoretical than a real
issue. I am convinced that deeper surgery into compaction is really due
as it has been mostly designed for THP case completely ignoring !costly
allocations.

> Please fix your fundamental assumption and don't add band-aid using
> compaction.

I do not consider compaction feedback design as a "band-aid". There is
no other reliable source of high order pages except for compaction.

> I said same thing again and again and I can't convince you until now.
> I'm not sure what I can do more.

Yes and yet I haven't seen any real life cases where this feedback
mechanism doesn't work from you. You keep claiming that more reclaiming
_might_ be useful without any grounds for that statement. Even when the
more reclaim would help to survive a particular case we have to weigh
pros and cons of the over reclaim and potential trashing which is worse
than an OOM killer sometimes (staring at your machine you can ping but
you cannot even log in...).

Considering that we are in a clear disagreement in the compaction aspect
I think we need others to either back your concern or you show a clear
justification why compaction feedback is not viable way longterm even
after we make further changes which would make it less THP oriented.
-- 
Michal Hocko
SUSE Labs

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

#1399648

On Tue, May 10, 2016 at 11:43:48AM +0200, Michal Hocko wrote:
> On Tue 10-05-16 15:41:04, Joonsoo Kim wrote:
> > 2016-05-05 3:16 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > > On Wed 04-05-16 23:32:31, Joonsoo Kim wrote:
> > >> 2016-05-04 17:47 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> [...]
> > >> > progress. What is the usual reason to disable compaction in the first
> > >> > place?
> > >>
> > >> I don't disable it. But, who knows who disable compaction? It's been *not*
> > >> a long time that CONFIG_COMPACTION is default enable. Maybe, 3 years?
> > >
> > > I would really like to hear about real life usecase before we go and
> > > cripple otherwise deterministic algorithms. It might be very well
> > > possible that those configurations simply do not have problems with high
> > > order allocations because they are too specific.
> 
> Sorry for insisting but I would really like to hear some answer for
> this, please.

I don't know. Who knows? How you can make sure that? And, I don't like
below fixup. Theoretically, it could retry forever.

> 
> [...]
> > >> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > >> > index 2e7e26c5d3ba..f48b9e9b1869 100644
> > >> > --- a/mm/page_alloc.c
> > >> > +++ b/mm/page_alloc.c
> > >> > @@ -3319,6 +3319,24 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
> > >> >                      enum migrate_mode *migrate_mode,
> > >> >                      int compaction_retries)
> > >> >  {
> > >> > +       struct zone *zone;
> > >> > +       struct zoneref *z;
> > >> > +
> > >> > +       if (order > PAGE_ALLOC_COSTLY_ORDER)
> > >> > +               return false;
> > >> > +
> > >> > +       /*
> > >> > +        * There are setups with compaction disabled which would prefer to loop
> > >> > +        * inside the allocator rather than hit the oom killer prematurely. Let's
> > >> > +        * give them a good hope and keep retrying while the order-0 watermarks
> > >> > +        * are OK.
> > >> > +        */
> > >> > +       for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> > >> > +                                       ac->nodemask) {
> > >> > +               if(zone_watermark_ok(zone, 0, min_wmark_pages(zone),
> > >> > +                                       ac->high_zoneidx, alloc_flags))
> > >> > +                       return true;
> > >> > +       }
> > >> >         return false;
> [...]
> > My benchmark is too specific so I make another one. It does very
> > simple things.
> > 
> > 1) Run the system with 256 MB memory and 2 GB swap
> > 2) Run memory-hogger which takes (anonymous memory) 256 MB
> > 3) Make 1000 new processes by fork (It will take 16 MB order-2 pages)
> > 
> > You can do it yourself with above instructions.
> > 
> > On current upstream kernel without CONFIG_COMPACTION, OOM doesn't happen.
> > On next-20160509 kernel without CONFIG_COMPACTION, OOM happens when
> > roughly *500* processes forked.
> > 
> > With CONFIG_COMPACTION, OOM doesn't happen on any kernel.
> 
> Does the patch I have posted helped?

I guess that it will help but please do it by yourself. It's simple.

> > Other kernels doesn't trigger OOM even if I make 10000 new processes.
> 
> Is this an usual load on !CONFIG_COMPACTION configurations?

I don't know. User-space developer doesn't take care about kernel
configuration and it seems that fork 500 times when memory is full is
not a corner case to me.

> > This example is very intuitive and reasonable. I think that it's not
> > artificial.  It has enough swap space so OOM should not happen.
> 
> I am not really convinced this is true actually. You can have an
> arbitrary amount of the swap space yet it still won't help you
> because more reclaimed memory simply doesn't imply a more continuous
> memory. This is a fundamental problem. So I think that relying on
> !CONFIG_COMPACTION for heavy fork (or other high order) loads simply
> never works reliably.

I think that you don't understand how powerful the reclaim and
compaction are. In the system with large disk swap, what compaction can do
is also possible for reclaim. Reclaim can do more.

Think about following examples.

_: free
U: used(unmovable)
M: used(migratable and reclaimable)

_MUU _U_U MMMM MMMM

With compaction (assume theoretically best algorithm),
just 3 contiguous region can be made like as following:

MMUU MUMU ___M MMMM

With reclaim, we can make 8 contiguous region.

__UU _U_U ____ ____

Reclaim can be easily affected by thrashing but it is fundamentally
more powerful than compaction.

Even, there are not migratable but reclaimable pages and it could weak
power of the compaction.

> > This failure shows that fundamental assumption of your patch is
> > wrong. You triggers OOM even if there is enough reclaimable memory but
> > no high order freepage depending on the fact that we can't guarantee
> > that we can make high order page with reclaiming these reclaimable
> > memory. Yes, we can't guarantee it but we also doesn't know if it
> > can be possible or not. We should not stop reclaim until this
> > estimation is is proved. Otherwise, it would be premature OOM.
> 
> We've been through this before and you keep repeating this argument. 
> I have tried to explain that a deterministic behavior is more reasonable
> than a random retry loops which pretty much depends on timing and which
> can hugely over-reclaim which might be even worse than an OOM killer
> invocation which would target a single process.

I didn't say that deterministic behavior is less reasonable. I like
it. What I insist is the your criteria for deterministic behavior
is wrong and please use another criteria for deterministic behavior.
That's what I want.

> I do agree that relying solely on the compaction is not the right way
> but combining the two (reclaim & compaction) should work reasonably well
> in practice. The only regression I have heard so far resulted from the
> lack of compaction feedback.

I agree that combining is needed. But, base criteria looks not
reasonable to me.

> > You applied band-aid for CONFIG_COMPACTION and fixed some reported
> > problem but it is also fragile. Assume almost pageblock's skipbit are
> > set. In this case, compaction easily returns COMPACT_COMPLETE and your
> > logic will stop retry. Compaction isn't designed to report accurate
> > fragmentation state of the system so depending on it's return value
> > for OOM is fragile.
> 
> Which is a deficiency of compaction. And the one which is worked on as
> already said by Vlastimil. Even with that deficiency, I am not able
> to trigger pre-mature OOM so it sounds more theoretical than a real
> issue. I am convinced that deeper surgery into compaction is really due
> as it has been mostly designed for THP case completely ignoring !costly
> allocations.
> 
> > Please fix your fundamental assumption and don't add band-aid using
> > compaction.
> 
> I do not consider compaction feedback design as a "band-aid". There is
> no other reliable source of high order pages except for compaction.
> 
> > I said same thing again and again and I can't convince you until now.
> > I'm not sure what I can do more.
> 
> Yes and yet I haven't seen any real life cases where this feedback
> mechanism doesn't work from you. You keep claiming that more reclaiming
> _might_ be useful without any grounds for that statement. Even when the
> more reclaim would help to survive a particular case we have to weigh
> pros and cons of the over reclaim and potential trashing which is worse
> than an OOM killer sometimes (staring at your machine you can ping but
> you cannot even log in...).

I didn't say that your OOM rework is totally wrong. I just said that
you should fix !costly order case since your criteria doesn't make
sense to this case.

> 
> Considering that we are in a clear disagreement in the compaction aspect
> I think we need others to either back your concern or you show a clear
> justification why compaction feedback is not viable way longterm even
> after we make further changes which would make it less THP oriented.

I can't understand why I need to convince you. Conventionally, patch
author needs to convince reviewer. Anyway, above exmaple would be helpful
to understand limitation of the compaction.

If explanation in this reply also would not convince you, I
won't insist more. Discussing more on this topic would not be
productive for us.

Thanks.

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

#1399670

On Thu, May 12, 2016 at 11:23:34AM +0900, Joonsoo Kim wrote:
> On Tue, May 10, 2016 at 11:43:48AM +0200, Michal Hocko wrote:
> > On Tue 10-05-16 15:41:04, Joonsoo Kim wrote:
> > > 2016-05-05 3:16 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > > > On Wed 04-05-16 23:32:31, Joonsoo Kim wrote:
> > > >> 2016-05-04 17:47 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > [...]
> > > >> > progress. What is the usual reason to disable compaction in the first
> > > >> > place?
> > > >>
> > > >> I don't disable it. But, who knows who disable compaction? It's been *not*
> > > >> a long time that CONFIG_COMPACTION is default enable. Maybe, 3 years?
> > > >
> > > > I would really like to hear about real life usecase before we go and
> > > > cripple otherwise deterministic algorithms. It might be very well
> > > > possible that those configurations simply do not have problems with high
> > > > order allocations because they are too specific.
> > 
> > Sorry for insisting but I would really like to hear some answer for
> > this, please.
> 
> I don't know. Who knows? How you can make sure that? And, I don't like
> below fixup. Theoretically, it could retry forever.
> 
> > 
> > [...]
> > > >> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > >> > index 2e7e26c5d3ba..f48b9e9b1869 100644
> > > >> > --- a/mm/page_alloc.c
> > > >> > +++ b/mm/page_alloc.c
> > > >> > @@ -3319,6 +3319,24 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
> > > >> >                      enum migrate_mode *migrate_mode,
> > > >> >                      int compaction_retries)
> > > >> >  {
> > > >> > +       struct zone *zone;
> > > >> > +       struct zoneref *z;
> > > >> > +
> > > >> > +       if (order > PAGE_ALLOC_COSTLY_ORDER)
> > > >> > +               return false;
> > > >> > +
> > > >> > +       /*
> > > >> > +        * There are setups with compaction disabled which would prefer to loop
> > > >> > +        * inside the allocator rather than hit the oom killer prematurely. Let's
> > > >> > +        * give them a good hope and keep retrying while the order-0 watermarks
> > > >> > +        * are OK.
> > > >> > +        */
> > > >> > +       for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> > > >> > +                                       ac->nodemask) {
> > > >> > +               if(zone_watermark_ok(zone, 0, min_wmark_pages(zone),
> > > >> > +                                       ac->high_zoneidx, alloc_flags))
> > > >> > +                       return true;
> > > >> > +       }
> > > >> >         return false;
> > [...]
> > > My benchmark is too specific so I make another one. It does very
> > > simple things.
> > > 
> > > 1) Run the system with 256 MB memory and 2 GB swap
> > > 2) Run memory-hogger which takes (anonymous memory) 256 MB
> > > 3) Make 1000 new processes by fork (It will take 16 MB order-2 pages)
> > > 
> > > You can do it yourself with above instructions.
> > > 
> > > On current upstream kernel without CONFIG_COMPACTION, OOM doesn't happen.
> > > On next-20160509 kernel without CONFIG_COMPACTION, OOM happens when
> > > roughly *500* processes forked.
> > > 
> > > With CONFIG_COMPACTION, OOM doesn't happen on any kernel.
> > 
> > Does the patch I have posted helped?
> 
> I guess that it will help but please do it by yourself. It's simple.
> 
> > > Other kernels doesn't trigger OOM even if I make 10000 new processes.
> > 
> > Is this an usual load on !CONFIG_COMPACTION configurations?
> 
> I don't know. User-space developer doesn't take care about kernel
> configuration and it seems that fork 500 times when memory is full is
> not a corner case to me.
> 
> > > This example is very intuitive and reasonable. I think that it's not
> > > artificial.  It has enough swap space so OOM should not happen.
> > 
> > I am not really convinced this is true actually. You can have an
> > arbitrary amount of the swap space yet it still won't help you
> > because more reclaimed memory simply doesn't imply a more continuous
> > memory. This is a fundamental problem. So I think that relying on
> > !CONFIG_COMPACTION for heavy fork (or other high order) loads simply
> > never works reliably.
> 
> I think that you don't understand how powerful the reclaim and
> compaction are. In the system with large disk swap, what compaction can do
> is also possible for reclaim. Reclaim can do more.
> 
> Think about following examples.
> 
> _: free
> U: used(unmovable)
> M: used(migratable and reclaimable)
> 
> _MUU _U_U MMMM MMMM
> 
> With compaction (assume theoretically best algorithm),
> just 3 contiguous region can be made like as following:
> 
> MMUU MUMU ___M MMMM
> 
> With reclaim, we can make 8 contiguous region.
> 
> __UU _U_U ____ ____
> 
> Reclaim can be easily affected by thrashing but it is fundamentally
> more powerful than compaction.

Hmm... I uses wrong example here because if there is enough freepage,
compaction using theoretically best algorithm can make enoguh
contiguous region. We ensure that by watermark check so it has no
problem like as above. Anyway, you can see that reclaim has at least
enough power to make high order page. That's what I'd like to express
by using this example.

> 
> Even, there are not migratable but reclaimable pages and it could weak
> power of the compaction.

This is still true and one of limitation of compaction.

Thanks.

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

#1399920

On Thu 12-05-16 11:23:34, Joonsoo Kim wrote:
> On Tue, May 10, 2016 at 11:43:48AM +0200, Michal Hocko wrote:
> > On Tue 10-05-16 15:41:04, Joonsoo Kim wrote:
> > > 2016-05-05 3:16 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > > > On Wed 04-05-16 23:32:31, Joonsoo Kim wrote:
> > > >> 2016-05-04 17:47 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > [...]
> > > >> > progress. What is the usual reason to disable compaction in the first
> > > >> > place?
> > > >>
> > > >> I don't disable it. But, who knows who disable compaction? It's been *not*
> > > >> a long time that CONFIG_COMPACTION is default enable. Maybe, 3 years?
> > > >
> > > > I would really like to hear about real life usecase before we go and
> > > > cripple otherwise deterministic algorithms. It might be very well
> > > > possible that those configurations simply do not have problems with high
> > > > order allocations because they are too specific.
> > 
> > Sorry for insisting but I would really like to hear some answer for
> > this, please.
> 
> I don't know. Who knows? How you can make sure that?

This is pretty much a corner case configuration. I would assume that
somebody who wants to save memory for such an important feature for high
order allocations would have a very specific workloads.

> And, I don't like below fixup. Theoretically, it could retry forever.

Sure it can retry forever if we are constantly over the watermark and the
reclaim makes progress. This is the primary thing I hate about the
current implementation and the follow up fix reintroduces that behavior
for !COMPACTION case. It will OOM as soon as there is no reclaim
progress or all available zones are not passing the watermark check so
there shouldn't be any regressions.

> > [...]
> > > >> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > >> > index 2e7e26c5d3ba..f48b9e9b1869 100644
> > > >> > --- a/mm/page_alloc.c
> > > >> > +++ b/mm/page_alloc.c
> > > >> > @@ -3319,6 +3319,24 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
> > > >> >                      enum migrate_mode *migrate_mode,
> > > >> >                      int compaction_retries)
> > > >> >  {
> > > >> > +       struct zone *zone;
> > > >> > +       struct zoneref *z;
> > > >> > +
> > > >> > +       if (order > PAGE_ALLOC_COSTLY_ORDER)
> > > >> > +               return false;
> > > >> > +
> > > >> > +       /*
> > > >> > +        * There are setups with compaction disabled which would prefer to loop
> > > >> > +        * inside the allocator rather than hit the oom killer prematurely. Let's
> > > >> > +        * give them a good hope and keep retrying while the order-0 watermarks
> > > >> > +        * are OK.
> > > >> > +        */
> > > >> > +       for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> > > >> > +                                       ac->nodemask) {
> > > >> > +               if(zone_watermark_ok(zone, 0, min_wmark_pages(zone),
> > > >> > +                                       ac->high_zoneidx, alloc_flags))
> > > >> > +                       return true;
> > > >> > +       }
> > > >> >         return false;
> > [...]
> > > My benchmark is too specific so I make another one. It does very
> > > simple things.
> > > 
> > > 1) Run the system with 256 MB memory and 2 GB swap
> > > 2) Run memory-hogger which takes (anonymous memory) 256 MB
> > > 3) Make 1000 new processes by fork (It will take 16 MB order-2 pages)
> > > 
> > > You can do it yourself with above instructions.
> > > 
> > > On current upstream kernel without CONFIG_COMPACTION, OOM doesn't happen.
> > > On next-20160509 kernel without CONFIG_COMPACTION, OOM happens when
> > > roughly *500* processes forked.
> > > 
> > > With CONFIG_COMPACTION, OOM doesn't happen on any kernel.
> > 
> > Does the patch I have posted helped?
> 
> I guess that it will help but please do it by yourself. It's simple.

Fair enough. I have prepared a similar setup (virtual machine with
2 CPUs, 256M RAM, 2G swap space, CONFIG_COMPACTION disabled and the
current mmotm tree). mem_eater does MAP_POPULATE 512MB of anon private
memory and then I start an aggressive fork test which is forking
short term children (which exit after a short <1s random timeout) as
quickly as possible and it makes sure there are always 1000 children
running. All this racing with the mem_eater. This was the test I was
originally using to test oom rework with COMPACTION enabled.

This triggered the OOM for order-2 allocation requests. With the patch
applied the test has survived.
             total       used       free     shared    buffers     cached
Mem:        232572     228748       3824          0       1164       2480
-/+ buffers/cache:     225104       7468
Swap:      2097148     348320    1748828
Node 0, zone      DMA
  pages free     282
        min      33
        low      41
--
Node 0, zone    DMA32
  pages free     610
        min      441
        low      551
nr_children:1000
^CCreated 11494416 children

I will post the patch shortly.

[...]

> I think that you don't understand how powerful the reclaim and
> compaction are. In the system with large disk swap, what compaction can do
> is also possible for reclaim. Reclaim can do more.
> 
> Think about following examples.
> 
> _: free
> U: used(unmovable)
> M: used(migratable and reclaimable)
> 
> _MUU _U_U MMMM MMMM
> 
> With compaction (assume theoretically best algorithm),
> just 3 contiguous region can be made like as following:
> 
> MMUU MUMU ___M MMMM
> 
> With reclaim, we can make 8 contiguous region.
> 
> __UU _U_U ____ ____
> 
> Reclaim can be easily affected by thrashing but it is fundamentally
> more powerful than compaction.

OK, it seems I was ambiguous in my previous statements, sorry about
that. Of course that reclaiming all (or large portion of) the memory
will free up more high order slots. But this is quite unreasonable
behavior to get few !costly blocks of memory because it affects most
processes. There should be some balance there.

> Even, there are not migratable but reclaimable pages and it could weak
> power of the compaction.
> 
> > > This failure shows that fundamental assumption of your patch is
> > > wrong. You triggers OOM even if there is enough reclaimable memory but
> > > no high order freepage depending on the fact that we can't guarantee
> > > that we can make high order page with reclaiming these reclaimable
> > > memory. Yes, we can't guarantee it but we also doesn't know if it
> > > can be possible or not. We should not stop reclaim until this
> > > estimation is is proved. Otherwise, it would be premature OOM.
> > 
> > We've been through this before and you keep repeating this argument. 
> > I have tried to explain that a deterministic behavior is more reasonable
> > than a random retry loops which pretty much depends on timing and which
> > can hugely over-reclaim which might be even worse than an OOM killer
> > invocation which would target a single process.
> 
> I didn't say that deterministic behavior is less reasonable. I like
> it. What I insist is the your criteria for deterministic behavior
> is wrong and please use another criteria for deterministic behavior.
> That's what I want.

I have structured my current criteria to be as independent on both
reclaim and compaction as possible and understandable at the same
time. I simply do not see how I would do it differently at this
moment. The current code behaves reasonably well with workloads I was
testing. I am not claiming this will need some surgery later on but I
would rather see oom reports and tweak the current implementation in
incremental steps than over engineer something from the early beginning
for theoretical issues which I even cannot get rid of completely. This
is a _heuristic_ and as such it can handle certain class of workloads
better than others. This is the case with the current implementation as
well.

[...]

> > Considering that we are in a clear disagreement in the compaction aspect
> > I think we need others to either back your concern or you show a clear
> > justification why compaction feedback is not viable way longterm even
> > after we make further changes which would make it less THP oriented.
> 
> I can't understand why I need to convince you. Conventionally, patch
> author needs to convince reviewer.

I am trying my best to clarify/justify my changes but it is really hard
when you disagree with some core principles with theoretical problems
which I do not see in practice. This is a heuristic and as such it will
never cover 100% cases. I aim to be as good as possible and the results
so far look reasonable to me.

> Anyway, above exmaple would be helpful to understand limitation of the
> compaction.

I understand that the compaction is not omnipotent and I can see there
will be corner cases but there always have been some in this area I am
just replacing the current by less fuzzy ones.
 
> If explanation in this reply also would not convince you, I
> won't insist more. Discussing more on this topic would not be
> productive for us.

I am afraid I haven't heard any such a strong argument to re-evaluate my
current position. As I've said we might need some tweaks here and there
in the future but at least we can build on a solid and deterministic
grounds which I find the most important aspect of the new
implementation.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web