[PATCH -mm 1/2] mm: vmscan: account slab pages on memcg reclaim

[PATCH -mm 1/2] mm: vmscan: account slab pages on memcg reclaim

[Vladimir Davydov]

Since try_to_free_mem_cgroup_pages() can now call slab shrinkers, we
should initialize reclaim_state and account reclaimed slab pages in
scan_control->nr_reclaimed.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/vmscan.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 16f3e45742d6..b2c041139a51 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -367,13 +367,16 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
  * the ->seeks setting of the shrink function, which indicates the
  * cost to recreate an object relative to that of an LRU page.
  *
- * Returns the number of reclaimed slab objects.
+ * Returns the number of reclaimed slab objects. The number of reclaimed
+ * pages is added to *@ret_nr_reclaimed.
  */
 static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
 				 struct mem_cgroup *memcg,
 				 unsigned long nr_scanned,
-				 unsigned long nr_eligible)
+				 unsigned long nr_eligible,
+				 unsigned long *ret_nr_reclaimed)
 {
+	struct reclaim_state *reclaim_state = current->reclaim_state;
 	struct shrinker *shrinker;
 	unsigned long freed = 0;
 
@@ -412,6 +415,10 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
 
 	up_read(&shrinker_rwsem);
 out:
+	if (reclaim_state) {
+		*ret_nr_reclaimed += reclaim_state->reclaimed_slab;
+		reclaim_state->reclaimed_slab = 0;
+	}
 	cond_resched();
 	return freed;
 }
@@ -419,6 +426,7 @@ out:
 void drop_slab_node(int nid)
 {
 	unsigned long freed;
+	unsigned long nr_reclaimed = 0;
 
 	do {
 		struct mem_cgroup *memcg = NULL;
@@ -426,7 +434,7 @@ void drop_slab_node(int nid)
 		freed = 0;
 		do {
 			freed += shrink_slab(GFP_KERNEL, nid, memcg,
-					     1000, 1000);
+					     1000, 1000, &nr_reclaimed);
 		} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL);
 	} while (freed > 10);
 }
@@ -2339,7 +2347,6 @@ static inline bool should_continue_reclaim(struct zone *zone,
 static bool shrink_zone(struct zone *zone, struct scan_control *sc,
 			bool is_classzone)
 {
-	struct reclaim_state *reclaim_state = current->reclaim_state;
 	unsigned long nr_reclaimed, nr_scanned;
 	bool reclaimable = false;
 
@@ -2371,7 +2378,7 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc,
 			if (memcg && is_classzone)
 				shrink_slab(sc->gfp_mask, zone_to_nid(zone),
 					    memcg, sc->nr_scanned - scanned,
-					    lru_pages);
+					    lru_pages, &sc->nr_reclaimed);
 
 			/*
 			 * Direct reclaim and kswapd have to scan all memory
@@ -2398,12 +2405,7 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc,
 		if (global_reclaim(sc) && is_classzone)
 			shrink_slab(sc->gfp_mask, zone_to_nid(zone), NULL,
 				    sc->nr_scanned - nr_scanned,
-				    zone_lru_pages);
-
-		if (reclaim_state) {
-			sc->nr_reclaimed += reclaim_state->reclaimed_slab;
-			reclaim_state->reclaimed_slab = 0;
-		}
+				    zone_lru_pages, &sc->nr_reclaimed);
 
 		vmpressure(sc->gfp_mask, sc->target_mem_cgroup,
 			   sc->nr_scanned - nr_scanned,
@@ -2865,6 +2867,9 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 		.may_unmap = 1,
 		.may_swap = may_swap,
 	};
+	struct reclaim_state reclaim_state = {
+		.reclaimed_slab = 0,
+	};
 
 	/*
 	 * Unlike direct reclaim via alloc_pages(), memcg's reclaim doesn't
@@ -2875,6 +2880,9 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 
 	zonelist = NODE_DATA(nid)->node_zonelists;
 
+	lockdep_set_current_reclaim_state(gfp_mask);
+	current->reclaim_state = &reclaim_state;
+
 	trace_mm_vmscan_memcg_reclaim_begin(0,
 					    sc.may_writepage,
 					    sc.gfp_mask);
@@ -2883,6 +2891,9 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 
 	trace_mm_vmscan_memcg_reclaim_end(nr_reclaimed);
 
+	current->reclaim_state = NULL;
+	lockdep_clear_current_reclaim_state();
+
 	return nr_reclaimed;
 }
 #endif
-- 
1.7.10.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH -mm 2/2] mm: vmscan: init reclaim_state in do_try_to_free_pages

[Vladimir Davydov]

All users of do_try_to_free_pages() want to have current->reclaim_state
set in order to account reclaimed slab pages. So instead of duplicating
the reclaim_state initialization code in each call site, let's do it
directly in do_try_to_free_pages().

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/page_alloc.c |    6 ------
 mm/vmscan.c     |   24 +++++++++---------------
 2 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e1963ea0684a..bdd43815c99a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2448,7 +2448,6 @@ static int
 __perform_reclaim(gfp_t gfp_mask, unsigned int order, struct zonelist *zonelist,
 		  nodemask_t *nodemask)
 {
-	struct reclaim_state reclaim_state;
 	int progress;
 
 	cond_resched();
@@ -2456,14 +2455,9 @@ __perform_reclaim(gfp_t gfp_mask, unsigned int order, struct zonelist *zonelist,
 	/* We now go into synchronous reclaim */
 	cpuset_memory_pressure_bump();
 	current->flags |= PF_MEMALLOC;
-	lockdep_set_current_reclaim_state(gfp_mask);
-	reclaim_state.reclaimed_slab = 0;
-	current->reclaim_state = &reclaim_state;
 
 	progress = try_to_free_pages(zonelist, order, gfp_mask, nodemask);
 
-	current->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
 	current->flags &= ~PF_MEMALLOC;
 
 	cond_resched();
diff --git a/mm/vmscan.c b/mm/vmscan.c
index b2c041139a51..0a9ddeb3b747 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2592,6 +2592,12 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 	unsigned long total_scanned = 0;
 	unsigned long writeback_threshold;
 	bool zones_reclaimable;
+	struct reclaim_state reclaim_state = {
+		.reclaimed_slab = 0,
+	};
+
+	lockdep_set_current_reclaim_state(sc->gfp_mask);
+	current->reclaim_state = &reclaim_state;
 
 	delayacct_freepages_start();
 
@@ -2635,6 +2641,9 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 
 	delayacct_freepages_end();
 
+	current->reclaim_state = NULL;
+	lockdep_clear_current_reclaim_state();
+
 	if (sc->nr_reclaimed)
 		return sc->nr_reclaimed;
 
@@ -2867,9 +2876,6 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 		.may_unmap = 1,
 		.may_swap = may_swap,
 	};
