* [PATCH -v2 next 1/4] memcg: use OFP_PEAK_UNSET instead of -1
2025-01-14 12:25 [PATCH -v2 next 0/4] Some cleanup for memcg Chen Ridong
@ 2025-01-14 12:25 ` Chen Ridong
2025-01-14 18:36 ` Roman Gushchin
2025-01-14 12:25 ` [PATCH -v2 next 2/4] memcg: call the free function when allocation of pn fails Chen Ridong
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Chen Ridong @ 2025-01-14 12:25 UTC (permalink / raw)
To: akpm, mhocko, hannes, yosryahmed, roman.gushchin, shakeel.butt,
muchun.song, davidf, vbabka, mkoutny
Cc: linux-mm, linux-kernel, cgroups, chenridong, wangweiyang2
From: Chen Ridong <chenridong@huawei.com>
The 'OFP_PEAK_UNSET' has been defined, use it instead of '-1'.
Signed-off-by: Chen Ridong <chenridong@huawei.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Acked-by: David Finkel <davidf@vimeo.com>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
mm/memcontrol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 46f8b372d212..05a32c860554 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4004,7 +4004,7 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
WRITE_ONCE(peer_ctx->value, usage);
/* initial write, register watcher */
- if (ofp->value == -1)
+ if (ofp->value == OFP_PEAK_UNSET)
list_add(&ofp->list, watchers);
WRITE_ONCE(ofp->value, usage);
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -v2 next 1/4] memcg: use OFP_PEAK_UNSET instead of -1
2025-01-14 12:25 ` [PATCH -v2 next 1/4] memcg: use OFP_PEAK_UNSET instead of -1 Chen Ridong
@ 2025-01-14 18:36 ` Roman Gushchin
0 siblings, 0 replies; 12+ messages in thread
From: Roman Gushchin @ 2025-01-14 18:36 UTC (permalink / raw)
To: Chen Ridong
Cc: akpm, mhocko, hannes, yosryahmed, shakeel.butt, muchun.song,
davidf, vbabka, mkoutny, linux-mm, linux-kernel, cgroups,
chenridong, wangweiyang2
On Tue, Jan 14, 2025 at 12:25:16PM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
>
> The 'OFP_PEAK_UNSET' has been defined, use it instead of '-1'.
>
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> Reviewed-by: Michal Koutný <mkoutny@suse.com>
> Acked-by: David Finkel <davidf@vimeo.com>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH -v2 next 2/4] memcg: call the free function when allocation of pn fails
2025-01-14 12:25 [PATCH -v2 next 0/4] Some cleanup for memcg Chen Ridong
2025-01-14 12:25 ` [PATCH -v2 next 1/4] memcg: use OFP_PEAK_UNSET instead of -1 Chen Ridong
@ 2025-01-14 12:25 ` Chen Ridong
2025-01-14 18:36 ` Roman Gushchin
2025-01-14 12:25 ` [PATCH -v2 next 3/4] memcg: factor out the replace_stock_objcg function Chen Ridong
2025-01-14 12:25 ` [PATCH -v2 next 4/4] memcg: factor out stat(event)/stat_local(event_local) reading functions Chen Ridong
3 siblings, 1 reply; 12+ messages in thread
From: Chen Ridong @ 2025-01-14 12:25 UTC (permalink / raw)
To: akpm, mhocko, hannes, yosryahmed, roman.gushchin, shakeel.butt,
muchun.song, davidf, vbabka, mkoutny
Cc: linux-mm, linux-kernel, cgroups, chenridong, wangweiyang2
From: Chen Ridong <chenridong@huawei.com>
The 'free_mem_cgroup_per_node_info' function is used to free
the 'mem_cgroup_per_node' struct. Using 'pn' as the input for the
free_mem_cgroup_per_node_info function will be much clearer.
Call 'free_mem_cgroup_per_node_info' when 'alloc_mem_cgroup_per_node_info'
fails, to free 'pn' as a whole, which makes the code more cohesive.
Signed-off-by: Chen Ridong <chenridong@huawei.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
mm/memcontrol.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 05a32c860554..98f84a9fa228 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3424,6 +3424,16 @@ struct mem_cgroup *mem_cgroup_get_from_ino(unsigned long ino)
}
#endif
+static void free_mem_cgroup_per_node_info(struct mem_cgroup_per_node *pn)
+{
+ if (!pn)
+ return;
+
+ free_percpu(pn->lruvec_stats_percpu);
+ kfree(pn->lruvec_stats);
+ kfree(pn);
+}
+
static bool alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
{
struct mem_cgroup_per_node *pn;
@@ -3448,23 +3458,10 @@ static bool alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
memcg->nodeinfo[node] = pn;
return true;
fail:
- kfree(pn->lruvec_stats);
- kfree(pn);
+ free_mem_cgroup_per_node_info(pn);
return false;
}
-static void free_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
-{
- struct mem_cgroup_per_node *pn = memcg->nodeinfo[node];
-
- if (!pn)
- return;
-
- free_percpu(pn->lruvec_stats_percpu);
- kfree(pn->lruvec_stats);
- kfree(pn);
-}
-
static void __mem_cgroup_free(struct mem_cgroup *memcg)
{
int node;
@@ -3472,7 +3469,7 @@ static void __mem_cgroup_free(struct mem_cgroup *memcg)
obj_cgroup_put(memcg->orig_objcg);
for_each_node(node)
- free_mem_cgroup_per_node_info(memcg, node);
+ free_mem_cgroup_per_node_info(memcg->nodeinfo[node]);
memcg1_free_events(memcg);
kfree(memcg->vmstats);
free_percpu(memcg->vmstats_percpu);
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -v2 next 2/4] memcg: call the free function when allocation of pn fails
2025-01-14 12:25 ` [PATCH -v2 next 2/4] memcg: call the free function when allocation of pn fails Chen Ridong
@ 2025-01-14 18:36 ` Roman Gushchin
0 siblings, 0 replies; 12+ messages in thread
From: Roman Gushchin @ 2025-01-14 18:36 UTC (permalink / raw)
To: Chen Ridong
Cc: akpm, mhocko, hannes, yosryahmed, shakeel.butt, muchun.song,
davidf, vbabka, mkoutny, linux-mm, linux-kernel, cgroups,
chenridong, wangweiyang2
On Tue, Jan 14, 2025 at 12:25:17PM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
>
> The 'free_mem_cgroup_per_node_info' function is used to free
> the 'mem_cgroup_per_node' struct. Using 'pn' as the input for the
> free_mem_cgroup_per_node_info function will be much clearer.
> Call 'free_mem_cgroup_per_node_info' when 'alloc_mem_cgroup_per_node_info'
> fails, to free 'pn' as a whole, which makes the code more cohesive.
>
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> Reviewed-by: Michal Koutný <mkoutny@suse.com>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH -v2 next 3/4] memcg: factor out the replace_stock_objcg function
2025-01-14 12:25 [PATCH -v2 next 0/4] Some cleanup for memcg Chen Ridong
2025-01-14 12:25 ` [PATCH -v2 next 1/4] memcg: use OFP_PEAK_UNSET instead of -1 Chen Ridong
2025-01-14 12:25 ` [PATCH -v2 next 2/4] memcg: call the free function when allocation of pn fails Chen Ridong
@ 2025-01-14 12:25 ` Chen Ridong
2025-01-14 18:39 ` Roman Gushchin
2025-01-16 0:56 ` Shakeel Butt
2025-01-14 12:25 ` [PATCH -v2 next 4/4] memcg: factor out stat(event)/stat_local(event_local) reading functions Chen Ridong
3 siblings, 2 replies; 12+ messages in thread
From: Chen Ridong @ 2025-01-14 12:25 UTC (permalink / raw)
To: akpm, mhocko, hannes, yosryahmed, roman.gushchin, shakeel.butt,
muchun.song, davidf, vbabka, mkoutny
Cc: linux-mm, linux-kernel, cgroups, chenridong, wangweiyang2
From: Chen Ridong <chenridong@huawei.com>
Factor out the 'replace_stock_objcg' function to make the code more
cohesive.
Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
mm/memcontrol.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 98f84a9fa228..b10e0a8f3375 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2691,6 +2691,20 @@ void __memcg_kmem_uncharge_page(struct page *page, int order)
obj_cgroup_put(objcg);
}
+/* Replace the stock objcg with objcg, return the old objcg */
+static struct obj_cgroup *replace_stock_objcg(struct memcg_stock_pcp *stock,
+ struct obj_cgroup *objcg)
+{
+ struct obj_cgroup *old = NULL;
+
+ old = drain_obj_stock(stock);
+ obj_cgroup_get(objcg);
+ stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
+ ? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;
+ WRITE_ONCE(stock->cached_objcg, objcg);
+ return old;
+}
+
static void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
enum node_stat_item idx, int nr)
{
@@ -2708,11 +2722,7 @@ static void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
* changes.
*/
if (READ_ONCE(stock->cached_objcg) != objcg) {
- old = drain_obj_stock(stock);
- obj_cgroup_get(objcg);
- stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
- ? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;
- WRITE_ONCE(stock->cached_objcg, objcg);
+ old = replace_stock_objcg(stock, objcg);
stock->cached_pgdat = pgdat;
} else if (stock->cached_pgdat != pgdat) {
/* Flush the existing cached vmstat data */
@@ -2866,11 +2876,7 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
stock = this_cpu_ptr(&memcg_stock);
if (READ_ONCE(stock->cached_objcg) != objcg) { /* reset if necessary */
- old = drain_obj_stock(stock);
- obj_cgroup_get(objcg);
- WRITE_ONCE(stock->cached_objcg, objcg);
- stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
- ? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;
+ old = replace_stock_objcg(stock, objcg);
allow_uncharge = true; /* Allow uncharge when objcg changes */
}
stock->nr_bytes += nr_bytes;
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -v2 next 3/4] memcg: factor out the replace_stock_objcg function
2025-01-14 12:25 ` [PATCH -v2 next 3/4] memcg: factor out the replace_stock_objcg function Chen Ridong
@ 2025-01-14 18:39 ` Roman Gushchin
2025-01-16 0:56 ` Shakeel Butt
1 sibling, 0 replies; 12+ messages in thread
From: Roman Gushchin @ 2025-01-14 18:39 UTC (permalink / raw)
To: Chen Ridong
Cc: akpm, mhocko, hannes, yosryahmed, shakeel.butt, muchun.song,
davidf, vbabka, mkoutny, linux-mm, linux-kernel, cgroups,
chenridong, wangweiyang2
On Tue, Jan 14, 2025 at 12:25:18PM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
>
> Factor out the 'replace_stock_objcg' function to make the code more
> cohesive.
>
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -v2 next 3/4] memcg: factor out the replace_stock_objcg function
2025-01-14 12:25 ` [PATCH -v2 next 3/4] memcg: factor out the replace_stock_objcg function Chen Ridong
2025-01-14 18:39 ` Roman Gushchin
@ 2025-01-16 0:56 ` Shakeel Butt
1 sibling, 0 replies; 12+ messages in thread
From: Shakeel Butt @ 2025-01-16 0:56 UTC (permalink / raw)
To: Chen Ridong
Cc: akpm, mhocko, hannes, yosryahmed, roman.gushchin, muchun.song,
davidf, vbabka, mkoutny, linux-mm, linux-kernel, cgroups,
chenridong, wangweiyang2
On Tue, Jan 14, 2025 at 12:25:18PM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
>
> Factor out the 'replace_stock_objcg' function to make the code more
> cohesive.
>
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH -v2 next 4/4] memcg: factor out stat(event)/stat_local(event_local) reading functions
2025-01-14 12:25 [PATCH -v2 next 0/4] Some cleanup for memcg Chen Ridong
` (2 preceding siblings ...)
2025-01-14 12:25 ` [PATCH -v2 next 3/4] memcg: factor out the replace_stock_objcg function Chen Ridong
@ 2025-01-14 12:25 ` Chen Ridong
2025-01-14 18:45 ` Roman Gushchin
3 siblings, 1 reply; 12+ messages in thread
From: Chen Ridong @ 2025-01-14 12:25 UTC (permalink / raw)
To: akpm, mhocko, hannes, yosryahmed, roman.gushchin, shakeel.butt,
muchun.song, davidf, vbabka, mkoutny
Cc: linux-mm, linux-kernel, cgroups, chenridong, wangweiyang2
From: Chen Ridong <chenridong@huawei.com>
The only difference between 'lruvec_page_state' and
'lruvec_page_state_local' is that they read 'state' and 'state_local',
respectively. Factor out an inner functions to make the code more concise.
Do the same for reading 'memcg_page_stat' and 'memcg_events'.
Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
mm/memcontrol.c | 72 +++++++++++++++++++++----------------------------
1 file changed, 30 insertions(+), 42 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b10e0a8f3375..14541610cad0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -375,7 +375,8 @@ struct lruvec_stats {
long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
};
-unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
+static unsigned long __lruvec_page_state(struct lruvec *lruvec,
+ enum node_stat_item idx, bool local)
{
struct mem_cgroup_per_node *pn;
long x;
@@ -389,7 +390,8 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
return 0;
pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
- x = READ_ONCE(pn->lruvec_stats->state[i]);
+ x = local ? READ_ONCE(pn->lruvec_stats->state_local[i]) :
+ READ_ONCE(pn->lruvec_stats->state[i]);
#ifdef CONFIG_SMP
if (x < 0)
x = 0;
@@ -397,27 +399,16 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
return x;
}
+
+unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
+{
+ return __lruvec_page_state(lruvec, idx, false);
+}
+
unsigned long lruvec_page_state_local(struct lruvec *lruvec,
enum node_stat_item idx)
{
- struct mem_cgroup_per_node *pn;
- long x;
- int i;
-
- if (mem_cgroup_disabled())
- return node_page_state(lruvec_pgdat(lruvec), idx);
-
- i = memcg_stats_index(idx);
- if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
- return 0;
-
- pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
- x = READ_ONCE(pn->lruvec_stats->state_local[i]);
-#ifdef CONFIG_SMP
- if (x < 0)
- x = 0;
-#endif
- return x;
+ return __lruvec_page_state(lruvec, idx, true);
}
/* Subset of vm_event_item to report for memcg event stats */
@@ -651,7 +642,7 @@ static void flush_memcg_stats_dwork(struct work_struct *w)
queue_delayed_work(system_unbound_wq, &stats_flush_dwork, FLUSH_TIME);
}
-unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
+static unsigned long __memcg_page_state(struct mem_cgroup *memcg, int idx, bool local)
{
long x;
int i = memcg_stats_index(idx);
@@ -659,7 +650,9 @@ unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
return 0;
- x = READ_ONCE(memcg->vmstats->state[i]);
+ x = local ? READ_ONCE(memcg->vmstats->state_local[i]) :
+ READ_ONCE(memcg->vmstats->state[i]);
+
#ifdef CONFIG_SMP
if (x < 0)
x = 0;
@@ -667,6 +660,11 @@ unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
return x;
}
+unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
+{
+ return __memcg_page_state(memcg, idx, false);
+}
+
static int memcg_page_state_unit(int item);
/*
@@ -709,18 +707,7 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
/* idx can be of type enum memcg_stat_item or node_stat_item. */
unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
{
- long x;
- int i = memcg_stats_index(idx);
-
- if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
- return 0;
-
- x = READ_ONCE(memcg->vmstats->state_local[i]);
-#ifdef CONFIG_SMP
- if (x < 0)
- x = 0;
-#endif
- return x;
+ return __memcg_page_state(memcg, idx, true);
}
static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
@@ -859,24 +846,25 @@ void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
memcg_stats_unlock();
}
-unsigned long memcg_events(struct mem_cgroup *memcg, int event)
+static unsigned long __memcg_events(struct mem_cgroup *memcg, int event, bool local)
{
int i = memcg_events_index(event);
if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, event))
return 0;
- return READ_ONCE(memcg->vmstats->events[i]);
+ return local ? READ_ONCE(memcg->vmstats->events_local[i]) :
+ READ_ONCE(memcg->vmstats->events[i]);
}
-unsigned long memcg_events_local(struct mem_cgroup *memcg, int event)
+unsigned long memcg_events(struct mem_cgroup *memcg, int event)
{
- int i = memcg_events_index(event);
-
- if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, event))
- return 0;
+ return __memcg_events(memcg, event, false);
+}
- return READ_ONCE(memcg->vmstats->events_local[i]);
+unsigned long memcg_events_local(struct mem_cgroup *memcg, int event)
+{
+ return __memcg_events(memcg, event, true);
}
struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -v2 next 4/4] memcg: factor out stat(event)/stat_local(event_local) reading functions
2025-01-14 12:25 ` [PATCH -v2 next 4/4] memcg: factor out stat(event)/stat_local(event_local) reading functions Chen Ridong
@ 2025-01-14 18:45 ` Roman Gushchin
2025-01-16 0:57 ` Shakeel Butt
0 siblings, 1 reply; 12+ messages in thread
From: Roman Gushchin @ 2025-01-14 18:45 UTC (permalink / raw)
To: Chen Ridong
Cc: akpm, mhocko, hannes, yosryahmed, shakeel.butt, muchun.song,
davidf, vbabka, mkoutny, linux-mm, linux-kernel, cgroups,
chenridong, wangweiyang2
On Tue, Jan 14, 2025 at 12:25:19PM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
>
> The only difference between 'lruvec_page_state' and
> 'lruvec_page_state_local' is that they read 'state' and 'state_local',
> respectively. Factor out an inner functions to make the code more concise.
> Do the same for reading 'memcg_page_stat' and 'memcg_events'.
>
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> ---
> mm/memcontrol.c | 72 +++++++++++++++++++++----------------------------
> 1 file changed, 30 insertions(+), 42 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b10e0a8f3375..14541610cad0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -375,7 +375,8 @@ struct lruvec_stats {
> long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
> };
>
> -unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> +static unsigned long __lruvec_page_state(struct lruvec *lruvec,
> + enum node_stat_item idx, bool local)
> {
> struct mem_cgroup_per_node *pn;
> long x;
> @@ -389,7 +390,8 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> return 0;
>
> pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> - x = READ_ONCE(pn->lruvec_stats->state[i]);
> + x = local ? READ_ONCE(pn->lruvec_stats->state_local[i]) :
> + READ_ONCE(pn->lruvec_stats->state[i]);
> #ifdef CONFIG_SMP
> if (x < 0)
> x = 0;
> @@ -397,27 +399,16 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> return x;
> }
>
> +
> +unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> +{
> + return __lruvec_page_state(lruvec, idx, false);
> +}
I'd move these wrapper function definitions to memcontrol.h and make them
static inline.
Other than that, lgtm.
Thank you!
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -v2 next 4/4] memcg: factor out stat(event)/stat_local(event_local) reading functions
2025-01-14 18:45 ` Roman Gushchin
@ 2025-01-16 0:57 ` Shakeel Butt
2025-01-16 1:21 ` Chen Ridong
0 siblings, 1 reply; 12+ messages in thread
From: Shakeel Butt @ 2025-01-16 0:57 UTC (permalink / raw)
To: Roman Gushchin
Cc: Chen Ridong, akpm, mhocko, hannes, yosryahmed, muchun.song,
davidf, vbabka, mkoutny, linux-mm, linux-kernel, cgroups,
chenridong, wangweiyang2
On Tue, Jan 14, 2025 at 06:45:20PM +0000, Roman Gushchin wrote:
> On Tue, Jan 14, 2025 at 12:25:19PM +0000, Chen Ridong wrote:
> > From: Chen Ridong <chenridong@huawei.com>
> >
> > The only difference between 'lruvec_page_state' and
> > 'lruvec_page_state_local' is that they read 'state' and 'state_local',
> > respectively. Factor out an inner functions to make the code more concise.
> > Do the same for reading 'memcg_page_stat' and 'memcg_events'.
> >
> > Signed-off-by: Chen Ridong <chenridong@huawei.com>
> > ---
> > mm/memcontrol.c | 72 +++++++++++++++++++++----------------------------
> > 1 file changed, 30 insertions(+), 42 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index b10e0a8f3375..14541610cad0 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -375,7 +375,8 @@ struct lruvec_stats {
> > long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
> > };
> >
> > -unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> > +static unsigned long __lruvec_page_state(struct lruvec *lruvec,
> > + enum node_stat_item idx, bool local)
> > {
> > struct mem_cgroup_per_node *pn;
> > long x;
> > @@ -389,7 +390,8 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> > return 0;
> >
> > pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> > - x = READ_ONCE(pn->lruvec_stats->state[i]);
> > + x = local ? READ_ONCE(pn->lruvec_stats->state_local[i]) :
> > + READ_ONCE(pn->lruvec_stats->state[i]);
> > #ifdef CONFIG_SMP
> > if (x < 0)
> > x = 0;
> > @@ -397,27 +399,16 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> > return x;
> > }
> >
> > +
> > +unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> > +{
> > + return __lruvec_page_state(lruvec, idx, false);
> > +}
>
> I'd move these wrapper function definitions to memcontrol.h and make them
> static inline.
+1
>
> Other than that, lgtm.
>
> Thank you!
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH -v2 next 4/4] memcg: factor out stat(event)/stat_local(event_local) reading functions
2025-01-16 0:57 ` Shakeel Butt
@ 2025-01-16 1:21 ` Chen Ridong
0 siblings, 0 replies; 12+ messages in thread
From: Chen Ridong @ 2025-01-16 1:21 UTC (permalink / raw)
To: Shakeel Butt, Roman Gushchin
Cc: akpm, mhocko, hannes, yosryahmed, muchun.song, davidf, vbabka,
mkoutny, linux-mm, linux-kernel, cgroups, chenridong,
wangweiyang2
On 2025/1/16 8:57, Shakeel Butt wrote:
> On Tue, Jan 14, 2025 at 06:45:20PM +0000, Roman Gushchin wrote:
>> On Tue, Jan 14, 2025 at 12:25:19PM +0000, Chen Ridong wrote:
>>> From: Chen Ridong <chenridong@huawei.com>
>>>
>>> The only difference between 'lruvec_page_state' and
>>> 'lruvec_page_state_local' is that they read 'state' and 'state_local',
>>> respectively. Factor out an inner functions to make the code more concise.
>>> Do the same for reading 'memcg_page_stat' and 'memcg_events'.
>>>
>>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
>>> ---
>>> mm/memcontrol.c | 72 +++++++++++++++++++++----------------------------
>>> 1 file changed, 30 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index b10e0a8f3375..14541610cad0 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -375,7 +375,8 @@ struct lruvec_stats {
>>> long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
>>> };
>>>
>>> -unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
>>> +static unsigned long __lruvec_page_state(struct lruvec *lruvec,
>>> + enum node_stat_item idx, bool local)
>>> {
>>> struct mem_cgroup_per_node *pn;
>>> long x;
>>> @@ -389,7 +390,8 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
>>> return 0;
>>>
>>> pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
>>> - x = READ_ONCE(pn->lruvec_stats->state[i]);
>>> + x = local ? READ_ONCE(pn->lruvec_stats->state_local[i]) :
>>> + READ_ONCE(pn->lruvec_stats->state[i]);
>>> #ifdef CONFIG_SMP
>>> if (x < 0)
>>> x = 0;
>>> @@ -397,27 +399,16 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
>>> return x;
>>> }
>>>
>>> +
>>> +unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
>>> +{
>>> + return __lruvec_page_state(lruvec, idx, false);
>>> +}
>>
>> I'd move these wrapper function definitions to memcontrol.h and make them
>> static inline.
>
> +1
>
Thank you.
Will update.
Best regards,
Ridong
>>
>> Other than that, lgtm.
>>
>> Thank you!
^ permalink raw reply [flat|nested] 12+ messages in thread