linux-kernel - Re: [PATCH 5/5] vmscan: Take order into consideration when deciding if kswapd is in trouble

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 14 Nov 2009 03:00:57 +0900 (JST)
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	Mel Gorman <mel@....ul.ie>
Cc:	kosaki.motohiro@...fujitsu.com,
	Andrew Morton <akpm@...ux-foundation.org>,
	Frans Pop <elendil@...net.nl>, Jiri Kosina <jkosina@...e.cz>,
	Sven Geggus <lists@...hsschwanzdomain.de>,
	Karol Lewandowski <karol.k.lewandowski@...il.com>,
	Tobias Oetiker <tobi@...iker.ch>, linux-kernel@...r.kernel.org,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Pekka Enberg <penberg@...helsinki.fi>,
	Rik van Riel <riel@...hat.com>,
	Christoph Lameter <cl@...ux-foundation.org>,
	Stephan von Krawczynski <skraw@...net.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Kernel Testers List <kernel-testers@...r.kernel.org>
Subject: Re: [PATCH 5/5] vmscan: Take order into consideration when deciding if kswapd is in trouble


> This makes a lot of sense. Tests look good and I added stats to make sure
> the logic was triggering. On X86, kswapd avoided a congestion_wait 11723
> times and X86-64 avoided it 5084 times. I think we should hold onto the
> stats temporarily until all these bugs are ironed out.
> 
> Would you like to sign off the following?
> 
> If you are ok to sign off, this patch should replace my patch 5 in
> the series.

I'm sorry, I found my bug.
Please see below.

> 
> ==== CUT HERE ====
> 
> vmscan: Stop kswapd waiting on congestion when the min watermark is not being met
> 
> If reclaim fails to make sufficient progress, the priority is raised.
> Once the priority is higher, kswapd starts waiting on congestion.  However,
> if the zone is below the min watermark then kswapd needs to continue working
> without delay as there is a danger of an increased rate of GFP_ATOMIC
> allocation failure.
> 
> This patch changes the conditions under which kswapd waits on
> congestion by only going to sleep if the min watermarks are being met.
> 
> [mel@....ul.ie: Add stats to track how relevant the logic is]
> Needs-signed-off-by-original-author
> 
> diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
> index 9716003..7d66695 100644
> --- a/include/linux/vmstat.h
> +++ b/include/linux/vmstat.h
> @@ -41,6 +41,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>  #endif
>  		PGINODESTEAL, SLABS_SCANNED, KSWAPD_STEAL, KSWAPD_INODESTEAL,
>  		KSWAPD_PREMATURE_FAST, KSWAPD_PREMATURE_SLOW,
> +		KSWAPD_NO_CONGESTION_WAIT,
>  		PAGEOUTRUN, ALLOCSTALL, PGROTATED,
>  #ifdef CONFIG_HUGETLB_PAGE
>  		HTLB_BUDDY_PGALLOC, HTLB_BUDDY_PGALLOC_FAIL,
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index ffa1766..70967e1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1966,6 +1966,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order)
>  	 * free_pages == high_wmark_pages(zone).
>  	 */
>  	int temp_priority[MAX_NR_ZONES];
> +	int has_under_min_watermark_zone = 0;

This is wrong declaration place.  It must change to

        for (priority = DEF_PRIORITY; priority >= 0; priority--) {
                int end_zone = 0;       /* Inclusive.  0 = ZONE_DMA */
                unsigned long lru_pages = 0;
+                int has_under_min_watermark_zone = 0;


because, has_under_min_watermark_zone should be initialized every priority.


>  loop_again:
>  	total_scanned = 0;
> @@ -2085,6 +2086,15 @@ loop_again:
>  			if (total_scanned > SWAP_CLUSTER_MAX * 2 &&
>  			    total_scanned > sc.nr_reclaimed + sc.nr_reclaimed / 2)
>  				sc.may_writepage = 1;
> +
> +			/*
> +			 * We are still under min water mark. it mean we have
> +			 * GFP_ATOMIC allocation failure risk. Hurry up!
> +			 */
> +			if (!zone_watermark_ok(zone, order, min_wmark_pages(zone),
> +					      end_zone, 0))
> +				has_under_min_watermark_zone = 1;
> +
>  		}
>  		if (all_zones_ok)
>  			break;		/* kswapd: all done */
> @@ -2092,8 +2102,13 @@ loop_again:
>  		 * OK, kswapd is getting into trouble.  Take a nap, then take
>  		 * another pass across the zones.
>  		 */
> -		if (total_scanned && priority < DEF_PRIORITY - 2)
> -			congestion_wait(BLK_RW_ASYNC, HZ/10);
> +		if (total_scanned && (priority < DEF_PRIORITY - 2)) {
> +

This blank line is unnecesary.

> +			if (!has_under_min_watermark_zone)

Probably "if (has_under_min_watermark_zone)" is correct.


> +				count_vm_event(KSWAPD_NO_CONGESTION_WAIT);
> +			else
> +				congestion_wait(BLK_RW_ASYNC, HZ/10);
> +		}

Otherthing looks pretty good to me. please feel free to add my s-o-b or reviewed-by.

Thanks.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/