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

Groups > linux.kernel > #1410269 > unrolled thread

Re: zone_reclaimable() leads to livelock in __alloc_pages_slowpath()

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: zone_reclaimable() leads to livelock in __alloc_pages_slowpath() Michal Hocko <mhocko@kernel.org> - 2016-05-31 15:00 +0200
    Re: zone_reclaimable() leads to livelock in __alloc_pages_slowpath() Oleg Nesterov <oleg@redhat.com> - 2016-06-01 02:00 +0200
      Re: zone_reclaimable() leads to livelock in __alloc_pages_slowpath() Michal Hocko <mhocko@kernel.org> - 2016-06-01 12:10 +0200
        Re: zone_reclaimable() leads to livelock in  __alloc_pages_slowpath() Oleg Nesterov <oleg@redhat.com> - 2016-06-02 00:50 +0200
          Re: zone_reclaimable() leads to livelock in __alloc_pages_slowpath() Michal Hocko <mhocko@kernel.org> - 2016-06-02 17:20 +0200

#1410269 — Re: zone_reclaimable() leads to livelock in __alloc_pages_slowpath()

On Sun 29-05-16 23:25:40, Oleg Nesterov wrote:
> sorry for delay,
> 
> On 05/25, Michal Hocko wrote:
[...]
> > This is a bit surprising but my testing shows that the result shouldn't
> > make much difference. I can see some discrepancies between lru_vec size
> > and zone_reclaimable_pages but they are too small to actually matter.
> 
> Yes, the difference is small but it does matter.
> 
> I do not pretend I understand this all, but finally it seems I understand
> whats going on on my system when it hangs. At least, why the change in
> lruvec_lru_size() or calculate_normal_threshold() makes a difference.
> 
> This single change in get_scan_count() under for_each_evictable_lru() loop
> 
> 	-	size = lruvec_lru_size(lruvec, lru);
> 	+	size = zone_page_state_snapshot(lruvec_zone(lruvec), NR_LRU_BASE + lru);
> 
> fixes the problem too.
> 
> Without this change shrink*() continues to scan the LRU_ACTIVE_FILE list
> while it is empty. LRU_INACTIVE_FILE is not empty (just a few pages) but
> we do not even try to scan it, lruvec_lru_size() returns zero.

OK, you seem to be really seeing a different issue than me. My debugging
patch was showing when nothing was really isolated from the LRU lists
(both for shrink_{in}active_list.

> Then later we recheck zone_reclaimable() and it notices the INACTIVE_FILE
> counter because it uses the _snapshot variant, this leads to livelock.
> 
> I guess this doesn't really matter, but in my particular case these
> ACTIVE/INACTIVE counters were screwed by the recent putback_inactive_pages()
> logic. The pages we "leak" in INACTIVE list were recently moved from ACTIVE
> to INACTIVE list, and this updated only the per-cpu ->vm_stat_diff[] counters,
> so the "non snapshot" lruvec_lru_size() in get_scan_count() sees the "old"
> numbers.

Hmm. I am not really sure we can use the _snapshot version in lruvec_lru_size.
It is called also outise of slow paths (like add_to_page_cache_lru).
Maybe a path like below would be acceptable for stable trees.

But I am thinking whether we should simply revert 0db2cb8da89d ("mm,
vmscan: make zone_reclaimable_pages more precise") in 4.6 stable tree.
Does that help as well? The patch is certainly needed for the oom
detection rework but it got merged one release cycle earlier.
--- 
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 3388ccbab7d6..9f46a29c06b6 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -764,7 +764,8 @@ static inline struct zone *lruvec_zone(struct lruvec *lruvec)
 #endif
 }
 
-extern unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru);
+extern unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru,
+		bool precise);
 
 #ifdef CONFIG_HAVE_MEMORY_PRESENT
 void memory_present(int nid, unsigned long start, unsigned long end);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index c4a2f4512fca..84420045090b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -213,11 +213,13 @@ bool zone_reclaimable(struct zone *zone)
 		zone_reclaimable_pages(zone) * 6;
 }
 
-unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru)
+unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, bool precise)
 {
 	if (!mem_cgroup_disabled())
 		return mem_cgroup_get_lru_size(lruvec, lru);
 
+	if (precise)
+		return zone_page_state_snapshot(lruvec_zone(lruvec), NR_LRU_BASE + lru);
 	return zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru);
 }
 
@@ -1902,8 +1904,8 @@ static bool inactive_list_is_low(struct lruvec *lruvec, bool file)
 	if (!file && !total_swap_pages)
 		return false;
 
-	inactive = lruvec_lru_size(lruvec, file * LRU_FILE);
-	active = lruvec_lru_size(lruvec, file * LRU_FILE + LRU_ACTIVE);
+	inactive = lruvec_lru_size(lruvec, file * LRU_FILE, true);
+	active = lruvec_lru_size(lruvec, file * LRU_FILE + LRU_ACTIVE, true);
 
 	gb = (inactive + active) >> (30 - PAGE_SHIFT);
 	if (gb)
@@ -2040,7 +2042,7 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 	 * system is under heavy pressure.
 	 */
 	if (!inactive_list_is_low(lruvec, true) &&
-	    lruvec_lru_size(lruvec, LRU_INACTIVE_FILE) >> sc->priority) {
+	    lruvec_lru_size(lruvec, LRU_INACTIVE_FILE, true) >> sc->priority) {
 		scan_balance = SCAN_FILE;
 		goto out;
 	}
@@ -2066,10 +2068,10 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 	 * anon in [0], file in [1]
 	 */
 
-	anon  = lruvec_lru_size(lruvec, LRU_ACTIVE_ANON) +
-		lruvec_lru_size(lruvec, LRU_INACTIVE_ANON);
-	file  = lruvec_lru_size(lruvec, LRU_ACTIVE_FILE) +
-		lruvec_lru_size(lruvec, LRU_INACTIVE_FILE);
+	anon  = lruvec_lru_size(lruvec, LRU_ACTIVE_ANON, true) +
+		lruvec_lru_size(lruvec, LRU_INACTIVE_ANON, true);
+	file  = lruvec_lru_size(lruvec, LRU_ACTIVE_FILE, true) +
+		lruvec_lru_size(lruvec, LRU_INACTIVE_FILE, true);
 
 	spin_lock_irq(&zone->lru_lock);
 	if (unlikely(reclaim_stat->recent_scanned[0] > anon / 4)) {
@@ -2107,7 +2109,7 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 			unsigned long size;
 			unsigned long scan;
 
-			size = lruvec_lru_size(lruvec, lru);
+			size = lruvec_lru_size(lruvec, lru, true);
 			scan = size >> sc->priority;
 
 			if (!scan && pass && force_scan)
diff --git a/mm/workingset.c b/mm/workingset.c
index 8a75f8d2916a..509cdf7a6fc9 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -269,7 +269,7 @@ bool workingset_refault(void *shadow)
 	}
 	lruvec = mem_cgroup_zone_lruvec(zone, memcg);
 	refault = atomic_long_read(&lruvec->inactive_age);
