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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Roman Gushchin <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Acked-by: Michal Hocko <[email protected]>
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
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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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.
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.
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.