Re: [v2 PATCH 8/9] mm: memcontrol: reparent nr_deferred when memcg offline

Yang Shi
2020-12-15T23:10:42Z

On Mon, Dec 14, 2020 at 7:08 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Mon, Dec 14, 2020 at 02:37:21PM -0800, Yang Shi wrote:
> > Now shrinker's nr_deferred is per memcg for memcg aware shrinkers, add to parent's
> > corresponding nr_deferred when memcg offline.
> >
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > ---
> > include/linux/shrinker.h | 4 ++++
> > mm/memcontrol.c | 24 ++++++++++++++++++++++++
> > mm/vmscan.c | 2 +-
> > 3 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > index 1eac79ce57d4..85cfc910dde4 100644
> > --- a/include/linux/shrinker.h
> > +++ b/include/linux/shrinker.h
> > @@ -78,6 +78,10 @@ struct shrinker {
> > };
> > #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
> >
> > +#ifdef CONFIG_MEMCG
> > +extern int shrinker_nr_max;
> > +#endif
> > +
> > /* Flags */
> > #define SHRINKER_REGISTERED (1 << 0)
> > #define SHRINKER_NUMA_AWARE (1 << 1)
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 321d1818ce3d..1f191a15bee1 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -59,6 +59,7 @@
> > #include <linux/tracehook.h>
> > #include <linux/psi.h>
> > #include <linux/seq_buf.h>
> > +#include <linux/shrinker.h>
> > #include "internal.h"
> > #include <net/sock.h>
> > #include <net/ip.h>
> > @@ -612,6 +613,28 @@ void memcg_set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id)
> > }
> > }
> >
> > +static void memcg_reparent_shrinker_deferred(struct mem_cgroup *memcg)
> > +{
> > + int i, nid;
> > + long nr;
> > + struct mem_cgroup *parent;
> > + struct memcg_shrinker_deferred *child_deferred, *parent_deferred;
> > +
> > + parent = parent_mem_cgroup(memcg);
> > + if (!parent)
> > + parent = root_mem_cgroup;
> > +
> > + for_each_node(nid) {
> > + child_deferred = memcg->nodeinfo[nid]->shrinker_deferred;
> > + parent_deferred = parent->nodeinfo[nid]->shrinker_deferred;
> > + for (i = 0; i < shrinker_nr_max; i ++) {
> > + nr = atomic_long_read(&child_deferred->nr_deferred[i]);
> > + atomic_long_add(nr,
> > + &parent_deferred->nr_deferred[i]);
> > + }
> > + }
> > +}
>
> I would place this function in vmscan.c alongside the
> shrink_slab_set_nr_deferred_memcg() function so that all the
> accounting is in the one place.

Fine to me. Will incorporate in v3.

>
> > +
> > /**
> > * mem_cgroup_css_from_page - css of the memcg associated with a page
> > * @page: page of interest
> > @@ -5543,6 +5566,7 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css)
> > page_counter_set_low(&memcg->memory, 0);
> >
> > memcg_offline_kmem(memcg);
> > + memcg_reparent_shrinker_deferred(memcg);
> > wb_memcg_offline(memcg);
> >
> > drain_all_stock(memcg);
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 8d5bfd818acd..693a41e89969 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -201,7 +201,7 @@ DECLARE_RWSEM(shrinker_rwsem);
> > #define SHRINKER_REGISTERING ((struct shrinker *)~0UL)
> >
> > static DEFINE_IDR(shrinker_idr);
> > -static int shrinker_nr_max;
> > +int shrinker_nr_max;
>
> Then we don't need to make yet another variable global...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com