Re: [PATCH 1/2] memcg: flush stats only if updated

From: Shakeel Butt <shakeelb@google.com>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: "Michal Hocko" <mhocko@kernel.org>,
	"Michal Koutný" <mkoutny@suse.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	Cgroups <cgroups@vger.kernel.org>,
	"Linux MM" <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] memcg: flush stats only if updated
Date: Fri, 1 Oct 2021 07:41:43 -0700	[thread overview]
Message-ID: <CALvZod7TENAMsmf6swD40RxGFFKOSUtSg9SvgaqE6pdRHy8nrA@mail.gmail.com> (raw)
In-Reply-To: <YVca+jJnjDn5RLsq@cmpxchg.org>

On Fri, Oct 1, 2021 at 7:26 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Wed, Sep 29, 2021 at 09:47:10PM -0700, Shakeel Butt wrote:
> > At the moment, the kernel flushes the memcg stats on every refault and
> > also on every reclaim iteration. Although rstat maintains per-cpu update
> > tree but on the flush the kernel still has to go through all the cpu
> > rstat update tree to check if there is anything to flush. This patch
> > adds the tracking on the stats update side to make flush side more
> > clever by skipping the flush if there is no update.
> >
> > The stats update codepath is very sensitive performance wise for many
> > workloads and benchmarks. So, we can not follow what the commit
> > aa48e47e3906 ("memcg: infrastructure to flush memcg stats") did which
> > was triggering async flush through queue_work() and caused a lot
> > performance regression reports. That got reverted by the commit
> > 1f828223b799 ("memcg: flush lruvec stats in the refault").
> >
> > In this patch we kept the stats update codepath very minimal and let the
> > stats reader side to flush the stats only when the updates are over a
> > specific threshold. For now the threshold is (nr_cpus * CHARGE_BATCH).
> >
> > To evaluate the impact of this patch, an 8 GiB tmpfs file is created on
> > a system with swap-on-zram and the file was pushed to swap through
> > memory.force_empty interface. On reading the whole file, the memcg stat
> > flush in the refault code path is triggered. With this patch, we
> > bserved 63% reduction in the read time of 8 GiB file.
> >
> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
>
> This is a great idea.
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks.

>
> One minor nit:
>
[...]
>
> Because of the way the updates and the flush interact through these
> variables now, it might be better to move these up and together.
>
> It'd also be good to have a small explanation of the optimization in
> the code as well - that we accept (limited) percpu fuzz in lieu of not
> having to check all percpus for every flush.

I will move the code and add the comment on the optimization in the
next version.