Re: [PATCH v5 3/3] cgroups: make procs file writable

[Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>]

David Rientjes wrote:
> On Wed, 29 Dec 2010, Li Zefan wrote:
> 
>>> I think it would be appropriate to use a shared nodemask with file scope 
>>> whenever you have cgroup_lock() to avoid the unnecessary kmalloc() even 
>>> with GFP_KERNEL.  Cpusets are traditionally used on very large machines in 
>>> the first place, so there is a higher likelihood that 
>>> CONFIG_NODES_SHIFT > 8 whenever CONFIG_CPUSETS is enabled.
>>>
>>> All users of NODEMASK_ALLOC() should be protected by cgroup_lock() other 
>>> than cpuset_sprintf_memlist(), right?  That should be the only remaining 
>>> user of NODEMASK_ALLOC() and works well since it can return -ENOMEM.
>>>
>> Changing cpuset->mems_allowed is protected by both cgroup_mutex and
>> cpuset-specific lock (callback_mutex), so you can read it under either
>> lock, so NODEMASK_ALLOC() is not needed. See cpuset_sprintf_cpulist().
>>
> 
> I'm not sure what you're saying.  Cpusets needs to allocate nodemasks for 
> certain functions and doing on the stack can be problemantic if 
> CONFIG_NODES_SHIFT is large because of overflow.  Thus, we can't have 
> temporary nodemasks available on the stack where necessary in functions 
> like cpuset_attach(), update_nodemask(), etc. that require them.  The 
> suggestion was to use a statically allocated "scratch" nodemask since 
> these functions are all protected by cgroup_lock().
> 

That's what we did for cpu masks :). See commit
2341d1b6598c7146d64a5050b53a72a5a819617f.

I made a patchset to remove on stack cpu masks.

What I meant is we don't have to allocate nodemasks in cpuset_sprintf_memlist().
This is sufficient:

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 4349935..a159612 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1620,20 +1620,12 @@ static int cpuset_sprintf_cpulist(char *page, struct cpu

 static int cpuset_sprintf_memlist(char *page, struct cpuset *cs)
 {
-       NODEMASK_ALLOC(nodemask_t, mask, GFP_KERNEL);
        int retval;

-       if (mask == NULL)
-               return -ENOMEM;
-
        mutex_lock(&callback_mutex);
-       *mask = cs->mems_allowed;
+       retval = nodelist_scnprintf(page, PAGE_SIZE, cs->mems_allowed);
        mutex_unlock(&callback_mutex);

-       retval = nodelist_scnprintf(page, PAGE_SIZE, *mask);
-
-       NODEMASK_FREE(mask);
-
        return retval;
 }