Re: [PATCH] [RFC] kmemcg: remove union from memcg_params

[Andrey Wagin]

2013/8/13 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>:
> On Fri,  9 Aug 2013 00:51:26 +0400 Andrey Vagin <avagin@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/