On Wed, Sep 05, 2018 at 01:51:52PM -0700, Andrew Morton wrote:
> On Tue, 4 Sep 2018 15:47:07 -0700 Roman Gushchin <guro@xxxxxx> wrote:
>
> > Commit 9092c71bb724 ("mm: use sc->priority for slab shrink targets")
> > changed the way how the target slab pressure is calculated and
> > made it priority-based:
> >
> > delta = freeable >> priority;
> > delta *= 4;
> > do_div(delta, shrinker->seeks);
> >
> > The problem is that on a default priority (which is 12) no pressure
> > is applied at all, if the number of potentially reclaimable objects
> > is less than 4096 (1<<12).
> >
> > This causes the last objects on slab caches of no longer used cgroups
> > to never get reclaimed, resulting in dead cgroups staying around forever.
>
> But this problem pertains to all types of objects, not just the cgroup
> cache, yes?
Well, of course, but there is a dramatic difference in size.
Most of these objects are taking few hundreds bytes (or less),
while a memcg can take few hundred kilobytes on a modern multi-CPU
machine. Mostly due to per-cpu stats and events counters.
>
> > Slab LRU lists are reparented on memcg offlining, but corresponding
> > objects are still holding a reference to the dying cgroup.
> > If we don't scan them at all, the dying cgroup can't go away.
> > Most likely, the parent cgroup hasn't any directly associated objects,
> > only remaining objects from dying children cgroups. So it can easily
> > hold a reference to hundreds of dying cgroups.
> >
> > If there are no big spikes in memory pressure, and new memory cgroups
> > are created and destroyed periodically, this causes the number of
> > dying cgroups grow steadily, causing a slow-ish and hard-to-detect
> > memory "leak". It's not a real leak, as the memory can be eventually
> > reclaimed, but it could not happen in a real life at all. I've seen
> > hosts with a steadily climbing number of dying cgroups, which doesn't
> > show any signs of a decline in months, despite the host is loaded
> > with a production workload.
> >
> > It is an obvious waste of memory, and to prevent it, let's apply
> > a minimal pressure even on small shrinker lists. E.g. if there are
> > freeable objects, let's scan at least min(freeable, scan_batch)
> > objects.
> >
> > This fix significantly improves a chance of a dying cgroup to be
> > reclaimed, and together with some previous patches stops the steady
> > growth of the dying cgroups number on some of our hosts.
> >
> > ...
> >
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -476,6 +476,17 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > delta = freeable >> priority;
> > delta *= 4;
> > do_div(delta, shrinker->seeks);
> > +
> > + /*
> > + * Make sure we apply some minimal pressure even on
> > + * small cgroups. This is necessary because some of
> > + * belonging objects can hold a reference to a dying
> > + * child cgroup. If we don't scan them, the dying
> > + * cgroup can't go away unless the memory pressure
> > + * (and the scanning priority) raise significantly.
> > + */
> > + delta = max(delta, min(freeable, batch_size));
> > +
>
> If so I think the comment should be cast in more general terms. Maybe
> with a final sentence "the cgroup cache is one such case".
So, I think that we have to leave explicitly explained memcg refcounting
case, but I'll add a line about other cases as well.
>
> Also, please use all 80 columns in block comments to save a few display
> lines.
>
> And `delta' has type ULL whereas the other two are longs. We'll
> presumably hit warnings here, preventable with max_t.
>
Let me fix this in v3.
Thank you!
