Re: [PATCH 2/2] mm/vmscan: shrink slab in node reclaim

[Yafang Shao <laoar.shao@gmail.com>]

On Thu, May 23, 2019 at 5:40 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu,  9 May 2019 16:07:49 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > In the node reclaim, may_shrinkslab is 0 by default,
> > hence shrink_slab will never be performed in it.
> > While shrik_slab should be performed if the relcaimable slab is over
> > min slab limit.
> >
> > This issue is very easy to produce, first you continuously cat a random
> > non-exist file to produce more and more dentry, then you read big file
> > to produce page cache. And finally you will find that the denty will
> > never be shrunk.
>
> It does sound like an oversight.
>
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -4141,6 +4141,8 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
> >               .may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
> >               .may_swap = 1,
> >               .reclaim_idx = gfp_zone(gfp_mask),
> > +             .may_shrinkslab = node_page_state(pgdat, NR_SLAB_RECLAIMABLE) >
> > +                               pgdat->min_slab_pages,
> >       };
> >
> >       trace_mm_vmscan_node_reclaim_begin(pgdat->node_id, order,
> > @@ -4158,15 +4160,13 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
> >       reclaim_state.reclaimed_slab = 0;
> >       p->reclaim_state = &reclaim_state;
> >
> > -     if (node_pagecache_reclaimable(pgdat) > pgdat->min_unmapped_pages) {
>
> Would it be better to do
>
>         if (node_pagecache_reclaimable(pgdat) > pgdat->min_unmapped_pages ||
>                         sc.may_shrinkslab) {
>

This if condition is always true here, because we already check them
in node_reclaim(),
see bellow,

    if (node_pagecache_reclaimable(pgdat) <= pgdat->min_unmapped_pages &&
        node_page_state(pgdat, NR_SLAB_RECLAIMABLE) <= pgdat->min_slab_pages)
        return NODE_RECLAIM_FULL;


> >               /*
> >                * Free memory by calling shrink node with increasing
> >                * priorities until we have enough memory freed.
> >                */
>
> The above will want re-indenting and re-right-justifying.
>

Sorry about the carelessness.

> > -             do {
> > -                     shrink_node(pgdat, &sc);
> > -             } while (sc.nr_reclaimed < nr_pages && --sc.priority >= 0);
> > -     }
> > +     do {
> > +             shrink_node(pgdat, &sc);
> > +     } while (sc.nr_reclaimed < nr_pages && --sc.priority >= 0);
>
> Won't this cause pagecache reclaim and compaction which previously did
> not occur?  If yes, what are the effects of this and are they
> desirable?  If no, perhaps call shrink_slab() directly in this case.
> Or something like that.
>

It may cause pagecache reclaim and compaction even if
node_pagecache_reclaimable() is still less than
pgdat->min_unmapped_pages.
The active file will be deactivated and the inactive file will be recaimed.
(I traced these behavior with mm_vmscan_lru_shrink_active and
mm_vmscan_lru_shrink_inactive tracepoint)

If we don't like these behavior, what about bellow change ?

@@ -4166,6 +4166,17 @@ static int __node_reclaim(struct pglist_data
*pgdat, gfp_t gfp_mask, unsigned in
                do {
                        shrink_node(pgdat, &sc);
                } while (sc.nr_reclaimed < nr_pages && --sc.priority >= 0);
+       } else {
+               struct mem_cgroup *memcg;
+               struct mem_cgroup_reclaim_cookie reclaim = {
+                        .pgdat = pgdat,
+                        .priority = sc.priority,
+                };
+
+               memcg = mem_cgroup_iter(false, NULL, &reclaim);
+               do {
+                       shrink_slab(sc.gfp_mask, pgdat->node_id,
memcg, sc.priority);
+               } while ((memcg = mem_cgroup_iter(false, memcg, &reclaim)));

        }


> It's unclear why min_unmapped_pages (min_unmapped_ratio) exists. Is it

I have tried to understand it, but still don't have a clear idea yet.
So I just let it as-is.

> a batch-things-up efficiency thing?

I guess so.

Thanks
Yafang