On Fri, Apr 24, 2020 at 8:18 PM Chris Down chris@chrisdown.name wrote:

Yafang Shao writes:

...

If the author can't understand deeply in the code worte by himself/herself, I think the author should do more test on his/her patches. Regarding the issue in this case, my understanding is you know the benefit of proportional reclaim, but I'm wondering that do you know the loss if the proportional is not correct ? I don't mean to affend you, while I just try to explain how the community should cooperate.

I'm pretty sure that since multiple people on mm list have already expressed confusion at this patch, this isn't a question of testing, but of lack of clarity in usage :-)

Promoting "testing" as a panacea for this issue misses a significant part of the real problem: that the intended semantics and room for allowed races is currently unclear, which is why there is a general sense of confusion around your proposed patch and what it solves. If more testing would help, then the benefit of your patch should be patently obvious -- but it isn't.

I have shown a testcase in my commit log. Bellow is the result without my patch,

[ 601.811428] vmscan: protection 1048576 memcg /foo target memcg /foo [ 601.811429] vmscan: [ 601.811429] vmscan: protection 1048576 memcg /foo target memcg /foo [ 601.811430] vmscan: [ 601.811430] vmscan: protection 1048576 memcg /foo target memcg /foo [ 601.811431] vmscan: [ 602.452791] vmscan: protection 1048576 memcg /foo target memcg /foo [ 602.452795] vmscan: [ 602.452796] vmscan: protection 1048576 memcg /foo target memcg /foo [ 602.452805] vmscan: [ 602.452805] vmscan: protection 1048576 memcg /foo target memcg /foo [ 602.452806] vmscan: [ 602.452807] vmscan: protection 1048576 memcg /foo target memcg /foo [ 602.452808] vmscan:

Here's patch to print the above info.

diff --git a/mm/vmscan.c b/mm/vmscan.c index b06868fc4926..7525665d7cec 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -2344,10 +2344,18 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc, unsigned long lruvec_size; unsigned long scan; unsigned long protection; + struct mem_cgroup *target = sc->target_mem_cgroup;

lruvec_size = lruvec_lru_size(lruvec, lru, sc->reclaim_idx); protection = mem_cgroup_protection(memcg, sc->memcg_low_reclaim); + if (memcg && memcg != root_mem_cgroup && target) { + pr_info("protection %lu memcg ", protection); + pr_cont_cgroup_path(memcg->css.cgroup); + pr_cont(" target memcg "); + pr_cont_cgroup_path(target->css.cgroup); + pr_info("\n"); + }

if (protection) {

So my question is that do you think the protection in these log is okay ? Can you answer me ?

Hint: what should protection be if memcg is the target memcg ?

On Fri, Apr 24, 2020 at 5:06 AM Roman Gushchin guro@fb.com wrote:

On Thu, Apr 23, 2020 at 02:16:29AM -0400, Yafang Shao wrote:

...

This patch is an improvement of a previous version[1], as the previous version is not easy to understand. This issue persists in the newest kernel, I have to resend the fix. As the implementation is changed, I drop Roman's ack from the previous version.

Here's the explanation of this issue. memory.{low,min} won't take effect if the to-be-reclaimed memcg is the sc->target_mem_cgroup, that can also be proved by the implementation in mem_cgroup_protected(), see bellow, mem_cgroup_protected if (memcg == root) [2] return MEMCG_PROT_NONE;

But this rule is ignored in mem_cgroup_protection(), which will read memory.{emin, elow} as the protection whatever the memcg is.

How would this issue happen? Because in mem_cgroup_protected() we forget to clear the memory.{emin, elow} if the memcg is target_mem_cgroup [2].

An example to illustrate this issue. root_mem_cgroup / A memory.max: 1024M memory.min: 512M memory.current: 800M ('current' must be greater than 'min') Once kswapd starts to reclaim memcg A, it assigns 512M to memory.emin of A. Then kswapd stops. As a result of it, the memory values of A will be, root_mem_cgroup / A memory.max: 1024M memory.min: 512M memory.current: 512M (approximately) memory.emin: 512M

Then a new workload starts to run in memcg A, and it will trigger memcg relcaim in A soon. As memcg A is the target_mem_cgroup of this reclaimer, so it return directly without touching memory.{emin, elow}.[2] The memory values of A will be, root_mem_cgroup / A memory.max: 1024M memory.min: 512M memory.current: 1024M (approximately) memory.emin: 512M Then this memory.emin will be used in mem_cgroup_protection() to get the scan count, which is obvoiusly a wrong scan count.

[1]. https://lore.kernel.org/linux-mm/20200216145249.6900-1-laoar.shao@gmail.com/

Fixes: 9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim") Cc: Chris Down chris@chrisdown.name Cc: Roman Gushchin guro@fb.com Cc: stable@vger.kernel.org Signed-off-by: Yafang Shao laoar.shao@gmail.com

---

include/linux/memcontrol.h | 13 +++++++++++-- mm/vmscan.c | 4 ++-- 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index d275c72c4f8e..114cfe06bf60 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -344,12 +344,20 @@ static inline bool mem_cgroup_disabled(void) return !cgroup_subsys_enabled(memory_cgrp_subsys); }