-	struct reclaim_state reclaim_state = {
-		.reclaimed_slab = 0,
-	};
 
 	/*
 	 * Unlike direct reclaim via alloc_pages(), memcg's reclaim doesn't
@@ -2880,9 +2886,6 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 
 	zonelist = NODE_DATA(nid)->node_zonelists;
 
-	lockdep_set_current_reclaim_state(gfp_mask);
-	current->reclaim_state = &reclaim_state;
-
 	trace_mm_vmscan_memcg_reclaim_begin(0,
 					    sc.may_writepage,
 					    sc.gfp_mask);
@@ -2891,9 +2894,6 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 
 	trace_mm_vmscan_memcg_reclaim_end(nr_reclaimed);
 
-	current->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
-
 	return nr_reclaimed;
 }
 #endif
@@ -3503,7 +3503,6 @@ void wakeup_kswapd(struct zone *zone, int order, enum zone_type classzone_idx)
  */
 unsigned long shrink_all_memory(unsigned long nr_to_reclaim)
 {
-	struct reclaim_state reclaim_state;
 	struct scan_control sc = {
 		.nr_to_reclaim = nr_to_reclaim,
 		.gfp_mask = GFP_HIGHUSER_MOVABLE,
@@ -3518,14 +3517,9 @@ unsigned long shrink_all_memory(unsigned long nr_to_reclaim)
 	unsigned long nr_reclaimed;
 
 	p->flags |= PF_MEMALLOC;
-	lockdep_set_current_reclaim_state(sc.gfp_mask);
-	reclaim_state.reclaimed_slab = 0;
-	p->reclaim_state = &reclaim_state;
 
 	nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
 
-	p->reclaim_state = NULL;
-	lockdep_clear_current_reclaim_state();
 	p->flags &= ~PF_MEMALLOC;
 
 	return nr_reclaimed;
-- 
1.7.10.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm 1/2] mm: vmscan: account slab pages on memcg reclaim

[Johannes Weiner]

On Mon, Jan 12, 2015 at 12:30:37PM +0300, Vladimir Davydov wrote:
> Since try_to_free_mem_cgroup_pages() can now call slab shrinkers, we
> should initialize reclaim_state and account reclaimed slab pages in
> scan_control->nr_reclaimed.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> ---
>  mm/vmscan.c |   33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 16f3e45742d6..b2c041139a51 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -367,13 +367,16 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
>   * the ->seeks setting of the shrink function, which indicates the
>   * cost to recreate an object relative to that of an LRU page.
>   *
> - * Returns the number of reclaimed slab objects.
> + * Returns the number of reclaimed slab objects. The number of reclaimed
> + * pages is added to *@ret_nr_reclaimed.
>
>  static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>  				 struct mem_cgroup *memcg,
>  				 unsigned long nr_scanned,
> -				 unsigned long nr_eligible)
> +				 unsigned long nr_eligible,
> +				 unsigned long *ret_nr_reclaimed)

