linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing
@ 2023-04-21 17:40 Yosry Ahmed
  2023-04-21 17:40 ` [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section Yosry Ahmed
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Yosry Ahmed @ 2023-04-21 17:40 UTC (permalink / raw)
  To: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Tejun Heo
  Cc: Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm, Yosry Ahmed

A previous patch series ([1] currently in mm-stable) changed most
atomic rstat flushing contexts to become non-atomic. This was done to
avoid an expensive operation that scales with # cgroups and # cpus to
happen with irqs disabled and scheduling not permitted. There were two
remaining atomic flushing contexts after that series. This series tries
to eliminate them as well, eliminating atomic rstat flushing completely.

The two remaining atomic flushing contexts are:
(a) wb_over_bg_thresh()->mem_cgroup_wb_stats()
(b) mem_cgroup_threshold()->mem_cgroup_usage()

For (a), flushing needs to be atomic as wb_writeback() calls
wb_over_bg_thresh() with a spinlock held. However, it seems like the
call to wb_over_bg_thresh() doesn't need to be protected by that
spinlock, so this series proposes a refactoring that moves the call
outside the lock criticial section and makes the stats flushing
in mem_cgroup_wb_stats() non-atomic.

For (b), flushing needs to be atomic as mem_cgroup_threshold() is called
with irqs disabled. We only flush the stats when calculating the root
usage, as it is approximated as the sum of some memcg stats (file, anon,
and optionally swap) instead of the conventional page counter. This
series proposes changing this calculation to use the global stats
instead, eliminating the need for a memcg stat flush.

After these 2 contexts are eliminated, we no longer need
mem_cgroup_flush_stats_atomic() or cgroup_rstat_flush_atomic(). We can
remove them and simplify the code.

[1] https://lore.kernel.org/linux-mm/20230330191801.1967435-1-yosryahmed@google.com/

RFC -> v1:
- Collected R-b's and A-b's (Thanks everyone!).
- Rebased onto mm-stable.
- Cosmetic changes to commit logs.

RFC: https://lore.kernel.org/linux-mm/20230403220337.443510-1-yosryahmed@google.com/

Yosry Ahmed (5):
  writeback: move wb_over_bg_thresh() call outside lock section
  memcg: flush stats non-atomically in mem_cgroup_wb_stats()
  memcg: calculate root usage from global state
  memcg: remove mem_cgroup_flush_stats_atomic()
  cgroup: remove cgroup_rstat_flush_atomic()

 fs/fs-writeback.c          | 16 +++++++----
 include/linux/cgroup.h     |  1 -
 include/linux/memcontrol.h |  5 ----
 kernel/cgroup/rstat.c      | 26 ++++--------------
 mm/memcontrol.c            | 54 ++++++++------------------------------
 5 files changed, 27 insertions(+), 75 deletions(-)

-- 
2.40.0.634.g4ca3ef3211-goog



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section
  2023-04-21 17:40 [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
@ 2023-04-21 17:40 ` Yosry Ahmed
  2023-04-24 17:16   ` Shakeel Butt
  2023-05-05 15:18   ` Tejun Heo
  2023-04-21 17:40 ` [PATCH v5 2/5] memcg: flush stats non-atomically in mem_cgroup_wb_stats() Yosry Ahmed
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 12+ messages in thread
From: Yosry Ahmed @ 2023-04-21 17:40 UTC (permalink / raw)
  To: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Tejun Heo
  Cc: Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm, Yosry Ahmed

wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat
flush, which can be expensive on large systems. Currently,
wb_writeback() calls wb_over_bg_thresh() within a lock section, so we
have to do the rstat flush atomically. On systems with a lot of
cpus and/or cgroups, this can cause us to disable irqs for a long time,
potentially causing problems.

Move the call to wb_over_bg_thresh() outside the lock section in
preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic.
The list_empty(&wb->work_list) check should be okay outside the lock
section of wb->list_lock as it is protected by a separate lock
(wb->work_lock), and wb_over_bg_thresh() doesn't seem like it is
modifying any of wb->b_* lists the wb->list_lock is protecting.
Also, the loop seems to be already releasing and reacquring the
lock, so this refactoring looks safe.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/fs-writeback.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 1db3e3c24b43..11aa1652fb84 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2024,7 +2024,6 @@ static long wb_writeback(struct bdi_writeback *wb,
 	struct blk_plug plug;
 
 	blk_start_plug(&plug);
-	spin_lock(&wb->list_lock);
 	for (;;) {
 		/*
 		 * Stop writeback when nr_pages has been consumed
@@ -2049,6 +2048,9 @@ static long wb_writeback(struct bdi_writeback *wb,
 		if (work->for_background && !wb_over_bg_thresh(wb))
 			break;
 
+
+		spin_lock(&wb->list_lock);
+
 		/*
 		 * Kupdate and background works are special and we want to
 		 * include all inodes that need writing. Livelock avoidance is
@@ -2078,13 +2080,19 @@ static long wb_writeback(struct bdi_writeback *wb,
 		 * mean the overall work is done. So we keep looping as long
 		 * as made some progress on cleaning pages or inodes.
 		 */
-		if (progress)
+		if (progress) {
+			spin_unlock(&wb->list_lock);
 			continue;
+		}
+
 		/*
 		 * No more inodes for IO, bail
 		 */
-		if (list_empty(&wb->b_more_io))
+		if (list_empty(&wb->b_more_io)) {
+			spin_unlock(&wb->list_lock);
 			break;
+		}
+
 		/*
 		 * Nothing written. Wait for some inode to
 		 * become available for writeback. Otherwise
@@ -2096,9 +2104,7 @@ static long wb_writeback(struct bdi_writeback *wb,
 		spin_unlock(&wb->list_lock);
 		/* This function drops i_lock... */
 		inode_sleep_on_writeback(inode);
-		spin_lock(&wb->list_lock);
 	}
-	spin_unlock(&wb->list_lock);
 	blk_finish_plug(&plug);
 
 	return nr_pages - work->nr_pages;
-- 
2.40.0.634.g4ca3ef3211-goog



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v5 2/5] memcg: flush stats non-atomically in mem_cgroup_wb_stats()
  2023-04-21 17:40 [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
  2023-04-21 17:40 ` [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section Yosry Ahmed
@ 2023-04-21 17:40 ` Yosry Ahmed
  2023-05-05 15:19   ` Tejun Heo
  2023-04-21 17:40 ` [PATCH v5 3/5] memcg: calculate root usage from global state Yosry Ahmed
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Yosry Ahmed @ 2023-04-21 17:40 UTC (permalink / raw)
  To: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Tejun Heo
  Cc: Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm, Yosry Ahmed

The previous patch moved the wb_over_bg_thresh()->mem_cgroup_wb_stats()
code path in wb_writeback() outside the lock section. We no longer need
to flush the stats atomically. Flush the stats non-atomically.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Acked-by: Shakeel Butt <shakeelb@google.com>
---
 mm/memcontrol.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4b27e245a055..5e79fdf8442b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4648,11 +4648,7 @@ void mem_cgroup_wb_stats(struct bdi_writeback *wb, unsigned long *pfilepages,
 	struct mem_cgroup *memcg = mem_cgroup_from_css(wb->memcg_css);
 	struct mem_cgroup *parent;
 
-	/*
-	 * wb_writeback() takes a spinlock and calls
-	 * wb_over_bg_thresh()->mem_cgroup_wb_stats(). Do not sleep.
-	 */
-	mem_cgroup_flush_stats_atomic();
+	mem_cgroup_flush_stats();
 
 	*pdirty = memcg_page_state(memcg, NR_FILE_DIRTY);
 	*pwriteback = memcg_page_state(memcg, NR_WRITEBACK);
-- 
2.40.0.634.g4ca3ef3211-goog



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v5 3/5] memcg: calculate root usage from global state
  2023-04-21 17:40 [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
  2023-04-21 17:40 ` [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section Yosry Ahmed
  2023-04-21 17:40 ` [PATCH v5 2/5] memcg: flush stats non-atomically in mem_cgroup_wb_stats() Yosry Ahmed
@ 2023-04-21 17:40 ` Yosry Ahmed
  2023-04-21 17:40 ` [PATCH v5 4/5] memcg: remove mem_cgroup_flush_stats_atomic() Yosry Ahmed
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Yosry Ahmed @ 2023-04-21 17:40 UTC (permalink / raw)
  To: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Tejun Heo
  Cc: Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm, Yosry Ahmed

Currently, we approximate the root usage by adding the memcg stats for
anon, file, and conditionally swap (for memsw). To read the memcg stats
we need to invoke an rstat flush. rstat flushes can be expensive, they
scale with the number of cpus and cgroups on the system.

mem_cgroup_usage() is called by memcg_events()->mem_cgroup_threshold()
with irqs disabled, so such an expensive operation with irqs disabled
can cause problems.

Instead, approximate the root usage from global state. This is not 100%
accurate, but the root usage has always been ill-defined anyway.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Acked-by: Shakeel Butt <shakeelb@google.com>
---
 mm/memcontrol.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 5e79fdf8442b..cb78bba5b4a4 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3699,27 +3699,13 @@ static unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
 
 	if (mem_cgroup_is_root(memcg)) {
 		/*
-		 * We can reach here from irq context through:
-		 * uncharge_batch()
-		 * |--memcg_check_events()
-		 *    |--mem_cgroup_threshold()
-		 *       |--__mem_cgroup_threshold()
-		 *          |--mem_cgroup_usage
-		 *
-		 * rstat flushing is an expensive operation that should not be
-		 * done from irq context; use stale stats in this case.
-		 * Arguably, usage threshold events are not reliable on the root
-		 * memcg anyway since its usage is ill-defined.
-		 *
-		 * Additionally, other call paths through memcg_check_events()
-		 * disable irqs, so make sure we are flushing stats atomically.
+		 * Approximate root's usage from global state. This isn't
+		 * perfect, but the root usage was always an approximation.
 		 */
-		if (in_task())
-			mem_cgroup_flush_stats_atomic();
-		val = memcg_page_state(memcg, NR_FILE_PAGES) +
-			memcg_page_state(memcg, NR_ANON_MAPPED);
+		val = global_node_page_state(NR_FILE_PAGES) +
+			global_node_page_state(NR_ANON_MAPPED);
 		if (swap)
-			val += memcg_page_state(memcg, MEMCG_SWAP);
+			val += total_swap_pages - get_nr_swap_pages();
 	} else {
 		if (!swap)
 			val = page_counter_read(&memcg->memory);
-- 
2.40.0.634.g4ca3ef3211-goog



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v5 4/5] memcg: remove mem_cgroup_flush_stats_atomic()
  2023-04-21 17:40 [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
                   ` (2 preceding siblings ...)
  2023-04-21 17:40 ` [PATCH v5 3/5] memcg: calculate root usage from global state Yosry Ahmed
@ 2023-04-21 17:40 ` Yosry Ahmed
  2023-04-21 17:40 ` [PATCH v5 5/5] cgroup: remove cgroup_rstat_flush_atomic() Yosry Ahmed
  2023-04-21 18:54 ` [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
  5 siblings, 0 replies; 12+ messages in thread
From: Yosry Ahmed @ 2023-04-21 17:40 UTC (permalink / raw)
  To: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Tejun Heo
  Cc: Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm, Yosry Ahmed

Previous patches removed all callers of mem_cgroup_flush_stats_atomic().
Remove the function and simplify the code.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Acked-by: Shakeel Butt <shakeelb@google.com>
---
 include/linux/memcontrol.h |  5 -----
 mm/memcontrol.c            | 24 +++++-------------------
 2 files changed, 5 insertions(+), 24 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 222d7370134c..00a88cf947e1 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1038,7 +1038,6 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec,
 }
 
 void mem_cgroup_flush_stats(void);
-void mem_cgroup_flush_stats_atomic(void);
 void mem_cgroup_flush_stats_ratelimited(void);
 
 void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
@@ -1537,10 +1536,6 @@ static inline void mem_cgroup_flush_stats(void)
 {
 }
 
-static inline void mem_cgroup_flush_stats_atomic(void)
-{
-}
-
 static inline void mem_cgroup_flush_stats_ratelimited(void)
 {
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index cb78bba5b4a4..eadfe371a8f2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -639,7 +639,7 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
 	}
 }
 
-static void do_flush_stats(bool atomic)
+static void do_flush_stats(void)
 {
 	/*
 	 * We always flush the entire tree, so concurrent flushers can just
@@ -652,30 +652,16 @@ static void do_flush_stats(bool atomic)
 
 	WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME);
 
-	if (atomic)
-		cgroup_rstat_flush_atomic(root_mem_cgroup->css.cgroup);
-	else
-		cgroup_rstat_flush(root_mem_cgroup->css.cgroup);
+	cgroup_rstat_flush(root_mem_cgroup->css.cgroup);
 
 	atomic_set(&stats_flush_threshold, 0);
 	atomic_set(&stats_flush_ongoing, 0);
 }
 
-static bool should_flush_stats(void)
-{
-	return atomic_read(&stats_flush_threshold) > num_online_cpus();
-}
-
 void mem_cgroup_flush_stats(void)
 {
-	if (should_flush_stats())
-		do_flush_stats(false);
-}
-
-void mem_cgroup_flush_stats_atomic(void)
-{
-	if (should_flush_stats())
-		do_flush_stats(true);
+	if (atomic_read(&stats_flush_threshold) > num_online_cpus())
+		do_flush_stats();
 }
 
 void mem_cgroup_flush_stats_ratelimited(void)
@@ -690,7 +676,7 @@ static void flush_memcg_stats_dwork(struct work_struct *w)
 	 * Always flush here so that flushing in latency-sensitive paths is
 	 * as cheap as possible.
 	 */
-	do_flush_stats(false);
+	do_flush_stats();
 	queue_delayed_work(system_unbound_wq, &stats_flush_dwork, FLUSH_TIME);
 }
 
-- 
2.40.0.634.g4ca3ef3211-goog



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v5 5/5] cgroup: remove cgroup_rstat_flush_atomic()
  2023-04-21 17:40 [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
                   ` (3 preceding siblings ...)
  2023-04-21 17:40 ` [PATCH v5 4/5] memcg: remove mem_cgroup_flush_stats_atomic() Yosry Ahmed
@ 2023-04-21 17:40 ` Yosry Ahmed
  2023-04-24 17:17   ` Shakeel Butt
  2023-05-05 15:20   ` Tejun Heo
  2023-04-21 18:54 ` [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
  5 siblings, 2 replies; 12+ messages in thread
From: Yosry Ahmed @ 2023-04-21 17:40 UTC (permalink / raw)
  To: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Tejun Heo
  Cc: Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm, Yosry Ahmed

Previous patches removed the only caller of cgroup_rstat_flush_atomic().
Remove the function and simplify the code.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
---
 include/linux/cgroup.h |  1 -
 kernel/cgroup/rstat.c  | 26 +++++---------------------
 2 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 885f5395fcd0..567c547cf371 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -692,7 +692,6 @@ static inline void cgroup_path_from_kernfs_id(u64 id, char *buf, size_t buflen)
  */
 void cgroup_rstat_updated(struct cgroup *cgrp, int cpu);
 void cgroup_rstat_flush(struct cgroup *cgrp);
-void cgroup_rstat_flush_atomic(struct cgroup *cgrp);
 void cgroup_rstat_flush_hold(struct cgroup *cgrp);
 void cgroup_rstat_flush_release(void);
 
diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index d3252b0416b6..f9ad33f117c8 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -171,7 +171,7 @@ __weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
 __diag_pop();
 
 /* see cgroup_rstat_flush() */
-static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep)
+static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
 	__releases(&cgroup_rstat_lock) __acquires(&cgroup_rstat_lock)
 {
 	int cpu;
@@ -207,9 +207,8 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep)
 		}
 		raw_spin_unlock_irqrestore(cpu_lock, flags);
 
