linux-kernel - Re: [PATCH 2/3] mm: vmscan: get rid of DEFAULT_SEEKS and document shrink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20140204135836.05c09c765073513e62edd174@linux-foundation.org>
Date:	Tue, 4 Feb 2014 13:58:36 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Vladimir Davydov <vdavydov@...allels.com>
Cc:	<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
	<devel@...nvz.org>, Mel Gorman <mgorman@...e.de>,
	Michal Hocko <mhocko@...e.cz>,
	Johannes Weiner <hannes@...xchg.org>,
	Rik van Riel <riel@...hat.com>,
	Dave Chinner <dchinner@...hat.com>,
	Glauber Costa <glommer@...il.com>
Subject: Re: [PATCH 2/3] mm: vmscan: get rid of DEFAULT_SEEKS and document
 shrink_slab logic

On Fri, 17 Jan 2014 23:25:30 +0400 Vladimir Davydov <vdavydov@...allels.com> wrote:

> Each shrinker must define the number of seeks it takes to recreate a
> shrinkable cache object. It is used to balance slab reclaim vs page
> reclaim: assuming it costs one seek to replace an LRU page, we age equal
> percentages of the LRU and ageable caches. So far, everything sounds
> clear, but the code implementing this behavior is rather confusing.
> 
> First, there is the DEFAULT_SEEKS constant, which equals 2 for some
> reason:
> 
>   #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
> 
> Most shrinkers define `seeks' to be equal to DEFAULT_SEEKS, some use
> DEFAULT_SEEKS*N, and there are a few that totally ignore it. What is
> peculiar, dcache and icache shrinkers have seeks=DEFAULT_SEEKS although
> recreating an inode typically requires one seek. Does this mean that we
> scan twice more inodes than we should?
> 
> Actually, no. The point is that vmscan handles DEFAULT_SEEKS as if it
> were 1 (`delta' is the number of objects we are going to scan):
> 
>   shrink_slab_node():
>     delta = (4 * nr_pages_scanned) / shrinker->seeks;
>     delta *= freeable;
>     do_div(delta, lru_pages + 1);
> 
> i.e.
> 
>             2 * nr_pages_scanned    DEFAULT_SEEKS
>     delta = -------------------- * --------------- * freeable;
>                  lru_pages         shrinker->seeks
> 
> Here we double the number of pages scanned in order to take into account
> moves of on-LRU pages from the inactive list to the active list, which
> we do not count in nr_pages_scanned.
> 
> That said, shrinker->seeks=DEFAULT_SEEKS*N is equivalent to N seeks, so
> why on the hell do we need it?
> 
> IMO, the existence of the DEFAULT_SEEKS constant only causes confusion
> for both users of the shrinker interface and those trying to understand
> how slab shrinking works. The meaning of the `seeks' is perfectly
> explained by the comment to it and there is no need in any obscure
> constants for using it.
> 
> That's why I'm sending this patch which completely removes DEFAULT_SEEKS
> and makes all shrinkers use N instead of N*DEFAULT_SEEKS, documenting
> the idea lying behind shrink_slab() in the meanwhile.
> 
> Unfortunately, there are a few shrinkers that define seeks=1, which is
> impossible to transfer to the new interface intact, namely:
> 
>   nfsd_reply_cache_shrinker
>   ttm_pool_manager::mm_shrink
>   ttm_pool_manager::mm_shrink
>   dm_bufio_client::shrinker
> 
> It seems to me their authors were simply deceived by this mysterious
> DEFAULT_SEEKS constant, because I've found no documentation why these
> particular caches should be scanned more aggressively than the page and
> other slab caches. For them, this patch leaves seeks=1. Thus, it DOES
> introduce a functional change: the shrinkers enumerated above will be
> scanned twice less intensively than they are now. I do not think that
> this will cause any problems though.
> 

um, yes.  DEFAULT_SEEKS is supposed to be "the number of seeks if you
don't know any better".  Using DEFAULT_SEEKS*n is just daft.

So why did I originally make DEFAULT_SEEKS=2?  Because I figured that to
recreate (say) an inode would require a seek to the inode data then a
seek back.  Is it legitimate to include the
seek-back-to-what-you-were-doing-before seek in the cost of an inode
reclaim?  I guess so...

If a filesystem were to require a seek to the superblock for every
inode read (ok, bad example) then the cost of reestablishing that inode
would be 3.

All that being said, why did you go through and halve everything?  The
cost of reestablishing an ext2 inode should be "2 seeks", but the patch
makes it "1".

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/