[PATCH 1/2] mm: memcontrol: remove dead code from memory_max_write()

[PATCH 1/2] mm: memcontrol: remove dead code from memory_max_write()

[Johannes Weiner]

When the reclaim loop in memory_max_write() is ^C'd or similar, we set
err to -EINTR. But we don't return err. Once the limit is set, we
always return success (nbytes). Delete the dead code.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memcontrol.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 055975b0b3a3..ff90d4e7df37 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6122,10 +6122,8 @@ static ssize_t memory_max_write(struct kernfs_open_file *of,
 		if (nr_pages <= max)
 			break;
 
-		if (signal_pending(current)) {
-			err = -EINTR;
+		if (signal_pending(current))
 			break;
-		}
 
 		if (!drained) {
 			drain_all_stock(memcg);
-- 
2.23.0

[PATCH 2/2] mm: memcontrol: try harder to set a new memory.high

[Johannes Weiner]

Setting a memory.high limit below the usage makes almost no effort to
shrink the cgroup to the new target size.

While memory.high is a "soft" limit that isn't supposed to cause OOM
situations, we should still try harder to meet a user request through
persistent reclaim.

For example, after setting a 10M memory.high on an 800M cgroup full of
file cache, the usage shrinks to about 350M:

+ cat /cgroup/workingset/memory.current
841568256
+ echo 10M
+ cat /cgroup/workingset/memory.current
355729408

This isn't exactly what the user would expect to happen. Setting the
value a few more times eventually whittles the usage down to what we
are asking for:

+ echo 10M
+ cat /cgroup/workingset/memory.current
104181760
+ echo 10M
+ cat /cgroup/workingset/memory.current
31801344
+ echo 10M
+ cat /cgroup/workingset/memory.current
10440704

To improve this, add reclaim retry loops to the memory.high write()
callback, similar to what we do for memory.max, to make a reasonable
effort that the usage meets the requested size after the call returns.

Afterwards, a single write() to memory.high is enough in all but
extreme cases:

+ cat /cgroup/workingset/memory.current
841609216
+ echo 10M
+ cat /cgroup/workingset/memory.current
10182656

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memcontrol.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ff90d4e7df37..8090b4c99ac7 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6074,7 +6074,8 @@ static ssize_t memory_high_write(struct kernfs_open_file *of,
 				 char *buf, size_t nbytes, loff_t off)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
-	unsigned long nr_pages;
+	unsigned int nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
+	bool drained = false;
 	unsigned long high;
 	int err;
 
@@ -6085,12 +6086,29 @@ static ssize_t memory_high_write(struct kernfs_open_file *of,
 
 	memcg->high = high;
 
-	nr_pages = page_counter_read(&memcg->memory);
-	if (nr_pages > high)
-		try_to_free_mem_cgroup_pages(memcg, nr_pages - high,
-					     GFP_KERNEL, true);
+	for (;;) {
+		unsigned long nr_pages = page_counter_read(&memcg->memory);
+		unsigned long reclaimed;
+
+		if (nr_pages <= high)
+			break;
+
+		if (signal_pending(current))
+			break;
+
+		if (!drained) {
+			drain_all_stock(memcg);
+			drained = true;
+			continue;
+		}
+
+		reclaimed = try_to_free_mem_cgroup_pages(memcg, nr_pages - high,
+							 GFP_KERNEL, true);
+
+		if (!reclaimed && !nr_retries--)
+			break;
+	}
 
-	memcg_wb_domain_size_changed(memcg);
 	return nbytes;
 }
 
-- 
2.23.0

Re: [PATCH 1/2] mm: memcontrol: remove dead code from memory_max_write()

[Michal Hocko]

On Tue 22-10-19 16:15:17, Johannes Weiner wrote:
> When the reclaim loop in memory_max_write() is ^C'd or similar, we set
> err to -EINTR. But we don't return err. Once the limit is set, we
> always return success (nbytes). Delete the dead code.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/memcontrol.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 055975b0b3a3..ff90d4e7df37 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6122,10 +6122,8 @@ static ssize_t memory_max_write(struct kernfs_open_file *of,
>  		if (nr_pages <= max)
>  			break;
>  
> -		if (signal_pending(current)) {
> -			err = -EINTR;
> +		if (signal_pending(current))
>  			break;
> -		}
>  
>  		if (!drained) {
>  			drain_all_stock(memcg);
> -- 
> 2.23.0
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] mm: memcontrol: try harder to set a new memory.high

[Michal Hocko]

On Tue 22-10-19 16:15:18, Johannes Weiner wrote:
> Setting a memory.high limit below the usage makes almost no effort to
> shrink the cgroup to the new target size.
> 
> While memory.high is a "soft" limit that isn't supposed to cause OOM
> situations, we should still try harder to meet a user request through
> persistent reclaim.
> 
> For example, after setting a 10M memory.high on an 800M cgroup full of
> file cache, the usage shrinks to about 350M:
> 
> + cat /cgroup/workingset/memory.current
> 841568256
> + echo 10M
> + cat /cgroup/workingset/memory.current
> 355729408
> 
> This isn't exactly what the user would expect to happen. Setting the
> value a few more times eventually whittles the usage down to what we
> are asking for:
> 
> + echo 10M
> + cat /cgroup/workingset/memory.current
> 104181760
> + echo 10M
> + cat /cgroup/workingset/memory.current
> 31801344
> + echo 10M
> + cat /cgroup/workingset/memory.current
> 10440704
> 
> To improve this, add reclaim retry loops to the memory.high write()
> callback, similar to what we do for memory.max, to make a reasonable
> effort that the usage meets the requested size after the call returns.

That suggests that the reclaim couldn't meet the given reclaim target
but later attempts just made it through. Is this due to amount of dirty
pages or what prevented the reclaim to do its job?

While I am not against the reclaim retry loop I would like to understand
the underlying issue. Because if this is really about dirty memory then
we should probably be more pro-active in flushing it. Otherwise the
retry might not be of any help.

> Afterwards, a single write() to memory.high is enough in all but
> extreme cases:
> 
> + cat /cgroup/workingset/memory.current
> 841609216
> + echo 10M
> + cat /cgroup/workingset/memory.current
> 10182656
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/memcontrol.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ff90d4e7df37..8090b4c99ac7 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6074,7 +6074,8 @@ static ssize_t memory_high_write(struct kernfs_open_file *of,
>  				 char *buf, size_t nbytes, loff_t off)
>  {
>  	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> -	unsigned long nr_pages;
> +	unsigned int nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
> +	bool drained = false;
>  	unsigned long high;
>  	int err;
>  
> @@ -6085,12 +6086,29 @@ static ssize_t memory_high_write(struct kernfs_open_file *of,
>  
>  	memcg->high = high;
>  
> -	nr_pages = page_counter_read(&memcg->memory);
> -	if (nr_pages > high)
> -		try_to_free_mem_cgroup_pages(memcg, nr_pages - high,
> -					     GFP_KERNEL, true);
> +	for (;;) {
> +		unsigned long nr_pages = page_counter_read(&memcg->memory);
> +		unsigned long reclaimed;
> +
> +		if (nr_pages <= high)
> +			break;
> +
> +		if (signal_pending(current))
> +			break;
> +
> +		if (!drained) {
> +			drain_all_stock(memcg);
> +			drained = true;
> +			continue;
> +		}
> +
> +		reclaimed = try_to_free_mem_cgroup_pages(memcg, nr_pages - high,
> +							 GFP_KERNEL, true);
> +
> +		if (!reclaimed && !nr_retries--)
> +			break;
> +	}
>  
> -	memcg_wb_domain_size_changed(memcg);
>  	return nbytes;
>  }
>  
> -- 
> 2.23.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] mm: memcontrol: try harder to set a new memory.high

[Johannes Weiner]

On Wed, Oct 23, 2019 at 08:59:49AM +0200, Michal Hocko wrote:
> On Tue 22-10-19 16:15:18, Johannes Weiner wrote:
> > Setting a memory.high limit below the usage makes almost no effort to
> > shrink the cgroup to the new target size.
> > 
> > While memory.high is a "soft" limit that isn't supposed to cause OOM
> > situations, we should still try harder to meet a user request through
> > persistent reclaim.
> > 
> > For example, after setting a 10M memory.high on an 800M cgroup full of
> > file cache, the usage shrinks to about 350M:
> > 
> > + cat /cgroup/workingset/memory.current
> > 841568256
> > + echo 10M
> > + cat /cgroup/workingset/memory.current
> > 355729408
> > 
> > This isn't exactly what the user would expect to happen. Setting the
> > value a few more times eventually whittles the usage down to what we
> > are asking for:
> > 
> > + echo 10M
> > + cat /cgroup/workingset/memory.current
> > 104181760
> > + echo 10M
> > + cat /cgroup/workingset/memory.current
> > 31801344
> > + echo 10M
> > + cat /cgroup/workingset/memory.current
> > 10440704
> > 
> > To improve this, add reclaim retry loops to the memory.high write()
> > callback, similar to what we do for memory.max, to make a reasonable
> > effort that the usage meets the requested size after the call returns.
> 
> That suggests that the reclaim couldn't meet the given reclaim target
> but later attempts just made it through. Is this due to amount of dirty
> pages or what prevented the reclaim to do its job?
> 
> While I am not against the reclaim retry loop I would like to understand
> the underlying issue. Because if this is really about dirty memory then
> we should probably be more pro-active in flushing it. Otherwise the
> retry might not be of any help.

All the pages in my test case are clean cache. But they are active,
and they need to go through the inactive list before reclaiming. The
inactive list size is designed to pre-age just enough pages for
regular reclaim targets, i.e. pages in the SWAP_CLUSTER_MAX ballpark,
In this case, the reclaim goal for a single invocation is 790M and the
inactive list is a small funnel to put all that through, and we need
several iterations to accomplish that.

But 790M is not a reasonable reclaim target to ask of a single reclaim
invocation. And it wouldn't be reasonable to optimize the reclaim code
for it. So asking for the full size but retrying is not a bad choice
here: we express our intent, and benefit if reclaim becomes better at
handling larger requests, but we also acknowledge that some of the
deltas we can encounter in memory_high_write() are just too
ridiculously big for a single reclaim invocation to manage.

Re: [PATCH 2/2] mm: memcontrol: try harder to set a new memory.high

[Michal Hocko]

On Wed 23-10-19 13:57:24, Johannes Weiner wrote:
> On Wed, Oct 23, 2019 at 08:59:49AM +0200, Michal Hocko wrote:
> > On Tue 22-10-19 16:15:18, Johannes Weiner wrote:
> > > Setting a memory.high limit below the usage makes almost no effort to
> > > shrink the cgroup to the new target size.
> > > 
> > > While memory.high is a "soft" limit that isn't supposed to cause OOM
> > > situations, we should still try harder to meet a user request through
> > > persistent reclaim.
> > > 
> > > For example, after setting a 10M memory.high on an 800M cgroup full of
> > > file cache, the usage shrinks to about 350M:
> > > 
> > > + cat /cgroup/workingset/memory.current
> > > 841568256
> > > + echo 10M
> > > + cat /cgroup/workingset/memory.current
> > > 355729408
> > > 
> > > This isn't exactly what the user would expect to happen. Setting the
> > > value a few more times eventually whittles the usage down to what we
> > > are asking for:
> > > 
> > > + echo 10M
> > > + cat /cgroup/workingset/memory.current
> > > 104181760
> > > + echo 10M
> > > + cat /cgroup/workingset/memory.current
> > > 31801344
> > > + echo 10M
> > > + cat /cgroup/workingset/memory.current
> > > 10440704
> > > 
> > > To improve this, add reclaim retry loops to the memory.high write()
> > > callback, similar to what we do for memory.max, to make a reasonable
> > > effort that the usage meets the requested size after the call returns.
> > 
> > That suggests that the reclaim couldn't meet the given reclaim target
> > but later attempts just made it through. Is this due to amount of dirty
> > pages or what prevented the reclaim to do its job?
> > 
> > While I am not against the reclaim retry loop I would like to understand
> > the underlying issue. Because if this is really about dirty memory then
> > we should probably be more pro-active in flushing it. Otherwise the
> > retry might not be of any help.
> 
> All the pages in my test case are clean cache. But they are active,
> and they need to go through the inactive list before reclaiming. The
> inactive list size is designed to pre-age just enough pages for
> regular reclaim targets, i.e. pages in the SWAP_CLUSTER_MAX ballpark,
> In this case, the reclaim goal for a single invocation is 790M and the
> inactive list is a small funnel to put all that through, and we need
> several iterations to accomplish that.

Thanks for the clarification.

> But 790M is not a reasonable reclaim target to ask of a single reclaim
> invocation. And it wouldn't be reasonable to optimize the reclaim code
> for it. So asking for the full size but retrying is not a bad choice
> here: we express our intent, and benefit if reclaim becomes better at
> handling larger requests, but we also acknowledge that some of the
> deltas we can encounter in memory_high_write() are just too
> ridiculously big for a single reclaim invocation to manage.

Yes that makes sense and I think it should be a part of the changelog.

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!
-- 
Michal Hocko
SUSE Labs