[PATCH] mm: Move maxable seq_file logic into a single place

[PATCH] mm: Move maxable seq_file logic into a single place

[Chris Down]

memcg has a significant number of files exposed to kernfs where their
value is either exposed directly or is "max" in the case of
PAGE_COUNTER_MAX.

There's a fair amount of duplicated code here, since each file involves
turning a seq_file to a css, getting the memcg from the css, safely
reading the counter value, and then doing the right thing depending on
whether the value is PAGE_COUNTER_MAX or not.

This patch adds the macro DEFINE_MEMCG_MAX_OR_VAL, which defines and
implements a generic way to do this work, avoiding fragmenting logic.

Signed-off-by: Chris Down <chris@chrisdown.name>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Roman Gushchin <guro@fb.com>
Cc: linux-kernel@vger.kernel.org
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: kernel-team@fb.com
---
 mm/memcontrol.c | 78 ++++++++++++-------------------------------------
 1 file changed, 18 insertions(+), 60 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 18f4aefbe0bf..90e2e0ff5ed9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -261,6 +261,19 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr)
 	return &container_of(vmpr, struct mem_cgroup, vmpressure)->css;
 }
 
+/* Convenience macro to define seq_file mutators that can return "max" */
+#define DEFINE_MEMCG_MAX_OR_VAL(name, key)				    \
+	static int name(struct seq_file *m, void *v)			    \
+	{								    \
+		struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m)); \
+		unsigned long val = READ_ONCE(memcg->key);		    \
+		if (val == PAGE_COUNTER_MAX)				    \
+			seq_puts(m, "max\n");				    \
+		else							    \
+			seq_printf(m, "%llu\n", (u64)val * PAGE_SIZE);	    \
+		return 0;						    \
+	}
+
 #ifdef CONFIG_MEMCG_KMEM
 /*
  * This will be the memcg's index in each cache's ->memcg_params.memcg_caches.
@@ -5383,18 +5396,7 @@ static u64 memory_current_read(struct cgroup_subsys_state *css,
 	return (u64)page_counter_read(&memcg->memory) * PAGE_SIZE;
 }
 
-static int memory_min_show(struct seq_file *m, void *v)
-{
-	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
-	unsigned long min = READ_ONCE(memcg->memory.min);
-
-	if (min == PAGE_COUNTER_MAX)
-		seq_puts(m, "max\n");
-	else
-		seq_printf(m, "%llu\n", (u64)min * PAGE_SIZE);
-
-	return 0;
-}
+DEFINE_MEMCG_MAX_OR_VAL(memory_min_show, memory.min)
 
 static ssize_t memory_min_write(struct kernfs_open_file *of,
 				char *buf, size_t nbytes, loff_t off)
@@ -5413,18 +5415,7 @@ static ssize_t memory_min_write(struct kernfs_open_file *of,
 	return nbytes;
 }
 
-static int memory_low_show(struct seq_file *m, void *v)
-{
-	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
-	unsigned long low = READ_ONCE(memcg->memory.low);
-
-	if (low == PAGE_COUNTER_MAX)
-		seq_puts(m, "max\n");
-	else
-		seq_printf(m, "%llu\n", (u64)low * PAGE_SIZE);
-
-	return 0;
-}
+DEFINE_MEMCG_MAX_OR_VAL(memory_low_show, memory.low)
 
 static ssize_t memory_low_write(struct kernfs_open_file *of,
 				char *buf, size_t nbytes, loff_t off)
@@ -5443,18 +5434,7 @@ static ssize_t memory_low_write(struct kernfs_open_file *of,
 	return nbytes;
 }
 
-static int memory_high_show(struct seq_file *m, void *v)
-{
-	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
-	unsigned long high = READ_ONCE(memcg->high);
-
-	if (high == PAGE_COUNTER_MAX)
-		seq_puts(m, "max\n");
-	else
-		seq_printf(m, "%llu\n", (u64)high * PAGE_SIZE);
-
-	return 0;
-}
+DEFINE_MEMCG_MAX_OR_VAL(memory_high_show, high)
 
 static ssize_t memory_high_write(struct kernfs_open_file *of,
 				 char *buf, size_t nbytes, loff_t off)
@@ -5480,18 +5460,7 @@ static ssize_t memory_high_write(struct kernfs_open_file *of,
 	return nbytes;
 }
 
-static int memory_max_show(struct seq_file *m, void *v)
-{
-	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
-	unsigned long max = READ_ONCE(memcg->memory.max);
-
-	if (max == PAGE_COUNTER_MAX)
-		seq_puts(m, "max\n");
-	else
-		seq_printf(m, "%llu\n", (u64)max * PAGE_SIZE);
-
-	return 0;
-}
+DEFINE_MEMCG_MAX_OR_VAL(memory_max_show, memory.max)
 
 static ssize_t memory_max_write(struct kernfs_open_file *of,
 				char *buf, size_t nbytes, loff_t off)
@@ -6620,18 +6589,7 @@ static u64 swap_current_read(struct cgroup_subsys_state *css,
 	return (u64)page_counter_read(&memcg->swap) * PAGE_SIZE;
 }
 
-static int swap_max_show(struct seq_file *m, void *v)
-{
-	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
-	unsigned long max = READ_ONCE(memcg->swap.max);
-
-	if (max == PAGE_COUNTER_MAX)
-		seq_puts(m, "max\n");
-	else
-		seq_printf(m, "%llu\n", (u64)max * PAGE_SIZE);
-
-	return 0;
-}
+DEFINE_MEMCG_MAX_OR_VAL(swap_max_show, swap.max)
 
 static ssize_t swap_max_write(struct kernfs_open_file *of,
 			      char *buf, size_t nbytes, loff_t off)
-- 
2.20.1

Re: [PATCH] mm: Move maxable seq_file logic into a single place

[Michal Hocko]

On Thu 24-01-19 01:17:18, Chris Down wrote:
> memcg has a significant number of files exposed to kernfs where their
> value is either exposed directly or is "max" in the case of
> PAGE_COUNTER_MAX.
> 
> There's a fair amount of duplicated code here, since each file involves
> turning a seq_file to a css, getting the memcg from the css, safely
> reading the counter value, and then doing the right thing depending on
> whether the value is PAGE_COUNTER_MAX or not.
> 
> This patch adds the macro DEFINE_MEMCG_MAX_OR_VAL, which defines and
> implements a generic way to do this work, avoiding fragmenting logic.

I am not a huge fan of macro defined functions but it is true this will
save more LOC than a simple helper used by each $foo_show function.

> Signed-off-by: Chris Down <chris@chrisdown.name>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Roman Gushchin <guro@fb.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: kernel-team@fb.com

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

Thanks!
> ---
> mm/memcontrol.c | 78 ++++++++++++-------------------------------------
> 1 file changed, 18 insertions(+), 60 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 18f4aefbe0bf..90e2e0ff5ed9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -261,6 +261,19 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr)
> 	return &container_of(vmpr, struct mem_cgroup, vmpressure)->css;
> }
> 
> +/* Convenience macro to define seq_file mutators that can return "max" */
> +#define DEFINE_MEMCG_MAX_OR_VAL(name, key)				    \
> +	static int name(struct seq_file *m, void *v)			    \
> +	{								    \
> +		struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m)); \
> +		unsigned long val = READ_ONCE(memcg->key);		    \
> +		if (val == PAGE_COUNTER_MAX)				    \
> +			seq_puts(m, "max\n");				    \
> +		else							    \
> +			seq_printf(m, "%llu\n", (u64)val * PAGE_SIZE);	    \
> +		return 0;						    \
> +	}
> +
> #ifdef CONFIG_MEMCG_KMEM
> /*
>  * This will be the memcg's index in each cache's ->memcg_params.memcg_caches.
> @@ -5383,18 +5396,7 @@ static u64 memory_current_read(struct cgroup_subsys_state *css,
> 	return (u64)page_counter_read(&memcg->memory) * PAGE_SIZE;
> }
> 
> -static int memory_min_show(struct seq_file *m, void *v)
> -{
> -	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
> -	unsigned long min = READ_ONCE(memcg->memory.min);
> -
> -	if (min == PAGE_COUNTER_MAX)
> -		seq_puts(m, "max\n");
> -	else
> -		seq_printf(m, "%llu\n", (u64)min * PAGE_SIZE);
> -
> -	return 0;
> -}
> +DEFINE_MEMCG_MAX_OR_VAL(memory_min_show, memory.min)
> 
> static ssize_t memory_min_write(struct kernfs_open_file *of,
> 				char *buf, size_t nbytes, loff_t off)
> @@ -5413,18 +5415,7 @@ static ssize_t memory_min_write(struct kernfs_open_file *of,
> 	return nbytes;
> }
> 
> -static int memory_low_show(struct seq_file *m, void *v)
> -{
> -	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
> -	unsigned long low = READ_ONCE(memcg->memory.low);
> -
> -	if (low == PAGE_COUNTER_MAX)
> -		seq_puts(m, "max\n");
> -	else
> -		seq_printf(m, "%llu\n", (u64)low * PAGE_SIZE);
> -
> -	return 0;
> -}
> +DEFINE_MEMCG_MAX_OR_VAL(memory_low_show, memory.low)
> 
> static ssize_t memory_low_write(struct kernfs_open_file *of,
> 				char *buf, size_t nbytes, loff_t off)
> @@ -5443,18 +5434,7 @@ static ssize_t memory_low_write(struct kernfs_open_file *of,
> 	return nbytes;
> }
> 
> -static int memory_high_show(struct seq_file *m, void *v)
> -{
> -	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
> -	unsigned long high = READ_ONCE(memcg->high);
> -
> -	if (high == PAGE_COUNTER_MAX)
> -		seq_puts(m, "max\n");
> -	else
> -		seq_printf(m, "%llu\n", (u64)high * PAGE_SIZE);
> -
> -	return 0;
> -}
> +DEFINE_MEMCG_MAX_OR_VAL(memory_high_show, high)
> 
> static ssize_t memory_high_write(struct kernfs_open_file *of,
> 				 char *buf, size_t nbytes, loff_t off)
> @@ -5480,18 +5460,7 @@ static ssize_t memory_high_write(struct kernfs_open_file *of,
> 	return nbytes;
> }
> 
> -static int memory_max_show(struct seq_file *m, void *v)
> -{
> -	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
> -	unsigned long max = READ_ONCE(memcg->memory.max);
> -
> -	if (max == PAGE_COUNTER_MAX)
> -		seq_puts(m, "max\n");
> -	else
> -		seq_printf(m, "%llu\n", (u64)max * PAGE_SIZE);
> -
> -	return 0;
> -}
> +DEFINE_MEMCG_MAX_OR_VAL(memory_max_show, memory.max)
> 
> static ssize_t memory_max_write(struct kernfs_open_file *of,
> 				char *buf, size_t nbytes, loff_t off)
> @@ -6620,18 +6589,7 @@ static u64 swap_current_read(struct cgroup_subsys_state *css,
> 	return (u64)page_counter_read(&memcg->swap) * PAGE_SIZE;
> }
> 
> -static int swap_max_show(struct seq_file *m, void *v)
> -{
> -	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
> -	unsigned long max = READ_ONCE(memcg->swap.max);
> -
> -	if (max == PAGE_COUNTER_MAX)
> -		seq_puts(m, "max\n");
> -	else
> -		seq_printf(m, "%llu\n", (u64)max * PAGE_SIZE);
> -
> -	return 0;
> -}
> +DEFINE_MEMCG_MAX_OR_VAL(swap_max_show, swap.max)
> 
> static ssize_t swap_max_write(struct kernfs_open_file *of,
> 			      char *buf, size_t nbytes, loff_t off)
> -- 
> 2.20.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: Move maxable seq_file logic into a single place

[Johannes Weiner]

On Thu, Jan 24, 2019 at 01:17:18AM -0500, Chris Down wrote:
> memcg has a significant number of files exposed to kernfs where their
> value is either exposed directly or is "max" in the case of
> PAGE_COUNTER_MAX.
> 
> There's a fair amount of duplicated code here, since each file involves
> turning a seq_file to a css, getting the memcg from the css, safely
> reading the counter value, and then doing the right thing depending on
> whether the value is PAGE_COUNTER_MAX or not.
> 
> This patch adds the macro DEFINE_MEMCG_MAX_OR_VAL, which defines and
> implements a generic way to do this work, avoiding fragmenting logic.
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Roman Gushchin <guro@fb.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: kernel-team@fb.com
> ---
> mm/memcontrol.c | 78 ++++++++++++-------------------------------------
> 1 file changed, 18 insertions(+), 60 deletions(-)

I think this increases complexity more than it saves LOC,
unfortunately.

The current situation is a bit repetitive, but much more obviously
correct. And we're not planning on adding many more of those memcg
interface files, so I this doesn't seem to be an improvement re:
maintainability and future extensibility of the code.

Re: [PATCH] mm: Move maxable seq_file logic into a single place

[Chris Down]

Johannes Weiner writes:
>I think this increases complexity more than it saves LOC,
>unfortunately.
>
>The current situation is a bit repetitive, but much more obviously
>correct. And we're not planning on adding many more of those memcg
>interface files, so I this doesn't seem to be an improvement re:
>maintainability and future extensibility of the code.

Hmm, my main goal in the patch was not really reduction of LOC, but more around 
centralising logic so that it's clear where these functions are unique and 
where they are completely the same. My initial reaction upon reading these was 
mostly to feel like I'm missing something due to the large amount of similarity 
between them, wondering if the change in mem_cgroup/page_counter access is 
really the only thing to look at, so my goal was primarily to reduce confusion 
(although of course this is subjective, I can also see your point about how 
having "memory.low" and the like without context can also be confusing).

I think using a macro is not ideal, but unfortunately without it a lot of the 
complexity can't really be abstracted easily.

If not this, what would you think about a patch that adds two new functions:

1. mem_cgroup_from_seq
2. mem_cgroup_write_max_or_val

This won't be able to reduce as much of the overlap as the macro, but it still 
will centralise a lot of the logic.

Re: [PATCH] mm: Move maxable seq_file logic into a single place

[Chris Down]

I'm going to abandon this patch in favour of a patch series which does it 
without macros, but still reduces code duplication fairly significantly. I'll 
send it out shortly.