linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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 v3 3/9] mm/workingset: extend the workingset detection for anon LRU
Date: Wed, 18 Mar 2020 14:06:38 -0400	[thread overview]
Message-ID: <20200318180638.GC154135@cmpxchg.org> (raw)
In-Reply-To: <1584423717-3440-4-git-send-email-iamjoonsoo.kim@lge.com>

On Tue, Mar 17, 2020 at 02:41:51PM +0900, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> In the following patch, workingset detection will be applied to
> anonymous LRU. To prepare it, this patch adds some code to
> distinguish/handle the both LRUs.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

This looks all correct to me, but I would ask for some style
fixes. With those applied, please feel free to add

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

> @@ -304,10 +308,10 @@ enum lruvec_flags {
>  struct lruvec {
>  	struct list_head		lists[NR_LRU_LISTS];
>  	struct zone_reclaim_stat	reclaim_stat;
> -	/* Evictions & activations on the inactive file list */
> -	atomic_long_t			inactive_age;
> +	/* Evictions & activations on the inactive list */
> +	atomic_long_t			inactive_age[2];

Can you please add anon=0, file=1 to the comment?

> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1431,10 +1431,14 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
>  	seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGMAJFAULT),
>  		       memcg_events(memcg, PGMAJFAULT));
>  
> -	seq_buf_printf(&s, "workingset_refault %lu\n",
> -		       memcg_page_state(memcg, WORKINGSET_REFAULT));
> -	seq_buf_printf(&s, "workingset_activate %lu\n",
> -		       memcg_page_state(memcg, WORKINGSET_ACTIVATE));
> +	seq_buf_printf(&s, "workingset_refault_anon %lu\n",
> +		       memcg_page_state(memcg, WORKINGSET_REFAULT_ANON));
> +	seq_buf_printf(&s, "workingset_refault_file %lu\n",
> +		       memcg_page_state(memcg, WORKINGSET_REFAULT_FILE));
> +	seq_buf_printf(&s, "workingset_activate_anon %lu\n",
> +		       memcg_page_state(memcg, WORKINGSET_ACTIVATE_ANON));
> +	seq_buf_printf(&s, "workingset_activate_file %lu\n",
> +		       memcg_page_state(memcg, WORKINGSET_ACTIVATE_FILE));
>  	seq_buf_printf(&s, "workingset_nodereclaim %lu\n",
>  		       memcg_page_state(memcg, WORKINGSET_NODERECLAIM));
>  
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c932141..0493c25 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2716,7 +2716,10 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
>  	if (!sc->force_deactivate) {
>  		unsigned long refaults;
>  
> -		if (inactive_is_low(target_lruvec, LRU_INACTIVE_ANON))
> +		refaults = lruvec_page_state(target_lruvec,
> +				WORKINGSET_ACTIVATE_ANON);
> +		if (refaults != target_lruvec->refaults[0] ||
> +			inactive_is_low(target_lruvec, LRU_INACTIVE_ANON))
>  			sc->may_deactivate |= DEACTIVATE_ANON;
>  		else
>  			sc->may_deactivate &= ~DEACTIVATE_ANON;
> @@ -2727,8 +2730,8 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
>  		 * rid of any stale active pages quickly.
>  		 */
>  		refaults = lruvec_page_state(target_lruvec,
> -					     WORKINGSET_ACTIVATE);
> -		if (refaults != target_lruvec->refaults ||
> +				WORKINGSET_ACTIVATE_FILE);
> +		if (refaults != target_lruvec->refaults[1] ||
>  		    inactive_is_low(target_lruvec, LRU_INACTIVE_FILE))
>  			sc->may_deactivate |= DEACTIVATE_FILE;
>  		else
> @@ -3007,8 +3010,10 @@ static void snapshot_refaults(struct mem_cgroup *target_memcg, pg_data_t *pgdat)
>  	unsigned long refaults;
>  
>  	target_lruvec = mem_cgroup_lruvec(target_memcg, pgdat);
> -	refaults = lruvec_page_state(target_lruvec, WORKINGSET_ACTIVATE);
> -	target_lruvec->refaults = refaults;
> +	refaults = lruvec_page_state(target_lruvec, WORKINGSET_ACTIVATE_ANON);
> +	target_lruvec->refaults[0] = refaults;
> +	refaults = lruvec_page_state(target_lruvec, WORKINGSET_ACTIVATE_FILE);
> +	target_lruvec->refaults[1] = refaults;
>  }
>  
>  /*
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 78d5337..3cdf8e9 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1146,8 +1146,10 @@ const char * const vmstat_text[] = {
>  	"nr_isolated_anon",
>  	"nr_isolated_file",
>  	"workingset_nodes",
> -	"workingset_refault",
> -	"workingset_activate",
> +	"workingset_refault_anon",
> +	"workingset_refault_file",
> +	"workingset_activate_anon",
> +	"workingset_activate_file",
>  	"workingset_restore",
>  	"workingset_nodereclaim",
>  	"nr_anon_pages",
> diff --git a/mm/workingset.c b/mm/workingset.c
> index 474186b..5fb8f85 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -15,6 +15,7 @@
>  #include <linux/dax.h>
>  #include <linux/fs.h>
>  #include <linux/mm.h>
> +#include <linux/mm_inline.h>

Please keep the line-length sorting of the includes intact. If that's
not possible, please separate the mm_inline.h include with an empty
line into its own section.

This makes it much easier to scan for specific headers, helps avoid
duplicate includes, and is generally just easier on the eyes.

> @@ -156,7 +157,7 @@
>   *
>   *		Implementation
>   *
> - * For each node's file LRU lists, a counter for inactive evictions
> + * For each node's anon/file LRU lists, a counter for inactive evictions
>   * and activations is maintained (node->inactive_age).
>   *
>   * On eviction, a snapshot of this counter (along with some bits to
> @@ -213,7 +214,8 @@ static void unpack_shadow(void *shadow, int *memcgidp, pg_data_t **pgdat,
>  	*workingsetp = workingset;
>  }
>  
> -static void advance_inactive_age(struct mem_cgroup *memcg, pg_data_t *pgdat)
> +static void advance_inactive_age(struct mem_cgroup *memcg, pg_data_t *pgdat,
> +				int is_file)

bool file, please.

>  {
>  	/*
>  	 * Reclaiming a cgroup means reclaiming all its children in a
> @@ -230,7 +232,7 @@ static void advance_inactive_age(struct mem_cgroup *memcg, pg_data_t *pgdat)
>  		struct lruvec *lruvec;
>  
>  		lruvec = mem_cgroup_lruvec(memcg, pgdat);
> -		atomic_long_inc(&lruvec->inactive_age);
> +		atomic_long_inc(&lruvec->inactive_age[is_file]);
>  	} while (memcg && (memcg = parent_mem_cgroup(memcg)));
>  }
>  
> @@ -248,18 +250,19 @@ void *workingset_eviction(struct page *page, struct mem_cgroup *target_memcg)
>  	unsigned long eviction;
>  	struct lruvec *lruvec;
>  	int memcgid;
> +	int is_file = page_is_file_cache(page);

Please keep the line-length ordering intact here as well.

>  	/* Page is fully exclusive and pins page->mem_cgroup */
>  	VM_BUG_ON_PAGE(PageLRU(page), page);
>  	VM_BUG_ON_PAGE(page_count(page), page);
>  	VM_BUG_ON_PAGE(!PageLocked(page), page);
>  
> -	advance_inactive_age(page_memcg(page), pgdat);
> +	advance_inactive_age(page_memcg(page), pgdat, is_file);
>  
>  	lruvec = mem_cgroup_lruvec(target_memcg, pgdat);
>  	/* XXX: target_memcg can be NULL, go through lruvec */
>  	memcgid = mem_cgroup_id(lruvec_memcg(lruvec));
> -	eviction = atomic_long_read(&lruvec->inactive_age);
> +	eviction = atomic_long_read(&lruvec->inactive_age[is_file]);
>  	return pack_shadow(memcgid, pgdat, eviction, PageWorkingset(page));
>  }
>  
> @@ -278,13 +281,16 @@ void workingset_refault(struct page *page, void *shadow)
>  	struct lruvec *eviction_lruvec;
>  	unsigned long refault_distance;
>  	struct pglist_data *pgdat;
> -	unsigned long active_file;
> +	unsigned long active;
>  	struct mem_cgroup *memcg;
>  	unsigned long eviction;
>  	struct lruvec *lruvec;
>  	unsigned long refault;
>  	bool workingset;
>  	int memcgid;
> +	int is_file = page_is_file_cache(page);
> +	enum lru_list active_lru = page_lru_base_type(page) + LRU_ACTIVE;
> +	enum node_stat_item workingset_stat;