-static inline unsigned long mem_cgroup_protection(struct mem_cgroup *memcg, +static inline unsigned long mem_cgroup_protection(struct mem_cgroup *root,

•                                           struct mem_cgroup *memcg,
                                            bool in_low_reclaim)
  

I'd rename "root" to "target", maybe it will make the whole thing more clear.

That would make it better. I will change it.

I'll think a bit more about it, but at the first glance the patch looks sane to me.

Thanks!

On Fri, Apr 24, 2020 at 09:14:52AM -0400, Johannes Weiner wrote:

However, mem_cgroup_protected() never expected anybody to look at the effective protection values when it indicated that the cgroup is above its protection. As a result, a query during limit reclaim may return stale protection values that were calculated by a previous reclaim cycle in which the cgroup did have siblings.

Btw, I think there is opportunity to make this a bit less error prone.

We have a mem_cgroup_protected() that returns yes or no, essentially, but protection isn't a binary state anymore.

It's also been a bit iffy that it looks like a simple predicate function, but it indeed needs to run procedurally for each cgroup in order for the calculations throughout the tree to be correct.

It might be better to have a

mem_cgroup_calculate_protection()

that runs for every cgroup we visit and sets up the internal state; then have more self-explanatory query functions on top of that:

mem_cgroup_below_min() mem_cgroup_below_low() mem_cgroup_protection()

What do you guys think?

diff --git a/mm/vmscan.c b/mm/vmscan.c index e0f502b5fca6..dbd3f75d39b9 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -2615,14 +2615,15 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) unsigned long reclaimed; unsigned long scanned;

- switch (mem_cgroup_protected(target_memcg, memcg)) { - case MEMCG_PROT_MIN: + mem_cgroup_calculate_protection(target_memcg, memcg); + + if (mem_cgroup_below_min(memcg)) { /* * Hard protection. * If there is no reclaimable memory, OOM. */ continue; - case MEMCG_PROT_LOW: + } else if (mem_cgroup_below_low(memcg)) { /* * Soft protection. * Respect the protection only as long as @@ -2634,16 +2635,6 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) continue; } memcg_memory_event(memcg, MEMCG_LOW); - break; - case MEMCG_PROT_NONE: - /* - * All protection thresholds breached. We may - * still choose to vary the scan pressure - * applied based on by how much the cgroup in - * question has exceeded its protection - * thresholds (see get_scan_count). - */ - break; }

reclaimed = sc->nr_reclaimed;

On Fri, Apr 24, 2020 at 9:44 PM Johannes Weiner hannes@cmpxchg.org wrote:

On Fri, Apr 24, 2020 at 09:14:52AM -0400, Johannes Weiner wrote:

...

However, mem_cgroup_protected() never expected anybody to look at the effective protection values when it indicated that the cgroup is above its protection. As a result, a query during limit reclaim may return stale protection values that were calculated by a previous reclaim cycle in which the cgroup did have siblings.

Btw, I think there is opportunity to make this a bit less error prone.

We have a mem_cgroup_protected() that returns yes or no, essentially, but protection isn't a binary state anymore.

It's also been a bit iffy that it looks like a simple predicate function, but it indeed needs to run procedurally for each cgroup in order for the calculations throughout the tree to be correct.

It might be better to have a

    mem_cgroup_calculate_protection()

that runs for every cgroup we visit and sets up the internal state; then have more self-explanatory query functions on top of that:

    mem_cgroup_below_min()
    mem_cgroup_below_low()
    mem_cgroup_protection()

What do you guys think?

diff --git a/mm/vmscan.c b/mm/vmscan.c index e0f502b5fca6..dbd3f75d39b9 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -2615,14 +2615,15 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) unsigned long reclaimed; unsigned long scanned;

