* [patch 1/3]vmscan: clear ZONE_CONGESTED for zone with good watermark
@ 2011-07-28 8:13 Shaohua Li
2011-07-28 10:56 ` Mel Gorman
0 siblings, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2011-07-28 8:13 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mm, mgorman, Minchan Kim
correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
should clear ZONE_CONGESTED regardless if this is a high order
allocation, because pages can be reclaimed in other tasks but
ZONE_CONGESTED is only cleared in kswapd.
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
mm/vmscan.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
Index: linux/mm/vmscan.c
===================================================================
--- linux.orig/mm/vmscan.c 2011-07-25 09:37:11.000000000 +0800
+++ linux/mm/vmscan.c 2011-07-28 15:17:56.000000000 +0800
@@ -2494,6 +2494,9 @@ loop_again:
high_wmark_pages(zone), 0, 0)) {
end_zone = i;
break;
+ } else {
+ /* If balanced, clear the congested flag */
+ zone_clear_flag(zone, ZONE_CONGESTED);
}
}
if (i < 0)
@@ -2665,26 +2668,25 @@ out:
* be cleared as kswapd is the only mechanism that clears the flag
* and it is potentially going to sleep here.
*/
- if (order) {
- for (i = 0; i <= end_zone; i++) {
- struct zone *zone = pgdat->node_zones + i;
+ for (i = 0; i <= end_zone; i++) {
+ struct zone *zone = pgdat->node_zones + i;
- if (!populated_zone(zone))
- continue;
+ if (!populated_zone(zone))
+ continue;
- if (zone->all_unreclaimable && priority != DEF_PRIORITY)
- continue;
+ if (zone->all_unreclaimable && priority != DEF_PRIORITY)
+ continue;
- /* Confirm the zone is balanced for order-0 */
- if (!zone_watermark_ok(zone, 0,
- high_wmark_pages(zone), 0, 0)) {
+ /* Confirm the zone is balanced for order-0 */
+ if (!zone_watermark_ok(zone, 0, high_wmark_pages(zone), 0, 0)) {
+ if (order) {
order = sc.order = 0;
goto loop_again;
}
-
- /* If balanced, clear the congested flag */
- zone_clear_flag(zone, ZONE_CONGESTED);
}
+
+ /* If balanced, clear the congested flag */
+ zone_clear_flag(zone, ZONE_CONGESTED);
}
/*
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/3]vmscan: clear ZONE_CONGESTED for zone with good watermark
2011-07-28 8:13 [patch 1/3]vmscan: clear ZONE_CONGESTED for zone with good watermark Shaohua Li
@ 2011-07-28 10:56 ` Mel Gorman
2011-07-29 0:35 ` Shaohua Li
0 siblings, 1 reply; 6+ messages in thread
From: Mel Gorman @ 2011-07-28 10:56 UTC (permalink / raw)
To: Shaohua Li; +Cc: Andrew Morton, linux-mm, Minchan Kim
On Thu, Jul 28, 2011 at 04:13:01PM +0800, Shaohua Li wrote:
> correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
> should clear ZONE_CONGESTED regardless if this is a high order
> allocation, because pages can be reclaimed in other tasks but
> ZONE_CONGESTED is only cleared in kswapd.
>
What problem does this solve?
As it is, for high order allocations it takes the following steps
If reclaiming at high order {
for each zone {
if all_unreclaimable
skip
if watermark is not met
order = 0
loop again
/* watermark is met */
clear congested
}
}
If high orders are failing, kswapd balances for order-0 where there
is already a cleaning of ZONE_CONGESTED if the zone was shrunk and
became balanced. I see the case for hunk 1 of the patch because now
it'll clear ZONE_CONGESTED for zones that are already balanced which
might have a noticable effect on wait_iff_congested. Is this what
you see? Even if it is, it does not explain hunk 2 of the patch.
--
Mel Gorman
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/3]vmscan: clear ZONE_CONGESTED for zone with good watermark
2011-07-28 10:56 ` Mel Gorman
@ 2011-07-29 0:35 ` Shaohua Li
2011-07-29 8:50 ` Mel Gorman
2011-07-29 9:13 ` Minchan Kim
0 siblings, 2 replies; 6+ messages in thread
From: Shaohua Li @ 2011-07-29 0:35 UTC (permalink / raw)
To: Mel Gorman; +Cc: Andrew Morton, linux-mm, Minchan Kim
On Thu, 2011-07-28 at 18:56 +0800, Mel Gorman wrote:
> On Thu, Jul 28, 2011 at 04:13:01PM +0800, Shaohua Li wrote:
> > correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
> > should clear ZONE_CONGESTED regardless if this is a high order
> > allocation, because pages can be reclaimed in other tasks but
> > ZONE_CONGESTED is only cleared in kswapd.
> >
>
> What problem does this solve?
>
> As it is, for high order allocations it takes the following steps
>
> If reclaiming at high order {
> for each zone {
> if all_unreclaimable
> skip
> if watermark is not met
> order = 0
> loop again
>
> /* watermark is met */
> clear congested
> }
> }
>
> If high orders are failing, kswapd balances for order-0 where there
> is already a cleaning of ZONE_CONGESTED if the zone was shrunk and
> became balanced. I see the case for hunk 1 of the patch because now
> it'll clear ZONE_CONGESTED for zones that are already balanced which
> might have a noticable effect on wait_iff_congested. Is this what
> you see? Even if it is, it does not explain hunk 2 of the patch.
I first looked at the hunk 2 place and thought we don't clear
ZONE_CONGESTED there. I then figured out we need do the same thing for
the hunk 1. But you are correct, with hunk 1, hunk 2 isn't required.
updated patch.
correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
should clear ZONE_CONGESTED because pages can be reclaimed in
other tasks but ZONE_CONGESTED is only cleared in kswapd.
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
mm/vmscan.c | 3 +++
1 file changed, 3 insertions(+)
Index: linux/mm/vmscan.c
===================================================================
--- linux.orig/mm/vmscan.c 2011-07-29 08:24:10.000000000 +0800
+++ linux/mm/vmscan.c 2011-07-29 08:26:29.000000000 +0800
@@ -2494,6 +2494,9 @@ loop_again:
high_wmark_pages(zone), 0, 0)) {
end_zone = i;
break;
+ } else {
+ /* If balanced, clear the congested flag */
+ zone_clear_flag(zone, ZONE_CONGESTED);
}
}
if (i < 0)
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/3]vmscan: clear ZONE_CONGESTED for zone with good watermark
2011-07-29 0:35 ` Shaohua Li
@ 2011-07-29 8:50 ` Mel Gorman
2011-07-29 11:01 ` Shaohua Li
2011-07-29 9:13 ` Minchan Kim
1 sibling, 1 reply; 6+ messages in thread
From: Mel Gorman @ 2011-07-29 8:50 UTC (permalink / raw)
To: Shaohua Li; +Cc: Andrew Morton, linux-mm, Minchan Kim
On Fri, Jul 29, 2011 at 08:35:25AM +0800, Shaohua Li wrote:
> On Thu, 2011-07-28 at 18:56 +0800, Mel Gorman wrote:
> > On Thu, Jul 28, 2011 at 04:13:01PM +0800, Shaohua Li wrote:
> > > correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
> > > should clear ZONE_CONGESTED regardless if this is a high order
> > > allocation, because pages can be reclaimed in other tasks but
> > > ZONE_CONGESTED is only cleared in kswapd.
> > >
> >
> > What problem does this solve?
> >
> > As it is, for high order allocations it takes the following steps
> >
> > If reclaiming at high order {
> > for each zone {
> > if all_unreclaimable
> > skip
> > if watermark is not met
> > order = 0
> > loop again
> >
> > /* watermark is met */
> > clear congested
> > }
> > }
> >
> > If high orders are failing, kswapd balances for order-0 where there
> > is already a cleaning of ZONE_CONGESTED if the zone was shrunk and
> > became balanced. I see the case for hunk 1 of the patch because now
> > it'll clear ZONE_CONGESTED for zones that are already balanced which
> > might have a noticable effect on wait_iff_congested. Is this what
> > you see? Even if it is, it does not explain hunk 2 of the patch.
> I first looked at the hunk 2 place and thought we don't clear
> ZONE_CONGESTED there. I then figured out we need do the same thing for
> the hunk 1. But you are correct, with hunk 1, hunk 2 isn't required.
> updated patch.
>
>
>
> correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
> should clear ZONE_CONGESTED because pages can be reclaimed in
> other tasks but ZONE_CONGESTED is only cleared in kswapd.
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
It would be nice if the changelog was expanded a bit to explain
why the patch is necessary. You say this is to "correctly clear
ZONE_CONGESTED" but do not explain why the current code is wrong
or what the user-visible impact is. For example, even cutting and
pasting bits of the discussion like the following would have been
an improvement.
==== CUT HERE ===
kswapd is responsible for clearing ZONE_CONGESTED after it balances
a zone. Unfortunately, if ZONE_CONGESTED was set during a high-order
allocation, it is possible that kswapd misses clearing it.
At the end of balance_pgdat(), kswapd uses the following logic;
If reclaiming at high order {
for each zone {
if all_unreclaimable
skip
if watermark is not met
order = 0
loop again
/* watermark is met */
clear congested
}
}
i.e. it clears ZONE_CONGESTED if it the zone is balanced. if not,
it restarts balancing at order-0. However, if the higher zones are
balanced for order-0, kswapd will miss clearing ZONE_CONGESTED
as that only happens after a zone is shrunk. This can mean that
wait_iff_congested() stalls unnecessarily. This patch makes kswapd
clear ZONE_CONGESTED during its initial highmem->dma scan for zones
that are already balanced.
==== CUT HERE ====
This makes review a lot easier and will be helpful in the future if
someone uses git blame.
Whether you update the changelog or not;
Acked-by: Mel Gorman <mgorman@suse.de>
Thanks.
--
Mel Gorman
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/3]vmscan: clear ZONE_CONGESTED for zone with good watermark
2011-07-29 0:35 ` Shaohua Li
2011-07-29 8:50 ` Mel Gorman
@ 2011-07-29 9:13 ` Minchan Kim
1 sibling, 0 replies; 6+ messages in thread
From: Minchan Kim @ 2011-07-29 9:13 UTC (permalink / raw)
To: Shaohua Li; +Cc: Mel Gorman, Andrew Morton, linux-mm
On Fri, Jul 29, 2011 at 08:35:25AM +0800, Shaohua Li wrote:
> On Thu, 2011-07-28 at 18:56 +0800, Mel Gorman wrote:
> > On Thu, Jul 28, 2011 at 04:13:01PM +0800, Shaohua Li wrote:
> > > correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
> > > should clear ZONE_CONGESTED regardless if this is a high order
> > > allocation, because pages can be reclaimed in other tasks but
> > > ZONE_CONGESTED is only cleared in kswapd.
> > >
> >
> > What problem does this solve?
> >
> > As it is, for high order allocations it takes the following steps
> >
> > If reclaiming at high order {
> > for each zone {
> > if all_unreclaimable
> > skip
> > if watermark is not met
> > order = 0
> > loop again
> >
> > /* watermark is met */
> > clear congested
> > }
> > }
> >
> > If high orders are failing, kswapd balances for order-0 where there
> > is already a cleaning of ZONE_CONGESTED if the zone was shrunk and
> > became balanced. I see the case for hunk 1 of the patch because now
> > it'll clear ZONE_CONGESTED for zones that are already balanced which
> > might have a noticable effect on wait_iff_congested. Is this what
> > you see? Even if it is, it does not explain hunk 2 of the patch.
> I first looked at the hunk 2 place and thought we don't clear
> ZONE_CONGESTED there. I then figured out we need do the same thing for
> the hunk 1. But you are correct, with hunk 1, hunk 2 isn't required.
> updated patch.
>
>
>
> correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
> should clear ZONE_CONGESTED because pages can be reclaimed in
> other tasks but ZONE_CONGESTED is only cleared in kswapd.
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
Even it will fix that when kswapd wakes up lately by order-0 and look at zones,
all zones would become okay so it jumps out with "if (i < 0) goto out" with missing
clearing ZONE_CONGESTED.
--
Kind regards,
Minchan Kim
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/3]vmscan: clear ZONE_CONGESTED for zone with good watermark
2011-07-29 8:50 ` Mel Gorman
@ 2011-07-29 11:01 ` Shaohua Li
0 siblings, 0 replies; 6+ messages in thread
From: Shaohua Li @ 2011-07-29 11:01 UTC (permalink / raw)
To: Mel Gorman; +Cc: Andrew Morton, linux-mm, Minchan Kim
On Fri, Jul 29, 2011 at 04:50:43PM +0800, Mel Gorman wrote:
> On Fri, Jul 29, 2011 at 08:35:25AM +0800, Shaohua Li wrote:
> > On Thu, 2011-07-28 at 18:56 +0800, Mel Gorman wrote:
> > > On Thu, Jul 28, 2011 at 04:13:01PM +0800, Shaohua Li wrote:
> > > > correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
> > > > should clear ZONE_CONGESTED regardless if this is a high order
> > > > allocation, because pages can be reclaimed in other tasks but
> > > > ZONE_CONGESTED is only cleared in kswapd.
> > > >
> > >
> > > What problem does this solve?
> > >
> > > As it is, for high order allocations it takes the following steps
> > >
> > > If reclaiming at high order {
> > > for each zone {
> > > if all_unreclaimable
> > > skip
> > > if watermark is not met
> > > order = 0
> > > loop again
> > >
> > > /* watermark is met */
> > > clear congested
> > > }
> > > }
> > >
> > > If high orders are failing, kswapd balances for order-0 where there
> > > is already a cleaning of ZONE_CONGESTED if the zone was shrunk and
> > > became balanced. I see the case for hunk 1 of the patch because now
> > > it'll clear ZONE_CONGESTED for zones that are already balanced which
> > > might have a noticable effect on wait_iff_congested. Is this what
> > > you see? Even if it is, it does not explain hunk 2 of the patch.
> > I first looked at the hunk 2 place and thought we don't clear
> > ZONE_CONGESTED there. I then figured out we need do the same thing for
> > the hunk 1. But you are correct, with hunk 1, hunk 2 isn't required.
> > updated patch.
> >
> >
> >
> > correctly clear ZONE_CONGESTED. If a zone watermark is ok, we
> > should clear ZONE_CONGESTED because pages can be reclaimed in
> > other tasks but ZONE_CONGESTED is only cleared in kswapd.
> >
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>
> It would be nice if the changelog was expanded a bit to explain
> why the patch is necessary. You say this is to "correctly clear
> ZONE_CONGESTED" but do not explain why the current code is wrong
> or what the user-visible impact is. For example, even cutting and
> pasting bits of the discussion like the following would have been
> an improvement.
>
> ==== CUT HERE ===
> kswapd is responsible for clearing ZONE_CONGESTED after it balances
> a zone. Unfortunately, if ZONE_CONGESTED was set during a high-order
> allocation, it is possible that kswapd misses clearing it.
>
> At the end of balance_pgdat(), kswapd uses the following logic;
>
> If reclaiming at high order {
> for each zone {
> if all_unreclaimable
> skip
> if watermark is not met
> order = 0
> loop again
>
> /* watermark is met */
> clear congested
> }
> }
>
> i.e. it clears ZONE_CONGESTED if it the zone is balanced. if not,
> it restarts balancing at order-0. However, if the higher zones are
> balanced for order-0, kswapd will miss clearing ZONE_CONGESTED
> as that only happens after a zone is shrunk. This can mean that
> wait_iff_congested() stalls unnecessarily. This patch makes kswapd
> clear ZONE_CONGESTED during its initial highmem->dma scan for zones
> that are already balanced.
>
> ==== CUT HERE ====
>
> This makes review a lot easier and will be helpful in the future if
> someone uses git blame.
>
> Whether you update the changelog or not;
>
> Acked-by: Mel Gorman <mgorman@suse.de>
Ok, updated the changelog.
ZONE_CONGESTED is only cleared in kswapd, but pages can be freed in any task.
It's possible ZONE_CONGESTED isn't cleared in some cases:
1. the zone is already balanced just entering balance_pgdat() for order-0 because
concurrent tasks free memory. In this case, later check will skip the zone as
it's balanced so the flag isn't cleared.
2. high order balance fallbacks to order-0. quote from Mel:
At the end of balance_pgdat(), kswapd uses the following logic;
If reclaiming at high order {
for each zone {
if all_unreclaimable
skip
if watermark is not met
order = 0
loop again
/* watermark is met */
clear congested
}
}
i.e. it clears ZONE_CONGESTED if it the zone is balanced. if not,
it restarts balancing at order-0. However, if the higher zones are
balanced for order-0, kswapd will miss clearing ZONE_CONGESTED
as that only happens after a zone is shrunk.
This can mean that wait_iff_congested() stalls unnecessarily. This patch
makes kswapd clear ZONE_CONGESTED during its initial highmem->dma scan
for zones that are already balanced.
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Acked-by: Mel Gorman <mgorman@suse.de>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
---
mm/vmscan.c | 3 +++
1 file changed, 3 insertions(+)
Index: linux/mm/vmscan.c
===================================================================
--- linux.orig/mm/vmscan.c 2011-07-29 08:24:10.000000000 +0800
+++ linux/mm/vmscan.c 2011-07-29 08:26:29.000000000 +0800
@@ -2494,6 +2494,9 @@ loop_again:
high_wmark_pages(zone), 0, 0)) {
end_zone = i;
break;
+ } else {
+ /* If balanced, clear the congested flag */
+ zone_clear_flag(zone, ZONE_CONGESTED);
}
}
if (i < 0)
--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-29 11:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-28 8:13 [patch 1/3]vmscan: clear ZONE_CONGESTED for zone with good watermark Shaohua Li
2011-07-28 10:56 ` Mel Gorman
2011-07-29 0:35 ` Shaohua Li
2011-07-29 8:50 ` Mel Gorman
2011-07-29 11:01 ` Shaohua Li
2011-07-29 9:13 ` Minchan Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.