Re: [v2 PATCH 2/9] mm: memcontrol: use shrinker_rwsem to protect shrinker_maps allocation

[Yang Shi]

On Wed, Dec 16, 2020 at 11:14 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Wed, Dec 16, 2020 at 08:59:38AM +1100, Dave Chinner wrote:
> > On Tue, Dec 15, 2020 at 02:53:48PM +0100, Johannes Weiner wrote:
> > > On Tue, Dec 15, 2020 at 01:09:57PM +1100, Dave Chinner wrote:
> > > > On Mon, Dec 14, 2020 at 02:37:15PM -0800, Yang Shi wrote:
> > > > > Since memcg_shrinker_map_size just can be changd under holding shrinker_rwsem
> > > > > exclusively, the read side can be protected by holding read lock, so it sounds
> > > > > superfluous to have a dedicated mutex.
> > > >
> > > > I'm not sure this is a good idea. This couples the shrinker
> > > > infrastructure to internal details of how cgroups are initialised
> > > > and managed. Sure, certain operations might be done in certain
> > > > shrinker lock contexts, but that doesn't mean we should share global
> > > > locks across otherwise independent subsystems....
> > >
> > > They're not independent subsystems. Most of the memory controller is
> > > an extension of core VM operations that is fairly difficult to
> > > understand outside the context of those operations. Then there are a
> > > limited number of entry points from the cgroup interface. We used to
> > > have our own locks for core VM structures (private page lock e.g.) to
> > > coordinate VM and cgroup, and that was mostly unintelligble.
> >
> > Yes, but OTOH you can CONFIG_MEMCG=n and the shrinker infrastructure
> > and shrinkers all still functions correctly.  Ergo, the shrinker
> > infrastructure is independent of memcgs. Yes, it may have functions
> > to iterate and manipulate memcgs, but it is not dependent on memcgs
> > existing for correct behaviour and functionality.
>
> Okay, but now do it the other way round and explain the memcg bits in
> a world where shrinkers don't exist ;-)
>
> Anyway, we seem to be mostly in agreement below.
>
> > > We have since established that those two components coordinate with
> > > native VM locking and lifetime management. If you need to lock the
> > > page, you lock the page - instead of having all VM paths that already
> > > hold the page lock acquire a nested lock to exclude one cgroup path.
> > >
> > > In this case, we have auxiliary shrinker data, subject to shrinker
> > > lifetime and exclusion rules. It's much easier to understand that
> > > cgroup creation needs a stable shrinker list (shrinker_rwsem) to
> > > manage this data, than having an aliased lock that is private to the
> > > memcg callbacks and obscures this real interdependency.
> >
> > Ok, so the way to do this is to move all the stuff that needs to be
> > done under a "subsystem global" lock to the one file, not turn a
> > static lock into a globally visible lock and spray it around random
> > source files.
>
> Sure, that works as well.
>
> > The shrinker map should be generic functionality for all shrinker
> > invocations because even a non-memcg machine can have thousands of
> > registered shrinkers that are mostly idle all the time.
>
> Agreed.
>
> > IOWs, I think the shrinker map management is not really memcg
> > specific - it's just allocation and assignment of a structure, and
> > the only memcg bit is the map is being stored in a memcg structure.
> > Therefore, if we are looking towards tighter integration then we
> > should acutally move the map management to the shrinker code, not
> > split the shrinker infrastructure management across different files.
> > There's already a heap of code in vmscan.c under #ifdef
> > CONFIG_MEMCG, like the prealloc_shrinker() code path:
> >
> > prealloc_shrinker()                           vmscan.c
> >   if (MEMCG_AWARE)                            vmscan.c
> >     prealloc_memcg_shrinker                   vmscan.c
> > #ifdef CONFIG_MEMCG                           vmscan.c
> >       down_write(shrinker_rwsem)              vmscan.c
> >       if (id > shrinker_id_max)                       vmscan.c
> >       memcg_expand_shrinker_maps              memcontrol.c
> >         for_each_memcg                        memcontrol.c
> >           reallocate shrinker map             memcontrol.c
> >           replace shrinker map                memcontrol.c
> >       shrinker_id_max = id                    vmscan.c
> >       down_write(shrinker_rwsem)              vmscan.c
> > #endif
> >
> > And, really, there's very little code in memcg_expand_shrinker_maps()
> > here - the only memcg part is the memcg iteration loop, and we
> > already have them in vmscan.c (e.g. shrink_node_memcgs(),
> > age_active_anon(), drop_slab_node()) so there's precedence for
> > moving this memcg iteration for shrinker map management all into
> > vmscan.c.
> >
> > Doing so would formalise the shrinker maps as first class shrinker
> > infrastructure rather than being tacked on to the side of the memcg
> > infrastructure. At this point it makes total sense to serialise map
> > manipulations under the shrinker_rwsem.
>
> Yes, that's a great idea.
>
> > That is, for the medium term, I think  we should be getting rid of
> > the "legacy" non-memcg shrinker path and everything runs under
> > memcgs.  With this patchset moving all the deferred counts to be
> > memcg aware, the only reason for keeping the non-memcg path around
> > goes away.  If sc->memcg is null, then after this patch set we can
> > simply use the root memcg and just use it's per-node accounting
> > rather than having a separate construct for non-memcg aware per-node
> > accounting.
> >
> > Hence if SHRINKER_MEMCG_AWARE is set, it simply means we should run
> > the shrinker if sc->memcg is set.  There is no difference in setup
> > of shrinkers, the duplicate non-memcg/memcg paths go away, and a
> > heap of code drops out of the shrinker infrastructure. It becomes
> > much simpler overall.
>
> Agreed as well.
>
> > It also means we have a path for further integrating memcg aware
> > shrinkers into the shrinker infrastructure because we can always
> > rely on the shrinker infrastructure being memcg aware. And with that
> > in mind, I think we should probably also be moving the shrinker code
> > out of vmscan.c into it's own file as it's really completely
> > separate infrastructure from the vast majority of page reclaim
> > infrastructure in vmscan.c...
>
> Right again.
>
> > That's the view I'm looking at this patchset from. Not just as a
> > standalone bug fix, but also from the perspective of what the
> > architectural change implies and the directions for tighter
> > integration it opens up for us.
>
> Makes sense, but I'm not sure it's getting in the way of that: a
> generalized first-class map would be managed under the shrinker_rwsem,
> so ditching the private lock is good progress. The widened lock scope
> (temporarily, and still mm/) is easy to reverse later on.
>
> That said, moving the map handling code from memcontrol.c to vmscan.c
> in preparation, and/or even reworking the shrinker around the concept
> of a memcg, indeed are great ideas.
>
> I'd support patches doing that.

Thanks a lot for all the great ideas and suggestions! Per the
discussion I will consolidate all shrinker map related code into
vmscan.c in v3 since the changeset seems manageable and won't get the
patch set bloat.

I will look into further cleanup/refactor for mid/long term once this
patch set is done.