Re: [PATCH] cgroups: make cftype.unregister_event() void-returning

[Kirill A. Shutemov]

On Sat, May 8, 2010 at 12:07 AM, Paul Menage <menage@xxxxxxxxxx> wrote:
> I like the principle. I think this patch leaks arrays, though.
>
> I think the sequence:
>
> register;register;unregister;unregister
>
> will leak the array of size 2. Using the notation Ax, Bx, Cx, etc to
> represent distinct buffers of size x, we have:
>
> initially: size = 0, thresholds = NULL, spare = NULL
> register: size = 1, thresholds = A1, spare = NULL
> register: size = 2, thresholds = B2, spare = A1
> unregister: size = 1, thresholds = A1, spare = B2
> unregister: size = 0, thresholds = NULL, spare = A1 (B2 is leaked)
>
> In the case when you're unregistering and the size goes down to 0, you
> need to free the spare before doing the swap.

Nice catch!

> Maybe get rid of the
> thresholds_new local variable, and instead in the if(!size) {} branch
> just free and the spare buffer and set its pointer to NULL? Then at
> swap_buffers:, unconditionally swap the two.

Good idea. Thanks.

> Also, I think the code would be cleaner if you created a structure to
> hold a primary threshold and its spare; then you could have one for
> each threshold set, and just pass that to the register/unregister
> functions, rather than them having to be aware of how the type maps to
> the primary and backup array pointers.

Ok. I'll try to implement it in separate patch.

Thank you for reviewing.