Please use bool file and keep line-length ordering.

The workingset_stat variable doesn't seem helpful, see below:

>  	unpack_shadow(shadow, &memcgid, &pgdat, &eviction, &workingset);
>  
> @@ -309,8 +315,8 @@ void workingset_refault(struct page *page, void *shadow)
>  	if (!mem_cgroup_disabled() && !eviction_memcg)
>  		goto out;
>  	eviction_lruvec = mem_cgroup_lruvec(eviction_memcg, pgdat);
> -	refault = atomic_long_read(&eviction_lruvec->inactive_age);
> -	active_file = lruvec_page_state(eviction_lruvec, NR_ACTIVE_FILE);
> +	refault = atomic_long_read(&eviction_lruvec->inactive_age[is_file]);
> +	active = lruvec_page_state(eviction_lruvec, active_lru);
>  
>  	/*
>  	 * Calculate the refault distance
> @@ -341,19 +347,21 @@ void workingset_refault(struct page *page, void *shadow)
>  	memcg = page_memcg(page);
>  	lruvec = mem_cgroup_lruvec(memcg, pgdat);
>  
> -	inc_lruvec_state(lruvec, WORKINGSET_REFAULT);
> +	workingset_stat = WORKINGSET_REFAULT_BASE + is_file;
> +	inc_lruvec_state(lruvec, workingset_stat);

	inc_lruvec_state(lruvec, WORKINGSET_REFAULT_BASE + file);

>  	/*
>  	 * Compare the distance to the existing workingset size. We
>  	 * don't act on pages that couldn't stay resident even if all
>  	 * the memory was available to the page cache.
>  	 */
> -	if (refault_distance > active_file)
> +	if (refault_distance > active)
>  		goto out;
>  
>  	SetPageActive(page);
> -	advance_inactive_age(memcg, pgdat);
> -	inc_lruvec_state(lruvec, WORKINGSET_ACTIVATE);
> +	advance_inactive_age(memcg, pgdat, is_file);
> +	workingset_stat = WORKINGSET_ACTIVATE_BASE + is_file;
> +	inc_lruvec_state(lruvec, workingset_stat);

	inc_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE + file);

