linux-kernel - Re: [v5 PATCH 02/11] mm: vmscan: consolidate shrinker

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: <CAHbLzkoW_vQNgfwrVRy4hXaJkqW+era885dFQZfZy-OHZOOZdg@mail.gmail.com>
Date:   Thu, 28 Jan 2021 13:16:41 -0800
From:   Yang Shi <shy828301@...il.com>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Roman Gushchin <guro@...com>, Kirill Tkhai <ktkhai@...tuozzo.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        Dave Chinner <david@...morbit.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...e.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux MM <linux-mm@...ck.org>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [v5 PATCH 02/11] mm: vmscan: consolidate shrinker_maps handling code

On Thu, Jan 28, 2021 at 8:10 AM Vlastimil Babka <vbabka@...e.cz> wrote:
>
> On 1/28/21 12:33 AM, Yang Shi wrote:
> > The shrinker map management is not purely memcg specific, it is at the intersection
> > between memory cgroup and shrinkers.  It's allocation and assignment of a structure,
> > and the only memcg bit is the map is being stored in a memcg structure.  So move the
> > shrinker_maps handling code into vmscan.c for tighter integration with shrinker code,
> > and remove the "memcg_" prefix.  There is no functional change.
> >
> > Signed-off-by: Yang Shi <shy828301@...il.com>
>
> Acked-by: Vlastimil Babka <vbabka@...e.cz>
>
> Nits below:
>
> > @@ -1581,10 +1581,10 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
> >       return false;
> >  }
> >
> > -extern int memcg_expand_shrinker_maps(int new_id);
> > -
> > -extern void memcg_set_shrinker_bit(struct mem_cgroup *memcg,
> > -                                int nid, int shrinker_id);
> > +extern int alloc_shrinker_maps(struct mem_cgroup *memcg);
> > +extern void free_shrinker_maps(struct mem_cgroup *memcg);
> > +extern void set_shrinker_bit(struct mem_cgroup *memcg,
> > +                          int nid, int shrinker_id);
>
> "extern" is unnecessary and people seem to be removing them nowadays when
> touching the code

OK, will fix in v6.

>
> >  /*
> >   * We allow subsystems to populate their shrinker-related
> >   * LRU lists before register_shrinker_prepared() is called
> > @@ -212,7 +338,7 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker)
> >               goto unlock;
> >
> >       if (id >= shrinker_nr_max) {
> > -             if (memcg_expand_shrinker_maps(id)) {
> > +             if (expand_shrinker_maps(id)) {
> >                       idr_remove(&shrinker_idr, id);
> >                       goto unlock;
> >               }
> > @@ -601,7 +727,7 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>
> Above this is a comment about barriers in memcg_set_shrinker_bit() that should
> be updated.

Thanks for catching this. Will fix in v6.

>
> >                       if (ret == SHRINK_EMPTY)
> >                               ret = 0;
> >                       else
> > -                             memcg_set_shrinker_bit(memcg, nid, i);
> > +                             set_shrinker_bit(memcg, nid, i);
> >               }
> >               freed += ret;
> >
> >
>