-		/* if @may_sleep, play nice and yield if necessary */
-		if (may_sleep && (need_resched() ||
-				  spin_needbreak(&cgroup_rstat_lock))) {
+		/* play nice and yield if necessary */
+		if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
 			spin_unlock_irq(&cgroup_rstat_lock);
 			if (!cond_resched())
 				cpu_relax();
@@ -236,25 +235,10 @@ __bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
 	might_sleep();
 
 	spin_lock_irq(&cgroup_rstat_lock);
-	cgroup_rstat_flush_locked(cgrp, true);
+	cgroup_rstat_flush_locked(cgrp);
 	spin_unlock_irq(&cgroup_rstat_lock);
 }
 
-/**
- * cgroup_rstat_flush_atomic- atomic version of cgroup_rstat_flush()
- * @cgrp: target cgroup
- *
- * This function can be called from any context.
- */
-void cgroup_rstat_flush_atomic(struct cgroup *cgrp)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&cgroup_rstat_lock, flags);
-	cgroup_rstat_flush_locked(cgrp, false);
-	spin_unlock_irqrestore(&cgroup_rstat_lock, flags);
-}
-
 /**
  * cgroup_rstat_flush_hold - flush stats in @cgrp's subtree and hold
  * @cgrp: target cgroup
@@ -269,7 +253,7 @@ void cgroup_rstat_flush_hold(struct cgroup *cgrp)
 {
 	might_sleep();
 	spin_lock_irq(&cgroup_rstat_lock);
-	cgroup_rstat_flush_locked(cgrp, true);
+	cgroup_rstat_flush_locked(cgrp);
 }
 
 /**
-- 
2.40.0.634.g4ca3ef3211-goog



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing
  2023-04-21 17:40 [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
                   ` (4 preceding siblings ...)
  2023-04-21 17:40 ` [PATCH v5 5/5] cgroup: remove cgroup_rstat_flush_atomic() Yosry Ahmed
@ 2023-04-21 18:54 ` Yosry Ahmed
  5 siblings, 0 replies; 12+ messages in thread
From: Yosry Ahmed @ 2023-04-21 18:54 UTC (permalink / raw)
  To: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Tejun Heo
  Cc: Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm

On Fri, Apr 21, 2023 at 10:40 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> A previous patch series ([1] currently in mm-stable) changed most
> atomic rstat flushing contexts to become non-atomic. This was done to
> avoid an expensive operation that scales with # cgroups and # cpus to
> happen with irqs disabled and scheduling not permitted. There were two
> remaining atomic flushing contexts after that series. This series tries
> to eliminate them as well, eliminating atomic rstat flushing completely.
>
> The two remaining atomic flushing contexts are:
> (a) wb_over_bg_thresh()->mem_cgroup_wb_stats()
> (b) mem_cgroup_threshold()->mem_cgroup_usage()
>
> For (a), flushing needs to be atomic as wb_writeback() calls
> wb_over_bg_thresh() with a spinlock held. However, it seems like the
> call to wb_over_bg_thresh() doesn't need to be protected by that
> spinlock, so this series proposes a refactoring that moves the call
> outside the lock criticial section and makes the stats flushing
> in mem_cgroup_wb_stats() non-atomic.
>
> For (b), flushing needs to be atomic as mem_cgroup_threshold() is called
> with irqs disabled. We only flush the stats when calculating the root
> usage, as it is approximated as the sum of some memcg stats (file, anon,
> and optionally swap) instead of the conventional page counter. This
> series proposes changing this calculation to use the global stats
> instead, eliminating the need for a memcg stat flush.
>
> After these 2 contexts are eliminated, we no longer need
> mem_cgroup_flush_stats_atomic() or cgroup_rstat_flush_atomic(). We can
> remove them and simplify the code.
>
> [1] https://lore.kernel.org/linux-mm/20230330191801.1967435-1-yosryahmed@google.com/
>
> RFC -> v1:
> - Collected R-b's and A-b's (Thanks everyone!).
> - Rebased onto mm-stable.
> - Cosmetic changes to commit logs.
>
> RFC: https://lore.kernel.org/linux-mm/20230403220337.443510-1-yosryahmed@google.com/

This is v1, not v5. I really suck at sending emails. Sorry.

>
> Yosry Ahmed (5):
>   writeback: move wb_over_bg_thresh() call outside lock section
>   memcg: flush stats non-atomically in mem_cgroup_wb_stats()
>   memcg: calculate root usage from global state
>   memcg: remove mem_cgroup_flush_stats_atomic()
>   cgroup: remove cgroup_rstat_flush_atomic()
>
>  fs/fs-writeback.c          | 16 +++++++----
>  include/linux/cgroup.h     |  1 -
>  include/linux/memcontrol.h |  5 ----
>  kernel/cgroup/rstat.c      | 26 ++++--------------
>  mm/memcontrol.c            | 54 ++++++++------------------------------
>  5 files changed, 27 insertions(+), 75 deletions(-)
>
> --
> 2.40.0.634.g4ca3ef3211-goog
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section
  2023-04-21 17:40 ` [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section Yosry Ahmed
@ 2023-04-24 17:16   ` Shakeel Butt
  2023-05-05 15:18   ` Tejun Heo
  1 sibling, 0 replies; 12+ messages in thread
From: Shakeel Butt @ 2023-04-24 17:16 UTC (permalink / raw)
  To: Yosry Ahmed
  Cc: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Muchun Song, Andrew Morton, Tejun Heo, Jan Kara,
	Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm

On Fri, Apr 21, 2023 at 10:40 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat
> flush, which can be expensive on large systems. Currently,
> wb_writeback() calls wb_over_bg_thresh() within a lock section, so we
> have to do the rstat flush atomically. On systems with a lot of
> cpus and/or cgroups, this can cause us to disable irqs for a long time,
> potentially causing problems.
>
> Move the call to wb_over_bg_thresh() outside the lock section in
> preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic.
> The list_empty(&wb->work_list) check should be okay outside the lock
> section of wb->list_lock as it is protected by a separate lock
> (wb->work_lock), and wb_over_bg_thresh() doesn't seem like it is
> modifying any of wb->b_* lists the wb->list_lock is protecting.
> Also, the loop seems to be already releasing and reacquring the
> lock, so this refactoring looks safe.
>
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> Reviewed-by: Michal Koutný <mkoutny@suse.com>
> Reviewed-by: Jan Kara <jack@suse.cz>

Acked-by: Shakeel Butt <shakeelb@google.com>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 5/5] cgroup: remove cgroup_rstat_flush_atomic()
  2023-04-21 17:40 ` [PATCH v5 5/5] cgroup: remove cgroup_rstat_flush_atomic() Yosry Ahmed
@ 2023-04-24 17:17   ` Shakeel Butt
  2023-05-05 15:20   ` Tejun Heo
  1 sibling, 0 replies; 12+ messages in thread
From: Shakeel Butt @ 2023-04-24 17:17 UTC (permalink / raw)
  To: Yosry Ahmed
  Cc: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Muchun Song, Andrew Morton, Tejun Heo, Jan Kara,
	Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm

On Fri, Apr 21, 2023 at 10:40 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> Previous patches removed the only caller of cgroup_rstat_flush_atomic().
> Remove the function and simplify the code.
>
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Acked-by: Shakeel Butt <shakeelb@google.com>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section
  2023-04-21 17:40 ` [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section Yosry Ahmed
  2023-04-24 17:16   ` Shakeel Butt
@ 2023-05-05 15:18   ` Tejun Heo
  1 sibling, 0 replies; 12+ messages in thread
From: Tejun Heo @ 2023-05-05 15:18 UTC (permalink / raw)
  To: Yosry Ahmed
  Cc: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm

On Fri, Apr 21, 2023 at 05:40:16PM +0000, Yosry Ahmed wrote:
> wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat
> flush, which can be expensive on large systems. Currently,
> wb_writeback() calls wb_over_bg_thresh() within a lock section, so we
> have to do the rstat flush atomically. On systems with a lot of
> cpus and/or cgroups, this can cause us to disable irqs for a long time,
> potentially causing problems.
> 
> Move the call to wb_over_bg_thresh() outside the lock section in
> preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic.
> The list_empty(&wb->work_list) check should be okay outside the lock
> section of wb->list_lock as it is protected by a separate lock
> (wb->work_lock), and wb_over_bg_thresh() doesn't seem like it is
> modifying any of wb->b_* lists the wb->list_lock is protecting.
> Also, the loop seems to be already releasing and reacquring the
> lock, so this refactoring looks safe.
> 
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> Reviewed-by: Michal Koutný <mkoutny@suse.com>
> Reviewed-by: Jan Kara <jack@suse.cz>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 2/5] memcg: flush stats non-atomically in mem_cgroup_wb_stats()
  2023-04-21 17:40 ` [PATCH v5 2/5] memcg: flush stats non-atomically in mem_cgroup_wb_stats() Yosry Ahmed
@ 2023-05-05 15:19   ` Tejun Heo
  0 siblings, 0 replies; 12+ messages in thread
From: Tejun Heo @ 2023-05-05 15:19 UTC (permalink / raw)
  To: Yosry Ahmed
  Cc: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm

On Fri, Apr 21, 2023 at 05:40:17PM +0000, Yosry Ahmed wrote:
> The previous patch moved the wb_over_bg_thresh()->mem_cgroup_wb_stats()
> code path in wb_writeback() outside the lock section. We no longer need
> to flush the stats atomically. Flush the stats non-atomically.
> 
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> Reviewed-by: Michal Koutný <mkoutny@suse.com>
> Acked-by: Shakeel Butt <shakeelb@google.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 5/5] cgroup: remove cgroup_rstat_flush_atomic()
  2023-04-21 17:40 ` [PATCH v5 5/5] cgroup: remove cgroup_rstat_flush_atomic() Yosry Ahmed
  2023-04-24 17:17   ` Shakeel Butt
@ 2023-05-05 15:20   ` Tejun Heo
  1 sibling, 0 replies; 12+ messages in thread
From: Tejun Heo @ 2023-05-05 15:20 UTC (permalink / raw)
  To: Yosry Ahmed
  Cc: Alexander Viro, Christian Brauner, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Shakeel Butt, Muchun Song, Andrew Morton,
	Jan Kara, Jens Axboe, Michal Koutný,
	linux-fsdevel, linux-kernel, cgroups, linux-mm

On Fri, Apr 21, 2023 at 05:40:20PM +0000, Yosry Ahmed wrote:
> Previous patches removed the only caller of cgroup_rstat_flush_atomic().
> Remove the function and simplify the code.
> 
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-05-05 15:20 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-21 17:40 [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed
2023-04-21 17:40 ` [PATCH v5 1/5] writeback: move wb_over_bg_thresh() call outside lock section Yosry Ahmed
2023-04-24 17:16   ` Shakeel Butt
2023-05-05 15:18   ` Tejun Heo
2023-04-21 17:40 ` [PATCH v5 2/5] memcg: flush stats non-atomically in mem_cgroup_wb_stats() Yosry Ahmed
2023-05-05 15:19   ` Tejun Heo
2023-04-21 17:40 ` [PATCH v5 3/5] memcg: calculate root usage from global state Yosry Ahmed
2023-04-21 17:40 ` [PATCH v5 4/5] memcg: remove mem_cgroup_flush_stats_atomic() Yosry Ahmed
2023-04-21 17:40 ` [PATCH v5 5/5] cgroup: remove cgroup_rstat_flush_atomic() Yosry Ahmed
2023-04-24 17:17   ` Shakeel Butt
2023-05-05 15:20   ` Tejun Heo
2023-04-21 18:54 ` [PATCH v5 0/5] cgroup: eliminate atomic rstat flushing Yosry Ahmed

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).