Can't we just return the number of pages directly from this function?

> @@ -426,7 +434,7 @@ void drop_slab_node(int nid)
>  		freed = 0;
>  		do {
>  			freed += shrink_slab(GFP_KERNEL, nid, memcg,
> -					     1000, 1000);
> +					     1000, 1000, &nr_reclaimed);
>  		} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL);
>  	} while (freed > 10);

This is the only caller that cares about the return value, and it's a
magic number that could probably be changed to comparing with a magic
number of pages instead.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm 2/2] mm: vmscan: init reclaim_state in do_try_to_free_pages

[Johannes Weiner]

On Mon, Jan 12, 2015 at 12:30:38PM +0300, Vladimir Davydov wrote:
> All users of do_try_to_free_pages() want to have current->reclaim_state
> set in order to account reclaimed slab pages. So instead of duplicating
> the reclaim_state initialization code in each call site, let's do it
> directly in do_try_to_free_pages().

Couldn't this be contained in shrink_slab() directly?

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm 1/2] mm: vmscan: account slab pages on memcg reclaim

[Vladimir Davydov]

On Mon, Jan 12, 2015 at 05:18:39PM -0500, Johannes Weiner wrote:
> On Mon, Jan 12, 2015 at 12:30:37PM +0300, Vladimir Davydov wrote:
> > Since try_to_free_mem_cgroup_pages() can now call slab shrinkers, we
> > should initialize reclaim_state and account reclaimed slab pages in
> > scan_control->nr_reclaimed.
> > 
> > Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> > ---
> >  mm/vmscan.c |   33 ++++++++++++++++++++++-----------
> >  1 file changed, 22 insertions(+), 11 deletions(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 16f3e45742d6..b2c041139a51 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -367,13 +367,16 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> >   * the ->seeks setting of the shrink function, which indicates the
> >   * cost to recreate an object relative to that of an LRU page.
> >   *
> > - * Returns the number of reclaimed slab objects.
> > + * Returns the number of reclaimed slab objects. The number of reclaimed
> > + * pages is added to *@ret_nr_reclaimed.
> >
> >  static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
> >  				 struct mem_cgroup *memcg,
> >  				 unsigned long nr_scanned,
> > -				 unsigned long nr_eligible)
> > +				 unsigned long nr_eligible,
> > +				 unsigned long *ret_nr_reclaimed)
> 
> Can't we just return the number of pages directly from this function?

Hmm, we can. That would look better, of course. However, reclaimed_slab
can be 0 even if we reclaimed tons of dentries/inodes, simply because
they are freed by rcu. In this case, we can abort drop_slab beforehand.
Do you think it's OK?

Thanks,
Vladimir

> 
> > @@ -426,7 +434,7 @@ void drop_slab_node(int nid)
> >  		freed = 0;
> >  		do {
> >  			freed += shrink_slab(GFP_KERNEL, nid, memcg,
> > -					     1000, 1000);
> > +					     1000, 1000, &nr_reclaimed);
> >  		} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL);
> >  	} while (freed > 10);
> 
> This is the only caller that cares about the return value, and it's a
> magic number that could probably be changed to comparing with a magic
> number of pages instead.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm 2/2] mm: vmscan: init reclaim_state in do_try_to_free_pages

[Vladimir Davydov]

On Mon, Jan 12, 2015 at 05:26:34PM -0500, Johannes Weiner wrote:
> On Mon, Jan 12, 2015 at 12:30:38PM +0300, Vladimir Davydov wrote:
> > All users of do_try_to_free_pages() want to have current->reclaim_state
> > set in order to account reclaimed slab pages. So instead of duplicating
> > the reclaim_state initialization code in each call site, let's do it
> > directly in do_try_to_free_pages().
> 
> Couldn't this be contained in shrink_slab() directly?

I had considered this possibility, but finally rejected it, because

 - some slab pages can be reclaimed from shrink_lruvec (e.g.
   buffer_head's); there shouldn't be too many of them though

 - struct reclaim_state looks to me as a generic placeholder for lots of
   reclaim-related stuff, though currently it is only used for counting
   reclaimed slab pages, so IMO it should be initialized before starting
   reclaim

Both arguments are not rock-solid as you can see, so if you think we can
neglect them, I'll do.

Thanks,
Vladimir

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm 1/2] mm: vmscan: account slab pages on memcg reclaim

[Michal Hocko]

On Mon 12-01-15 12:30:37, Vladimir Davydov wrote:
> Since try_to_free_mem_cgroup_pages() can now call slab shrinkers, we
> should initialize reclaim_state and account reclaimed slab pages in
> scan_control->nr_reclaimed.