>  	/* Page was active prior to eviction */
>  	if (workingset) {
> @@ -371,6 +379,7 @@ void workingset_refault(struct page *page, void *shadow)
>  void workingset_activation(struct page *page)
>  {
>  	struct mem_cgroup *memcg;
> +	int is_file = page_is_file_cache(page);

bool file & length-ordering

  reply	other threads:[~2020-03-18 18:06 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-17  5:41 [PATCH v3 0/9] workingset protection/detection on the anonymous LRU list js1304
2020-03-17  5:41 ` [PATCH v3 1/9] mm/vmscan: make active/inactive ratio as 1:1 for anon lru js1304
2020-03-18 17:45   ` Johannes Weiner
2020-03-17  5:41 ` [PATCH v3 2/9] mm/vmscan: protect the workingset on anonymous LRU js1304
2020-03-18 17:51   ` Johannes Weiner
2020-03-19  4:01     ` Joonsoo Kim
2020-03-17  5:41 ` [PATCH v3 3/9] mm/workingset: extend the workingset detection for anon LRU js1304
2020-03-18 18:06   ` Johannes Weiner [this message]
2020-03-19  4:13     ` Joonsoo Kim
2020-03-17  5:41 ` [PATCH v3 4/9] mm/swapcache: support to handle the value in swapcache js1304
2020-03-18 18:33   ` Johannes Weiner
2020-03-19  6:01     ` Joonsoo Kim
2020-03-17  5:41 ` [PATCH v3 5/9] mm/workingset: use the node counter if memcg is the root memcg js1304
2020-03-18 19:18   ` Johannes Weiner
2020-03-19  6:20     ` Joonsoo Kim
2020-03-17  5:41 ` [PATCH v3 6/9] mm/workingset: handle the page without memcg js1304
2020-03-18 19:59   ` Johannes Weiner
2020-03-19  8:31     ` Joonsoo Kim
2020-03-17  5:41 ` [PATCH v3 7/9] mm/swap: implement workingset detection for anonymous LRU js1304
2020-03-17  5:41 ` [PATCH v3 8/9] mm/vmscan: restore active/inactive ratio " js1304
2020-03-17  5:41 ` [PATCH v3 9/9] mm/swap: count a new anonymous page as a reclaim_state's rotate js1304

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200318180638.GC154135@cmpxchg.org \
    --to=hannes@cmpxchg.org \
    --cc=akpm@linux-foundation.org \
    --cc=hughd@google.com \
    --cc=iamjoonsoo.kim@lge.com \
    --cc=js1304@gmail.com \
    --cc=kernel-team@lge.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@techsingularity.net \
    --cc=mhocko@kernel.org \
    --cc=minchan@kernel.org \
    --cc=vbabka@suse.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).