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;
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
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.
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..
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.
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 :)
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.
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.
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.
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.
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.
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 :)
linaro-kernel@lists.linaro.org
-
David Rientjes -
Li Zefan -
Tejun Heo -
Viresh Kumar