| Message ID | 20220226002412.113819-1-shakeelb@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | memcg: async flush memcg stats from perf sensitive codepaths | expand |
On Fri, Feb 25, 2022 at 05:42:57PM -0800, Shakeel Butt <shakeelb@google.com> wrote: > Yes, the right fix would be to optimize the flushing code (but that > would require more work/time). However I still think letting > performance critical code paths to skip the sync flush would be good > in general. So, if the current patch is not to your liking we can > remove mem_cgroup_flush_stats() from workingset_refault(). What about flushing just the subtree of the memcg where the refault happens? It doesn't reduce the overall work and there's still full-tree cgroup_rstat_lock but it should make the chunks of work smaller durations more regular. Michal
On Mon, Feb 28, 2022 at 10:46 AM Michal Koutný <mkoutny@suse.com> wrote: > > On Fri, Feb 25, 2022 at 05:42:57PM -0800, Shakeel Butt <shakeelb@google.com> wrote: > > Yes, the right fix would be to optimize the flushing code (but that > > would require more work/time). However I still think letting > > performance critical code paths to skip the sync flush would be good > > in general. So, if the current patch is not to your liking we can > > remove mem_cgroup_flush_stats() from workingset_refault(). > > What about flushing just the subtree of the memcg where the refault > happens? > It doesn't reduce the overall work and there's still full-tree > cgroup_rstat_lock but it should make the chunks of work smaller > durations more regular. > We can try that and I will send a patch to Ivan and Daniel to try on their workload to see the real impact of targeted memcg flushing. However I am not very optimistic about it.
On Fri 25-02-22 16:24:12, Shakeel Butt wrote: > Daniel Dao has reported [1] a regression on workloads that may trigger > a lot of refaults (anon and file). The underlying issue is that flushing > rstat is expensive. Although rstat flush are batched with (nr_cpus * > MEMCG_BATCH) stat updates, it seems like there are workloads which > genuinely do stat updates larger than batch value within short amount of > time. Since the rstat flush can happen in the performance critical > codepaths like page faults, such workload can suffer greatly. > > The easiest fix for now is for performance critical codepaths trigger > the rstat flush asynchronously. This patch converts the refault codepath > to use async rstat flush. In addition, this patch has premptively > converted mem_cgroup_wb_stats and shrink_node to also use the async > rstat flush as they may also similar performance regressions. Why do we need to trigger flushing in the first place from those paths. Later in the thread you are saying there is a regular flushing done every 2 seconds. What would happen if these paths didn't flush at all? Also please note that WQ context can be overwhelmed by other work so these flushes can happen much much later. So in other words why does async work (that can happen at any time without any control) make more sense than no flushing?
On Tue, Mar 1, 2022 at 1:05 AM Michal Hocko <mhocko@suse.com> wrote: > > On Fri 25-02-22 16:24:12, Shakeel Butt wrote: > > Daniel Dao has reported [1] a regression on workloads that may trigger > > a lot of refaults (anon and file). The underlying issue is that flushing > > rstat is expensive. Although rstat flush are batched with (nr_cpus * > > MEMCG_BATCH) stat updates, it seems like there are workloads which > > genuinely do stat updates larger than batch value within short amount of > > time. Since the rstat flush can happen in the performance critical > > codepaths like page faults, such workload can suffer greatly. > > > > The easiest fix for now is for performance critical codepaths trigger > > the rstat flush asynchronously. This patch converts the refault codepath > > to use async rstat flush. In addition, this patch has premptively > > converted mem_cgroup_wb_stats and shrink_node to also use the async > > rstat flush as they may also similar performance regressions. > > Why do we need to trigger flushing in the first place from those paths. > Later in the thread you are saying there is a regular flushing done > every 2 seconds. What would happen if these paths didn't flush at all? > Also please note that WQ context can be overwhelmed by other work so > these flushes can happen much much later. > > So in other words why does async work (that can happen at any time > without any control) make more sense than no flushing? > -- Without flushing the worst that can happen in the refault path is false (or missed) activations of the refaulted page. For reclaim code, some heuristics (like deactivating active LRU or cache-trim) may act on old information. However I don't think these are too much concerning as the kernel can already missed or do false activations on refault. For the reclaim code, the kernel does force deactivation if it has skipped it in the initial iterations, so, not much to worry. Now, coming to your question, yes, we can remove the flushing from these performance critical codepaths as the stats at most will be 2 second old due to periodic flush. Now for the worst case scenario where that periodic flush (WQ) is not getting CPU, I think it is reasonable to put a sync flush if periodic flush has not happened for, let's say, 10 seconds.
Making decisions based on up to 2 s old information. On Tue, Mar 01, 2022 at 09:21:12AM -0800, Shakeel Butt <shakeelb@google.com> wrote: > Without flushing the worst that can happen in the refault path is > false (or missed) activations of the refaulted page. Yeah, this may under- or overestimate workingset size (when it's changing), the result is likely only less efficient reclaim. > For reclaim code, some heuristics (like deactivating active LRU or > cache-trim) may act on old information. Here, I'd be more careful whether such a delay cannot introduce some unstable behavior (permanent oscillation in the worst case). > Now, coming to your question, yes, we can remove the flushing from > these performance critical codepaths as the stats at most will be 2 > second old due to periodic flush. Another aspect is that people will notice and report such a narrowly located performance regression more easily than reduced/less predictable reclaim behavior. (IMO the former is better, OTOH, it can also be interpreted that noone notices (is able to notice).) Michal
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index ef4b445392a9..bfdd48be60ff 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -998,6 +998,7 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, } void mem_cgroup_flush_stats(void); +void mem_cgroup_flush_stats_async(void); void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, int val); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index c695608c521c..4338e8d779b2 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -690,6 +690,14 @@ void mem_cgroup_flush_stats(void) __mem_cgroup_flush_stats(); } +void mem_cgroup_flush_stats_async(void) +{ + if (atomic_read(&stats_flush_threshold) > num_online_cpus()) { + atomic_set(&stats_flush_threshold, 0); + mod_delayed_work(system_unbound_wq, &stats_flush_dwork, 0); + } +} + static void flush_memcg_stats_dwork(struct work_struct *w) { __mem_cgroup_flush_stats(); @@ -4522,7 +4530,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; - mem_cgroup_flush_stats(); + mem_cgroup_flush_stats_async(); *pdirty = memcg_page_state(memcg, NR_FILE_DIRTY); *pwriteback = memcg_page_state(memcg, NR_WRITEBACK); diff --git a/mm/vmscan.c b/mm/vmscan.c index c6f77e3e6d59..b6c6b165c1ef 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -3188,7 +3188,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) * Flush the memory cgroup stats, so that we read accurate per-memcg * lruvec stats for heuristics. */ - mem_cgroup_flush_stats(); + mem_cgroup_flush_stats_async(); memset(&sc->nr, 0, sizeof(sc->nr)); diff --git a/mm/workingset.c b/mm/workingset.c index b717eae4e0dd..a4f2b1aa5bcc 100644 --- a/mm/workingset.c +++ b/mm/workingset.c @@ -355,7 +355,7 @@ void workingset_refault(struct folio *folio, void *shadow) mod_lruvec_state(lruvec, WORKINGSET_REFAULT_BASE + file, nr); - mem_cgroup_flush_stats(); + mem_cgroup_flush_stats_async(); /* * Compare the distance to the existing workingset size. We * don't activate pages that couldn't stay resident even if
Daniel Dao has reported [1] a regression on workloads that may trigger a lot of refaults (anon and file). The underlying issue is that flushing rstat is expensive. Although rstat flush are batched with (nr_cpus * MEMCG_BATCH) stat updates, it seems like there are workloads which genuinely do stat updates larger than batch value within short amount of time. Since the rstat flush can happen in the performance critical codepaths like page faults, such workload can suffer greatly. The easiest fix for now is for performance critical codepaths trigger the rstat flush asynchronously. This patch converts the refault codepath to use async rstat flush. In addition, this patch has premptively converted mem_cgroup_wb_stats and shrink_node to also use the async rstat flush as they may also similar performance regressions. Link: https://lore.kernel.org/all/CA+wXwBSyO87ZX5PVwdHm-=dBjZYECGmfnydUicUyrQqndgX2MQ@mail.gmail.com [1] Fixes: 1f828223b799 ("memcg: flush lruvec stats in the refault") Reported-by: Daniel Dao <dqminh@cloudflare.com> Signed-off-by: Shakeel Butt <shakeelb@google.com> Cc: <stable@vger.kernel.org> --- include/linux/memcontrol.h | 1 + mm/memcontrol.c | 10 +++++++++- mm/vmscan.c | 2 +- mm/workingset.c | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-)