Re: [PATCH 2/4] cpuset: Remove unneeded NODEMASK_ALLOC() in cpuset_attch()

[Li Zefan <lizf@cn.fujitsu.com>]

Paul Menage wrote:
> On Wed, Feb 16, 2011 at 5:49 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> oldcs->mems_allowed is not modified during cpuset_attch(), so
>> we don't have to copy it to a buffer allocated by NODEMASK_ALLOC().
>> Just pass it to cpuset_migrate_mm().
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> I'd be inclined to skip this one - we're already allocating one
> nodemask, so one more isn't really any extra complexity, and we're
> doing horrendously complicated stuff in cpuset_migrate_mm() that's
> much more likely to fail in low-memory situations.

That's true, but it's not a reason to add more cases that can fail.

> 
> It's true that mems_allowed can't change during the call to

Sorry to lead you to mistake what I meant. I meant 'from' is not modified
after it's copied from oldcs->mems_allowed, so the two are exactly the
same and thus we only need one.

> cpuset_attach(), but that's due to the fact that both cgroup_attach()
> and the cpuset.mems write paths take cgroup_mutex. I might prefer to
> leave the allocated nodemask here and wrap callback_mutex around the
> places in cpuset_attach() where we're reading from a cpuset's
> mems_allowed - that would remove the implicit synchronization via
> cgroup_mutex and leave the code a little more understandable.

It's not an implicit synchronization, but instead the lock rule for
reading/writing a cpuset's mems/cpus is described in the comment.

> 
>> ---
>>  kernel/cpuset.c |    7 ++-----
>>  1 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
>> index f13ff2e..70c9ca2 100644
>> --- a/kernel/cpuset.c
>> +++ b/kernel/cpuset.c
>> @@ -1438,10 +1438,9 @@ static void cpuset_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>>        struct mm_struct *mm;
>>        struct cpuset *cs = cgroup_cs(cont);
>>        struct cpuset *oldcs = cgroup_cs(oldcont);
>> -       NODEMASK_ALLOC(nodemask_t, from, GFP_KERNEL);
>>        NODEMASK_ALLOC(nodemask_t, to, GFP_KERNEL);
>>
>> -       if (from == NULL || to == NULL)
>> +       if (to == NULL)
>>                goto alloc_fail;
>>
>>        if (cs == &top_cpuset) {
>> @@ -1463,18 +1462,16 @@ static void cpuset_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>>        }
>>
>>        /* change mm; only needs to be done once even if threadgroup */
>> -       *from = oldcs->mems_allowed;
>>        *to = cs->mems_allowed;
>>        mm = get_task_mm(tsk);
>>        if (mm) {
>>                mpol_rebind_mm(mm, to);
>>                if (is_memory_migrate(cs))
>> -                       cpuset_migrate_mm(mm, from, to);
>> +                       cpuset_migrate_mm(mm, &oldcs->mems_allowed, to);
>>                mmput(mm);
>>        }
>>
>>  alloc_fail:
>> -       NODEMASK_FREE(from);
>>        NODEMASK_FREE(to);
>>  }
>>
>> --
>> 1.7.3.1
>>
>