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

[Daniel Jordan <daniel.m.jordan@oracle.com>]

Hi Yafang,

On Tue, Jul 30, 2019 at 10:00:01PM -0400, Yafang Shao 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.

Nice catch, I think this needs

Fixes: 1c30844d2dfe ("mm: reclaim small amounts of memory when an external fragmentation event occurs")

> If reclaimable pagecache is less than min_unmapped_pages while
> reclaimable slab is greater than min_slab_pages, we only shrink slab.
> Otherwise the min_unmapped_pages will be useless under this condition.
> A new bitmask no_pagecache is introduced in scan_control for this
> purpose, which is 0 by default.
> Once __node_reclaim() is called, either the reclaimable pagecache is
> greater than min_unmapped_pages or reclaimable slab is greater than
> min_slab_pages, that is ensured in function node_reclaim(). So wen can
> remove the if statement in __node_reclaim().

Why is the if statement there to begin with then, if the condition has
already been checked in node_reclaim?  Looks like it came in with
0ff38490c836 ("[PATCH] zone_reclaim: dynamic slab reclaim"), but it's not
obvious to me why.  Maybe Christoph remembers.

I found this part of the changelog kind of hard to parse.  This instead instead
of above block?

    Add scan_control::no_pagecache so shrink_node can decide to reclaim page
    cache, slab, or both as dictated by min_unmapped_pages and min_slab_pages.
    shrink_node will do at least one of the two because otherwise node_reclaim
    returns early.

Maybe start the next paragraph with

  __node_reclaim can detect when enough slab has been reclaimed because...

> sc.reclaim_state.reclaimed_slab will tell us how many pages are
> reclaimed in shrink slab.
...

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 47aa215..1e410ef 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -91,6 +91,9 @@ struct scan_control {
>  	/* e.g. boosted watermark reclaim leaves slabs alone */
>  	unsigned int may_shrinkslab:1;
>  
> +	/* in node relcaim mode, we may shrink slab only */

                   reclaim

> @@ -4268,6 +4273,10 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
>  		.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
>  		.may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
>  		.may_swap = 1,
> +		.may_shrinkslab = (node_page_state(pgdat, NR_SLAB_RECLAIMABLE) >
> +				   pgdat->min_slab_pages),
> +		.no_pagecache = !(node_pagecache_reclaimable(pgdat) >
> +				  pgdat->min_unmapped_pages),

It's less awkward to do away with the ! and invert the condition.