I am sorry, I didn't get to this one yet. As pointed out in othere email
(http://marc.info/?l=linux-mm&m=142132670609578&w=2) reclaim_state might
catch unrelated pages freed from slab. I do not like expanding its usage
for memcg.

> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> ---
>  mm/vmscan.c |   33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 16f3e45742d6..b2c041139a51 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -367,13 +367,16 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
>   * the ->seeks setting of the shrink function, which indicates the
>   * cost to recreate an object relative to that of an LRU page.
>   *
> - * Returns the number of reclaimed slab objects.
> + * Returns the number of reclaimed slab objects. The number of reclaimed
> + * pages is added to *@ret_nr_reclaimed.
>   */
>  static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>  				 struct mem_cgroup *memcg,
>  				 unsigned long nr_scanned,
> -				 unsigned long nr_eligible)
> +				 unsigned long nr_eligible,
> +				 unsigned long *ret_nr_reclaimed)
>  {
> +	struct reclaim_state *reclaim_state = current->reclaim_state;
>  	struct shrinker *shrinker;
>  	unsigned long freed = 0;
>  
> @@ -412,6 +415,10 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>  
>  	up_read(&shrinker_rwsem);
>  out:
> +	if (reclaim_state) {
> +		*ret_nr_reclaimed += reclaim_state->reclaimed_slab;
> +		reclaim_state->reclaimed_slab = 0;
> +	}
>  	cond_resched();
>  	return freed;
>  }
> @@ -419,6 +426,7 @@ out:
>  void drop_slab_node(int nid)
>  {
>  	unsigned long freed;
> +	unsigned long nr_reclaimed = 0;
>  
>  	do {
>  		struct mem_cgroup *memcg = NULL;
> @@ -426,7 +434,7 @@ void drop_slab_node(int nid)
>  		freed = 0;
>  		do {
>  			freed += shrink_slab(GFP_KERNEL, nid, memcg,
> -					     1000, 1000);
> +					     1000, 1000, &nr_reclaimed);
>  		} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL);
>  	} while (freed > 10);
>  }
> @@ -2339,7 +2347,6 @@ static inline bool should_continue_reclaim(struct zone *zone,
>  static bool shrink_zone(struct zone *zone, struct scan_control *sc,
>  			bool is_classzone)
>  {
> -	struct reclaim_state *reclaim_state = current->reclaim_state;
>  	unsigned long nr_reclaimed, nr_scanned;
>  	bool reclaimable = false;
>  
> @@ -2371,7 +2378,7 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc,
>  			if (memcg && is_classzone)
>  				shrink_slab(sc->gfp_mask, zone_to_nid(zone),
>  					    memcg, sc->nr_scanned - scanned,
> -					    lru_pages);
> +					    lru_pages, &sc->nr_reclaimed);
>  
>  			/*
>  			 * Direct reclaim and kswapd have to scan all memory
> @@ -2398,12 +2405,7 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc,
>  		if (global_reclaim(sc) && is_classzone)
>  			shrink_slab(sc->gfp_mask, zone_to_nid(zone), NULL,
>  				    sc->nr_scanned - nr_scanned,
> -				    zone_lru_pages);
> -
> -		if (reclaim_state) {
> -			sc->nr_reclaimed += reclaim_state->reclaimed_slab;
> -			reclaim_state->reclaimed_slab = 0;
> -		}
> +				    zone_lru_pages, &sc->nr_reclaimed);
>  
>  		vmpressure(sc->gfp_mask, sc->target_mem_cgroup,
>  			   sc->nr_scanned - nr_scanned,
> @@ -2865,6 +2867,9 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
>  		.may_unmap = 1,
>  		.may_swap = may_swap,
>  	};
> +	struct reclaim_state reclaim_state = {
> +		.reclaimed_slab = 0,
> +	};
>  
>  	/*
>  	 * Unlike direct reclaim via alloc_pages(), memcg's reclaim doesn't
> @@ -2875,6 +2880,9 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
>  
>  	zonelist = NODE_DATA(nid)->node_zonelists;
>  
> +	lockdep_set_current_reclaim_state(gfp_mask);
> +	current->reclaim_state = &reclaim_state;
> +
>  	trace_mm_vmscan_memcg_reclaim_begin(0,
>  					    sc.may_writepage,
>  					    sc.gfp_mask);
> @@ -2883,6 +2891,9 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
>  
>  	trace_mm_vmscan_memcg_reclaim_end(nr_reclaimed);
>  
> +	current->reclaim_state = NULL;
> +	lockdep_clear_current_reclaim_state();
> +
>  	return nr_reclaimed;
>  }
>  #endif
> -- 
> 1.7.10.4
> 

-- 
Michal Hocko
SUSE Labs

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>