Message-ID: <52F1E561.8020804@parallels.com>
Date:	Wed, 5 Feb 2014 11:16:49 +0400
From:	Vladimir Davydov <vdavydov@...allels.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
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 02/05/2014 01:58 AM, Andrew Morton wrote:
> 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...

Hmm, that explains this 2. Since we typically don't need to "seek back"
when recreating a cache page, as they are usually read in bunches by
readahead, the number of seeks to bring back a user page is 1, while the
number of seeks to recreate an average inode is 2, right?

Then to scan inodes and user pages so that they would generate
approximately the same number of seeks, we should calculate the number
of objects to scan as follows:

nr_objects_to_scan = nr_pages_scanned / lru_pages *
                                        nr_freeable_objects /
shrinker->seeks

where shrinker->seeks = DEFAULT_SEEKS = 2 for inodes. But currently we
have four times that. I can explain why we should multiply this by 2 -
we do not count pages moving from active to inactive lrus in
nr_pages_scanned, and 2*nr_pages_scanned can be a good approximation for
that - but I have no idea why we multiply it by 4...

Thanks.

>
> 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/

Powered by blists - more mailing lists