[PATCH] mm: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[PATCH] mm: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[Roman Gushchin]

Historically, PGSTEAL*/PGSCAN*/ALLOCSTALL counters were used to
account only for global reclaim events, memory cgroup targeted reclaim
was ignored.

It doesn't make sense anymore, because the whole reclaim path
is designed around cgroups. Also, per-cgroup counters can exceed the
corresponding global counters, what can be confusing.

So, make PGSTEAL*/PGSCAN*/ALLOCSTALL counters reflect sum of any
reclaim activity in the system.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: kernel-team@fb.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/vmscan.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7c2a36b..77253b1 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1765,13 +1765,11 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	reclaim_stat->recent_scanned[file] += nr_taken;
 
 	if (current_is_kswapd()) {
-		if (global_reclaim(sc))
-			__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
+		__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
 		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_KSWAPD,
 				   nr_scanned);
 	} else {
-		if (global_reclaim(sc))
-			__count_vm_events(PGSCAN_DIRECT, nr_scanned);
+		__count_vm_events(PGSCAN_DIRECT, nr_scanned);
 		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_DIRECT,
 				   nr_scanned);
 	}
@@ -1786,13 +1784,11 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	spin_lock_irq(&pgdat->lru_lock);
 
 	if (current_is_kswapd()) {
-		if (global_reclaim(sc))
-			__count_vm_events(PGSTEAL_KSWAPD, nr_reclaimed);
+		__count_vm_events(PGSTEAL_KSWAPD, nr_reclaimed);
 		count_memcg_events(lruvec_memcg(lruvec), PGSTEAL_KSWAPD,
 				   nr_reclaimed);
 	} else {
-		if (global_reclaim(sc))
-			__count_vm_events(PGSTEAL_DIRECT, nr_reclaimed);
+		__count_vm_events(PGSTEAL_DIRECT, nr_reclaimed);
 		count_memcg_events(lruvec_memcg(lruvec), PGSTEAL_DIRECT,
 				   nr_reclaimed);
 	}
@@ -2828,8 +2824,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 retry:
 	delayacct_freepages_start();
 
-	if (global_reclaim(sc))
-		__count_zid_vm_events(ALLOCSTALL, sc->reclaim_idx, 1);
+	__count_zid_vm_events(ALLOCSTALL, sc->reclaim_idx, 1);
 
 	do {
 		vmpressure_prio(sc->gfp_mask, sc->target_mem_cgroup,
-- 
2.7.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: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[Roman Gushchin]

Historically, PGSTEAL*/PGSCAN*/ALLOCSTALL counters were used to
account only for global reclaim events, memory cgroup targeted reclaim
was ignored.

It doesn't make sense anymore, because the whole reclaim path
is designed around cgroups. Also, per-cgroup counters can exceed the
corresponding global counters, what can be confusing.

So, make PGSTEAL*/PGSCAN*/ALLOCSTALL counters reflect sum of any
reclaim activity in the system.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: kernel-team@fb.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/vmscan.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7c2a36b..77253b1 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1765,13 +1765,11 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	reclaim_stat->recent_scanned[file] += nr_taken;
 
 	if (current_is_kswapd()) {
-		if (global_reclaim(sc))
-			__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
+		__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
 		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_KSWAPD,
 				   nr_scanned);
 	} else {
-		if (global_reclaim(sc))
-			__count_vm_events(PGSCAN_DIRECT, nr_scanned);
+		__count_vm_events(PGSCAN_DIRECT, nr_scanned);
 		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_DIRECT,
 				   nr_scanned);
 	}
@@ -1786,13 +1784,11 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	spin_lock_irq(&pgdat->lru_lock);
 
 	if (current_is_kswapd()) {
-		if (global_reclaim(sc))
-			__count_vm_events(PGSTEAL_KSWAPD, nr_reclaimed);
+		__count_vm_events(PGSTEAL_KSWAPD, nr_reclaimed);
 		count_memcg_events(lruvec_memcg(lruvec), PGSTEAL_KSWAPD,
 				   nr_reclaimed);
 	} else {
-		if (global_reclaim(sc))
-			__count_vm_events(PGSTEAL_DIRECT, nr_reclaimed);
+		__count_vm_events(PGSTEAL_DIRECT, nr_reclaimed);
 		count_memcg_events(lruvec_memcg(lruvec), PGSTEAL_DIRECT,
 				   nr_reclaimed);
 	}
@@ -2828,8 +2824,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 retry:
 	delayacct_freepages_start();
 