-	active_file = lruvec_lru_size(lruvec, LRU_ACTIVE_FILE);
+	active_file = lruvec_lru_size(lruvec, LRU_ACTIVE_FILE, false);
 	rcu_read_unlock();
 
 	/*
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1410694

On 05/31, Michal Hocko wrote:
>
> On Sun 29-05-16 23:25:40, Oleg Nesterov wrote:
> >
> > This single change in get_scan_count() under for_each_evictable_lru() loop
> >
> > 	-	size = lruvec_lru_size(lruvec, lru);
> > 	+	size = zone_page_state_snapshot(lruvec_zone(lruvec), NR_LRU_BASE + lru);
> >
> > fixes the problem too.
> >
> > Without this change shrink*() continues to scan the LRU_ACTIVE_FILE list
> > while it is empty. LRU_INACTIVE_FILE is not empty (just a few pages) but
> > we do not even try to scan it, lruvec_lru_size() returns zero.
>
> OK, you seem to be really seeing a different issue than me.

quite possibly, but

> My debugging
> patch was showing when nothing was really isolated from the LRU lists
> (both for shrink_{in}active_list.

in my debugging session too. LRU_ACTIVE_FILE was empty, so there is nothing to
isolate even if shrink_active_list() is (wrongly called) with nr_to_scan != 0.
LRU_INACTIVE_FILE is not empty but it is not scanned because nr_to_scan == 0.

But I am afraid I misunderstood you, and you meant something else.

> > Then later we recheck zone_reclaimable() and it notices the INACTIVE_FILE
> > counter because it uses the _snapshot variant, this leads to livelock.
> >
> > I guess this doesn't really matter, but in my particular case these
> > ACTIVE/INACTIVE counters were screwed by the recent putback_inactive_pages()
> > logic. The pages we "leak" in INACTIVE list were recently moved from ACTIVE
> > to INACTIVE list, and this updated only the per-cpu ->vm_stat_diff[] counters,
> > so the "non snapshot" lruvec_lru_size() in get_scan_count() sees the "old"
> > numbers.
>
> Hmm. I am not really sure we can use the _snapshot version in lruvec_lru_size.

Yes, yes, I  understand,

> But I am thinking whether we should simply revert 0db2cb8da89d ("mm,
> vmscan: make zone_reclaimable_pages more precise") in 4.6 stable tree.
> Does that help as well?

I'll test this tomorrow, but even if it helps I am not sure... Yes, this
way zone_reclaimable() and get_scan_count() will see the same numbers, but
how this can help to make zone_reclaimable() == F at the end?

Again, suppose that (say) ACTIVE list is empty but zone->vm_stat != 0
because there is something in per-cpu counter (so that _snapshot == 0).
This means that we sill continue to try to scan this list for no reason.

But Michal, let me repeat that I do not understand this code, so I can
be easily wrong.

Oleg.

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

#1411054

On Wed 01-06-16 01:56:26, Oleg Nesterov wrote:
> On 05/31, Michal Hocko wrote:
> >
> > On Sun 29-05-16 23:25:40, Oleg Nesterov wrote:
> > >
> > > This single change in get_scan_count() under for_each_evictable_lru() loop
> > >
> > > 	-	size = lruvec_lru_size(lruvec, lru);
> > > 	+	size = zone_page_state_snapshot(lruvec_zone(lruvec), NR_LRU_BASE + lru);
> > >
> > > fixes the problem too.
> > >
> > > Without this change shrink*() continues to scan the LRU_ACTIVE_FILE list
> > > while it is empty. LRU_INACTIVE_FILE is not empty (just a few pages) but
> > > we do not even try to scan it, lruvec_lru_size() returns zero.
> >
> > OK, you seem to be really seeing a different issue than me.
> 
> quite possibly, but
> 
> > My debugging
> > patch was showing when nothing was really isolated from the LRU lists
> > (both for shrink_{in}active_list.
> 
> in my debugging session too. LRU_ACTIVE_FILE was empty, so there is nothing to
> isolate even if shrink_active_list() is (wrongly called) with nr_to_scan != 0.
> LRU_INACTIVE_FILE is not empty but it is not scanned because nr_to_scan == 0.
> 
> But I am afraid I misunderstood you, and you meant something else.

What I wanted to say is that my debugging hasn't shown a single case
when nothing would be isolated. Which seems to be the case for you.

[...]

> > But I am thinking whether we should simply revert 0db2cb8da89d ("mm,
> > vmscan: make zone_reclaimable_pages more precise") in 4.6 stable tree.
> > Does that help as well?
> 
> I'll test this tomorrow, but even if it helps I am not sure... Yes, this
> way zone_reclaimable() and get_scan_count() will see the same numbers, but
> how this can help to make zone_reclaimable() == F at the end?

It won't in some cases. And that has been the case for ages so I do not
think we need any steps for the stable. What meant to address is a
potential regression caused by 0db2cb8da89d which would make this more
likely because of the mismatch because the patch really makes much more
sense for the oom detection rework which really wants more precise
numbers. If the revert doesn't help then I would just leave it as it is
and just note that the zone_reclaimable was a bad decision which
fortunatelly didn't blow up that often...

Thanks
-- 
Michal Hocko
SUSE Labs

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

#1411638 — Re: zone_reclaimable() leads to livelock in __alloc_pages_slowpath()

On 06/01, Michal Hocko wrote:
>
> On Wed 01-06-16 01:56:26, Oleg Nesterov wrote:
> > On 05/31, Michal Hocko wrote:
> > >
> > > On Sun 29-05-16 23:25:40, Oleg Nesterov wrote:
> > > >
> > > > This single change in get_scan_count() under for_each_evictable_lru() loop
> > > >
> > > > 	-	size = lruvec_lru_size(lruvec, lru);
> > > > 	+	size = zone_page_state_snapshot(lruvec_zone(lruvec), NR_LRU_BASE + lru);
> > > >
> > > > fixes the problem too.
> > > >
> > > > Without this change shrink*() continues to scan the LRU_ACTIVE_FILE list
> > > > while it is empty. LRU_INACTIVE_FILE is not empty (just a few pages) but
> > > > we do not even try to scan it, lruvec_lru_size() returns zero.
> > >
> > > OK, you seem to be really seeing a different issue than me.
> >
> > quite possibly, but
> >
> > > My debugging
> > > patch was showing when nothing was really isolated from the LRU lists
> > > (both for shrink_{in}active_list.
> >
> > in my debugging session too. LRU_ACTIVE_FILE was empty, so there is nothing to
> > isolate even if shrink_active_list() is (wrongly called) with nr_to_scan != 0.
> > LRU_INACTIVE_FILE is not empty but it is not scanned because nr_to_scan == 0.
> >
> > But I am afraid I misunderstood you, and you meant something else.
>
> What I wanted to say is that my debugging hasn't shown a single case
> when nothing would be isolated. Which seems to be the case for you.

Ah, got it, thanks. Yes, I see that there is no "nothing scanned" in
oom-test.qcow_serial.log.gz from http://marc.info/?l=linux-kernel&m=146417822608902
you sent. I applied this patch and I do see "nothing scanned".

But, unlike you, I do not see the messages from free-pages... perhaps you
have more active tasks. To remind, I tested this with the single user-space
process, /bin/sh running with pid==1, then I did "while true; do ./oom; done".

So of course I do not know if you see another issue or the same, but now I am
wondering if the change in get_scan_count() above fixes the problem for you.

Probably not, but the fact you do not see "nothing scanned" can't prove this,
it is possible that shrink_*_list() was not called because vm_stat == 0 but
zone_reclaimable() sees the per-cpu counter. In this case 0db2cb8da89d can
make a difference, but see below.

> > > But I am thinking whether we should simply revert 0db2cb8da89d ("mm,
> > > vmscan: make zone_reclaimable_pages more precise") in 4.6 stable tree.
> > > Does that help as well?
> >
> > I'll test this tomorrow,

So it doesn't help.

> but even if it helps I am not sure... Yes, this
> > way zone_reclaimable() and get_scan_count() will see the same numbers, but
> > how this can help to make zone_reclaimable() == F at the end?
>
> It won't in some cases.

And unless I am notally confused  hit exactly this case.

> And that has been the case for ages so I do not
> think we need any steps for the stable.

OK, agreed.

> What meant to address is a
> potential regression caused by 0db2cb8da89d which would make this more
> likely because of the mismatch

Again, I can be easily wrong, but I do not see how 0db2cb8da89d could make
the things worse...

Unless both get_scan_count() and zone_reclaimable() use "snapshot" variant,
we can't guarantee zone_reclaimable() becomes false. The fact that they see
different numbers (after 0db2cb8da89d) doesn't really matter.

Anyway, this was already fixed, so lets forget it ;)

Oleg.

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

#1412342

On Wed 01-06-16 23:38:30, Oleg Nesterov wrote:
> On 06/01, Michal Hocko wrote:
> >
> > On Wed 01-06-16 01:56:26, Oleg Nesterov wrote:
> > > On 05/31, Michal Hocko wrote:
> > > >
> > > > On Sun 29-05-16 23:25:40, Oleg Nesterov wrote:
> > > > >
> > > > > This single change in get_scan_count() under for_each_evictable_lru() loop
> > > > >
> > > > > 	-	size = lruvec_lru_size(lruvec, lru);
> > > > > 	+	size = zone_page_state_snapshot(lruvec_zone(lruvec), NR_LRU_BASE + lru);
> > > > >
> > > > > fixes the problem too.
> > > > >
> > > > > Without this change shrink*() continues to scan the LRU_ACTIVE_FILE list
> > > > > while it is empty. LRU_INACTIVE_FILE is not empty (just a few pages) but
> > > > > we do not even try to scan it, lruvec_lru_size() returns zero.
> > > >
> > > > OK, you seem to be really seeing a different issue than me.
> > >
> > > quite possibly, but
> > >
> > > > My debugging
> > > > patch was showing when nothing was really isolated from the LRU lists
> > > > (both for shrink_{in}active_list.
> > >
> > > in my debugging session too. LRU_ACTIVE_FILE was empty, so there is nothing to
> > > isolate even if shrink_active_list() is (wrongly called) with nr_to_scan != 0.
> > > LRU_INACTIVE_FILE is not empty but it is not scanned because nr_to_scan == 0.
> > >
> > > But I am afraid I misunderstood you, and you meant something else.
> >
> > What I wanted to say is that my debugging hasn't shown a single case
> > when nothing would be isolated. Which seems to be the case for you.
> 
> Ah, got it, thanks. Yes, I see that there is no "nothing scanned" in
> oom-test.qcow_serial.log.gz from http://marc.info/?l=linux-kernel&m=146417822608902
> you sent. I applied this patch and I do see "nothing scanned".
> 
> But, unlike you, I do not see the messages from free-pages... perhaps you
> have more active tasks. To remind, I tested this with the single user-space
> process, /bin/sh running with pid==1, then I did "while true; do ./oom; done".

Well, I was booting into a standard init which will have a couple of
processes. So yes this would make a slight difference.
 
> So of course I do not know if you see another issue or the same, but now I am
> wondering if the change in get_scan_count() above fixes the problem for you.

I have played with it but the interfering freed pages just ruined the
whole zone_reclaimable expectations.
 
> Probably not, but the fact you do not see "nothing scanned" can't prove this,
> it is possible that shrink_*_list() was not called because vm_stat == 0 but
> zone_reclaimable() sees the per-cpu counter. In this case 0db2cb8da89d can
> make a difference, but see below.
> 
> > > > But I am thinking whether we should simply revert 0db2cb8da89d ("mm,
> > > > vmscan: make zone_reclaimable_pages more precise") in 4.6 stable tree.
> > > > Does that help as well?
> > >
> > > I'll test this tomorrow,
> 
> So it doesn't help.

OK, so we at least know this is not a regression.

> > but even if it helps I am not sure... Yes, this
> > > way zone_reclaimable() and get_scan_count() will see the same numbers, but
> > > how this can help to make zone_reclaimable() == F at the end?
> >
> > It won't in some cases.
> 
> And unless I am notally confused  hit exactly this case.
> 
> > And that has been the case for ages so I do not
> > think we need any steps for the stable.
> 
> OK, agreed.
> 
> > What meant to address is a
> > potential regression caused by 0db2cb8da89d which would make this more
> > likely because of the mismatch
> 
> Again, I can be easily wrong, but I do not see how 0db2cb8da89d could make
> the things worse...
> 
> Unless both get_scan_count() and zone_reclaimable() use "snapshot" variant,
> we can't guarantee zone_reclaimable() becomes false. The fact that they see
> different numbers (after 0db2cb8da89d) doesn't really matter.
> 
> Anyway, this was already fixed, so lets forget it ;)

Yes, especially as this doesn't seem to be a regression.

Thanks for your effort anyway.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web