•           switch (mem_cgroup_protected(target_memcg, memcg)) {
  

•           case MEMCG_PROT_MIN:
  

•           mem_cgroup_calculate_protection(target_memcg, memcg);
  

•           if (mem_cgroup_below_min(memcg)) {
                    /*
                     * Hard protection.
                     * If there is no reclaimable memory, OOM.
                     */
                    continue;
  

•           case MEMCG_PROT_LOW:
  

•           } else if (mem_cgroup_below_low(memcg)) {
                    /*
                     * Soft protection.
                     * Respect the protection only as long as
  

@@ -2634,16 +2635,6 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) continue; } memcg_memory_event(memcg, MEMCG_LOW);

•                   break;
  

•           case MEMCG_PROT_NONE:
  

•                   /*
  

•                    * All protection thresholds breached. We may
  

•                    * still choose to vary the scan pressure
  

•                    * applied based on by how much the cgroup in
  

•                    * question has exceeded its protection
  

•                    * thresholds (see get_scan_count).
  

•                    */
  

•                   break;
            }
  
            reclaimed = sc->nr_reclaimed;
  

After my revist of the memcg protection. I have another idea.

The emin and elow is not decided by the memcg(struct mem_cgroup), but they are really decided by the reclaim context(struct srhink_control). So they should not be bound into struct mem_cgroup, while they are really should be bound into struct srhink_control. IOW, we should move emin and elow from struct mem_cgroup into struct srhink_control. And they two members in shrink_control will be updated when a new memcg is to be shrinked. I haven't thought it deeply, but I think this should be the right thing to do.

Thanks Yafang

On Fri, Apr 24, 2020 at 04:29:58PM +0200, Michal Hocko wrote:

On Fri 24-04-20 09:14:50, Johannes Weiner wrote:

...

On Thu, Apr 23, 2020 at 02:16:29AM -0400, Yafang Shao wrote:

...

This patch is an improvement of a previous version[1], as the previous version is not easy to understand. This issue persists in the newest kernel, I have to resend the fix. As the implementation is changed, I drop Roman's ack from the previous version.

Now that I understand the problem, I much prefer the previous version.

diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 745697906ce3..2bf91ae1e640 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6332,8 +6332,19 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, if (!root) root = root_mem_cgroup;

• if (memcg == root)

• if (memcg == root) {

• /*
  

•  * The cgroup is the reclaim root in this reclaim
  

•  * cycle, and therefore not protected. But it may have
  

•  * stale effective protection values from previous
  

•  * cycles in which it was not the reclaim root - for
  

•  * example, global reclaim followed by limit reclaim.
  

•  * Reset these values for mem_cgroup_protection().
  

•  */
  

• memcg->memory.emin = 0;
  

• memcg->memory.elow = 0;
  

  return MEMCG_PROT_NONE;
• }

Could you be more specific why you prefer this over the mem_cgroup_protection which doesn't change the effective value? Isn't it easier to simply ignore effective value for the reclaim roots?

Because now both mem_cgroup_protection() and mem_cgroup_protected() have to know about the reclaim root semantics, instead of just the one central place.

And the query function has to know additional rules about when the emin/elow values are uptodate or it could silently be looking at stale data, which isn't very robust.

"The effective protection values are uptodate after calling mem_cgroup_protected() inside the reclaim cycle - UNLESS the group you're looking at happens to be..."

It's much easier to make the rule: The values are uptodate after you called mem_cgroup_protected().

Or mem_cgroup_calculate_protection(), if we go with that later.

...

As others have noted, it's fairly hard to understand the problem from the above changelog. How about the following:

A cgroup can have both memory protection and a memory limit to isolate it from its siblings in both directions - for example, to prevent it from being shrunk below 2G under high pressure from outside, but also from growing beyond 4G under low pressure.

9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim") implemented proportional scan pressure so that multiple siblings in excess of their protection settings don't get reclaimed equally but instead in accordance to their unprotected portion.

During limit reclaim, this proportionality shouldn't apply of course: there is no competition, all pressure is from within the cgroup and should be applied as such. Reclaim should operate at full efficiency.

However, mem_cgroup_protected() never expected anybody to look at the effective protection values when it indicated that the cgroup is above its protection. As a result, a query during limit reclaim may return stale protection values that were calculated by a previous reclaim cycle in which the cgroup did have siblings.

This is better. Thanks!

...

When this happens, reclaim is unnecessarily hesitant and potentially slow to meet the desired limit. In theory this could lead to premature OOM kills, although it's not obvious this has occurred in practice.

I do not see how this would lead all the way to OOM killer but it certainly can lead to unnecessary increase of the reclaim priority. The smaller the difference between the reclaim target and protection the more visible the effect would be. But if there are reclaimable pages then the reclaim should see them sooner or later

It would be a pretty extreme case, but not impossible AFAICS, because OOM is just a sampled state, not deterministic.

If memory.max is 64G and memory.low is 64G minus one page, this bug could cause limit reclaim to look at no more than SWAP_CLUSTER_MAX pages at priority 0. It's possible it wouldn't get through the full 64G worth of memory before giving up and declaring OOM.

Not that that would be a sensical configuration... My point is that OOM is defined as "I've looked at X pages and found nothing" and this bug can significantly lower X.

On Fri, Apr 24, 2020 at 06:21:03PM +0200, Michal Hocko wrote:

On Fri 24-04-20 11:10:13, Johannes Weiner wrote:

...

On Fri, Apr 24, 2020 at 04:29:58PM +0200, Michal Hocko wrote:

...

On Fri 24-04-20 09:14:50, Johannes Weiner wrote:

...

On Thu, Apr 23, 2020 at 02:16:29AM -0400, Yafang Shao wrote:

...

This patch is an improvement of a previous version[1], as the previous version is not easy to understand. This issue persists in the newest kernel, I have to resend the fix. As the implementation is changed, I drop Roman's ack from the previous version.

Now that I understand the problem, I much prefer the previous version.

diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 745697906ce3..2bf91ae1e640 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6332,8 +6332,19 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, if (!root) root = root_mem_cgroup;

• if (memcg == root)

• if (memcg == root) {

• /*
  

•  * The cgroup is the reclaim root in this reclaim
  

•  * cycle, and therefore not protected. But it may have
  

•  * stale effective protection values from previous
  

•  * cycles in which it was not the reclaim root - for
  

•  * example, global reclaim followed by limit reclaim.
  

•  * Reset these values for mem_cgroup_protection().
  

•  */
  

• memcg->memory.emin = 0;
  

• memcg->memory.elow = 0;
  

  return MEMCG_PROT_NONE;
• }

Could you be more specific why you prefer this over the mem_cgroup_protection which doesn't change the effective value? Isn't it easier to simply ignore effective value for the reclaim roots?

Because now both mem_cgroup_protection() and mem_cgroup_protected() have to know about the reclaim root semantics, instead of just the one central place.

Yes this is true but it is also potentially overwriting the state with a parallel reclaim which can lead to surprising results

Checking in mem_cgroup_protection() doesn't avoid the fundamental race:

root `- A (low=2G, elow=2G, max=3G) `- A1 (low=2G, elow=2G)

If A does limit reclaim while global reclaim races, the memcg == root check in mem_cgroup_protection() will reliably calculate the "right" scan value for A, which has no pages, and the wrong scan value for A1 where the memory actually is.

I'm okay with fixing the case where a really old left-over value is used by target reclaim.

I don't see a point in special casing this one instance of a fundamental race condition at the expense of less robust code.

On Mon, Apr 27, 2020 at 4:25 PM Michal Hocko mhocko@kernel.org wrote:

On Fri 24-04-20 12:51:03, Johannes Weiner wrote:

...

On Fri, Apr 24, 2020 at 06:21:03PM +0200, Michal Hocko wrote:

...

On Fri 24-04-20 11:10:13, Johannes Weiner wrote:

...

On Fri, Apr 24, 2020 at 04:29:58PM +0200, Michal Hocko wrote:

...

On Fri 24-04-20 09:14:50, Johannes Weiner wrote:

...

On Thu, Apr 23, 2020 at 02:16:29AM -0400, Yafang Shao wrote: > This patch is an improvement of a previous version[1], as the previous > version is not easy to understand. > This issue persists in the newest kernel, I have to resend the fix. As > the implementation is changed, I drop Roman's ack from the previous > version.

Now that I understand the problem, I much prefer the previous version.

diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 745697906ce3..2bf91ae1e640 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6332,8 +6332,19 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root,

  if (!root)
          root = root_mem_cgroup;

• if (memcg == root)
  

• if (memcg == root) {
  

•         /*
  

•          * The cgroup is the reclaim root in this reclaim
  

•          * cycle, and therefore not protected. But it may have
  

•          * stale effective protection values from previous
  

•          * cycles in which it was not the reclaim root - for
  

•          * example, global reclaim followed by limit reclaim.
  

•          * Reset these values for mem_cgroup_protection().
  

•          */
  

•         memcg->memory.emin = 0;
  

•         memcg->memory.elow = 0;
          return MEMCG_PROT_NONE;
  

• }
  

Could you be more specific why you prefer this over the mem_cgroup_protection which doesn't change the effective value? Isn't it easier to simply ignore effective value for the reclaim roots?

Because now both mem_cgroup_protection() and mem_cgroup_protected() have to know about the reclaim root semantics, instead of just the one central place.

Yes this is true but it is also potentially overwriting the state with a parallel reclaim which can lead to surprising results

Checking in mem_cgroup_protection() doesn't avoid the fundamental race:

root `- A (low=2G, elow=2G, max=3G) `- A1 (low=2G, elow=2G)

If A does limit reclaim while global reclaim races, the memcg == root check in mem_cgroup_protection() will reliably calculate the "right" scan value for A, which has no pages, and the wrong scan value for A1 where the memory actually is.

I am sorry but I do not see how A1 would get wrong scan value.

• Global reclaim

  • A.elow = 2G
  • A1.elow = min(A1.low, A1.usage) ; if (A.children_low_usage < A.elow)

• A reclaim.

  • A.elow = stale/undefined
  • A1.elow = A1.low

if mem_cgroup_protection returns 0 for A's reclaim targeting A (assuming the check is there) then not a big deal as there are no pages there as you say.

Let's compare the GR (global reclaim), AR (A reclaim). GR(A1.elow) <= AR(A1.elow) by definition, right? For A1.low overcommitted we have min(A1.low, A1.usage) * A.elow / A.children_low_usage <= min(A1.low, A1.usage) because A.elow <= A.children_low_usage

so in both cases we have GR(A1.elow) <= AR(A1.elow) which means that racing reclaims will behave sanely because the protection for the external pressure pressure is not violated. A is going to reclaim A1 less than the global reclaim but that should be OK.

Or what do I miss?

...

I'm okay with fixing the case where a really old left-over value is used by target reclaim.

I don't see a point in special casing this one instance of a fundamental race condition at the expense of less robust code.

I am definitely not calling to fragment the code. I do agree that having a special case in mem_cgroup_protection is quite non-intuitive. The existing code is quite hard to reason about in its current form as we can see. If we can fix all that in mem_cgroup_protected then no objections from me at all.

Hi Michal,

Pls. help take a look at my refactor on proportional memcg protection[1]. I think my new patchset can fix all that in mem_cgroup_protected and make the existing code clear.

[1]. https://lore.kernel.org/linux-mm/20200425152418.28388-1-laoar.shao@gmail.com...

On Fri, Apr 24, 2020 at 04:29:58PM +0200, Michal Hocko wrote:

On Fri 24-04-20 09:14:50, Johannes Weiner wrote:

...

On Thu, Apr 23, 2020 at 02:16:29AM -0400, Yafang Shao wrote:

...

This patch is an improvement of a previous version[1], as the previous version is not easy to understand. This issue persists in the newest kernel, I have to resend the fix. As the implementation is changed, I drop Roman's ack from the previous version.

Now that I understand the problem, I much prefer the previous version.

diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 745697906ce3..2bf91ae1e640 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6332,8 +6332,19 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, if (!root) root = root_mem_cgroup;

• if (memcg == root)

• if (memcg == root) {

• /*
  

•  * The cgroup is the reclaim root in this reclaim
  

•  * cycle, and therefore not protected. But it may have
  

•  * stale effective protection values from previous
  

•  * cycles in which it was not the reclaim root - for
  

•  * example, global reclaim followed by limit reclaim.
  

•  * Reset these values for mem_cgroup_protection().
  

•  */
  

• memcg->memory.emin = 0;
  

• memcg->memory.elow = 0;
  

  return MEMCG_PROT_NONE;
• }

Could you be more specific why you prefer this over the mem_cgroup_protection which doesn't change the effective value? Isn't it easier to simply ignore effective value for the reclaim roots?

Hm, I think I like the new version better, because it feels "safer" in terms of preserving sane effective protection values for concurrent reclaimers.

[...]

...

As others have noted, it's fairly hard to understand the problem from the above changelog. How about the following:

A cgroup can have both memory protection and a memory limit to isolate it from its siblings in both directions - for example, to prevent it from being shrunk below 2G under high pressure from outside, but also from growing beyond 4G under low pressure.

9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim") implemented proportional scan pressure so that multiple siblings in excess of their protection settings don't get reclaimed equally but instead in accordance to their unprotected portion.

During limit reclaim, this proportionality shouldn't apply of course: there is no competition, all pressure is from within the cgroup and should be applied as such. Reclaim should operate at full efficiency.

However, mem_cgroup_protected() never expected anybody to look at the effective protection values when it indicated that the cgroup is above its protection. As a result, a query during limit reclaim may return stale protection values that were calculated by a previous reclaim cycle in which the cgroup did have siblings.

This is better. Thanks!

+1

and I like the proposed renaming/cleanup. Thanks, Johannes!

...

When this happens, reclaim is unnecessarily hesitant and potentially slow to meet the desired limit. In theory this could lead to premature OOM kills, although it's not obvious this has occurred in practice.

I do not see how this would lead all the way to OOM killer but it certainly can lead to unnecessary increase of the reclaim priority. The smaller the difference between the reclaim target and protection the more visible the effect would be. But if there are reclaimable pages then the reclaim should see them sooner or later

I guess if all memory is protected by emin and the targeted reclaim will be unable to reclaim anything, OOM can be triggered.

Btw, I wonder if this case can be covered by a new memcg kselftest? I'm not sure it can be easily reproducible, but if it can, it would be the best demonstration of a problem and the fix. Yafang, don't you want to try?

Thanks!

On Fri, Apr 24, 2020 at 9:14 PM Johannes Weiner hannes@cmpxchg.org wrote:

On Thu, Apr 23, 2020 at 02:16:29AM -0400, Yafang Shao wrote:

...

This patch is an improvement of a previous version[1], as the previous version is not easy to understand. This issue persists in the newest kernel, I have to resend the fix. As the implementation is changed, I drop Roman's ack from the previous version.

Now that I understand the problem, I much prefer the previous version.

Great news that this issue is understood by one more reviewer.

diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 745697906ce3..2bf91ae1e640 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6332,8 +6332,19 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root,

    if (!root)
            root = root_mem_cgroup;

•   if (memcg == root)
  

•   if (memcg == root) {
  

•           /*
  

•            * The cgroup is the reclaim root in this reclaim
  

•            * cycle, and therefore not protected. But it may have
  

•            * stale effective protection values from previous
  

•            * cycles in which it was not the reclaim root - for
  

•            * example, global reclaim followed by limit reclaim.
  

•            * Reset these values for mem_cgroup_protection().
  

•            */
  

•           memcg->memory.emin = 0;
  

•           memcg->memory.elow = 0;
            return MEMCG_PROT_NONE;
  

•   }
  
    usage = page_counter_read(&memcg->memory);
    if (!usage)
  
  

...

Here's the explanation of this issue. memory.{low,min} won't take effect if the to-be-reclaimed memcg is the sc->target_mem_cgroup, that can also be proved by the implementation in mem_cgroup_protected(), see bellow, mem_cgroup_protected if (memcg == root) [2] return MEMCG_PROT_NONE;

But this rule is ignored in mem_cgroup_protection(), which will read memory.{emin, elow} as the protection whatever the memcg is.

How would this issue happen? Because in mem_cgroup_protected() we forget to clear the memory.{emin, elow} if the memcg is target_mem_cgroup [2].

An example to illustrate this issue. root_mem_cgroup / A memory.max: 1024M memory.min: 512M memory.current: 800M ('current' must be greater than 'min') Once kswapd starts to reclaim memcg A, it assigns 512M to memory.emin of A. Then kswapd stops. As a result of it, the memory values of A will be, root_mem_cgroup / A memory.max: 1024M memory.min: 512M memory.current: 512M (approximately) memory.emin: 512M

Then a new workload starts to run in memcg A, and it will trigger memcg relcaim in A soon. As memcg A is the target_mem_cgroup of this reclaimer, so it return directly without touching memory.{emin, elow}.[2] The memory values of A will be, root_mem_cgroup / A memory.max: 1024M memory.min: 512M memory.current: 1024M (approximately) memory.emin: 512M Then this memory.emin will be used in mem_cgroup_protection() to get the scan count, which is obvoiusly a wrong scan count.

[1]. https://lore.kernel.org/linux-mm/20200216145249.6900-1-laoar.shao@gmail.com/

Fixes: 9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim") Cc: Chris Down chris@chrisdown.name Cc: Roman Gushchin guro@fb.com Cc: stable@vger.kernel.org Signed-off-by: Yafang Shao laoar.shao@gmail.com

As others have noted, it's fairly hard to understand the problem from the above changelog. How about the following:

A cgroup can have both memory protection and a memory limit to isolate it from its siblings in both directions - for example, to prevent it from being shrunk below 2G under high pressure from outside, but also from growing beyond 4G under low pressure.

9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim") implemented proportional scan pressure so that multiple siblings in excess of their protection settings don't get reclaimed equally but instead in accordance to their unprotected portion.

During limit reclaim, this proportionality shouldn't apply of course: there is no competition, all pressure is from within the cgroup and should be applied as such. Reclaim should operate at full efficiency.

However, mem_cgroup_protected() never expected anybody to look at the effective protection values when it indicated that the cgroup is above its protection. As a result, a query during limit reclaim may return stale protection values that were calculated by a previous reclaim cycle in which the cgroup did have siblings.

When this happens, reclaim is unnecessarily hesitant and potentially slow to meet the desired limit. In theory this could lead to premature OOM kills, although it's not obvious this has occurred in practice.

Thanks a lot for your improvement on the change log.

-- Thanks Yafang