linux-ext4 - Re: [PATCH 2/3] mm: Ensure that mark_page_accessed moves pages to the active list

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]

Message-ID: <5180AB0E.6030407@gmail.com>
Date:	Wed, 01 May 2013 13:41:34 +0800
From:	Sam Ben <sam.bennn@...il.com>
To:	Mel Gorman <mgorman@...e.de>
CC:	Alexey Lyahkov <alexey.lyashkov@...il.com>,
	Andrew Perepechko <anserper@...ru>,
	Robin Dong <sanbai@...bao.com>, Theodore Tso <tytso@....edu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Hugh Dickins <hughd@...gle.com>,
	Rik van Riel <riel@...hat.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Bernd Schubert <bernd.schubert@...tmail.fm>,
	David Howells <dhowells@...hat.com>,
	Trond Myklebust <Trond.Myklebust@...app.com>,
	Linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Linux-ext4 <linux-ext4@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux-mm <linux-mm@...ck.org>
Subject: Re: [PATCH 2/3] mm: Ensure that mark_page_accessed moves pages to
 the active list

Hi Mel,
On 04/30/2013 12:31 AM, Mel Gorman wrote:
> If a page is on a pagevec then it is !PageLRU and mark_page_accessed()
> may fail to move a page to the active list as expected. Now that the
> LRU is selected at LRU drain time, mark pages PageActive if they are
> on a pagevec so it gets moved to the correct list at LRU drain time.
> Using a debugging patch it was found that for a simple git checkout
> based workload that pages were never added to the active file list in

Could you show us the details of your workload?

> practice but with this patch applied they are.
>
> 				before   after
> LRU Add Active File                  0  757121
> LRU Add Active Anon            2678833 2633924
> LRU Add Inactive File          8821711 8085543
> LRU Add Inactive Anon              183     200
>
> The question to consider is if this is universally safe. If the page
> was isolated for reclaim and there is a parallel mark_page_accessed()
> then vmscan.c will get upset when it finds an isolated PageActive page.
> Similarly a potential race exists between a per-cpu drain on a pagevec
> list and an activation on a remote CPU.
>
> 				lru_add_drain_cpu
> 				__pagevec_lru_add
> 				  lru = page_lru(page);
> mark_page_accessed
>    if (PageLRU(page))
>      activate_page
>    else
>      SetPageActive
> 				  SetPageLRU(page);
> 				  add_page_to_lru_list(page, lruvec, lru);
>
> A PageActive page is now added to the inactivate list.
>
> While this looks strange, I think it is sufficiently harmless that additional
> barriers to address the case is not justified.  Unfortunately, while I never
> witnessed it myself, these parallel updates potentially trigger defensive
> DEBUG_VM checks on PageActive and hence they are removed by this patch.
>
> Signed-off-by: Mel Gorman <mgorman@...e.de>
> ---
>   mm/swap.c   | 18 ++++++++++++------
>   mm/vmscan.c |  3 ---
>   2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/mm/swap.c b/mm/swap.c
> index 80fbc37..2a10d08 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -437,8 +437,17 @@ void activate_page(struct page *page)
>   void mark_page_accessed(struct page *page)
>   {
>   	if (!PageActive(page) && !PageUnevictable(page) &&
> -			PageReferenced(page) && PageLRU(page)) {
> -		activate_page(page);
> +			PageReferenced(page)) {
> +
> +		/*
> +		 * If the page is on the LRU, promote immediately. Otherwise,
> +		 * assume the page is on a pagevec, mark it active and it'll
> +		 * be moved to the active LRU on the next drain
> +		 */
> +		if (PageLRU(page))
> +			activate_page(page);
> +		else
> +			SetPageActive(page);
>   		ClearPageReferenced(page);
>   	} else if (!PageReferenced(page)) {
>   		SetPageReferenced(page);
> @@ -478,11 +487,8 @@ EXPORT_SYMBOL(__lru_cache_add);
>    */
>   void lru_cache_add_lru(struct page *page, enum lru_list lru)
>   {
> -	if (PageActive(page)) {
> +	if (PageActive(page))
>   		VM_BUG_ON(PageUnevictable(page));
> -	} else if (PageUnevictable(page)) {
> -		VM_BUG_ON(PageActive(page));
> -	}
>   
>   	VM_BUG_ON(PageLRU(page));
>   	__lru_cache_add(page, lru);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 88c5fed..751b897 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -704,7 +704,6 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>   		if (!trylock_page(page))
>   			goto keep;
>   
> -		VM_BUG_ON(PageActive(page));
>   		VM_BUG_ON(page_zone(page) != zone);
>   
>   		sc->nr_scanned++;
> @@ -935,7 +934,6 @@ activate_locked:
>   		/* Not a candidate for swapping, so reclaim swap space. */
>   		if (PageSwapCache(page) && vm_swap_full())
>   			try_to_free_swap(page);
> -		VM_BUG_ON(PageActive(page));
>   		SetPageActive(page);
>   		pgactivate++;
>   keep_locked:
> @@ -3488,7 +3486,6 @@ void check_move_unevictable_pages(struct page **pages, int nr_pages)
>   		if (page_evictable(page)) {
>   			enum lru_list lru = page_lru_base_type(page);
>   
> -			VM_BUG_ON(PageActive(page));
>   			ClearPageUnevictable(page);
>   			del_page_from_lru_list(page, lruvec, LRU_UNEVICTABLE);
>   			add_page_to_lru_list(page, lruvec, lru);

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