* [PATCH 0/3] mm/memcg: Miscellaneous cleanups and streamlining
@ 2020-08-20 13:03 Waiman Long
2020-08-20 13:03 ` [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type Waiman Long
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Waiman Long @ 2020-08-20 13:03 UTC (permalink / raw)
To: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo
Cc: linux-kernel, cgroups, linux-mm, Shakeel Butt, Chris Down,
Roman Gushchin, Yafang Shao, Waiman Long
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 | 3 +--
mm/memcontrol.c | 30 ++++++++++--------------------
2 files changed, 11 insertions(+), 22 deletions(-)
--
2.18.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type
2020-08-20 13:03 [PATCH 0/3] mm/memcg: Miscellaneous cleanups and streamlining Waiman Long
@ 2020-08-20 13:03 ` Waiman Long
2020-08-20 17:27 ` Johannes Weiner
2020-08-20 21:16 ` Shakeel Butt
2020-08-20 13:03 ` [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max() Waiman Long
2020-08-20 13:03 ` [PATCH 3/3] mm/memcg: Unify swap and memsw page counters Waiman Long
2 siblings, 2 replies; 12+ messages in thread
From: Waiman Long @ 2020-08-20 13:03 UTC (permalink / raw)
To: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo
Cc: linux-kernel, cgroups, linux-mm, Shakeel Butt, Chris Down,
Roman Gushchin, Yafang Shao, 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>
---
mm/memcontrol.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b807952b4d43..26b7a48d3afb 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()
2020-08-20 13:03 [PATCH 0/3] mm/memcg: Miscellaneous cleanups and streamlining Waiman Long
2020-08-20 13:03 ` [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type Waiman Long
@ 2020-08-20 13:03 ` Waiman Long
2020-08-20 17:35 ` Johannes Weiner
2020-08-20 13:03 ` [PATCH 3/3] mm/memcg: Unify swap and memsw page counters Waiman Long
2 siblings, 1 reply; 12+ messages in thread
From: Waiman Long @ 2020-08-20 13:03 UTC (permalink / raw)
To: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo
Cc: linux-kernel, cgroups, linux-mm, Shakeel Butt, Chris Down,
Roman Gushchin, Yafang Shao, 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 | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 26b7a48d3afb..d219dca5239f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1633,17 +1633,13 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
*/
unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
{
- unsigned long max;
+ unsigned long max = READ_ONCE(memcg->memory.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);
+ if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
+ max += READ_ONCE(memcg->swap.max);
+ else
+ max = memcg->memsw.max;
}
return max;
}
--
2.18.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] mm/memcg: Unify swap and memsw page counters
2020-08-20 13:03 [PATCH 0/3] mm/memcg: Miscellaneous cleanups and streamlining Waiman Long
2020-08-20 13:03 ` [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type Waiman Long
2020-08-20 13:03 ` [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max() Waiman Long
@ 2020-08-20 13:03 ` Waiman Long
2020-08-20 15:46 ` Shakeel Butt
2 siblings, 1 reply; 12+ messages in thread
From: Waiman Long @ 2020-08-20 13:03 UTC (permalink / raw)
To: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo
Cc: linux-kernel, cgroups, linux-mm, Shakeel Butt, Chris Down,
Roman Gushchin, Yafang Shao, 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.
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/linux/memcontrol.h | 3 +--
mm/memcontrol.c | 8 +++++---
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index d0b036123c6a..d2a819d7db70 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -216,10 +216,9 @@ struct mem_cgroup {
/* Accounted resources */
struct page_counter memory;
- struct page_counter swap;
+ struct page_counter swap; /* memsw (memory+swap) for v1 */
/* Legacy consumer-oriented counters */
- struct page_counter memsw;
struct page_counter kmem;
struct page_counter tcpmem;
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d219dca5239f..04c3794cdc98 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -68,6 +68,11 @@
#include <trace/events/vmscan.h>
+/*
+ * The v1 memsw page counter is aliased to the v2 swap page counter.
+ */
+#define memsw swap
+
struct cgroup_subsys memory_cgrp_subsys __read_mostly;
EXPORT_SYMBOL(memory_cgrp_subsys);
@@ -5279,13 +5284,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);
/*
@@ -5414,7 +5417,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] mm/memcg: Unify swap and memsw page counters
2020-08-20 13:03 ` [PATCH 3/3] mm/memcg: Unify swap and memsw page counters Waiman Long
@ 2020-08-20 15:46 ` Shakeel Butt
2020-08-24 16:02 ` Waiman Long
0 siblings, 1 reply; 12+ messages in thread
From: Shakeel Butt @ 2020-08-20 15:46 UTC (permalink / raw)
To: Waiman Long
Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, LKML, Cgroups, Linux MM, Chris Down, Roman Gushchin,
Yafang Shao
On Thu, Aug 20, 2020 at 6:04 AM 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.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> include/linux/memcontrol.h | 3 +--
> mm/memcontrol.c | 8 +++++---
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index d0b036123c6a..d2a819d7db70 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -216,10 +216,9 @@ struct mem_cgroup {
>
> /* Accounted resources */
> struct page_counter memory;
> - struct page_counter swap;
> + struct page_counter swap; /* memsw (memory+swap) for v1 */
>
> /* Legacy consumer-oriented counters */
> - struct page_counter memsw;
> struct page_counter kmem;
> struct page_counter tcpmem;
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d219dca5239f..04c3794cdc98 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -68,6 +68,11 @@
>
> #include <trace/events/vmscan.h>
>
> +/*
> + * The v1 memsw page counter is aliased to the v2 swap page counter.
> + */
> +#define memsw swap
> +
Personally I would prefer a union instead of #define.
> struct cgroup_subsys memory_cgrp_subsys __read_mostly;
> EXPORT_SYMBOL(memory_cgrp_subsys);
>
> @@ -5279,13 +5284,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);
> /*
> @@ -5414,7 +5417,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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type
2020-08-20 13:03 ` [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type Waiman Long
@ 2020-08-20 17:27 ` Johannes Weiner
2020-08-20 21:16 ` Shakeel Butt
1 sibling, 0 replies; 12+ messages in thread
From: Johannes Weiner @ 2020-08-20 17:27 UTC (permalink / raw)
To: Waiman Long
Cc: Michal Hocko, Vladimir Davydov, Andrew Morton, Tejun Heo,
linux-kernel, cgroups, linux-mm, Shakeel Butt, Chris Down,
Roman Gushchin, Yafang Shao
On Thu, Aug 20, 2020 at 09:03:48AM -0400, 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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()
2020-08-20 13:03 ` [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max() Waiman Long
@ 2020-08-20 17:35 ` Johannes Weiner
2020-08-20 20:29 ` Waiman Long
0 siblings, 1 reply; 12+ messages in thread
From: Johannes Weiner @ 2020-08-20 17:35 UTC (permalink / raw)
To: Waiman Long
Cc: Michal Hocko, Vladimir Davydov, Andrew Morton, Tejun Heo,
linux-kernel, cgroups, linux-mm, Shakeel Butt, Chris Down,
Roman Gushchin, Yafang Shao
On Thu, Aug 20, 2020 at 09:03:49AM -0400, 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 | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 26b7a48d3afb..d219dca5239f 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1633,17 +1633,13 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
> */
> unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
> {
> - unsigned long max;
> + unsigned long max = READ_ONCE(memcg->memory.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);
> + if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
> + max += READ_ONCE(memcg->swap.max);
> + else
> + max = memcg->memsw.max;
I agree with the premise of the patch, but v1 and v2 have sufficiently
different logic, and the way v1 overrides max from the innermost
branch again also doesn't help in understanding what's going on.
Can you please split out the v1 and v2 code?
if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
max = READ_ONCE(memcg->memory.max);
if (mem_cgroup_swappiness(memcg))
max += READ_ONCE(memcg->swap.max);
} else {
if (mem_cgroup_swappiness(memcg))
max = memcg->memsw.max;
else
max = READ_ONCE(memcg->memory.max);
}
It's slightly repetitive, but IMO much more readable.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()
2020-08-20 17:35 ` Johannes Weiner
@ 2020-08-20 20:29 ` Waiman Long
2020-08-20 21:25 ` Shakeel Butt
0 siblings, 1 reply; 12+ messages in thread
From: Waiman Long @ 2020-08-20 20:29 UTC (permalink / raw)
To: Johannes Weiner
Cc: Michal Hocko, Vladimir Davydov, Andrew Morton, Tejun Heo,
linux-kernel, cgroups, linux-mm, Shakeel Butt, Chris Down,
Roman Gushchin, Yafang Shao
On 8/20/20 1:35 PM, Johannes Weiner wrote:
> On Thu, Aug 20, 2020 at 09:03:49AM -0400, 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 | 14 +++++---------
>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 26b7a48d3afb..d219dca5239f 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1633,17 +1633,13 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
>> */
>> unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
>> {
>> - unsigned long max;
>> + unsigned long max = READ_ONCE(memcg->memory.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);
>> + if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
>> + max += READ_ONCE(memcg->swap.max);
>> + else
>> + max = memcg->memsw.max;
> I agree with the premise of the patch, but v1 and v2 have sufficiently
> different logic, and the way v1 overrides max from the innermost
> branch again also doesn't help in understanding what's going on.
>
> Can you please split out the v1 and v2 code?
>
> if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
> max = READ_ONCE(memcg->memory.max);
> if (mem_cgroup_swappiness(memcg))
> max += READ_ONCE(memcg->swap.max);
> } else {
> if (mem_cgroup_swappiness(memcg))
> max = memcg->memsw.max;
> else
> max = READ_ONCE(memcg->memory.max);
> }
>
> It's slightly repetitive, but IMO much more readable.
>
Sure. That makes it even better.
Cheers,
Longman
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type
2020-08-20 13:03 ` [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type Waiman Long
2020-08-20 17:27 ` Johannes Weiner
@ 2020-08-20 21:16 ` Shakeel Butt
1 sibling, 0 replies; 12+ messages in thread
From: Shakeel Butt @ 2020-08-20 21:16 UTC (permalink / raw)
To: Waiman Long
Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, LKML, Cgroups, Linux MM, Chris Down, Roman Gushchin,
Yafang Shao
On Thu, Aug 20, 2020 at 6:04 AM Waiman Long <longman@redhat.com> 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>
Reviewed-by: Shakeel Butt <shakeelb@google.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()
2020-08-20 20:29 ` Waiman Long
@ 2020-08-20 21:25 ` Shakeel Butt
2020-09-14 0:49 ` Waiman Long
0 siblings, 1 reply; 12+ messages in thread
From: Shakeel Butt @ 2020-08-20 21:25 UTC (permalink / raw)
To: Waiman Long
Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, LKML, Cgroups, Linux MM, Chris Down, Roman Gushchin,
Yafang Shao
On Thu, Aug 20, 2020 at 1:29 PM Waiman Long <longman@redhat.com> wrote:
>
> On 8/20/20 1:35 PM, Johannes Weiner wrote:
> > On Thu, Aug 20, 2020 at 09:03:49AM -0400, 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 | 14 +++++---------
> >> 1 file changed, 5 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >> index 26b7a48d3afb..d219dca5239f 100644
> >> --- a/mm/memcontrol.c
> >> +++ b/mm/memcontrol.c
> >> @@ -1633,17 +1633,13 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
> >> */
> >> unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
> >> {
> >> - unsigned long max;
> >> + unsigned long max = READ_ONCE(memcg->memory.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);
> >> + if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
> >> + max += READ_ONCE(memcg->swap.max);
> >> + else
> >> + max = memcg->memsw.max;
> > I agree with the premise of the patch, but v1 and v2 have sufficiently
> > different logic, and the way v1 overrides max from the innermost
> > branch again also doesn't help in understanding what's going on.
> >
> > Can you please split out the v1 and v2 code?
> >
> > if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
> > max = READ_ONCE(memcg->memory.max);
> > if (mem_cgroup_swappiness(memcg))
> > max += READ_ONCE(memcg->swap.max);
> > } else {
> > if (mem_cgroup_swappiness(memcg))
> > max = memcg->memsw.max;
> > else
> > max = READ_ONCE(memcg->memory.max);
> > }
> >
> > It's slightly repetitive, but IMO much more readable.
> >
> Sure. That makes it even better.
>
Can you please also add in the commit message why it is ok to drop
total_swap_pages comparison from mem_cgroup_get_max()?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] mm/memcg: Unify swap and memsw page counters
2020-08-20 15:46 ` Shakeel Butt
@ 2020-08-24 16:02 ` Waiman Long
0 siblings, 0 replies; 12+ messages in thread
From: Waiman Long @ 2020-08-24 16:02 UTC (permalink / raw)
To: Shakeel Butt
Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, LKML, Cgroups, Linux MM, Chris Down, Roman Gushchin,
Yafang Shao
On 8/20/20 11:46 AM, Shakeel Butt wrote:
> On Thu, Aug 20, 2020 at 6:04 AM 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.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>> include/linux/memcontrol.h | 3 +--
>> mm/memcontrol.c | 8 +++++---
>> 2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index d0b036123c6a..d2a819d7db70 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -216,10 +216,9 @@ struct mem_cgroup {
>>
>> /* Accounted resources */
>> struct page_counter memory;
>> - struct page_counter swap;
>> + struct page_counter swap; /* memsw (memory+swap) for v1 */
>>
>> /* Legacy consumer-oriented counters */
>> - struct page_counter memsw;
>> struct page_counter kmem;
>> struct page_counter tcpmem;
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index d219dca5239f..04c3794cdc98 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -68,6 +68,11 @@
>>
>> #include <trace/events/vmscan.h>
>>
>> +/*
>> + * The v1 memsw page counter is aliased to the v2 swap page counter.
>> + */
>> +#define memsw swap
>> +
> Personally I would prefer a union instead of #define.
Yes, that is also what I am thinking about in the v2.
Cheers,
Longman
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max()
2020-08-20 21:25 ` Shakeel Butt
@ 2020-09-14 0:49 ` Waiman Long
0 siblings, 0 replies; 12+ messages in thread
From: Waiman Long @ 2020-09-14 0:49 UTC (permalink / raw)
To: Shakeel Butt
Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
Tejun Heo, LKML, Cgroups, Linux MM, Chris Down, Roman Gushchin,
Yafang Shao
On 8/20/20 5:25 PM, Shakeel Butt wrote:
> On Thu, Aug 20, 2020 at 1:29 PM Waiman Long <longman@redhat.com> wrote:
>> On 8/20/20 1:35 PM, Johannes Weiner wrote:
>>> On Thu, Aug 20, 2020 at 09:03:49AM -0400, 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 | 14 +++++---------
>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>>> index 26b7a48d3afb..d219dca5239f 100644
>>>> --- a/mm/memcontrol.c
>>>> +++ b/mm/memcontrol.c
>>>> @@ -1633,17 +1633,13 @@ void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
>>>> */
>>>> unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg)
>>>> {
>>>> - unsigned long max;
>>>> + unsigned long max = READ_ONCE(memcg->memory.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);
>>>> + if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
>>>> + max += READ_ONCE(memcg->swap.max);
>>>> + else
>>>> + max = memcg->memsw.max;
>>> I agree with the premise of the patch, but v1 and v2 have sufficiently
>>> different logic, and the way v1 overrides max from the innermost
>>> branch again also doesn't help in understanding what's going on.
>>>
>>> Can you please split out the v1 and v2 code?
>>>
>>> if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
>>> max = READ_ONCE(memcg->memory.max);
>>> if (mem_cgroup_swappiness(memcg))
>>> max += READ_ONCE(memcg->swap.max);
>>> } else {
>>> if (mem_cgroup_swappiness(memcg))
>>> max = memcg->memsw.max;
>>> else
>>> max = READ_ONCE(memcg->memory.max);
>>> }
>>>
>>> It's slightly repetitive, but IMO much more readable.
>>>
>> Sure. That makes it even better.
>>
> Can you please also add in the commit message why it is ok to drop
> total_swap_pages comparison from mem_cgroup_get_max()?
>
My bad. I accidentally skipped the total_swap_pages check. Will add it
back in v2.
Cheers,
Longman
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-14 0:49 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-20 13:03 [PATCH 0/3] mm/memcg: Miscellaneous cleanups and streamlining Waiman Long
2020-08-20 13:03 ` [PATCH 1/3] mm/memcg: Clean up obsolete enum charge_type Waiman Long
2020-08-20 17:27 ` Johannes Weiner
2020-08-20 21:16 ` Shakeel Butt
2020-08-20 13:03 ` [PATCH 2/3] mm/memcg: Simplify mem_cgroup_get_max() Waiman Long
2020-08-20 17:35 ` Johannes Weiner
2020-08-20 20:29 ` Waiman Long
2020-08-20 21:25 ` Shakeel Butt
2020-09-14 0:49 ` Waiman Long
2020-08-20 13:03 ` [PATCH 3/3] mm/memcg: Unify swap and memsw page counters Waiman Long
2020-08-20 15:46 ` Shakeel Butt
2020-08-24 16:02 ` Waiman Long
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).