Re: [PATCH v2 2/9] mm/vmscan: protect the workingset on anonymous LRU

From: Johannes Weiner <hannes@cmpxchg.org>
To: js1304@gmail.com
Cc: Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Michal Hocko <mhocko@kernel.org>, Hugh Dickins <hughd@google.com>,
	Minchan Kim <minchan@kernel.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	Mel Gorman <mgorman@techsingularity.net>,
	kernel-team@lge.com, Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH v2 2/9] mm/vmscan: protect the workingset on anonymous LRU
Date: Thu, 12 Mar 2020 11:14:23 -0400	[thread overview]
Message-ID: <20200312151423.GH29835@cmpxchg.org> (raw)
In-Reply-To: <1582175513-22601-3-git-send-email-iamjoonsoo.kim@lge.com>

On Thu, Feb 20, 2020 at 02:11:46PM +0900, js1304@gmail.com wrote:
> @@ -1010,8 +1010,15 @@ static enum page_references page_check_references(struct page *page,
>  		return PAGEREF_RECLAIM;
>  
>  	if (referenced_ptes) {
> -		if (PageSwapBacked(page))
> -			return PAGEREF_ACTIVATE;
> +		if (PageSwapBacked(page)) {
> +			if (referenced_page) {
> +				ClearPageReferenced(page);
> +				return PAGEREF_ACTIVATE;
> +			}

This looks odd to me. referenced_page = TestClearPageReferenced()
above, so it's already be clear. Why clear it again?

> +
> +			SetPageReferenced(page);
> +			return PAGEREF_KEEP;
> +		}

The existing file code already does:

		SetPageReferenced(page);
		if (referenced_page || referenced_ptes > 1)
			return PAGEREF_ACTIVATE;
		if (vm_flags & VM_EXEC)
			return PAGEREF_ACTIVATE;
		return PAGEREF_KEEP;

The differences are:

1) referenced_ptes > 1. We did this so that heavily shared file
mappings are protected a bit better than others. Arguably the same
could apply for anon pages when we put them on the inactive list.

2) vm_flags & VM_EXEC. This mostly doesn't apply to anon pages. The
exception would be jit code pages, but if we put anon pages on the
inactive list we should protect jit code the same way we protect file
executables.

Seems to me you don't need to add anything. Just remove the
PageSwapBacked branch and apply equal treatment to both types.

> @@ -2056,6 +2063,15 @@ static void shrink_active_list(unsigned long nr_to_scan,
>  			}
>  		}
>  
> +		/*
> +		 * Now, newly created anonymous page isn't appened to the
> +		 * active list. We don't need to clear the reference bit here.
> +		 */
> +		if (PageSwapBacked(page)) {
> +			ClearPageReferenced(page);
> +			goto deactivate;
> +		}

I don't understand this.

If you don't clear the pte references, you're leaving behind stale
data. You already decide here that we consider the page referenced
when it reaches the end of the inactive list, regardless of what
happens in between. That makes the deactivation kind of useless.

And it blurs the lines between the inactive and active list.

shrink_page_list() (and page_check_references()) are written with the
notion that any references they look at are from the inactive list. If
you carry over stale data, this can cause more subtle bugs later on.

And again, I don't quite understand why anon would need different
treatment here than file.

> +
>  		if (page_referenced(page, 0, sc->target_mem_cgroup,
>  				    &vm_flags)) {
>  			nr_rotated += hpage_nr_pages(page);
> @@ -2074,6 +2090,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
>  			}
>  		}
>  
> +deactivate:
>  		ClearPageActive(page);	/* we are de-activating */
>  		SetPageWorkingset(page);
>  		list_add(&page->lru, &l_inactive);
> -- 
> 2.7.4
> 