[Patch v2 1/3] mm/memcg: mz already removed from rb_tree in mem_cgroup_largest_soft_limit_node()

[Patch v2 1/3] mm/memcg: mz already removed from rb_tree in mem_cgroup_largest_soft_limit_node()

[Wei Yang]

When mz is not NULL, mem_cgroup_largest_soft_limit_node() has removed
it from rb_tree.

Not necessary to call __mem_cgroup_remove_exceeded() again.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/memcontrol.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f898320b678a..d70bf5cf04eb 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3458,7 +3458,6 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 		nr_reclaimed += reclaimed;
 		*total_scanned += nr_scanned;
 		spin_lock_irq(&mctz->lock);
-		__mem_cgroup_remove_exceeded(mz, mctz);
 
 		/*
 		 * If we failed to reclaim anything from this memory cgroup
-- 
2.33.1

[Patch v2 2/3] mm/memcg: __mem_cgroup_remove_exceeded could handle a !on-tree mz properly

[Wei Yang]

There is no tree operation if mz is not on-tree.

Let's remove the extra check.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/memcontrol.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d70bf5cf04eb..344a7e891bc5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -545,9 +545,11 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, int nid)
 			unsigned long flags;
 
 			spin_lock_irqsave(&mctz->lock, flags);
-			/* if on-tree, remove it */
-			if (mz->on_tree)
-				__mem_cgroup_remove_exceeded(mz, mctz);
+			/*
+			 * remove it first
+			 * If not on-tree, no tree ops.
+			 */
+			__mem_cgroup_remove_exceeded(mz, mctz);
 			/*
 			 * Insert again. mz->usage_in_excess will be updated.
 			 * If excess is 0, no tree ops.
-- 
2.33.1

Re: [Patch v2 2/3] mm/memcg: __mem_cgroup_remove_exceeded could handle a !on-tree mz properly

[Michal Hocko]

On Sat 12-03-22 07:16:22, Wei Yang wrote:
> There is no tree operation if mz is not on-tree.

This doesn't explain problem you are trying to solve nor does it make
much sense to me TBH.

> Let's remove the extra check.

What would happen if the mz was already in the excess tree and the
excess has grown?

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  mm/memcontrol.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d70bf5cf04eb..344a7e891bc5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -545,9 +545,11 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, int nid)
>  			unsigned long flags;
>  
>  			spin_lock_irqsave(&mctz->lock, flags);
> -			/* if on-tree, remove it */
> -			if (mz->on_tree)
> -				__mem_cgroup_remove_exceeded(mz, mctz);
> +			/*
> +			 * remove it first
> +			 * If not on-tree, no tree ops.
> +			 */
> +			__mem_cgroup_remove_exceeded(mz, mctz);
>  			/*
>  			 * Insert again. mz->usage_in_excess will be updated.
>  			 * If excess is 0, no tree ops.
> -- 
> 2.33.1

-- 
Michal Hocko
SUSE Labs

Re: [Patch v2 2/3] mm/memcg: __mem_cgroup_remove_exceeded could handle a !on-tree mz properly

[Wei Yang]

On Mon, Mar 14, 2022 at 10:54:03AM +0100, Michal Hocko wrote:
>On Sat 12-03-22 07:16:22, Wei Yang wrote:
>> There is no tree operation if mz is not on-tree.
>
>This doesn't explain problem you are trying to solve nor does it make
>much sense to me TBH.
>

This just tries to make the code looks consistent.

>> Let's remove the extra check.
>
>What would happen if the mz was already in the excess tree and the
>excess has grown?

The purpose mem_cgroup_update_tree() is to update the soft limit tree. And the
approach is to remove and add it back to the tree with new excess.

I don't get your point for this question.


-- 
Wei Yang
Help you, Help me

Re: [Patch v2 2/3] mm/memcg: __mem_cgroup_remove_exceeded could handle a !on-tree mz properly

[Michal Hocko]

On Mon 14-03-22 22:51:50, Wei Yang wrote:
> On Mon, Mar 14, 2022 at 10:54:03AM +0100, Michal Hocko wrote:
> >On Sat 12-03-22 07:16:22, Wei Yang wrote:
> >> There is no tree operation if mz is not on-tree.
> >
> >This doesn't explain problem you are trying to solve nor does it make
> >much sense to me TBH.
> >
> 
> This just tries to make the code looks consistent.

I guess this is rather subjective. One could argue that the check is
more descriptive because it obviously removes the node from the tree
when it is on the tree.
-- 
Michal Hocko
SUSE Labs

Re: [Patch v2 2/3] mm/memcg: __mem_cgroup_remove_exceeded could handle a !on-tree mz properly

[Wei Yang]

On Tue, Mar 15, 2022 at 4:52 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Mon 14-03-22 22:51:50, Wei Yang wrote:
> > On Mon, Mar 14, 2022 at 10:54:03AM +0100, Michal Hocko wrote:
> > >On Sat 12-03-22 07:16:22, Wei Yang wrote:
> > >> There is no tree operation if mz is not on-tree.
> > >
> > >This doesn't explain problem you are trying to solve nor does it make
> > >much sense to me TBH.
> > >
> >
> > This just tries to make the code looks consistent.
>
> I guess this is rather subjective. One could argue that the check is
> more descriptive because it obviously removes the node from the tree
> when it is on the tree.

Hmm... maybe yes.

If someone else prefer the original one, I am ok with it.

> --
> Michal Hocko
> SUSE Labs

[Patch v2 3/3] mm/memcg: add next_mz back to soft limit tree if not reclaimed yet

[Wei Yang]

When memory reclaim failed for a maximum number of attempts and we bail
out of the reclaim loop, we forgot to put the target mem_cgroup chosen
for next reclaim back to the soft limit tree. This prevented pages in
the mem_cgroup from being reclaimed in the future even though the
mem_cgroup exceeded its soft limit.

Let's say there are two mem_cgroup and both of them exceed the soft
limit, while the first one is more active then the second. Since we add
a mem_cgroup to soft limit tree every 1024 event, the second one just
get a rare chance to be put on soft limit tree even it exceeds the
limit.

As time goes on, the first mem_cgroup was kept close to its soft limit
due to reclaim activities, while the memory usage of the second
mem_cgroup keeps growing over the soft limit for a long time due to its
relatively rare occurrence.

This patch adds next_mz back to prevent this sceanrio.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/memcontrol.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 344a7e891bc5..e803ff02aae2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3493,8 +3493,13 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 			loop > MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS))
 			break;
 	} while (!nr_reclaimed);
-	if (next_mz)
+	if (next_mz) {
+		spin_lock_irq(&mctz->lock);
+		excess = soft_limit_excess(next_mz->memcg);
+		__mem_cgroup_insert_exceeded(next_mz, mctz, excess);
+		spin_unlock_irq(&mctz->lock);
 		css_put(&next_mz->memcg->css);
+	}
 	return nr_reclaimed;
 }
 
-- 
2.33.1

Re: [Patch v2 3/3] mm/memcg: add next_mz back to soft limit tree if not reclaimed yet

[Michal Hocko]

On Sat 12-03-22 07:16:23, Wei Yang wrote:
> When memory reclaim failed for a maximum number of attempts and we bail
> out of the reclaim loop, we forgot to put the target mem_cgroup chosen
> for next reclaim back to the soft limit tree. This prevented pages in
> the mem_cgroup from being reclaimed in the future even though the
> mem_cgroup exceeded its soft limit.
> 
> Let's say there are two mem_cgroup and both of them exceed the soft
> limit, while the first one is more active then the second. Since we add
> a mem_cgroup to soft limit tree every 1024 event, the second one just
> get a rare chance to be put on soft limit tree even it exceeds the
> limit.

yes, 1024 could be just 4MB of memory or 2GB if all the charged pages
are THPs. So the excess can build up considerably.

> As time goes on, the first mem_cgroup was kept close to its soft limit
> due to reclaim activities, while the memory usage of the second
> mem_cgroup keeps growing over the soft limit for a long time due to its
> relatively rare occurrence.
> 
> This patch adds next_mz back to prevent this sceanrio.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Even though your changelog is different the change itself is identical to
https://lore.kernel.org/linux-mm/8d35206601ccf0e1fe021d24405b2a0c2f4e052f.1613584277.git.tim.c.chen@linux.intel.com/
In those cases I would preserve the the original authorship by
From: Tim Chen <tim.c.chen@linux.intel.com>
and add his s-o-b before yours.

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

Thanks!

> ---
>  mm/memcontrol.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 344a7e891bc5..e803ff02aae2 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3493,8 +3493,13 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
>  			loop > MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS))
>  			break;
>  	} while (!nr_reclaimed);
> -	if (next_mz)
> +	if (next_mz) {
> +		spin_lock_irq(&mctz->lock);
> +		excess = soft_limit_excess(next_mz->memcg);
> +		__mem_cgroup_insert_exceeded(next_mz, mctz, excess);
> +		spin_unlock_irq(&mctz->lock);
>  		css_put(&next_mz->memcg->css);
> +	}
>  	return nr_reclaimed;
>  }
>  
> -- 
> 2.33.1

-- 
Michal Hocko
SUSE Labs

Re: [Patch v2 3/3] mm/memcg: add next_mz back to soft limit tree if not reclaimed yet

[Wei Yang]

On Mon, Mar 14, 2022 at 10:41:13AM +0100, Michal Hocko wrote:
>On Sat 12-03-22 07:16:23, Wei Yang wrote:
>> When memory reclaim failed for a maximum number of attempts and we bail
>> out of the reclaim loop, we forgot to put the target mem_cgroup chosen
>> for next reclaim back to the soft limit tree. This prevented pages in
>> the mem_cgroup from being reclaimed in the future even though the
>> mem_cgroup exceeded its soft limit.
>> 
>> Let's say there are two mem_cgroup and both of them exceed the soft
>> limit, while the first one is more active then the second. Since we add
>> a mem_cgroup to soft limit tree every 1024 event, the second one just
>> get a rare chance to be put on soft limit tree even it exceeds the
>> limit.
>
>yes, 1024 could be just 4MB of memory or 2GB if all the charged pages
>are THPs. So the excess can build up considerably.
>
>> As time goes on, the first mem_cgroup was kept close to its soft limit
>> due to reclaim activities, while the memory usage of the second
>> mem_cgroup keeps growing over the soft limit for a long time due to its
>> relatively rare occurrence.
>> 
>> This patch adds next_mz back to prevent this sceanrio.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>Even though your changelog is different the change itself is identical to
>https://lore.kernel.org/linux-mm/8d35206601ccf0e1fe021d24405b2a0c2f4e052f.1613584277.git.tim.c.chen@linux.intel.com/
>In those cases I would preserve the the original authorship by
>From: Tim Chen <tim.c.chen@linux.intel.com>
>and add his s-o-b before yours.

TBH I don't think this is fair.

I didn't see his original change before I sent this patch. This is a
coincidence we found the same point for improvement.

It hurts me if you want to change authorship. Well, if you really thinks this
is what it should be, please remove my s-o-b.

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

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 3/3] mm/memcg: add next_mz back to soft limit tree if not reclaimed yet

[Michal Hocko]

On Mon 14-03-22 23:05:48, Wei Yang wrote:
> On Mon, Mar 14, 2022 at 10:41:13AM +0100, Michal Hocko wrote:
> >On Sat 12-03-22 07:16:23, Wei Yang wrote:
> >> When memory reclaim failed for a maximum number of attempts and we bail
> >> out of the reclaim loop, we forgot to put the target mem_cgroup chosen
> >> for next reclaim back to the soft limit tree. This prevented pages in
> >> the mem_cgroup from being reclaimed in the future even though the
> >> mem_cgroup exceeded its soft limit.
> >> 
> >> Let's say there are two mem_cgroup and both of them exceed the soft
> >> limit, while the first one is more active then the second. Since we add
> >> a mem_cgroup to soft limit tree every 1024 event, the second one just
> >> get a rare chance to be put on soft limit tree even it exceeds the
> >> limit.
> >
> >yes, 1024 could be just 4MB of memory or 2GB if all the charged pages
> >are THPs. So the excess can build up considerably.
> >
> >> As time goes on, the first mem_cgroup was kept close to its soft limit
> >> due to reclaim activities, while the memory usage of the second
> >> mem_cgroup keeps growing over the soft limit for a long time due to its
> >> relatively rare occurrence.
> >> 
> >> This patch adds next_mz back to prevent this sceanrio.
> >> 
> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >
> >Even though your changelog is different the change itself is identical to
> >https://lore.kernel.org/linux-mm/8d35206601ccf0e1fe021d24405b2a0c2f4e052f.1613584277.git.tim.c.chen@linux.intel.com/
> >In those cases I would preserve the the original authorship by
> >From: Tim Chen <tim.c.chen@linux.intel.com>
> >and add his s-o-b before yours.
> 
> TBH I don't think this is fair.
> 
> I didn't see his original change before I sent this patch. This is a
> coincidence we found the same point for improvement.
> 
> It hurts me if you want to change authorship. Well, if you really thinks this
> is what it should be, please remove my s-o-b.

OK, fair enough.
-- 
Michal Hocko
SUSE Labs

Re: [Patch v2 1/3] mm/memcg: mz already removed from rb_tree in mem_cgroup_largest_soft_limit_node()

[Michal Hocko]

On Sat 12-03-22 07:16:21, Wei Yang wrote:
> When mz is not NULL, mem_cgroup_largest_soft_limit_node() has removed
> it from rb_tree.
> 
> Not necessary to call __mem_cgroup_remove_exceeded() again.

Yes, the call seems to be unnecessary with the current code. mz can
either come from mem_cgroup_largest_soft_limit_node or
__mem_cgroup_largest_soft_limit_node both rely on the latter so the mz
is always off the tree indeed.
 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

After the changelog is completed you can add
Acked-by: Michal Hocko <mhocko@suse.com>

In general, though, I am not a super fan of changes like these. The code
works as expected, the call for __mem_cgroup_remove_exceeded will not
really add much of an overhead and at least we can see that mz is always
removed before it is re-added back. In a hot path I would care much more
of course but this is effectivelly a dead code as the soft limit itself
is mostly a relict of past.

Please keep this in mind when you want to make further changes to this
area. The review is not free of cost and I am not sure spending time on
this area is worthwhile unless there is a real usecase in mind.

Thanks!

> ---
>  mm/memcontrol.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f898320b678a..d70bf5cf04eb 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3458,7 +3458,6 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
>  		nr_reclaimed += reclaimed;
>  		*total_scanned += nr_scanned;
>  		spin_lock_irq(&mctz->lock);
> -		__mem_cgroup_remove_exceeded(mz, mctz);
>  
>  		/*
>  		 * If we failed to reclaim anything from this memory cgroup
> -- 
> 2.33.1

-- 
Michal Hocko
SUSE Labs

Re: [Patch v2 1/3] mm/memcg: mz already removed from rb_tree in mem_cgroup_largest_soft_limit_node()

[Wei Yang]

On Mon, Mar 14, 2022 at 10:51:51AM +0100, Michal Hocko wrote:
>On Sat 12-03-22 07:16:21, Wei Yang wrote:
>> When mz is not NULL, mem_cgroup_largest_soft_limit_node() has removed
>> it from rb_tree.
>> 
>> Not necessary to call __mem_cgroup_remove_exceeded() again.
>
>Yes, the call seems to be unnecessary with the current code. mz can
>either come from mem_cgroup_largest_soft_limit_node or
>__mem_cgroup_largest_soft_limit_node both rely on the latter so the mz
>is always off the tree indeed.
> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>After the changelog is completed you can add
>Acked-by: Michal Hocko <mhocko@suse.com>

Will adjust it.

>
>In general, though, I am not a super fan of changes like these. The code
>works as expected, the call for __mem_cgroup_remove_exceeded will not
>really add much of an overhead and at least we can see that mz is always
>removed before it is re-added back. In a hot path I would care much more
>of course but this is effectivelly a dead code as the soft limit itself
>is mostly a relict of past.
>
>Please keep this in mind when you want to make further changes to this
>area. The review is not free of cost and I am not sure spending time on
>this area is worthwhile unless there is a real usecase in mind.
>

Yes, after more understanding of the code, I found soft reclaim seems to be
not that often.

Thanks for your time and will choose some more important area for change.

>Thanks!
>
>> ---
>>  mm/memcontrol.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index f898320b678a..d70bf5cf04eb 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3458,7 +3458,6 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
>>  		nr_reclaimed += reclaimed;
>>  		*total_scanned += nr_scanned;
>>  		spin_lock_irq(&mctz->lock);
>> -		__mem_cgroup_remove_exceeded(mz, mctz);
>>  
>>  		/*
>>  		 * If we failed to reclaim anything from this memory cgroup
>> -- 
>> 2.33.1
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me