linux-kernel - Re: [PATCH v12 10/18] memcg,list_lru: add per-memcg LRU list infrastructure

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]

Date:	Tue, 3 Dec 2013 16:29:14 +0400
From:	Vladimir Davydov <vdavydov@...allels.com>
To:	Dave Chinner <david@...morbit.com>
CC:	<hannes@...xchg.org>, <mhocko@...e.cz>, <dchinner@...hat.com>,
	<akpm@...ux-foundation.org>, <linux-kernel@...r.kernel.org>,
	<linux-mm@...ck.org>, <cgroups@...r.kernel.org>,
	<devel@...nvz.org>, <glommer@...nvz.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Balbir Singh <bsingharora@...il.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [PATCH v12 10/18] memcg,list_lru: add per-memcg LRU list infrastructure

On 12/03/2013 03:18 PM, Dave Chinner wrote:
> On Mon, Dec 02, 2013 at 03:19:45PM +0400, Vladimir Davydov wrote:
>> FS-shrinkers, which shrink dcaches and icaches, keep dentries and inodes
>> in list_lru structures in order to evict least recently used objects.
>> With per-memcg kmem shrinking infrastructure introduced, we have to make
>> those LRU lists per-memcg in order to allow shrinking FS caches that
>> belong to different memory cgroups independently.
>>
>> This patch addresses the issue by introducing struct memcg_list_lru.
>> This struct aggregates list_lru objects for each kmem-active memcg, and
>> keeps it uptodate whenever a memcg is created or destroyed. Its
>> interface is very simple: it only allows to get the pointer to the
>> appropriate list_lru object from a memcg or a kmem ptr, which should be
>> further operated with conventional list_lru methods.
> Basically The idea was that the memcg LRUs hide entirely behind the
> generic list_lru interface so that any cache that used the list_lru
> insfrastructure got memcg capabilities for free. memcg's to shrink
> were to be passed through the shrinker control shrinkers to the list
> LRU code, and it then did all the "which lru are we using" logic
> internally.
>
> What you've done is driven all the "which LRU are we using" logic
> into every single caller location. i.e. you've just broken the
> underlying design principle that Glauber and I had worked towards
> with this code - that memcg aware LRUs should be completely
> transparent to list_lru users. Just like NUMA awareness came for
> free with the list_lru code, so should memcg awareness....
>
>> +/*
>> + * The following structure can be used to reclaim kmem objects accounted to
>> + * different memory cgroups independently. It aggregates a set of list_lru
>> + * objects, one for each kmem-enabled memcg, and provides the method to get
>> + * the lru corresponding to a memcg.
>> + */
>> +struct memcg_list_lru {
>> +	struct list_lru global_lru;
>> +
>> +#ifdef CONFIG_MEMCG_KMEM
>> +	struct list_lru **memcg_lrus;	/* rcu-protected array of per-memcg
>> +					   lrus, indexed by memcg_cache_id() */
>> +
>> +	struct list_head list;		/* list of all memcg-aware lrus */
>> +
>> +	/*
>> +	 * The memcg_lrus array is rcu protected, so we can only free it after
>> +	 * a call to synchronize_rcu(). To avoid multiple calls to
>> +	 * synchronize_rcu() when many lrus get updated at the same time, which
>> +	 * is a typical scenario, we will store the pointer to the previous
>> +	 * version of the array in the old_lrus variable for each lru, and then
>> +	 * free them all at once after a single call to synchronize_rcu().
>> +	 */
>> +	void *old_lrus;
>> +#endif
>> +};
> Really, this should be embedded in the struct list_lru, not wrapping
> around the outside. I don't see any changelog to tell me why you
> changed the code from what was last in Glauber's tree, so can you
> explain why exposing all this memcg stuff to everyone is a good
> idea?

I preferred to move from list_lru to memcg_list_lru, because the
connection between list_lru and memcgs' turned memcontrol.c and
list_lru.c into a monolithic structure. When I read comments to the last
version of this patchset submitted by Glauber (v10), I found that Andrew
Morton disliked it, that was why I tried to "fix" it the way you observe
in this patch. Besides, I though that the list_lru may be used w/o memcgs.

I didn't participate in the previous discussion so I don't know all your
plans on it :-( If you think it's unacceptable, I'll try to find another
way around.

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