Message-ID: <52F10F95.4050204@parallels.com>
Date:	Tue, 4 Feb 2014 20:04:37 +0400
From:	Vladimir Davydov <vdavydov@...allels.com>
To:	Glauber Costa <glommer@...il.com>
CC:	Michal Hocko <mhocko@...e.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Rientjes <rientjes@...gle.com>,
	Pekka Enberg <penberg@...nel.org>,
	Christoph Lameter <cl@...ux.com>, <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>, <devel@...nvz.org>
Subject: Re: [PATCH 3/8] memcg, slab: never try to merge memcg caches

On 02/04/2014 07:43 PM, Glauber Costa wrote:
> On Tue, Feb 4, 2014 at 7:27 PM, Vladimir Davydov <vdavydov@...allels.com> wrote:
>> On 02/04/2014 07:11 PM, Michal Hocko wrote:
>>> On Tue 04-02-14 18:59:23, Vladimir Davydov wrote:
>>>> On 02/04/2014 06:52 PM, Michal Hocko wrote:
>>>>> On Sun 02-02-14 20:33:48, Vladimir Davydov wrote:
>>>>>> Suppose we are creating memcg cache A that could be merged with cache B
>>>>>> of the same memcg. Since any memcg cache has the same parameters as its
>>>>>> parent cache, parent caches PA and PB of memcg caches A and B must be
>>>>>> mergeable too. That means PA was merged with PB on creation or vice
>>>>>> versa, i.e. PA = PB. From that it follows that A = B, and we couldn't
>>>>>> even try to create cache B, because it already exists - a contradiction.
>>>>> I cannot tell I understand the above but I am totally not sure about the
>>>>> statement bellow.
>>>>>
>>>>>> So let's remove unused code responsible for merging memcg caches.
>>>>> How come the code was unused? find_mergeable called cache_match_memcg...
>>>> Oh, sorry for misleading comment. I mean the code handling merging of
>>>> per-memcg caches is useless, AFAIU: if we find an alias for a per-memcg
>>>> cache on kmem_cache_create_memcg(), the parent of the found alias must
>>>> be the same as the parent_cache passed to kmem_cache_create_memcg(), but
>>>> if it were so, we would never proceed to the memcg cache creation,
>>>> because the cache we want to create already exists.
>>> I am still not sure I understand this correctly. So the outcome of this
>>> patch is that compatible caches of different memcgs can be merged
>>> together? Sorry if this is a stupid question but I am not that familiar
>>> with this area much I am just seeing that cache_match_memcg goes away
>>> and my understanding of the function is that it should prevent from
>>> different memcg's caches merging.
>> Let me try to explain how I understand it.
>>
>> What is cache merging/aliasing? When we create a cache
>> (kmem_cache_create()), we first try to find a compatible cache that
>> already exists and can handle requests from the new cache. If it is, we
>> do not create any new caches, instead we simply increment the old cache
>> refcount and return it.
>>
>> What about memcgs? Currently, it operates in the same way, i.e. on memcg
>> cache creation we also try to find a compatible cache of the same memcg
>> first. But if there were such a cache, they parents would have been
>> merged (i.e. it would be the same cache). That means we would not even
>> get to this memcg cache creation, because it already exists. That's why
>> the code handling memcg caches merging seems pointless to me.
>>
> IIRC, this may not always hold. Some of the properties are configurable via
> sysfs, and it might be that you haven't merged two parent caches because they
> properties differ, but would be fine merging the child caches.
>
> If all properties we check are compile-time parameters, then it should be okay.

AFAIK, we decide if a cache should be merged only basing on its internal
parameters, such as size, ctor, flags, align (see find_mergeable()), but
they are the same for root and memcg caches.

The only way to disable slub merging is via the "slub_nomerge" kernel
parameter, so it is impossible to get a situation when parents can not
be merged, while children can.

The only point of concern may be so called boot caches
(create_boot_cache()), which are forcefully not allowed to be merged by
setting refcount = -1. There are actually only two of them kmem_cache
and kmem_cache_node used for internal slub allocations. I guess it was
done preliminary, and we should not merge them for memcgs neither.

To sum it up, if a particular root cache is allowed to be merged, it was
allowed to be merged since its creation and all its children caches are
also allowed to be merged. If merging was not allowed for a root cache
when it was created, we should not merge its children caches.

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/

Powered by blists - more mailing lists