-	if (global_reclaim(sc))
-		__count_zid_vm_events(ALLOCSTALL, sc->reclaim_idx, 1);
+	__count_zid_vm_events(ALLOCSTALL, sc->reclaim_idx, 1);
 
 	do {
 		vmpressure_prio(sc->gfp_mask, sc->target_mem_cgroup,
-- 
2.7.4

Re: [PATCH] mm: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[Michal Hocko]

On Mon 29-05-17 14:01:41, Roman Gushchin wrote:
> Historically, PGSTEAL*/PGSCAN*/ALLOCSTALL counters were used to
> account only for global reclaim events, memory cgroup targeted reclaim
> was ignored.
> 
> It doesn't make sense anymore, because the whole reclaim path
> is designed around cgroups. Also, per-cgroup counters can exceed the
> corresponding global counters, what can be confusing.

The whole reclaim is designed around cgroups but the source of the
memory pressure is different. I agree that checking global_reclaim()
for PGSTEAL_KSWAPD doesn't make much sense because we are _always_ in
the global reclaim context but counting ALLOCSTALL even for targetted
memcg reclaim is more confusing than helpful. We usually consider this
counter to see whether the kswapd catches up with the memory demand
and the global direct reclaim is indicator it doesn't. The similar
applies to other counters as well.

So I do not think this is correct. What is the problem you are trying to
solve here anyway.

> So, make PGSTEAL*/PGSCAN*/ALLOCSTALL counters reflect sum of any
> reclaim activity in the system.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: kernel-team@fb.com
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/vmscan.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 7c2a36b..77253b1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1765,13 +1765,11 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>  	reclaim_stat->recent_scanned[file] += nr_taken;
>  
>  	if (current_is_kswapd()) {
> -		if (global_reclaim(sc))
> -			__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
> +		__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
>  		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_KSWAPD,
>  				   nr_scanned);
>  	} else {
> -		if (global_reclaim(sc))
> -			__count_vm_events(PGSCAN_DIRECT, nr_scanned);
> +		__count_vm_events(PGSCAN_DIRECT, nr_scanned);
>  		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_DIRECT,
>  				   nr_scanned);
>  	}
> @@ -1786,13 +1784,11 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>  	spin_lock_irq(&pgdat->lru_lock);
>  
>  	if (current_is_kswapd()) {
> -		if (global_reclaim(sc))
> -			__count_vm_events(PGSTEAL_KSWAPD, nr_reclaimed);
> +		__count_vm_events(PGSTEAL_KSWAPD, nr_reclaimed);
>  		count_memcg_events(lruvec_memcg(lruvec), PGSTEAL_KSWAPD,
>  				   nr_reclaimed);
>  	} else {
> -		if (global_reclaim(sc))
> -			__count_vm_events(PGSTEAL_DIRECT, nr_reclaimed);
> +		__count_vm_events(PGSTEAL_DIRECT, nr_reclaimed);
>  		count_memcg_events(lruvec_memcg(lruvec), PGSTEAL_DIRECT,
>  				   nr_reclaimed);
>  	}
> @@ -2828,8 +2824,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
>  retry:
>  	delayacct_freepages_start();
>  
> -	if (global_reclaim(sc))
> -		__count_zid_vm_events(ALLOCSTALL, sc->reclaim_idx, 1);
> +	__count_zid_vm_events(ALLOCSTALL, sc->reclaim_idx, 1);
>  
>  	do {
>  		vmpressure_prio(sc->gfp_mask, sc->target_mem_cgroup,
> -- 
> 2.7.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>

Re: [PATCH] mm: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[Michal Hocko]

On Mon 29-05-17 14:01:41, Roman Gushchin wrote:
> Historically, PGSTEAL*/PGSCAN*/ALLOCSTALL counters were used to
> account only for global reclaim events, memory cgroup targeted reclaim
> was ignored.
> 
> It doesn't make sense anymore, because the whole reclaim path
> is designed around cgroups. Also, per-cgroup counters can exceed the
> corresponding global counters, what can be confusing.

The whole reclaim is designed around cgroups but the source of the
memory pressure is different. I agree that checking global_reclaim()
for PGSTEAL_KSWAPD doesn't make much sense because we are _always_ in
the global reclaim context but counting ALLOCSTALL even for targetted
memcg reclaim is more confusing than helpful. We usually consider this
counter to see whether the kswapd catches up with the memory demand
and the global direct reclaim is indicator it doesn't. The similar
applies to other counters as well.

So I do not think this is correct. What is the problem you are trying to
solve here anyway.

> So, make PGSTEAL*/PGSCAN*/ALLOCSTALL counters reflect sum of any
> reclaim activity in the system.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: kernel-team@fb.com
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/vmscan.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 7c2a36b..77253b1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1765,13 +1765,11 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>  	reclaim_stat->recent_scanned[file] += nr_taken;
>  
>  	if (current_is_kswapd()) {
> -		if (global_reclaim(sc))
> -			__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
> +		__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
>  		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_KSWAPD,
>  				   nr_scanned);
>  	} else {
> -		if (global_reclaim(sc))
> -			__count_vm_events(PGSCAN_DIRECT, nr_scanned);
> +		__count_vm_events(PGSCAN_DIRECT, nr_scanned);
>  		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_DIRECT,
>  				   nr_scanned);
>  	}
> @@ -1786,13 +1784,11 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>  	spin_lock_irq(&pgdat->lru_lock);
>  
>  	if (current_is_kswapd()) {
> -		if (global_reclaim(sc))
> -			__count_vm_events(PGSTEAL_KSWAPD, nr_reclaimed);
> +		__count_vm_events(PGSTEAL_KSWAPD, nr_reclaimed);
>  		count_memcg_events(lruvec_memcg(lruvec), PGSTEAL_KSWAPD,
>  				   nr_reclaimed);
>  	} else {
> -		if (global_reclaim(sc))
> -			__count_vm_events(PGSTEAL_DIRECT, nr_reclaimed);
> +		__count_vm_events(PGSTEAL_DIRECT, nr_reclaimed);
>  		count_memcg_events(lruvec_memcg(lruvec), PGSTEAL_DIRECT,
>  				   nr_reclaimed);
>  	}
> @@ -2828,8 +2824,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
>  retry:
>  	delayacct_freepages_start();
>  
> -	if (global_reclaim(sc))
> -		__count_zid_vm_events(ALLOCSTALL, sc->reclaim_idx, 1);
> +	__count_zid_vm_events(ALLOCSTALL, sc->reclaim_idx, 1);
>  
>  	do {
>  		vmpressure_prio(sc->gfp_mask, sc->target_mem_cgroup,
> -- 
> 2.7.4
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[Roman Gushchin]

On Tue, May 30, 2017 at 02:24:36PM +0200, Michal Hocko wrote:
> On Mon 29-05-17 14:01:41, Roman Gushchin wrote:
> > Historically, PGSTEAL*/PGSCAN*/ALLOCSTALL counters were used to
> > account only for global reclaim events, memory cgroup targeted reclaim
> > was ignored.
> > 
> > It doesn't make sense anymore, because the whole reclaim path
> > is designed around cgroups. Also, per-cgroup counters can exceed the
> > corresponding global counters, what can be confusing.
> 
> The whole reclaim is designed around cgroups but the source of the
> memory pressure is different. I agree that checking global_reclaim()
> for PGSTEAL_KSWAPD doesn't make much sense because we are _always_ in
> the global reclaim context but counting ALLOCSTALL even for targetted
> memcg reclaim is more confusing than helpful. We usually consider this
> counter to see whether the kswapd catches up with the memory demand
> and the global direct reclaim is indicator it doesn't. The similar
> applies to other counters as well.
> 
> So I do not think this is correct. What is the problem you are trying to
> solve here anyway.

This is a follow-up patch after the discussion here:
https://lkml.org/lkml/2017/5/16/706.

I can agree with you, that a per-cgroup ALLOCSTALL is something different
from a global one, and it's better to keep them separated.

But what about PGSTEAL*/PGSCAN* counters, isn't it better to make them
reflect __all__ reclaim activity, no matter what was a root cause?

Thanks!

Roman

--
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: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[Roman Gushchin]

On Tue, May 30, 2017 at 02:24:36PM +0200, Michal Hocko wrote:
> On Mon 29-05-17 14:01:41, Roman Gushchin wrote:
> > Historically, PGSTEAL*/PGSCAN*/ALLOCSTALL counters were used to
> > account only for global reclaim events, memory cgroup targeted reclaim
> > was ignored.
> > 
> > It doesn't make sense anymore, because the whole reclaim path
> > is designed around cgroups. Also, per-cgroup counters can exceed the
> > corresponding global counters, what can be confusing.
> 
> The whole reclaim is designed around cgroups but the source of the
> memory pressure is different. I agree that checking global_reclaim()
> for PGSTEAL_KSWAPD doesn't make much sense because we are _always_ in
> the global reclaim context but counting ALLOCSTALL even for targetted
> memcg reclaim is more confusing than helpful. We usually consider this
> counter to see whether the kswapd catches up with the memory demand
> and the global direct reclaim is indicator it doesn't. The similar
> applies to other counters as well.
> 
> So I do not think this is correct. What is the problem you are trying to
> solve here anyway.

This is a follow-up patch after the discussion here:
https://lkml.org/lkml/2017/5/16/706.

I can agree with you, that a per-cgroup ALLOCSTALL is something different
from a global one, and it's better to keep them separated.

But what about PGSTEAL*/PGSCAN* counters, isn't it better to make them
reflect __all__ reclaim activity, no matter what was a root cause?

Thanks!

Roman

Re: [PATCH] mm: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[Michal Hocko]

On Tue 30-05-17 14:21:14, Roman Gushchin wrote:
[...]
> But what about PGSTEAL*/PGSCAN* counters, isn't it better to make them
> reflect __all__ reclaim activity, no matter what was a root cause?

What would be the advantage? Those counters have always been global and
we should better have a strong reason to change that.

-- 
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>

Re: [PATCH] mm: bump PGSTEAL*/PGSCAN*/ALLOCSTALL counters in memcg reclaim

[Michal Hocko]

On Tue 30-05-17 14:21:14, Roman Gushchin wrote:
[...]
> But what about PGSTEAL*/PGSCAN* counters, isn't it better to make them
> reflect __all__ reclaim activity, no matter what was a root cause?

What would be the advantage? Those counters have always been global and
we should better have a strong reason to change that.

-- 
Michal Hocko
SUSE Labs