[PATCH v2 0/3] mm/memcg: Miscellaneous cleanups and streamlining

[PATCH v2 0/3] mm/memcg: Miscellaneous cleanups and streamlining

[Waiman Long]

v2:
 - For patch 2, separate v1 and v2 processing and add back the missing
   total_swap_pages check.
 - For patch 3, use union to merge swap and memsw page counters.

Patch 1 removes an unused enum charge_type.

Patch 2 streamlines mem_cgroup_get_max().

Patch 3 unifies swap and memsw page counters in mem_cgroup.

Waiman Long (3):
  mm/memcg: Clean up obsolete enum charge_type
  mm/memcg: Simplify mem_cgroup_get_max()
  mm/memcg: Unify swap and memsw page counters

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

-- 
2.18.1

[PATCH v2 1/3] mm/memcg: Clean up obsolete enum charge_type

[Waiman Long]

Since commit 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") and
commit 00501b531c47 ("mm: memcontrol: rewrite charge API") in v3.17, the
enum charge_type was no longer used anywhere. However, the enum itself
was not removed at that time. Remove the obsolete enum charge_type now.

Signed-off-by: Waiman Long <longman@redhat.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Shakeel Butt <shakeelb@google.com>
---
 mm/memcontrol.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index cfa6cbad21d5..8c74f1200261 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -197,14 +197,6 @@ static struct move_charge_struct {
 #define	MEM_CGROUP_MAX_RECLAIM_LOOPS		100
 #define	MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS	2
 
-enum charge_type {
-	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
-	MEM_CGROUP_CHARGE_TYPE_ANON,
-	MEM_CGROUP_CHARGE_TYPE_SWAPOUT,	/* for accounting swapcache */
-	MEM_CGROUP_CHARGE_TYPE_DROP,	/* a page was unused swap cache */
-	NR_CHARGE_TYPE,
-};
-
 /* for encoding cft->private value on file */
 enum res_type {
 	_MEM,
-- 
2.18.1

Re: [PATCH v2 1/3] mm/memcg: Clean up obsolete enum charge_type

[Michal Hocko]

On Sun 13-09-20 22:44:50, Waiman Long wrote:
> Since commit 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") and
> commit 00501b531c47 ("mm: memcontrol: rewrite charge API") in v3.17, the
> enum charge_type was no longer used anywhere. However, the enum itself
> was not removed at that time. Remove the obsolete enum charge_type now.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Reviewed-by: Shakeel Butt <shakeelb@google.com>

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

> ---
>  mm/memcontrol.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index cfa6cbad21d5..8c74f1200261 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -197,14 +197,6 @@ static struct move_charge_struct {
>  #define	MEM_CGROUP_MAX_RECLAIM_LOOPS		100
>  #define	MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS	2
>  
> -enum charge_type {
> -	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
> -	MEM_CGROUP_CHARGE_TYPE_ANON,
> -	MEM_CGROUP_CHARGE_TYPE_SWAPOUT,	/* for accounting swapcache */
> -	MEM_CGROUP_CHARGE_TYPE_DROP,	/* a page was unused swap cache */
> -	NR_CHARGE_TYPE,
> -};
> -
>  /* for encoding cft->private value on file */
>  enum res_type {
>  	_MEM,
> -- 
> 2.18.1
> 

-- 
Michal Hocko
SUSE Labs

[PATCH v2 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Waiman Long]

The mem_cgroup_get_max() function used to get memory+swap max from
both the v1 memsw and v2 memory+swap page counters & return the maximum
of these 2 values. This is redundant and it is more efficient to just
get either the v1 or the v2 values depending on which one is currently
in use.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 mm/memcontrol.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8c74f1200261..ca36bed588d1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1633,17 +1633,15 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
  */
 unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
 {
-	unsigned long max;
-
-	max = READ_ONCE(memcg->memory.max);
-	if (mem_cgroup_swappiness(memcg)) {
-		unsigned long memsw_max;
-		unsigned long swap_max;
-
-		memsw_max = memcg->memsw.max;
-		swap_max = READ_ONCE(memcg->swap.max);
-		swap_max = min(swap_max, (unsigned long)total_swap_pages);
-		max = min(max + swap_max, memsw_max);
+	unsigned long max = READ_ONCE(memcg->memory.max);
+
+	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
+		if (mem_cgroup_swappiness(memcg))
+			max += min(READ_ONCE(memcg->swap.max),
+				   (unsigned long)total_swap_pages);
+	} else { /* v1 */
+		if (mem_cgroup_swappiness(memcg))
+			max = memcg->memsw.max;
 	}
 	return max;
 }
-- 
2.18.1

Re: [PATCH v2 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Michal Hocko]

On Sun 13-09-20 22:44:51, Waiman Long wrote:
> The mem_cgroup_get_max() function used to get memory+swap max from
> both the v1 memsw and v2 memory+swap page counters & return the maximum
> of these 2 values. This is redundant and it is more efficient to just
> get either the v1 or the v2 values depending on which one is currently
> in use.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  mm/memcontrol.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8c74f1200261..ca36bed588d1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1633,17 +1633,15 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
>   */
>  unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
>  {
> -	unsigned long max;
> -
> -	max = READ_ONCE(memcg->memory.max);
> -	if (mem_cgroup_swappiness(memcg)) {
> -		unsigned long memsw_max;
> -		unsigned long swap_max;
> -
> -		memsw_max = memcg->memsw.max;
> -		swap_max = READ_ONCE(memcg->swap.max);
> -		swap_max = min(swap_max, (unsigned long)total_swap_pages);
> -		max = min(max + swap_max, memsw_max);
> +	unsigned long max = READ_ONCE(memcg->memory.max);
> +
> +	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
> +		if (mem_cgroup_swappiness(memcg))
> +			max += min(READ_ONCE(memcg->swap.max),
> +				   (unsigned long)total_swap_pages);
> +	} else { /* v1 */
> +		if (mem_cgroup_swappiness(memcg))
> +			max = memcg->memsw.max;

I agree that making v1 vs. v2 distinction here makes the code more
obvious. But I do not think your code is correct for v1. In a default
state it would lead to max = PAGE_COUNTER_MAX which is not something
we want, right?

instead you want
		max += min(READ_ONCE(memcg->memsw.max), total_swap_pages);



>  	}
>  	return max;
>  }
> -- 
> 2.18.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Waiman Long]

On 9/14/20 7:48 AM, Michal Hocko wrote:
> On Sun 13-09-20 22:44:51, Waiman Long wrote:
>> The mem_cgroup_get_max() function used to get memory+swap max from
>> both the v1 memsw and v2 memory+swap page counters & return the maximum
>> of these 2 values. This is redundant and it is more efficient to just
>> get either the v1 or the v2 values depending on which one is currently
>> in use.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>   mm/memcontrol.c | 20 +++++++++-----------
>>   1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 8c74f1200261..ca36bed588d1 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1633,17 +1633,15 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
>>    */
>>   unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
>>   {
>> -	unsigned long max;
>> -
>> -	max = READ_ONCE(memcg->memory.max);
>> -	if (mem_cgroup_swappiness(memcg)) {
>> -		unsigned long memsw_max;
>> -		unsigned long swap_max;
>> -
>> -		memsw_max = memcg->memsw.max;
>> -		swap_max = READ_ONCE(memcg->swap.max);
>> -		swap_max = min(swap_max, (unsigned long)total_swap_pages);
>> -		max = min(max + swap_max, memsw_max);
>> +	unsigned long max = READ_ONCE(memcg->memory.max);
>> +
>> +	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
>> +		if (mem_cgroup_swappiness(memcg))
>> +			max += min(READ_ONCE(memcg->swap.max),
>> +				   (unsigned long)total_swap_pages);
>> +	} else { /* v1 */
>> +		if (mem_cgroup_swappiness(memcg))
>> +			max = memcg->memsw.max;
> I agree that making v1 vs. v2 distinction here makes the code more
> obvious. But I do not think your code is correct for v1. In a default
> state it would lead to max = PAGE_COUNTER_MAX which is not something
> we want, right?
>
> instead you want
> 		max += min(READ_ONCE(memcg->memsw.max), total_swap_pages);
>
You are right, it is a bit tricky for v1.

I will change it to

     max += min(READ_ONCE(memcg->memsw.max) - max, total_swap_pages):

Thanks,
Longman

Re: [PATCH v2 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Johannes Weiner]

On Mon, Sep 14, 2020 at 09:51:26AM -0400, Waiman Long wrote:
> On 9/14/20 7:48 AM, Michal Hocko wrote:
> > On Sun 13-09-20 22:44:51, Waiman Long wrote:
> > > The mem_cgroup_get_max() function used to get memory+swap max from
> > > both the v1 memsw and v2 memory+swap page counters & return the maximum
> > > of these 2 values. This is redundant and it is more efficient to just
> > > get either the v1 or the v2 values depending on which one is currently
> > > in use.
> > > 
> > > Signed-off-by: Waiman Long <longman@redhat.com>
> > > ---
> > >   mm/memcontrol.c | 20 +++++++++-----------
> > >   1 file changed, 9 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 8c74f1200261..ca36bed588d1 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -1633,17 +1633,15 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
> > >    */
> > >   unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
> > >   {
> > > -	unsigned long max;
> > > -
> > > -	max = READ_ONCE(memcg->memory.max);
> > > -	if (mem_cgroup_swappiness(memcg)) {
> > > -		unsigned long memsw_max;
> > > -		unsigned long swap_max;
> > > -
> > > -		memsw_max = memcg->memsw.max;
> > > -		swap_max = READ_ONCE(memcg->swap.max);
> > > -		swap_max = min(swap_max, (unsigned long)total_swap_pages);
> > > -		max = min(max + swap_max, memsw_max);
> > > +	unsigned long max = READ_ONCE(memcg->memory.max);
> > > +
> > > +	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
> > > +		if (mem_cgroup_swappiness(memcg))
> > > +			max += min(READ_ONCE(memcg->swap.max),
> > > +				   (unsigned long)total_swap_pages);
> > > +	} else { /* v1 */
> > > +		if (mem_cgroup_swappiness(memcg))
> > > +			max = memcg->memsw.max;
> > I agree that making v1 vs. v2 distinction here makes the code more
> > obvious. But I do not think your code is correct for v1. In a default
> > state it would lead to max = PAGE_COUNTER_MAX which is not something
> > we want, right?
> > 
> > instead you want
> > 		max += min(READ_ONCE(memcg->memsw.max), total_swap_pages);
> > 
> You are right, it is a bit tricky for v1.
> 
> I will change it to
> 
>     max += min(READ_ONCE(memcg->memsw.max) - max, total_swap_pages):

memsw.max can be smaller than max.

max = min3(max, READ_ONCE(memcg->memsw.max), total_swap_pages)?

Re: [PATCH v2 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Johannes Weiner]

On Mon, Sep 14, 2020 at 05:29:06PM -0400, Johannes Weiner wrote:
> On Mon, Sep 14, 2020 at 09:51:26AM -0400, Waiman Long wrote:
> > On 9/14/20 7:48 AM, Michal Hocko wrote:
> > > On Sun 13-09-20 22:44:51, Waiman Long wrote:
> > > > The mem_cgroup_get_max() function used to get memory+swap max from
> > > > both the v1 memsw and v2 memory+swap page counters & return the maximum
> > > > of these 2 values. This is redundant and it is more efficient to just
> > > > get either the v1 or the v2 values depending on which one is currently
> > > > in use.
> > > > 
> > > > Signed-off-by: Waiman Long <longman@redhat.com>
> > > > ---
> > > >   mm/memcontrol.c | 20 +++++++++-----------
> > > >   1 file changed, 9 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > index 8c74f1200261..ca36bed588d1 100644
> > > > --- a/mm/memcontrol.c
> > > > +++ b/mm/memcontrol.c
> > > > @@ -1633,17 +1633,15 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
> > > >    */
> > > >   unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
> > > >   {
> > > > -	unsigned long max;
> > > > -
> > > > -	max = READ_ONCE(memcg->memory.max);
> > > > -	if (mem_cgroup_swappiness(memcg)) {
> > > > -		unsigned long memsw_max;
> > > > -		unsigned long swap_max;
> > > > -
> > > > -		memsw_max = memcg->memsw.max;
> > > > -		swap_max = READ_ONCE(memcg->swap.max);
> > > > -		swap_max = min(swap_max, (unsigned long)total_swap_pages);
> > > > -		max = min(max + swap_max, memsw_max);
> > > > +	unsigned long max = READ_ONCE(memcg->memory.max);
> > > > +
> > > > +	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
> > > > +		if (mem_cgroup_swappiness(memcg))
> > > > +			max += min(READ_ONCE(memcg->swap.max),
> > > > +				   (unsigned long)total_swap_pages);
> > > > +	} else { /* v1 */
> > > > +		if (mem_cgroup_swappiness(memcg))
> > > > +			max = memcg->memsw.max;
> > > I agree that making v1 vs. v2 distinction here makes the code more
> > > obvious. But I do not think your code is correct for v1. In a default
> > > state it would lead to max = PAGE_COUNTER_MAX which is not something
> > > we want, right?
> > > 
> > > instead you want
> > > 		max += min(READ_ONCE(memcg->memsw.max), total_swap_pages);
> > > 
> > You are right, it is a bit tricky for v1.
> > 
> > I will change it to
> > 
> >     max += min(READ_ONCE(memcg->memsw.max) - max, total_swap_pages):
> 
> memsw.max can be smaller than max.
> 
> max = min3(max, READ_ONCE(memcg->memsw.max), total_swap_pages)?

Nevermind, I saw the follow-up below, and it's indeed not allowed to
configure it like that.

[PATCH v2 3/3] mm/memcg: Unify swap and memsw page counters

[Waiman Long]

The swap page counter is v2 only while memsw is v1 only. As v1 and v2
controllers cannot be active at the same time, there is no point to keep
both swap and memsw page counters in mem_cgroup. The previous patch has
made sure that memsw page counter is updated and accessed only when in
v1 code paths. So it is now safe to alias the v1 memsw page counter to v2
swap page counter. This saves 14 long's in the size of mem_cgroup. This
is a saving of 112 bytes for 64-bit archs.

While at it, also document which page counters are used in v1 and/or v2.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/memcontrol.h | 13 ++++++++-----
 mm/memcontrol.c            |  3 ---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index d0b036123c6a..6ef4a552e09d 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -215,13 +215,16 @@ struct mem_cgroup {
 	struct mem_cgroup_id id;
 
 	/* Accounted resources */
-	struct page_counter memory;
-	struct page_counter swap;
+	struct page_counter memory;		/* Both v1 & v2 */
+
+	union {
+		struct page_counter swap;	/* v2 only */
+		struct page_counter memsw;	/* v1 only */
+	};
 
 	/* Legacy consumer-oriented counters */
-	struct page_counter memsw;
-	struct page_counter kmem;
-	struct page_counter tcpmem;
+	struct page_counter kmem;		/* v1 only */
+	struct page_counter tcpmem;		/* v1 only */
 
 	/* Range enforcement for interrupt charges */
 	struct work_struct high_work;
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ca36bed588d1..188901f3a3db 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5281,13 +5281,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 		memcg->use_hierarchy = true;
 		page_counter_init(&memcg->memory, &parent->memory);
 		page_counter_init(&memcg->swap, &parent->swap);
-		page_counter_init(&memcg->memsw, &parent->memsw);
 		page_counter_init(&memcg->kmem, &parent->kmem);
 		page_counter_init(&memcg->tcpmem, &parent->tcpmem);
 	} else {
 		page_counter_init(&memcg->memory, NULL);
 		page_counter_init(&memcg->swap, NULL);
-		page_counter_init(&memcg->memsw, NULL);
 		page_counter_init(&memcg->kmem, NULL);
 		page_counter_init(&memcg->tcpmem, NULL);
 		/*
@@ -5416,7 +5414,6 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
 
 	page_counter_set_max(&memcg->memory, PAGE_COUNTER_MAX);
 	page_counter_set_max(&memcg->swap, PAGE_COUNTER_MAX);
-	page_counter_set_max(&memcg->memsw, PAGE_COUNTER_MAX);
 	page_counter_set_max(&memcg->kmem, PAGE_COUNTER_MAX);
 	page_counter_set_max(&memcg->tcpmem, PAGE_COUNTER_MAX);
 	page_counter_set_min(&memcg->memory, 0);
-- 
2.18.1

Re: [PATCH v2 3/3] mm/memcg: Unify swap and memsw page counters

[Michal Hocko]

On Sun 13-09-20 22:44:52, Waiman Long wrote:
> The swap page counter is v2 only while memsw is v1 only. As v1 and v2
> controllers cannot be active at the same time, there is no point to keep
> both swap and memsw page counters in mem_cgroup. The previous patch has
> made sure that memsw page counter is updated and accessed only when in
> v1 code paths. So it is now safe to alias the v1 memsw page counter to v2
> swap page counter. This saves 14 long's in the size of mem_cgroup. This
> is a saving of 112 bytes for 64-bit archs.

I didn't realize that page_counter is so large.

> While at it, also document which page counters are used in v1 and/or v2.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

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

> ---
>  include/linux/memcontrol.h | 13 ++++++++-----
>  mm/memcontrol.c            |  3 ---
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index d0b036123c6a..6ef4a552e09d 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -215,13 +215,16 @@ struct mem_cgroup {
>  	struct mem_cgroup_id id;
>  
>  	/* Accounted resources */
> -	struct page_counter memory;
> -	struct page_counter swap;
> +	struct page_counter memory;		/* Both v1 & v2 */
> +
> +	union {
> +		struct page_counter swap;	/* v2 only */
> +		struct page_counter memsw;	/* v1 only */
> +	};
>  
>  	/* Legacy consumer-oriented counters */
> -	struct page_counter memsw;
> -	struct page_counter kmem;
> -	struct page_counter tcpmem;
> +	struct page_counter kmem;		/* v1 only */
> +	struct page_counter tcpmem;		/* v1 only */
>  
>  	/* Range enforcement for interrupt charges */
>  	struct work_struct high_work;
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ca36bed588d1..188901f3a3db 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5281,13 +5281,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
>  		memcg->use_hierarchy = true;
>  		page_counter_init(&memcg->memory, &parent->memory);
>  		page_counter_init(&memcg->swap, &parent->swap);
> -		page_counter_init(&memcg->memsw, &parent->memsw);
>  		page_counter_init(&memcg->kmem, &parent->kmem);
>  		page_counter_init(&memcg->tcpmem, &parent->tcpmem);
>  	} else {
>  		page_counter_init(&memcg->memory, NULL);
>  		page_counter_init(&memcg->swap, NULL);
> -		page_counter_init(&memcg->memsw, NULL);
>  		page_counter_init(&memcg->kmem, NULL);
>  		page_counter_init(&memcg->tcpmem, NULL);
>  		/*
> @@ -5416,7 +5414,6 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
>  
>  	page_counter_set_max(&memcg->memory, PAGE_COUNTER_MAX);
>  	page_counter_set_max(&memcg->swap, PAGE_COUNTER_MAX);
> -	page_counter_set_max(&memcg->memsw, PAGE_COUNTER_MAX);
>  	page_counter_set_max(&memcg->kmem, PAGE_COUNTER_MAX);
>  	page_counter_set_max(&memcg->tcpmem, PAGE_COUNTER_MAX);
>  	page_counter_set_min(&memcg->memory, 0);
> -- 
> 2.18.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 3/3] mm/memcg: Unify swap and memsw page counters

[Shakeel Butt]

On Sun, Sep 13, 2020 at 7:45 PM Waiman Long <longman@redhat.com> wrote:
>
> The swap page counter is v2 only while memsw is v1 only. As v1 and v2
> controllers cannot be active at the same time, there is no point to keep
> both swap and memsw page counters in mem_cgroup. The previous patch has
> made sure that memsw page counter is updated and accessed only when in
> v1 code paths. So it is now safe to alias the v1 memsw page counter to v2
> swap page counter. This saves 14 long's in the size of mem_cgroup. This
> is a saving of 112 bytes for 64-bit archs.
>
> While at it, also document which page counters are used in v1 and/or v2.
>
> Signed-off-by: Waiman Long <longman@redhat.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

[PATCH v3 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Waiman Long]

The mem_cgroup_get_max() function used to get memory+swap max from
both the v1 memsw and v2 memory+swap page counters & return the maximum
of these 2 values. This is redundant and it is more efficient to just
get either the v1 or the v2 values depending on which one is currently
in use.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 mm/memcontrol.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8c74f1200261..2331d4bc7c4d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1633,17 +1633,19 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
  */
 unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
 {
-	unsigned long max;
-
-	max = READ_ONCE(memcg->memory.max);
-	if (mem_cgroup_swappiness(memcg)) {
-		unsigned long memsw_max;
-		unsigned long swap_max;
-
-		memsw_max = memcg->memsw.max;
-		swap_max = READ_ONCE(memcg->swap.max);
-		swap_max = min(swap_max, (unsigned long)total_swap_pages);
-		max = min(max + swap_max, memsw_max);
+	unsigned long max = READ_ONCE(memcg->memory.max);
+
+	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
+		if (mem_cgroup_swappiness(memcg))
+			max += min(READ_ONCE(memcg->swap.max),
+				   (unsigned long)total_swap_pages);
+	} else { /* v1 */
+		if (mem_cgroup_swappiness(memcg)) {
+			unsigned long memsw = READ_ONCE(memcg->memsw.max);
+
+			if (memsw > max)
+				max += min(memsw - max, (unsigned long)total_swap_pages);
+		}
 	}
 	return max;
 }
-- 
2.18.1

Re: [PATCH v3 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Michal Hocko]

On Mon 14-09-20 10:19:55, Waiman Long wrote:
> The mem_cgroup_get_max() function used to get memory+swap max from
> both the v1 memsw and v2 memory+swap page counters & return the maximum
> of these 2 values. This is redundant and it is more efficient to just
> get either the v1 or the v2 values depending on which one is currently
> in use.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  mm/memcontrol.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8c74f1200261..2331d4bc7c4d 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1633,17 +1633,19 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
>   */
>  unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
>  {
> -	unsigned long max;
> -
> -	max = READ_ONCE(memcg->memory.max);
> -	if (mem_cgroup_swappiness(memcg)) {
> -		unsigned long memsw_max;
> -		unsigned long swap_max;
> -
> -		memsw_max = memcg->memsw.max;
> -		swap_max = READ_ONCE(memcg->swap.max);
> -		swap_max = min(swap_max, (unsigned long)total_swap_pages);
> -		max = min(max + swap_max, memsw_max);
> +	unsigned long max = READ_ONCE(memcg->memory.max);
> +
> +	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
> +		if (mem_cgroup_swappiness(memcg))
> +			max += min(READ_ONCE(memcg->swap.max),
> +				   (unsigned long)total_swap_pages);
> +	} else { /* v1 */
> +		if (mem_cgroup_swappiness(memcg)) {
> +			unsigned long memsw = READ_ONCE(memcg->memsw.max);
> +
> +			if (memsw > max)
> +				max += min(memsw - max, (unsigned long)total_swap_pages);

Yes this looks better. But, memsw can never be smaller than the hard
limit (mem_cgroup_resize_max). I would find it slightly easier to
understand if you did
	/* calculate swap excess capacity from memsw limit*/
	unsigned long memsw = READ_ONCE(memcg->memsw.max) - max;
	max += min (memsw, total_swap_pages);


> +		}
>  	}
>  	return max;
>  }
> -- 
> 2.18.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Waiman Long]

On 9/14/20 10:47 AM, Michal Hocko wrote:
> On Mon 14-09-20 10:19:55, Waiman Long wrote:
>> The mem_cgroup_get_max() function used to get memory+swap max from
>> both the v1 memsw and v2 memory+swap page counters & return the maximum
>> of these 2 values. This is redundant and it is more efficient to just
>> get either the v1 or the v2 values depending on which one is currently
>> in use.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>   mm/memcontrol.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 8c74f1200261..2331d4bc7c4d 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1633,17 +1633,19 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
>>    */
>>   unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
>>   {
>> -	unsigned long max;
>> -
>> -	max = READ_ONCE(memcg->memory.max);
>> -	if (mem_cgroup_swappiness(memcg)) {
>> -		unsigned long memsw_max;
>> -		unsigned long swap_max;
>> -
>> -		memsw_max = memcg->memsw.max;
>> -		swap_max = READ_ONCE(memcg->swap.max);
>> -		swap_max = min(swap_max, (unsigned long)total_swap_pages);
>> -		max = min(max + swap_max, memsw_max);
>> +	unsigned long max = READ_ONCE(memcg->memory.max);
>> +
>> +	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
>> +		if (mem_cgroup_swappiness(memcg))
>> +			max += min(READ_ONCE(memcg->swap.max),
>> +				   (unsigned long)total_swap_pages);
>> +	} else { /* v1 */
>> +		if (mem_cgroup_swappiness(memcg)) {
>> +			unsigned long memsw = READ_ONCE(memcg->memsw.max);
>> +
>> +			if (memsw > max)
>> +				max += min(memsw - max, (unsigned long)total_swap_pages);
> Yes this looks better. But, memsw can never be smaller than the hard
> limit (mem_cgroup_resize_max). I would find it slightly easier to
> understand if you did
> 	/* calculate swap excess capacity from memsw limit*/
> 	unsigned long memsw = READ_ONCE(memcg->memsw.max) - max;
> 	max += min (memsw, total_swap_pages);

Right, I thought it was possible to set memsw lower than mem. It was not 
allowed. So the extra check is unnecessary. Will fix that.

Cheers,
Longman

[PATCH v4 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Waiman Long]

The mem_cgroup_get_max() function used to get memory+swap max from
both the v1 memsw and v2 memory+swap page counters & return the maximum
of these 2 values. This is redundant and it is more efficient to just
get either the v1 or the v2 values depending on which one is currently
in use.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 mm/memcontrol.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8c74f1200261..cad1ac4551ad 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1633,17 +1633,19 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
  */
 unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
 {
-	unsigned long max;
-
-	max = READ_ONCE(memcg->memory.max);
-	if (mem_cgroup_swappiness(memcg)) {
-		unsigned long memsw_max;
-		unsigned long swap_max;
-
-		memsw_max = memcg->memsw.max;
-		swap_max = READ_ONCE(memcg->swap.max);
-		swap_max = min(swap_max, (unsigned long)total_swap_pages);
-		max = min(max + swap_max, memsw_max);
+	unsigned long max = READ_ONCE(memcg->memory.max);
+
+	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
+		if (mem_cgroup_swappiness(memcg))
+			max += min(READ_ONCE(memcg->swap.max),
+				   (unsigned long)total_swap_pages);
+	} else { /* v1 */
+		if (mem_cgroup_swappiness(memcg)) {
+			/* Calculate swap excess capacity from memsw limit */
+			unsigned long swap = READ_ONCE(memcg->memsw.max) - max;
+
+			max += min(swap, (unsigned long)total_swap_pages);
+		}
 	}
 	return max;
 }
-- 
2.18.1

Re: [PATCH v4 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Michal Hocko]

On Mon 14-09-20 11:09:28, Waiman Long wrote:
> The mem_cgroup_get_max() function used to get memory+swap max from
> both the v1 memsw and v2 memory+swap page counters & return the maximum
> of these 2 values. This is redundant and it is more efficient to just
> get either the v1 or the v2 values depending on which one is currently
> in use.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

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

Thanks!
> ---
>  mm/memcontrol.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8c74f1200261..cad1ac4551ad 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1633,17 +1633,19 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
>   */
>  unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
>  {
> -	unsigned long max;
> -
> -	max = READ_ONCE(memcg->memory.max);
> -	if (mem_cgroup_swappiness(memcg)) {
> -		unsigned long memsw_max;
> -		unsigned long swap_max;
> -
> -		memsw_max = memcg->memsw.max;
> -		swap_max = READ_ONCE(memcg->swap.max);
> -		swap_max = min(swap_max, (unsigned long)total_swap_pages);
> -		max = min(max + swap_max, memsw_max);
> +	unsigned long max = READ_ONCE(memcg->memory.max);
> +
> +	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
> +		if (mem_cgroup_swappiness(memcg))
> +			max += min(READ_ONCE(memcg->swap.max),
> +				   (unsigned long)total_swap_pages);
> +	} else { /* v1 */
> +		if (mem_cgroup_swappiness(memcg)) {
> +			/* Calculate swap excess capacity from memsw limit */
> +			unsigned long swap = READ_ONCE(memcg->memsw.max) - max;
> +
> +			max += min(swap, (unsigned long)total_swap_pages);
> +		}
>  	}
>  	return max;
>  }
> -- 
> 2.18.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v4 2/3] mm/memcg: Simplify mem_cgroup_get_max()

[Shakeel Butt]

On Mon, Sep 14, 2020 at 8:09 AM Waiman Long <longman@redhat.com> wrote:
>
> The mem_cgroup_get_max() function used to get memory+swap max from
> both the v1 memsw and v2 memory+swap page counters & return the maximum
> of these 2 values. This is redundant and it is more efficient to just
> get either the v1 or the v2 values depending on which one is currently
> in use.
>
> Signed-off-by: Waiman Long <longman@redhat.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>