On Thu, Sep 14, 2023 at 10:19 AM Waiman Long <longman@xxxxxxxxxx> wrote: > > > On 9/13/23 03:38, Yosry Ahmed wrote: > > Stats flushing for memcg currently follows the following rules: > > - Always flush the entire memcg hierarchy (i.e. flush the root). > > - Only one flusher is allowed at a time. If someone else tries to flush > > concurrently, they skip and return immediately. > > - A periodic flusher flushes all the stats every 2 seconds. > > > > The reason this approach is followed is because all flushes are > > serialized by a global rstat spinlock. On the memcg side, flushing is > > invoked from userspace reads as well as in-kernel flushers (e.g. > > reclaim, refault, etc). This approach aims to avoid serializing all > > flushers on the global lock, which can cause a significant performance > > hit under high concurrency. > > > > This approach has the following problems: > > - Occasionally a userspace read of the stats of a non-root cgroup will > > be too expensive as it has to flush the entire hierarchy [1]. > > - Sometimes the stats accuracy are compromised if there is an ongoing > > flush, and we skip and return before the subtree of interest is > > actually flushed. This is more visible when reading stats from > > userspace, but can also affect in-kernel flushers. > > > > This patch aims to solve both problems by reworking how flushing > > currently works as follows: > > - Without contention, there is no need to flush the entire tree. In this > > case, only flush the subtree of interest. This avoids the latency of a > > full root flush if unnecessary. > > - With contention, fallback to a coalesced (aka unified) flush of the > > entire hierarchy, a root flush. In this case, instead of returning > > immediately if a root flush is ongoing, wait for it to finish > > *without* attempting to acquire the lock or flush. This is done using > > a completion. Compared to competing directly on the underlying lock, > > this approach makes concurrent flushing a synchronization point > > instead of a serialization point. Once a root flush finishes, *all* > > waiters can wake up and continue at once. > > - Finally, with very high contention, bound the number of waiters to the > > number of online cpus. This keeps the flush latency bounded at the tail > > (very high concurrency). We fallback to sacrificing stats freshness only > > in such cases in favor of performance. > > > > This was tested in two ways on a machine with 384 cpus: > > - A synthetic test with 5000 concurrent workers doing allocations and > > reclaim, as well as 1000 readers for memory.stat (variation of [2]). > > No significant regressions were noticed in the total runtime. > > Note that if concurrent flushers compete directly on the spinlock > > instead of waiting for a completion, this test shows 2x-3x slowdowns. > > Even though subsequent flushers would have nothing to flush, just the > > serialization and lock contention is a major problem. Using a > > completion for synchronization instead seems to overcome this problem. > > > > - A synthetic stress test for concurrently reading memcg stats provided > > by Wei Xu. > > With 10k threads reading the stats every 100ms: > > - 98.8% of reads take <100us > > - 1.09% of reads take 100us to 1ms. > > - 0.11% of reads take 1ms to 10ms. > > - Almost no reads take more than 10ms. > > With 10k threads reading the stats every 10ms: > > - 82.3% of reads take <100us. > > - 4.2% of reads take 100us to 1ms. > > - 4.7% of reads take 1ms to 10ms. > > - 8.8% of reads take 10ms to 100ms. > > - Almost no reads take more than 100ms. > > > > [1] https://lore.kernel.org/lkml/CABWYdi0c6__rh-K7dcM_pkf9BJdTRtAU08M43KO9ME4-dsgfoQ@xxxxxxxxxxxxxx/ > > [2] https://lore.kernel.org/lkml/CAJD7tka13M-zVZTyQJYL1iUAYvuQ1fcHbCjcOBZcz6POYTV-4g@xxxxxxxxxxxxxx/ > > [3] https://lore.kernel.org/lkml/CAAPL-u9D2b=iF5Lf_cRnKxUfkiEe0AMDTu6yhrUAzX0b6a6rDg@xxxxxxxxxxxxxx/ > > > > [weixugc@xxxxxxxxxx: suggested the fallback logic and bounding the > > number of waiters] > > > > Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > > --- > > include/linux/memcontrol.h | 4 +- > > mm/memcontrol.c | 100 ++++++++++++++++++++++++++++--------- > > mm/vmscan.c | 2 +- > > mm/workingset.c | 8 ++- > > 4 files changed, 85 insertions(+), 29 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index 11810a2cfd2d..4453cd3fc4b8 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -1034,7 +1034,7 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, > > return x; > > } > > > > -void mem_cgroup_flush_stats(void); > > +void mem_cgroup_flush_stats(struct mem_cgroup *memcg); > > void mem_cgroup_flush_stats_ratelimited(void); > > > > void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > > @@ -1519,7 +1519,7 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, > > return node_page_state(lruvec_pgdat(lruvec), idx); > > } > > > > -static inline void mem_cgroup_flush_stats(void) > > +static inline void mem_cgroup_flush_stats(struct mem_cgroup *memcg) > > { > > } > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index d729870505f1..edff41e4b4e7 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -588,7 +588,6 @@ mem_cgroup_largest_soft_limit_node(struct mem_cgroup_tree_per_node *mctz) > > static void flush_memcg_stats_dwork(struct work_struct *w); > > static DECLARE_DEFERRABLE_WORK(stats_flush_dwork, flush_memcg_stats_dwork); > > static DEFINE_PER_CPU(unsigned int, stats_updates); > > -static atomic_t stats_flush_ongoing = ATOMIC_INIT(0); > > /* stats_updates_order is in multiples of MEMCG_CHARGE_BATCH */ > > static atomic_t stats_updates_order = ATOMIC_INIT(0); > > static u64 flush_last_time; > > @@ -639,36 +638,87 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val) > > } > > } > > > > -static void do_flush_stats(void) > > +/* > > + * do_flush_stats - flush the statistics of a memory cgroup and its tree > > + * @memcg: the memory cgroup to flush > > + * @wait: wait for an ongoing root flush to complete before returning > > + * > > + * All flushes are serialized by the underlying rstat global lock. If there is > > + * no contention, we try to only flush the subtree of the passed @memcg to > > + * minimize the work. Otherwise, we coalesce multiple flushing requests into a > > + * single flush of the root memcg. When there is an ongoing root flush, we wait > > + * for its completion (unless otherwise requested), to get fresh stats. If the > > + * number of waiters exceeds the number of cpus just skip the flush to bound the > > + * flush latency at the tail with very high concurrency. > > + * > > + * This is a trade-off between stats accuracy and flush latency. > > + */ > > +static void do_flush_stats(struct mem_cgroup *memcg, bool wait) > > { > > + static DECLARE_COMPLETION(root_flush_done); > > + static DEFINE_SPINLOCK(root_flusher_lock); > > + static DEFINE_MUTEX(subtree_flush_mutex); > > + static atomic_t waiters = ATOMIC_INIT(0); > > + static bool root_flush_ongoing; > > + bool root_flusher = false; > > + > > + /* Ongoing root flush, just wait for it (unless otherwise requested) */ > > + if (READ_ONCE(root_flush_ongoing)) > > + goto root_flush_or_wait; > > + > > /* > > - * We always flush the entire tree, so concurrent flushers can just > > - * skip. This avoids a thundering herd problem on the rstat global lock > > - * from memcg flushers (e.g. reclaim, refault, etc). > > + * Opportunistically try to only flush the requested subtree. Otherwise > > + * fallback to a coalesced flush below. > > */ > > - if (atomic_read(&stats_flush_ongoing) || > > - atomic_xchg(&stats_flush_ongoing, 1)) > > + if (!mem_cgroup_is_root(memcg) && mutex_trylock(&subtree_flush_mutex)) { > > + cgroup_rstat_flush(memcg->css.cgroup); > > + mutex_unlock(&subtree_flush_mutex); > > return; > > + } > > If mutex_trylock() is the only way to acquire subtree_flush_mutex, you > don't really need a mutex. Just a simple integer flag with xchg() call > should be enough. Thanks for pointing this out. Agreed. If we keep this approach I will drop that mutex. > > Cheers, > Longman >
