struct memcg_cache_params {
bool is_root_cache;
union {
struct kmem_cache *memcg_caches[0];
struct {
struct mem_cgroup *memcg;
struct list_head list;
struct kmem_cache *root_cache;
bool dead;
atomic_t nr_pages;
struct work_struct destroy;
};
};
};
This union is a bit dangerous. //Andrew Morton
The first problem was fixed in v3.10-rc5-67-gf101a94.
The second problem is that the size of memory for root
caches is calculated incorrectly:
ssize_t size = memcg_caches_array_size(num_groups);
size *= sizeof(void *);
size += sizeof(struct memcg_cache_params);
The last line should be fixed like this:
size += offsetof(struct memcg_cache_params, memcg_caches)
Andrew suggested to rework this code without union and
this patch tries to do that.
This patch removes is_root_cache and union. The size of the
memcg_cache_params structure is not changed.
memcg_caches is moved from memcg_cache_params to kmem_cache.
Cc: Glauber Costa <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: Matt Mackall <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Balbir Singh <[email protected]>
Cc: KAMEZAWA Hiroyuki <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Andrey Vagin <[email protected]>
---
include/linux/slab.h | 18 ++++--------
include/linux/slab_def.h | 1 +
include/linux/slub_def.h | 1 +
mm/memcontrol.c | 74 +++++++++++++++++++++---------------------------
mm/slab.h | 4 +--
5 files changed, 43 insertions(+), 55 deletions(-)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 6c5cc0e..fbed77f 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -352,18 +352,12 @@ static __always_inline int kmalloc_size(int n)
* ready, to destroy this cache.
*/
struct memcg_cache_params {
- bool is_root_cache;
- union {
- struct kmem_cache *memcg_caches[0];
- struct {
- struct mem_cgroup *memcg;
- struct list_head list;
- struct kmem_cache *root_cache;
- bool dead;
- atomic_t nr_pages;
- struct work_struct destroy;
- };
- };
+ struct mem_cgroup *memcg;
+ struct list_head list;
+ struct kmem_cache *root_cache;
+ bool dead;
+ atomic_t nr_pages;
+ struct work_struct destroy;
};
int memcg_update_all_caches(int num_memcgs);
diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
index cd40158..15751cd 100644
--- a/include/linux/slab_def.h
+++ b/include/linux/slab_def.h
@@ -80,6 +80,7 @@ struct kmem_cache {
int obj_offset;
#endif /* CONFIG_DEBUG_SLAB */
#ifdef CONFIG_MEMCG_KMEM
+ struct kmem_cache **memcg_caches;
struct memcg_cache_params *memcg_params;
#endif
diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 027276f..b9dfc3c 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -91,6 +91,7 @@ struct kmem_cache {
struct kobject kobj; /* For sysfs */
#endif
#ifdef CONFIG_MEMCG_KMEM
+ struct kmem_cache **memcg_caches;
struct memcg_cache_params *memcg_params;
int max_attr_size; /* for propagation, maximum size of a stored attr */
#endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c5792a5..d1041de 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -41,6 +41,7 @@
#include <linux/mutex.h>
#include <linux/rbtree.h>
#include <linux/slab.h>
+#include "slab.h"
#include <linux/swap.h>
#include <linux/swapops.h>
#include <linux/spinlock.h>
@@ -2948,9 +2949,8 @@ static struct kmem_cache *memcg_params_to_cache(struct memcg_cache_params *p)
{
struct kmem_cache *cachep;
- VM_BUG_ON(p->is_root_cache);
cachep = p->root_cache;
- return cachep->memcg_params->memcg_caches[memcg_cache_id(p->memcg)];
+ return cachep->memcg_caches[memcg_cache_id(p->memcg)];
}
#ifdef CONFIG_SLABINFO
@@ -3131,25 +3131,22 @@ static void kmem_cache_destroy_work_func(struct work_struct *w);
int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
{
- struct memcg_cache_params *cur_params = s->memcg_params;
+ struct kmem_cache **cur_caches = s->memcg_caches;
- VM_BUG_ON(s->memcg_params && !s->memcg_params->is_root_cache);
+ VM_BUG_ON(!is_root_cache(s));
if (num_groups > memcg_limited_groups_array_size) {
int i;
ssize_t size = memcg_caches_array_size(num_groups);
size *= sizeof(void *);
- size += sizeof(struct memcg_cache_params);
- s->memcg_params = kzalloc(size, GFP_KERNEL);
- if (!s->memcg_params) {
- s->memcg_params = cur_params;
+ s->memcg_caches = kzalloc(size, GFP_KERNEL);
+ if (!s->memcg_caches) {
+ s->memcg_caches = cur_caches;
return -ENOMEM;
}
- s->memcg_params->is_root_cache = true;
-
/*
* There is the chance it will be bigger than
* memcg_limited_groups_array_size, if we failed an allocation
@@ -3160,10 +3157,9 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
* memcg_limited_groups_array_size is certainly unused
*/
for (i = 0; i < memcg_limited_groups_array_size; i++) {
- if (!cur_params->memcg_caches[i])
+ if (!cur_caches[i])
continue;
- s->memcg_params->memcg_caches[i] =
- cur_params->memcg_caches[i];
+ s->memcg_caches[i] = cur_caches[i];
}
/*
@@ -3175,7 +3171,7 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
* bigger than the others. And all updates will reset this
* anyway.
*/
- kfree(cur_params);
+ kfree(cur_caches);
}
return 0;
}
@@ -3183,25 +3179,28 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s,
struct kmem_cache *root_cache)
{
- size_t size = sizeof(struct memcg_cache_params);
+ size_t size;
if (!memcg_kmem_enabled())
return 0;
- if (!memcg)
- size += memcg_limited_groups_array_size * sizeof(void *);
-
- s->memcg_params = kzalloc(size, GFP_KERNEL);
- if (!s->memcg_params)
- return -ENOMEM;
-
- if (memcg) {
+ if (!memcg) {
+ VM_BUG_ON(s->memcg_params);
+ size = memcg_limited_groups_array_size * sizeof(void *);
+ s->memcg_caches = kzalloc(size, GFP_KERNEL);
+ if (!s->memcg_caches)
+ return -ENOMEM;
+ } else {
+ VM_BUG_ON(s->memcg_caches);
+ size = sizeof(struct memcg_cache_params);
+ s->memcg_params = kzalloc(size, GFP_KERNEL);
+ if (!s->memcg_params)
+ return -ENOMEM;
s->memcg_params->memcg = memcg;
s->memcg_params->root_cache = root_cache;
INIT_WORK(&s->memcg_params->destroy,
kmem_cache_destroy_work_func);
- } else
- s->memcg_params->is_root_cache = true;
+ }
return 0;
}
@@ -3212,6 +3211,8 @@ void memcg_release_cache(struct kmem_cache *s)
struct mem_cgroup *memcg;
int id;
+ kfree(s->memcg_caches);
+
/*
* This happens, for instance, when a root cache goes away before we
* add any memcg.
@@ -3219,21 +3220,17 @@ void memcg_release_cache(struct kmem_cache *s)
if (!s->memcg_params)
return;
- if (s->memcg_params->is_root_cache)
- goto out;
-
memcg = s->memcg_params->memcg;
id = memcg_cache_id(memcg);
root = s->memcg_params->root_cache;
- root->memcg_params->memcg_caches[id] = NULL;
+ root->memcg_caches[id] = NULL;
mutex_lock(&memcg->slab_caches_mutex);
list_del(&s->memcg_params->list);
mutex_unlock(&memcg->slab_caches_mutex);
css_put(&memcg->css);
-out:
kfree(s->memcg_params);
}
@@ -3391,7 +3388,7 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
idx = memcg_cache_id(memcg);
mutex_lock(&memcg_cache_mutex);
- new_cachep = cachep->memcg_params->memcg_caches[idx];
+ new_cachep = cachep->memcg_caches[idx];
if (new_cachep) {
css_put(&memcg->css);
goto out;
@@ -3406,7 +3403,7 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
atomic_set(&new_cachep->memcg_params->nr_pages , 0);
- cachep->memcg_params->memcg_caches[idx] = new_cachep;
+ cachep->memcg_caches[idx] = new_cachep;
/*
* the readers won't lock, make sure everybody sees the updated value,
* so they won't put stuff in the queue again for no reason
@@ -3422,9 +3419,7 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
struct kmem_cache *c;
int i;
- if (!s->memcg_params)
- return;
- if (!s->memcg_params->is_root_cache)
+ if (!s->memcg_caches)
return;
/*
@@ -3438,7 +3433,7 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
*/
mutex_lock(&set_limit_mutex);
for (i = 0; i < memcg_limited_groups_array_size; i++) {
- c = s->memcg_params->memcg_caches[i];
+ c = s->memcg_caches[i];
if (!c)
continue;
@@ -3552,9 +3547,6 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep,
struct mem_cgroup *memcg;
int idx;
- VM_BUG_ON(!cachep->memcg_params);
- VM_BUG_ON(!cachep->memcg_params->is_root_cache);
-
if (!current->mm || current->memcg_kmem_skip_account)
return cachep;
@@ -3571,8 +3563,8 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep,
* code updating memcg_caches will issue a write barrier to match this.
*/
read_barrier_depends();
- if (likely(cachep->memcg_params->memcg_caches[idx])) {
- cachep = cachep->memcg_params->memcg_caches[idx];
+ if (likely(cachep->memcg_caches[idx])) {
+ cachep = cachep->memcg_caches[idx];
goto out;
}
diff --git a/mm/slab.h b/mm/slab.h
index 620ceed..fe82b4b 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -116,7 +116,7 @@ ssize_t slabinfo_write(struct file *file, const char __user *buffer,
#ifdef CONFIG_MEMCG_KMEM
static inline bool is_root_cache(struct kmem_cache *s)
{
- return !s->memcg_params || s->memcg_params->is_root_cache;
+ return !s->memcg_params;
}
static inline bool cache_match_memcg(struct kmem_cache *cachep,
@@ -162,7 +162,7 @@ static inline const char *cache_name(struct kmem_cache *s)
static inline struct kmem_cache *cache_from_memcg(struct kmem_cache *s, int idx)
{
- return s->memcg_params->memcg_caches[idx];
+ return s->memcg_caches[idx];
}
static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s)
--
1.8.3.1
On Fri, 9 Aug 2013 00:51:26 +0400 Andrey Vagin <[email protected]> wrote:
> struct memcg_cache_params {
> bool is_root_cache;
> union {
> struct kmem_cache *memcg_caches[0];
> struct {
> struct mem_cgroup *memcg;
> struct list_head list;
> struct kmem_cache *root_cache;
> bool dead;
> atomic_t nr_pages;
> struct work_struct destroy;
> };
> };
> };
>
> This union is a bit dangerous. //Andrew Morton
>
> The first problem was fixed in v3.10-rc5-67-gf101a94.
> The second problem is that the size of memory for root
> caches is calculated incorrectly:
>
> ssize_t size = memcg_caches_array_size(num_groups);
>
> size *= sizeof(void *);
> size += sizeof(struct memcg_cache_params);
>
> The last line should be fixed like this:
> size += offsetof(struct memcg_cache_params, memcg_caches)
>
> Andrew suggested to rework this code without union and
> this patch tries to do that.
hm, did I?
> This patch removes is_root_cache and union. The size of the
> memcg_cache_params structure is not changed.
>
It's a bit sad to consume more space because we're sucky programmers.
It would be better to retain the union and to stop writing buggy code
to handle it!
Maybe there are things we can do to reduce the likelihood of people
mishandling the union - don't use anonymous fields, name each member,
access it via helper functions, etc.
2013/8/13 Andrew Morton <[email protected]>:
> On Fri, 9 Aug 2013 00:51:26 +0400 Andrey Vagin <[email protected]> wrote:
>
>> struct memcg_cache_params {
>> bool is_root_cache;
>> union {
>> struct kmem_cache *memcg_caches[0];
>> struct {
>> struct mem_cgroup *memcg;
>> struct list_head list;
>> struct kmem_cache *root_cache;
>> bool dead;
>> atomic_t nr_pages;
>> struct work_struct destroy;
>> };
>> };
>> };
>>
>> This union is a bit dangerous. //Andrew Morton
>>
>> The first problem was fixed in v3.10-rc5-67-gf101a94.
>> The second problem is that the size of memory for root
>> caches is calculated incorrectly:
>>
>> ssize_t size = memcg_caches_array_size(num_groups);
>>
>> size *= sizeof(void *);
>> size += sizeof(struct memcg_cache_params);
>>
>> The last line should be fixed like this:
>> size += offsetof(struct memcg_cache_params, memcg_caches)
>>
>> Andrew suggested to rework this code without union and
>> this patch tries to do that.
>
> hm, did I?
I reread your messages. I have seen in it, what I want. Sorry, you
suggested to rework this code how you explained bellow in this
message. "without union" is my fantasy.
http://lkml.indiana.edu/hypermail/linux/kernel/1305.3/01985.html
>
>> This patch removes is_root_cache and union. The size of the
>> memcg_cache_params structure is not changed.
>>
>
> It's a bit sad to consume more space because we're sucky programmers.
> It would be better to retain the union and to stop writing buggy code
> to handle it!
I decided to implement this approach, because it doesn't increase
memory consumptions. This patch is replace is_root_cache on a pointer,
but due to alignment the size of the structure is not changed.
but the size of struct kmem_cache is increased on one pointer, if
accounting of kernel memory is not enabled.
The overhead of this patch on a real system is about 1K if the kernel
memory accounting is disabled and the overhead is zero after enabling
the accounting.
>
> Maybe there are things we can do to reduce the likelihood of people
> mishandling the union - don't use anonymous fields, name each member,
> access it via helper functions, etc.
Ok, I wil try this way.
Thanks,
Andrey