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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: David Rientjes <[email protected]>
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 <[email protected]>
>
> Acked-by: David Rientjes <[email protected]>
Acked-by: Li Zefan <[email protected]>
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 <[email protected]> wrote:
>> Acked-by: David Rientjes <[email protected]>
>
> Acked-by: Li Zefan <[email protected]>
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 <[email protected]> wrote:
> >> Acked-by: David Rientjes <[email protected]>
> >
> > Acked-by: Li Zefan <[email protected]>
>
> 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
On 24 January 2014 15:57, Tejun Heo <[email protected]> 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, 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.
--
tejun
On Fri, Jan 24, 2014 at 04:03:18PM +0530, Viresh Kumar wrote:
> On 24 January 2014 15:57, Tejun Heo <[email protected]> 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
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.
--
tejun
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 :)