On Tue, Aug 6, 2019 at 5:44 AM Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> wrote:
>
> 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")
>
Thanks. I will add it.
> > 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?
In node_reclaim it is
if (condition_pagecache || condition_slab)
will_do___node_reclaim();
After scan_control::no_pagecache is introuduced, we don't need the if
statement in
___node_reclaim() any more.
> 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...
>
That's better. I appreciate your improvement on the changlog. I will update it.
> > 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
Thanks. I will correct it.
>
> > @@ -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.
Sure.
Thanks
Yafang
