diff mbox series

[v3,next,4/5] memcg: factor out stat(event)/stat_local(event_local) reading functions

Message ID 20250117014645.1673127-5-chenridong@huaweicloud.com (mailing list archive)
State New
Headers show
Series Some cleanup for memcg | expand

Commit Message

Chen Ridong Jan. 17, 2025, 1:46 a.m. UTC 
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>
---
 include/linux/memcontrol.h | 31 +++++++++++++++---
 mm/memcontrol-v1.h         | 14 ++++++--
 mm/memcontrol.c            | 67 +++++++-------------------------------
 3 files changed, 49 insertions(+), 63 deletions(-)

Comments

Johannes Weiner Jan. 17, 2025, 4:56 p.m. UTC | #1 
On Fri, Jan 17, 2025 at 01:46:44AM +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>

bool parameters make for poor readability at the callsites :(

With the next patch moving most of the duplication to memcontrol-v1.c,
I think it's probably not worth refactoring this.
Yosry Ahmed Jan. 17, 2025, 5:01 p.m. UTC | #2 
On Fri, Jan 17, 2025 at 8:56 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, Jan 17, 2025 at 01:46:44AM +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>
>
> bool parameters make for poor readability at the callsites :(
>
> With the next patch moving most of the duplication to memcontrol-v1.c,
> I think it's probably not worth refactoring this.

Arguably the duplication would now be across two different files,
making it more difficult to notice and keep the implementations in
sync.
Johannes Weiner Jan. 17, 2025, 6:02 p.m. UTC | #3 
On Fri, Jan 17, 2025 at 09:01:59AM -0800, Yosry Ahmed wrote:
> On Fri, Jan 17, 2025 at 8:56 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Fri, Jan 17, 2025 at 01:46:44AM +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>
> >
> > bool parameters make for poor readability at the callsites :(
> >
> > With the next patch moving most of the duplication to memcontrol-v1.c,
> > I think it's probably not worth refactoring this.
> 
> Arguably the duplication would now be across two different files,
> making it more difficult to notice and keep the implementations in
> sync.

Dependencies between the files is a bigger pain. E.g. try_charge()
being defined in memcontrol-v1.h makes memcontrol.c more difficult to
work with. That shared state also immediately bitrotted when charge
moving was removed and the last cgroup1 caller disappeared.

The whole point of the cgroup1 split was to simplify cgroup2 code. The
tiny amount of duplication in this case doesn't warrant further
entanglement between the codebases.
diff mbox series

Patch

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 6e74b8254d9b..ec469c5f7491 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -936,10 +936,33 @@  static inline void mod_memcg_page_state(struct page *page,
 	rcu_read_unlock();
 }
 
-unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx);
-unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx);
-unsigned long lruvec_page_state_local(struct lruvec *lruvec,
-				      enum node_stat_item idx);
+unsigned long __memcg_page_state(struct mem_cgroup *memcg, int idx, bool local);
+
+/* idx can be of type enum memcg_stat_item or node_stat_item. */
+static inline unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
+{
+	return __memcg_page_state(memcg, idx, true);
+}
+
+static inline unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
+{
+	return __memcg_page_state(memcg, idx, false);
+}
+
+unsigned long __lruvec_page_state(struct lruvec *lruvec,
+		enum node_stat_item idx, bool local);
+
+static inline unsigned long lruvec_page_state(struct lruvec *lruvec,
+					      enum node_stat_item idx)
+{
+	return __lruvec_page_state(lruvec, idx, false);
+}
+
+static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec,
+						    enum node_stat_item idx)
+{
+	return __lruvec_page_state(lruvec, idx, true);
+}
 
 void mem_cgroup_flush_stats(struct mem_cgroup *memcg);
 void mem_cgroup_flush_stats_ratelimited(struct mem_cgroup *memcg);
diff --git a/mm/memcontrol-v1.h b/mm/memcontrol-v1.h
index 144d71b65907..f68c0064d674 100644
--- a/mm/memcontrol-v1.h
+++ b/mm/memcontrol-v1.h
@@ -59,9 +59,17 @@  unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap);
 
 void drain_all_stock(struct mem_cgroup *root_memcg);
 
-unsigned long memcg_events(struct mem_cgroup *memcg, int event);
-unsigned long memcg_events_local(struct mem_cgroup *memcg, int event);
-unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx);
+unsigned long __memcg_events(struct mem_cgroup *memcg, int event, bool local);
+static inline unsigned long memcg_events(struct mem_cgroup *memcg, int event)
+{
+	return __memcg_events(memcg, event, false);
+}
+
+static inline unsigned long memcg_events_local(struct mem_cgroup *memcg, int event)
+{
+	return __memcg_events(memcg, event, true);
+}
+
 unsigned long memcg_page_state_output(struct mem_cgroup *memcg, int item);
 unsigned long memcg_page_state_local_output(struct mem_cgroup *memcg, int item);
 int memory_stat_show(struct seq_file *m, void *v);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b10e0a8f3375..404bbdfa352f 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)
+unsigned long __lruvec_page_state(struct lruvec *lruvec,
+		enum node_stat_item idx, bool local)
 {
 	struct mem_cgroup_per_node *pn;
 	long x;
@@ -389,30 +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]);
-#ifdef CONFIG_SMP
-	if (x < 0)
-		x = 0;
-#endif
-	return x;
-}
-
-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]);
+	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;
@@ -651,7 +630,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)
+unsigned long __memcg_page_state(struct mem_cgroup *memcg, int idx, bool local)
 {
 	long x;
 	int i = memcg_stats_index(idx);
@@ -659,7 +638,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;
@@ -706,23 +687,6 @@  void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
 	trace_mod_memcg_state(memcg, idx, val);
 }
 
-/* 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;
-}
-
 static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
 				     enum node_stat_item idx,
 				     int val)
@@ -859,24 +823,15 @@  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)
-{
-	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]);
-}
-
-unsigned long memcg_events_local(struct mem_cgroup *memcg, int event)
+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_local[i]);
+	return local ? READ_ONCE(memcg->vmstats->events_local[i]) :
+		    READ_ONCE(memcg->vmstats->events[i]);
 }
 
 struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)