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.
