| Message ID | 20230821205458.1764662-3-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memcg: non-unified flushing for userspace stats | expand |
On Mon, Aug 21, 2023 at 08:54:57PM +0000, Yosry Ahmed <yosryahmed@google.com> wrote: > +static void do_stats_flush(struct mem_cgroup *memcg) > +{ > + cgroup_rstat_flush(memcg->css.cgroup); if(memcg == root_mem_cgroup) atomic_set(&stats_flush_threshold, 0); > +} > + > /* > * do_unified_stats_flush - do a unified flush of memory cgroup statistics > * > @@ -656,7 +667,7 @@ static void do_unified_stats_flush(void) > > WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME); > > - cgroup_rstat_flush(root_mem_cgroup->css.cgroup); > + do_stats_flush(root_mem_cgroup); > - atomic_set(&stats_flush_threshold, 0); > atomic_set(&stats_flush_ongoing, 0); You may reset stats_flush_threshold to save the unified flushers some work. Michal
On Tue, Aug 22, 2023 at 6:01 AM Michal Koutný <mkoutny@suse.com> wrote: > > On Mon, Aug 21, 2023 at 08:54:57PM +0000, Yosry Ahmed <yosryahmed@google.com> wrote: > > +static void do_stats_flush(struct mem_cgroup *memcg) > > +{ > > + cgroup_rstat_flush(memcg->css.cgroup); > if(memcg == root_mem_cgroup) > atomic_set(&stats_flush_threshold, 0); > > +} > > + > > /* > > * do_unified_stats_flush - do a unified flush of memory cgroup statistics > > * > > @@ -656,7 +667,7 @@ static void do_unified_stats_flush(void) > > > > WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME); > > > > - cgroup_rstat_flush(root_mem_cgroup->css.cgroup); > > + do_stats_flush(root_mem_cgroup); > > > - atomic_set(&stats_flush_threshold, 0); > > atomic_set(&stats_flush_ongoing, 0); > > You may reset stats_flush_threshold to save the unified flushers some > work. We can probably also kick FLUSH_TIME forward as well. Perhaps I can move both into do_stats_flush() then. If I understand correctly this is what you mean? static void do_stats_flush(struct mem_cgroup *memcg) { if (mem_cgroup_is_root(memcg)) WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME); cgroup_rstat_flush(memcg->css.cgroup); if (mem_cgroup_is_root(memcg)) atomic_set(&stats_flush_threshold, 0); } static void do_unified_stats_flush(void) { if (atomic_read(&stats_flush_ongoing) || atomic_xchg(&stats_flush_ongoing, 1)) return; do_stats_flush(root_mem_cgroup); atomic_set(&stats_flush_ongoing, 0); } , or simplify it further by just resetting stats_flush_threshold before we flush as well: static void do_stats_flush(struct mem_cgroup *memcg) { /* for unified flushing, root non-unified flushing can help as well */ if (mem_cgroup_is_root(memcg)) { WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME); atomic_set(&stats_flush_threshold, 0); } cgroup_rstat_flush(memcg->css.cgroup); } static void do_unified_stats_flush(void) { if (atomic_read(&stats_flush_ongoing) || atomic_xchg(&stats_flush_ongoing, 1)) return; do_stats_flush(root_mem_cgroup); atomic_set(&stats_flush_ongoing, 0); } What do you think?
On Tue, Aug 22, 2023 at 09:00:06AM -0700, Yosry Ahmed <yosryahmed@google.com> wrote: > We can probably also kick FLUSH_TIME forward as well. True, they guard same work. > Perhaps I can move both into do_stats_flush() then. If I understand > correctly this is what you mean? Yes. > What do you think? The latter is certainly better looking code. I wasn't sure at first about moving stats_flush_threshold reset before actual flush but on second thought it should not be a significant change - readers: may skip flushing, the values that they read should still be below the error threshold, - writers: may be slowed down a bit (because of conditional atomic write optimization in memcg_rstat_updates), presumably not on average though. So the latter should work too. Michal
On Tue, Aug 22, 2023 at 9:35 AM Michal Koutný <mkoutny@suse.com> wrote: > > On Tue, Aug 22, 2023 at 09:00:06AM -0700, Yosry Ahmed <yosryahmed@google.com> wrote: > > We can probably also kick FLUSH_TIME forward as well. > > True, they guard same work. > > > Perhaps I can move both into do_stats_flush() then. If I understand > > correctly this is what you mean? > > Yes. > > > What do you think? > > The latter is certainly better looking code. > > I wasn't sure at first about moving stats_flush_threshold reset before > actual flush but on second thought it should not be a significant change > - readers: may skip flushing, the values that they read should still be > below the error threshold, Unified readers will skip anyway as there's an ongoing flush, non-unified readers don't check the threshold anyway (with this patch series). So for readers it should not be a change. > - writers: may be slowed down a bit (because of conditional atomic write > optimization in memcg_rstat_updates), presumably not on average > though. Yeah writers will start doing atomic writes once a flush starts instead of once it ends. I don't think it will matter in practice though. The optimization is only effective if we manage to surpass the threshold before the periodic flusher (or any unified flusher) kicks in and resets it. If we start doing atomic writes earlier, then we will also stop earlier; the number of atomic writes should stay the same. I think the only difference will be the scenario where we start atomic writes early but the periodic flush happens before we reach the threshold, in which case we aren't doing a lot of updates anyway. I hope this makes _any_ sense :) > > So the latter should work too. > I will include that in v2. I will wait for a bit of further review comments on this version first. Thanks for taking a look! > Michal
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index c6150ea54d48..90f08b35fa77 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -639,6 +639,17 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val) } } +/* + * do_stats_flush - do a flush of the memory cgroup statistics + * @memcg: memory cgroup to flush + * + * Only flushes the subtree of @memcg, does not skip under any conditions. + */ +static void do_stats_flush(struct mem_cgroup *memcg) +{ + cgroup_rstat_flush(memcg->css.cgroup); +} + /* * do_unified_stats_flush - do a unified flush of memory cgroup statistics * @@ -656,7 +667,7 @@ static void do_unified_stats_flush(void) WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME); - cgroup_rstat_flush(root_mem_cgroup->css.cgroup); + do_stats_flush(root_mem_cgroup); atomic_set(&stats_flush_threshold, 0); atomic_set(&stats_flush_ongoing, 0); @@ -7790,7 +7801,7 @@ bool obj_cgroup_may_zswap(struct obj_cgroup *objcg) break; } - cgroup_rstat_flush(memcg->css.cgroup); + do_stats_flush(memcg); pages = memcg_page_state(memcg, MEMCG_ZSWAP_B) / PAGE_SIZE; if (pages < max) continue; @@ -7855,8 +7866,10 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size) static u64 zswap_current_read(struct cgroup_subsys_state *css, struct cftype *cft) { - cgroup_rstat_flush(css->cgroup); - return memcg_page_state(mem_cgroup_from_css(css), MEMCG_ZSWAP_B); + struct mem_cgroup *memcg = mem_cgroup_from_css(css); + + do_stats_flush(memcg); + return memcg_page_state(memcg, MEMCG_ZSWAP_B); } static int zswap_max_show(struct seq_file *m, void *v)
Some contexts flush memcg stats outside of unified flushing, directly using cgroup_rstat_flush(). Add a helper for non-unified flushing, a counterpart for do_unified_stats_flush(), and use it in those contexts, as well as in do_unified_stats_flush() itself. This abstracts the rstat API and makes it easy to introduce modifications to either unified or non-unified flushing functions without changing callers. No functional change intended. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- mm/memcontrol.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)