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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href