[PATCH] cpusets: Allocate heap only when required

[PATCH] cpusets: Allocate heap only when required

[Viresh Kumar]

update_flag() routine uses heap only when spread_flag_changed is true. Otherwise
heap isn't used, but is allocated and freed unnecessarily.

Fix this by allocating heap only when required.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Rebased over linux-next/master

 kernel/cpuset.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 4410ac6..9ccdfb2 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1326,16 +1326,18 @@ static int update_flag(cpuset_flagbits_t bit, struct cpuset *cs,
 	if (err < 0)
 		goto out;
 
-	err = heap_init(&heap, PAGE_SIZE, GFP_KERNEL, NULL);
-	if (err < 0)
-		goto out;
-
 	balance_flag_changed = (is_sched_load_balance(cs) !=
 				is_sched_load_balance(trialcs));
 
 	spread_flag_changed = ((is_spread_slab(cs) != is_spread_slab(trialcs))
 			|| (is_spread_page(cs) != is_spread_page(trialcs)));
 
+	if (spread_flag_changed) {
+		err = heap_init(&heap, PAGE_SIZE, GFP_KERNEL, NULL);
+		if (err < 0)
+			goto out;
+	}
+
 	mutex_lock(&callback_mutex);
 	cs->flags = trialcs->flags;
 	mutex_unlock(&callback_mutex);
@@ -1343,9 +1345,10 @@ static int update_flag(cpuset_flagbits_t bit, struct cpuset *cs,
 	if (!cpumask_empty(trialcs->cpus_allowed) && balance_flag_changed)
 		rebuild_sched_domains_locked();
 
-	if (spread_flag_changed)
+	if (spread_flag_changed) {
 		update_tasks_flags(cs, &heap);
-	heap_free(&heap);
+		heap_free(&heap);
+	}
 out:
 	free_trial_cpuset(trialcs);
 	return err;
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH] cpusets: Allocate heap only when required

[David Rientjes]

On Thu, 23 Jan 2014, Viresh Kumar wrote:

> update_flag() routine uses heap only when spread_flag_changed is true. Otherwise
> heap isn't used, but is allocated and freed unnecessarily.
> 
> Fix this by allocating heap only when required.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] cpusets: Allocate heap only when required

[Li Zefan]

Cc: Tejun

On 2014/1/24 5:31, David Rientjes wrote:
> On Thu, 23 Jan 2014, Viresh Kumar wrote:
> 
>> update_flag() routine uses heap only when spread_flag_changed is true. Otherwise
>> heap isn't used, but is allocated and freed unnecessarily.
>>

but harmless

>> Fix this by allocating heap only when required.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Acked-by: David Rientjes <rientjes@google.com>

Acked-by: Li Zefan <lizefan@huawei.com>

I would like this patch be picked up by Tejun. I'll send out a patchset
for cpuset which may have confliction with this one.

Re: [PATCH] cpusets: Allocate heap only when required

[Viresh Kumar]

On 24 January 2014 07:28, Li Zefan <lizefan@huawei.com> wrote:
>> Acked-by: David Rientjes <rientjes@google.com>
>
> Acked-by: Li Zefan <lizefan@huawei.com>

Thanks..

> I would like this patch be picked up by Tejun. I'll send out a patchset
> for cpuset which may have confliction with this one.

Its already applied by Andrew Morton..

Re: [PATCH] cpusets: Allocate heap only when required

[Tejun Heo]

On Fri, Jan 24, 2014 at 10:27:34AM +0530, Viresh Kumar wrote:
> On 24 January 2014 07:28, Li Zefan <lizefan@huawei.com> wrote:
> >> Acked-by: David Rientjes <rientjes@google.com>
> >
> > Acked-by: Li Zefan <lizefan@huawei.com>
> 
> Thanks..
> 
> > I would like this patch be picked up by Tejun. I'll send out a patchset
> > for cpuset which may have confliction with this one.
> 
> Its already applied by Andrew Morton..

Ummm... so, the original posting forgot to cc Li (the maintainer), me
or cgroups mailing list.  Please don't do this in the future.

Thanks.

-- 
tejun

Re: [PATCH] cpusets: Allocate heap only when required

[Viresh Kumar]

On 24 January 2014 15:57, Tejun Heo <tj@kernel.org> wrote:
> Ummm... so, the original posting forgot to cc Li (the maintainer), me
> or cgroups mailing list.  Please don't do this in the future.

I thought Ingo/PeterZ are maintainers of this as well, just after sending
patch I had a look at MAINTAINERS to see if I was wrong and forgot
somebody.

I saw Li there and sent a mail (private) to Li about this patch, I failed to
see you or the cgroups list mentioned there for CPUSETS and so just
sent mail to Li, otherwise I would have got all of you in loop..

Anyways, I forgot to add even Li in the first place, so yes accepted,
probably would be better if we can updated Maintainers :)

Re: [PATCH] cpusets: Allocate heap only when required

[Tejun Heo]

