Re: [PATCH] mm: vmscan: count slab shrinking results after each shrink_slab()

[Johannes Weiner]

On Tue, Oct 20, 2015 at 03:19:20PM +0300, Vladimir Davydov wrote:
> On Mon, Oct 19, 2015 at 02:13:35PM -0400, Johannes Weiner wrote:
> > cb731d6 ("vmscan: per memory cgroup slab shrinkers") sought to
> > optimize accumulating slab reclaim results in sc->nr_reclaimed only
> > once per zone, but the memcg hierarchy walk itself uses
> > sc->nr_reclaimed as an exit condition. This can lead to overreclaim.
> > 
> > Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> > ---
> >  mm/vmscan.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 27d580b..a02654e 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -2441,11 +2441,18 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc,
> >  			shrink_lruvec(lruvec, swappiness, sc, &lru_pages);
> >  			zone_lru_pages += lru_pages;
> >  
> > -			if (memcg && is_classzone)
> > +			if (memcg && is_classzone) {
> >  				shrink_slab(sc->gfp_mask, zone_to_nid(zone),
> >  					    memcg, sc->nr_scanned - scanned,
> >  					    lru_pages);
> >  
> > +				if (reclaim_state) {
> 
> current->reclaim_state is only set on global reclaim, so when performing
> memcg reclaim we'll never get here. Hence, since we check nr_reclaimed
> in the loop only on memcg reclaim, this patch doesn't change anything.
> 
> Setting current->reclaim_state on memcg reclaim doesn't seem to be an
> option, because it accounts objects freed by any cgroup (e.g. via RCU
> callback) - see https://lkml.org/lkml/2015/1/20/91

Ah, I was not aware of that. Thanks for clarifying. Scratch this patch
then.

Do you think it would make sense to take the shrink_slab() return
value into account? Or are most objects expected to be RCU-freed
anyway so it wouldn't make a difference?

> About overreclaim that might happen due to the current behavior. Inodes
> and dentries are small and usually freed by RCU so not accounting them
> to nr_reclaimed shouldn't make much difference. The only reason I see
> why overreclaim can happen is ignoring eviction of an inode full of page
> cache, speaking of which makes me wonder if it'd be better to refrain
> from dropping inodes which have page cache left, at least unless the
> scan priority is low?

Unless we have evidence that it drops cache pages prematurely, I think
it should be okay to leave it as is.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/