From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vladimir Davydov Subject: Re: [PATCH] mm: vmscan: count slab shrinking results after each shrink_slab() Date: Tue, 20 Oct 2015 18:43:25 +0300 Message-ID: <20151020154325.GI18351@esperanza> References: <1445278415-21138-1-git-send-email-hannes@cmpxchg.org> <20151020121920.GE18351@esperanza> <20151020135606.GB22383@cmpxchg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Content-Disposition: inline In-Reply-To: <20151020135606.GB22383-druUgvl0LCNAfugRpC6u6w@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Transfer-Encoding: 7bit To: Johannes Weiner Cc: Andrew Morton , linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org On Tue, Oct 20, 2015 at 09:56:06AM -0400, Johannes Weiner wrote: > 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 > > > --- > > > 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? On memcg pressure we don't shrink anything except inodes/dentries, which are usually RCU-freed - e.g. see dentry_free, destroy_inode, ext4_destroy_inode, xfs_fs_destroy_inode. So I don't think the number of objects shrunk would tell us much. Thanks, Vladimir