>
> Paul
>
> On Fri, May 7, 2010 at 4:46 AM, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
>> Since we unable to handle error returned by cftype.unregister_event()
>> properly, let's make the callback void-returning.
>>
>> mem_cgroup_unregister_event() has been rewritten to be "never fail"
>> function. On mem_cgroup_usage_register_event() we save old buffer
>> for thresholds array and reuse it in mem_cgroup_usage_unregister_event()
>> to avoid allocation.
>>
>> Signed-off-by: Kirill A. Shutemov <kirill@xxxxxxxxxxxxx>
>> ---
>> Âinclude/linux/cgroup.h | Â Â2 +-
>> Âkernel/cgroup.c    Â|  Â1 -
>> Âmm/memcontrol.c    Â|  64 ++++++++++++++++++++++++++++++------------------
>> Â3 files changed, 41 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 8f78073..0c62160 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -397,7 +397,7 @@ struct cftype {
>> Â Â Â Â * This callback must be implemented, if you want provide
>> Â Â Â Â * notification functionality.
>> Â Â Â Â */
>> - Â Â Â int (*unregister_event)(struct cgroup *cgrp, struct cftype *cft,
>> + Â Â Â void (*unregister_event)(struct cgroup *cgrp, struct cftype *cft,
>> Â Â Â Â Â Â Â Â Â Â Â Âstruct eventfd_ctx *eventfd);
>> Â};
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index 06dbf97..6675e8c 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -2988,7 +2988,6 @@ static void cgroup_event_remove(struct work_struct *work)
>> Â Â Â Â Â Â Â Â Â Â Â Âremove);
>> Â Â Â Âstruct cgroup *cgrp = event->cgrp;
>>
>> - Â Â Â /* TODO: check return code */
>> Â Â Â Âevent->cft->unregister_event(cgrp, event->cft, event->eventfd);
>>
>> Â Â Â Âeventfd_ctx_put(event->eventfd);
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 8cb2722..0a37b5d 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -226,9 +226,19 @@ struct mem_cgroup {
>> Â Â Â Â/* thresholds for memory usage. RCU-protected */
>> Â Â Â Âstruct mem_cgroup_threshold_ary *thresholds;
>>
>> + Â Â Â /*
>> + Â Â Â Â* Preallocated buffer to be used in mem_cgroup_unregister_event()
>> + Â Â Â Â* to make it "never fail".
>> + Â Â Â Â* It must be able to store at least thresholds->size - 1 entries.
>> + Â Â Â Â*/
>> + Â Â Â struct mem_cgroup_threshold_ary *__thresholds;
>> +
>> Â Â Â Â/* thresholds for mem+swap usage. RCU-protected */
>> Â Â Â Âstruct mem_cgroup_threshold_ary *memsw_thresholds;
>>
>> + Â Â Â /* the same as __thresholds, but for memsw_thresholds */
>> + Â Â Â struct mem_cgroup_threshold_ary *__memsw_thresholds;
>> +
>> Â Â Â Â/* For oom notifier event fd */
>> Â Â Â Âstruct list_head oom_notify;
>>
>> @@ -3575,17 +3585,27 @@ static int
>> mem_cgroup_usage_register_event(struct cgroup *cgrp,
>> Â Â Â Âelse
>> Â Â Â Â Â Â Â Ârcu_assign_pointer(memcg->memsw_thresholds, thresholds_new);
>>
>> - Â Â Â /* To be sure that nobody uses thresholds before freeing it */
>> + Â Â Â /* To be sure that nobody uses thresholds */
>> Â Â Â Âsynchronize_rcu();
>>
>> - Â Â Â kfree(thresholds);
>> + Â Â Â /*
>> + Â Â Â Â* Free old preallocated buffer and use thresholds as new
>> + Â Â Â Â* preallocated buffer.
>> + Â Â Â Â*/
>> + Â Â Â if (type == _MEM) {
>> + Â Â Â Â Â Â Â kfree(memcg->__thresholds);
>> + Â Â Â Â Â Â Â memcg->__thresholds = thresholds;
>> + Â Â Â } else {
>> + Â Â Â Â Â Â Â kfree(memcg->__memsw_thresholds);
>> + Â Â Â Â Â Â Â memcg->__memsw_thresholds = thresholds;
>> + Â Â Â }
>> Âunlock:
>> Â Â Â Âmutex_unlock(&memcg->thresholds_lock);
>>
>> Â Â Â Âreturn ret;
>> Â}
>>
>> -static int mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>> +static void mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>> Â Â Â Âstruct cftype *cft, struct eventfd_ctx *eventfd)
>> Â{
>> Â Â Â Âstruct mem_cgroup *memcg = mem_cgroup_from_cont(cgrp);
>> @@ -3593,7 +3613,7 @@ static int
>> mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>> Â Â Â Âint type = MEMFILE_TYPE(cft->private);
>> Â Â Â Âu64 usage;
>> Â Â Â Âint size = 0;
>> - Â Â Â int i, j, ret = 0;
>> + Â Â Â int i, j;
>>
>> Â Â Â Âmutex_lock(&memcg->thresholds_lock);
>> Â Â Â Âif (type == _MEM)
>> @@ -3623,17 +3643,15 @@ static int
>> mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>> Â Â Â Â/* Set thresholds array to NULL if we don't have thresholds */
>> Â Â Â Âif (!size) {
>> Â Â Â Â Â Â Â Âthresholds_new = NULL;
>> - Â Â Â Â Â Â Â goto assign;
>> + Â Â Â Â Â Â Â goto swap_buffers;
>> Â Â Â Â}
>>
>> - Â Â Â /* Allocate memory for new array of thresholds */
>> - Â Â Â thresholds_new = kmalloc(sizeof(*thresholds_new) +
>> - Â Â Â Â Â Â Â Â Â Â Â size * sizeof(struct mem_cgroup_threshold),
>> - Â Â Â Â Â Â Â Â Â Â Â GFP_KERNEL);
>> - Â Â Â if (!thresholds_new) {
>> - Â Â Â Â Â Â Â ret = -ENOMEM;
>> - Â Â Â Â Â Â Â goto unlock;
>> - Â Â Â }
>> + Â Â Â /* Use preallocated buffer for new array of thresholds */
>> + Â Â Â if (type == _MEM)
>> + Â Â Â Â Â Â Â thresholds_new = memcg->__thresholds;
>> + Â Â Â else
>> + Â Â Â Â Â Â Â thresholds_new = memcg->__memsw_thresholds;
>> +
>> Â Â Â Âthresholds_new->size = size;
>>
>> Â Â Â Â/* Copy thresholds and find current threshold */
>> @@ -3654,20 +3672,20 @@ static int
>> mem_cgroup_usage_unregister_event(struct cgroup *cgrp,
>> Â Â Â Â Â Â Â Âj++;
>> Â Â Â Â}
>>
>> -assign:
>> - Â Â Â if (type == _MEM)
>> +swap_buffers:
>> + Â Â Â /* Swap thresholds array and preallocated buffer */
>> + Â Â Â if (type == _MEM) {
>> + Â Â Â Â Â Â Â memcg->__thresholds = thresholds;
>> Â Â Â Â Â Â Â Ârcu_assign_pointer(memcg->thresholds, thresholds_new);
>> - Â Â Â else
>> + Â Â Â } else {
>> + Â Â Â Â Â Â Â memcg->__memsw_thresholds = thresholds;
>> Â Â Â Â Â Â Â Ârcu_assign_pointer(memcg->memsw_thresholds, thresholds_new);
>> + Â Â Â }
>>
>> - Â Â Â /* To be sure that nobody uses thresholds before freeing it */
>> + Â Â Â /* To be sure that nobody uses thresholds */
>> Â Â Â Âsynchronize_rcu();
>>
>> - Â Â Â kfree(thresholds);
>> -unlock:
>> Â Â Â Âmutex_unlock(&memcg->thresholds_lock);
>> -
>> - Â Â Â return ret;
>> Â}
>>
>> Âstatic int mem_cgroup_oom_register_event(struct cgroup *cgrp,
>> @@ -3695,7 +3713,7 @@ static int mem_cgroup_oom_register_event(struct
>> cgroup *cgrp,
>> Â Â Â Âreturn 0;
>> Â}
>>
>> -static int mem_cgroup_oom_unregister_event(struct cgroup *cgrp,
>> +static void mem_cgroup_oom_unregister_event(struct cgroup *cgrp,
>> Â Â Â Âstruct cftype *cft, struct eventfd_ctx *eventfd)
>> Â{
>> Â Â Â Âstruct mem_cgroup *mem = mem_cgroup_from_cont(cgrp);
>> @@ -3714,8 +3732,6 @@ static int
>> mem_cgroup_oom_unregister_event(struct cgroup *cgrp,
>> Â Â Â Â}
>>
>> Â Â Â Âmutex_unlock(&memcg_oom_mutex);
>> -
>> - Â Â Â return 0;
>> Â}
>>
>> Âstatic int mem_cgroup_oom_control_read(struct cgroup *cgrp,
>> --
>> 1.7.0.4
>>
>
--
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/