linux-kernel - Re: [PATCH] mm: memcg: provide accurate stats for userspace reads

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALvZod6HUtYhDaXiwXSrcwfxLSrZ37sZhKY1Mg4kmpDFk13aYw@mail.gmail.com>
Date:   Tue, 15 Aug 2023 18:14:30 -0700
From:   Shakeel Butt <shakeelb@...gle.com>
To:     Yosry Ahmed <yosryahmed@...gle.com>
Cc:     Tejun Heo <tj@...nel.org>, Michal Hocko <mhocko@...e.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Muchun Song <muchun.song@...ux.dev>, cgroups@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Ivan Babrou <ivan@...udflare.com>
Subject: Re: [PATCH] mm: memcg: provide accurate stats for userspace reads

On Tue, Aug 15, 2023 at 5:29 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
[...]
> >
> > I thought we already reached the decision on how to proceed here. Let
> > me summarize what I think we should do:
> >
> > 1. Completely remove the sync flush from stat files read from userspace.
> > 2. Provide a separate way/interface to explicitly flush stats for
> > users who want more accurate stats and can pay the cost. This is
> > similar to the stat_refresh interface.
> > 3. Keep the 2 sec periodic stats flusher.
>
> I think this solution is suboptimal to be honest, I think we can do better.
>
> With recent improvements to spinlocks/mutexes, and flushers becoming
> sleepable, I think a better solution would be to remove unified
> flushing and let everyone only flush the subtree they care about. Sync
> flushing becomes much better (unless you're flushing root ofc), and
> concurrent flushing wouldn't cause too many problems (ideally no
> thundering herd, and rstat lock can be dropped at cpu boundaries in
> cgroup_rstat_flush_locked()).
>
> If we do this, stat reads can be much faster as Ivan demonstrated with
> his patch that only flushes the cgroup being read, and we do not
> sacrifice accuracy as we never skip flushing. We also do not need a
> separate interface for explicit refresh.
>
> In all cases, we need to keep the 2 sec periodic flusher. What we need
> to figure out if we remove unified flushing is:
>
> 1. Handling stats_flush_threshold.
> 2. Handling flush_next_time.
>
> Both of these are global now, and will need to be adapted to
> non-unified non-global flushing.

The only thing we are disagreeing on is (1) the complete removal of
sync flush and an explicit flush interface versus (2) keep doing the
sync flush of the subtree.

To me (1) seems more optimal particularly for the server use-case
where a node controller reads stats of root and as well as cgroups of
a couple of top levels (we actually do this internally). Doing flush
once explicitly and then reading the stats for all such cgroups seems
better to me.