From mboxrd@z Thu Jan 1 00:00:00 1970 From: srivatsa@MIT.EDU (Srivatsa S. Bhat) Date: Wed, 16 Jul 2014 13:14:40 +0530 Subject: [PATCH v2] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend In-Reply-To: <03e76981eb3d84e8397a39616ac0cfa1.squirrel@www.codeaurora.org> References: <1404959850-11617-1-git-send-email-skannan@codeaurora.org> <1405052287-4744-1-git-send-email-skannan@codeaurora.org> <2f549e6e4871ccf2a94dd4c8872c7a0b.squirrel@www.codeaurora.org> <53C0A12A.2060204@codeaurora.org> <53C42AA8.8010107@codeaurora.org> <53C4BDFB.70707@codeaurora.org> <53C4D12E.3040807@mit.edu> <03e76981eb3d84e8397a39616ac0cfa1.squirrel@www.codeaurora.org> Message-ID: <53C62D68.6080608@mit.edu> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 07/15/2014 11:05 PM, skannan at codeaurora.org wrote: > > Srivatsa S. Bhat wrote: >> On 07/15/2014 11:06 AM, Saravana Kannan wrote: >>> On 07/14/2014 09:35 PM, Viresh Kumar wrote: >>>> On 15 July 2014 00:38, Saravana Kannan wrote: >>>>> Yeah, it definitely crashes if policy->cpu if an offline cpu. Because >>>>> the >>>>> mutex would be uninitialized if it's stopped after boot or it would >>>>> never >>>>> have been initialized (depending on how you fix policy->cpu at boot). >>>>> >>>>> Look at this snippet on the actual tree and it should be pretty >>>>> evident. >>>> >>>> Yeah, I missed it. So the problem is we initialize timer_mutex's for >>>> policy->cpus. So we need to do that just for policy->cpu and also we >>>> don't >>>> need a per-cpu timer_mutex anymore. >>>> >>> >>> Btw, I tried to take a stab at removing any assumption in cpufreq code >>> about policy->cpu being ONLINE. >> >> Wait, allowing an offline CPU to be the policy->cpu (i.e., the CPU which >> is >> considered as the master of the policy/group) is just absurd. If there is >> no leader, there is no army. We should NOT sacrifice sane semantics for >> the >> sake of simplifying the code. >> >>> There are 160 instances of those of with >>> 23 are in cpufreq.c >>> >> >> And that explains why. It is just *natural* to assume that the CPUs >> governed >> by a policy are online. Especially so for the CPU which is supposed to be >> the policy leader. Let us please not change that - it will become >> counter-intuitive if we do so. [ The other reason is that physical hotplug >> is also possible on some systems... in that case your code might make a >> CPU >> which is not even present (but possible) as the policy->cpu.. and great >> 'fun' >> will ensue after that ;-( ] >> >> The goal of this patchset should be to just de-couple the sysfs >> files/ownership >> from the policy->cpu to an extent where it doesn't matter who owns those >> files, and probably make it easier to do CPU hotplug without having to >> destroy and recreate the files on every hotplug operation. >> >> This is exactly why the _implementation_ matters in this particular case - >> if we can't achieve the simplification by keeping sane semantics, then we >> shouldn't do the simplification! >> >> That said, I think we should keep trying - we haven't exhausted all ideas >> yet :-) >> > > I don't think we disagree. To summarize this topic: I tried to keep the > policy->cpu an actual online CPU so as to not break existing semantics in > this patch. Viresh asked "why not fix it at boot?". My response was to > keep it an online CPU and give it a shot in a separate patch if we really > want that. It's too risky to do that in this patch and also not a > mandatory change for this patch. > > I think we can work out the details on the need to fixing policy->cpu at > boot and whether there's even a need for policy->cpu (when we already have > policy->cpus) in a separate thread after the dust settles on this one? > Sure, that sounds good! Regards, Srivatsa S. Bhat