[patch]vmscan: avoid set zone congested if no page dirty

[patch]vmscan: avoid set zone congested if no page dirty

[Shaohua Li]

nr_dirty and nr_congested are increased only when page is dirty. So if all pages
are clean, both them will be zero. In this case, we should not mark the zone
congested.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/mm/vmscan.c b/mm/vmscan.c
index b8a6fdc..d31d7ce 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -913,7 +913,7 @@ keep_lumpy:
 	 * back off and wait for congestion to clear because further reclaim
 	 * will encounter the same problem
 	 */
-	if (nr_dirty == nr_congested)
+	if (nr_dirty == nr_congested && nr_dirty != 0)
 		zone_set_flag(zone, ZONE_CONGESTED);
 
 	free_page_list(&free_pages);


--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch]vmscan: avoid set zone congested if no page dirty

[Johannes Weiner]

On Thu, Nov 04, 2010 at 08:50:58AM +0800, Shaohua Li wrote:
> nr_dirty and nr_congested are increased only when page is dirty. So if all pages
> are clean, both them will be zero. In this case, we should not mark the zone
> congested.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch]vmscan: avoid set zone congested if no page dirty

[Minchan Kim]

On Thu, Nov 4, 2010 at 9:50 AM, Shaohua Li <shaohua.li@intel.com> wrote:
> nr_dirty and nr_congested are increased only when page is dirty. So if all pages
> are clean, both them will be zero. In this case, we should not mark the zone
> congested.
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>


-- 
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch]vmscan: avoid set zone congested if no page dirty

[Mel Gorman]

On Thu, Nov 04, 2010 at 08:50:58AM +0800, Shaohua Li wrote:
> nr_dirty and nr_congested are increased only when page is dirty. So if all pages
> are clean, both them will be zero. In this case, we should not mark the zone
> congested.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Acked-by: Mel Gorman <mel@csn.ul.ie>

> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index b8a6fdc..d31d7ce 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -913,7 +913,7 @@ keep_lumpy:
>  	 * back off and wait for congestion to clear because further reclaim
>  	 * will encounter the same problem
>  	 */
> -	if (nr_dirty == nr_congested)
> +	if (nr_dirty == nr_congested && nr_dirty != 0)
>  		zone_set_flag(zone, ZONE_CONGESTED);
>  
>  	free_page_list(&free_pages);
> 
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch]vmscan: avoid set zone congested if no page dirty

[Andrew Morton]

On Thu, 04 Nov 2010 08:50:58 +0800
Shaohua Li <shaohua.li@intel.com> wrote:

> nr_dirty and nr_congested are increased only when page is dirty. So if all pages
> are clean, both them will be zero. In this case, we should not mark the zone
> congested.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index b8a6fdc..d31d7ce 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -913,7 +913,7 @@ keep_lumpy:
>  	 * back off and wait for congestion to clear because further reclaim
>  	 * will encounter the same problem
>  	 */
> -	if (nr_dirty == nr_congested)
> +	if (nr_dirty == nr_congested && nr_dirty != 0)
>  		zone_set_flag(zone, ZONE_CONGESTED);
>  
>  	free_page_list(&free_pages);

In a way, this was a really big bug.  Reclaim will set the zone as
congested a *lot* - when reclaiming simple, clean pagecache.  It does
appear that kswapd will unset it a lot too, so the net effect isn't
obvious.

However most of the time, the atomic_read(&nr_bdi_congested[sync]) in
wait_iff_congested() will prevent this bug from causing harm.



btw, it's irritating that we have this asymmetry:

setter: zone_set_flag(zone, ZONE_CONGESTED)
getter: zone_is_reclaim_congested(zone)


--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch]vmscan: avoid set zone congested if no page dirty

[Mel Gorman]

On Wed, Nov 10, 2010 at 03:16:37PM -0800, Andrew Morton wrote:
> On Thu, 04 Nov 2010 08:50:58 +0800
> Shaohua Li <shaohua.li@intel.com> wrote:
> 
> > nr_dirty and nr_congested are increased only when page is dirty. So if all pages
> > are clean, both them will be zero. In this case, we should not mark the zone
> > congested.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index b8a6fdc..d31d7ce 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -913,7 +913,7 @@ keep_lumpy:
> >  	 * back off and wait for congestion to clear because further reclaim
> >  	 * will encounter the same problem
> >  	 */
> > -	if (nr_dirty == nr_congested)
> > +	if (nr_dirty == nr_congested && nr_dirty != 0)
> >  		zone_set_flag(zone, ZONE_CONGESTED);
> >  
> >  	free_page_list(&free_pages);
> 
> In a way, this was a really big bug.  Reclaim will set the zone as
> congested a *lot* - when reclaiming simple, clean pagecache. 

This is true and you're right, it was a bad mistake on my part. My test
machines are tied up at the moment but I intend to run this patch through
the same tests as were used to introduce wait_iff_congested to ensure nothing
bad has happened.

> It does
> appear that kswapd will unset it a lot too, so the net effect isn't
> obvious.
>

The unset log is more straight-forward. The flag is cleared when the watermark
is met. Granted, there might be still congestion in there but not congestion
that a called of alloc_pages() should use congestion_wait() for.

> However most of the time, the atomic_read(&nr_bdi_congested[sync]) in
> wait_iff_congested() will prevent this bug from causing harm.
> 

Bit of a happy coincidence though. You'd think that if the congestion
settting/clearing logic was perfect that nr_bdi_congested[] might become
unnecessary.

> btw, it's irritating that we have this asymmetry:
> 
> setter: zone_set_flag(zone, ZONE_CONGESTED)
> getter: zone_is_reclaim_congested(zone)
> 

I struggled with this. Early versions used a simple getter but I
eventually decided to match functions like zone_is_reclaim_locked() and
zone_is_oom_locked().

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>