On Fri, Jan 24, 2014 at 04:03:18PM +0530, Viresh Kumar wrote:
> On 24 January 2014 15:57, Tejun Heo <tj@kernel.org> wrote:
> > Ummm... so, the original posting forgot to cc Li (the maintainer), me
> > or cgroups mailing list.  Please don't do this in the future.
> 
> I thought Ingo/PeterZ are maintainers of this as well, just after sending
> patch I had a look at MAINTAINERS to see if I was wrong and forgot
> somebody.
> 
> I saw Li there and sent a mail (private) to Li about this patch, I failed to
> see you or the cgroups list mentioned there for CPUSETS and so just
> sent mail to Li, otherwise I would have got all of you in loop..
>
> Anyways, I forgot to add even Li in the first place, so yes accepted,
> probably would be better if we can updated Maintainers :)

Li, can you please send a patch to add mailing list and tree tags to
the maintainers entry?  And, eh, it happens.  No worries.  Just don't
forget next time.

Thanks.

-- 
tejun

Re: [PATCH] cpusets: Allocate heap only when required

[David Rientjes]

On Fri, 24 Jan 2014, Li Zefan wrote:

> >> update_flag() routine uses heap only when spread_flag_changed is true. Otherwise
> >> heap isn't used, but is allocated and freed unnecessarily.
> >>
> 
> but harmless
> 

It's not harmless, if heap_init() fails with -ENOMEM then the write fails 
even though it may not be for memory_spread_page or memory_spread_slab, 
which is the minority of the callers of this function.

Re: [PATCH] cpusets: Allocate heap only when required

[Tejun Heo]

On Fri, Jan 24, 2014 at 02:33:27AM -0800, David Rientjes wrote:
> It's not harmless, if heap_init() fails with -ENOMEM then the write fails 
> even though it may not be for memory_spread_page or memory_spread_slab, 
> which is the minority of the callers of this function.

And depending on details like that would actually be more harmful.
Please remember that all writes through cgroupfs may fail under very
high memory pressure.  There's no "this specific set of writes to this
specific knob isn't affected by memory pressure" guarantee.

Thanks.

-- 
tejun

Re: [PATCH] cpusets: Allocate heap only when required

[David Rientjes]

On Fri, 24 Jan 2014, Tejun Heo wrote:

> > It's not harmless, if heap_init() fails with -ENOMEM then the write fails 
> > even though it may not be for memory_spread_page or memory_spread_slab, 
> > which is the minority of the callers of this function.
> 
> And depending on details like that would actually be more harmful.
> Please remember that all writes through cgroupfs may fail under very
> high memory pressure.  There's no "this specific set of writes to this
> specific knob isn't affected by memory pressure" guarantee.
> 

Nobody is depending on shit, the patch is removing a completely pointless 
memory allocation in braindead cpuset code.  What you think is "harmful" 
or "more harmful" is irrelevant, but nobody said anything about depending 
on that behavior to do anything.

Thanks.

Re: [PATCH] cpusets: Allocate heap only when required

[Tejun Heo]

Hey,

On Fri, Jan 24, 2014 at 02:51:12AM -0800, David Rientjes wrote:
> Nobody is depending on shit, the patch is removing a completely pointless 
> memory allocation in braindead cpuset code.  What you think is "harmful" 
> or "more harmful" is irrelevant, but nobody said anything about depending 
> on that behavior to do anything.

Weren't you talking something of that effect in memcg?  Or was it
Michal?  At any rate, I think you're missing the point why Li replied
that it's harmless.  He, I think, meant that it doesn't make any
semantical difference to userland, so your reply saying that it's not
harmless listing the failure mode under memory pressure seemed
misleading, so I thought clarification was necessary.  Probably my
(false?) memory of you talking about that contributed.  Anyways, we
agree.  Don't depend on it.

Thanks.

-- 
tejun

Re: [PATCH] cpusets: Allocate heap only when required

[David Rientjes]

On Fri, 24 Jan 2014, Tejun Heo wrote:

> > Nobody is depending on shit, the patch is removing a completely pointless 
> > memory allocation in braindead cpuset code.  What you think is "harmful" 
> > or "more harmful" is irrelevant, but nobody said anything about depending 
> > on that behavior to do anything.
> 
> Weren't you talking something of that effect in memcg?  Or was it
> Michal?

In a completely different thread, I was talking about how we'd like to 
provide a small amount of memory in oom conditions so that you could do 
things like read the cgroup tasks file, but you'd also need the same thing 
to do just about anything, ls, ps, read /proc/pid/status, etc with true 
slab accounting.  Forget about this unnecessary heap allocation, you 
couldn't even do the open() in an oom condition.

That functionality would be provided by the memory reserves set aside for 
userspace oom handlers as part of that feature, cgroups wouldn't be 
different than anything else in that regard, it's a memcg and page 
allocator issue only.

> At any rate, I think you're missing the point why Li replied
> that it's harmless.  He, I think, meant that it doesn't make any
> semantical difference to userland, so your reply saying that it's not
> harmless listing the failure mode under memory pressure seemed
> misleading, so I thought clarification was necessary.

I would consider any memory allocation that is completely unnecessary to 
cause anything to fail unnecessarily to be harmful, nothing specific here 
about update_flag(), cpusets, or cgroups.  Saying something is "harmless" 
makes it sound like there's no downside to doing it, and that's obviously 
not the case.

We agree and I think the only outcome of this discussion is that we both 
wasted time :)