| Message ID | 2811202.iOFZ6YHztY@kreacher (mailing list archive) |
|---|---|
| Headers | show |
| Series | cpufreq / PM: QoS: Introduce frequency QoS and use it in cpufreq | expand |
On Wed, Oct 16, 2019 at 12:37:58PM +0200, Rafael J. Wysocki wrote: > Hi All, > > The motivation for this series is to address the problem discussed here: > > https://lore.kernel.org/linux-pm/5ad2624194baa2f53acc1f1e627eb7684c577a19.1562210705.git.viresh.kumar@linaro.org/T/#md2d89e95906b8c91c15f582146173dce2e86e99f > > and also reported here: > > https://lore.kernel.org/linux-pm/20191015155735.GA29105@bogus/ > > Plus, generally speaking, using the policy CPU as a proxy for the policy > with respect to PM QoS does not feel particularly straightforward to me > and adds extra complexity. > > Anyway, the first patch adds frequency QoS that is based on "raw" PM QoS (kind > of in analogy with device PM QoS) and is just about min and max frequency > requests (no direct relationship to devices). > > The second patch switches over cpufreq and its users to the new frequency QoS. > [The Fixes: tag has been tentatively added to it.] > > The third one removes frequency request types from device PM QoS. > > Unfortunately, the patches are rather big, but also they are quite > straightforward. > > I didn't have the time to test this series, so giving it a go would be much > appreciated. Thanks for the spinning these patches so quickly. I did give it a spin, but unfortunately it doesn't fix the bug I reported. So I looked at my bug report in detail and looks like the cpufreq_driver variable is set to NULL at that point and it fails to dereference it while trying to execute: ret = cpufreq_driver->verify(new_policy); (Hint verify is at offset 0x1c/28) So I suspect some race as this platform with bL switcher tries to unregister and re-register the cpufreq driver during the boot. I need to spend more time on this as reverting the initial PM QoS patch to cpufreq.c makes the issue disappear. -- Regards, Sudeep
On 16-10-19, 12:37, Rafael J. Wysocki wrote: > Hi All, > > The motivation for this series is to address the problem discussed here: > > https://lore.kernel.org/linux-pm/5ad2624194baa2f53acc1f1e627eb7684c577a19.1562210705.git.viresh.kumar@linaro.org/T/#md2d89e95906b8c91c15f582146173dce2e86e99f > > and also reported here: > > https://lore.kernel.org/linux-pm/20191015155735.GA29105@bogus/ > > Plus, generally speaking, using the policy CPU as a proxy for the policy > with respect to PM QoS does not feel particularly straightforward to me > and adds extra complexity. > > Anyway, the first patch adds frequency QoS that is based on "raw" PM QoS (kind > of in analogy with device PM QoS) and is just about min and max frequency > requests (no direct relationship to devices). > > The second patch switches over cpufreq and its users to the new frequency QoS. > [The Fixes: tag has been tentatively added to it.] > > The third one removes frequency request types from device PM QoS. > > Unfortunately, the patches are rather big, but also they are quite > straightforward. > > I didn't have the time to test this series, so giving it a go would be much > appreciated. Apart from the minor comment on one of the patches, these look okay to me. Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On 16-10-19, 15:23, Sudeep Holla wrote: > Thanks for the spinning these patches so quickly. > > I did give it a spin, but unfortunately it doesn't fix the bug I reported. > So I looked at my bug report in detail and looks like the cpufreq_driver > variable is set to NULL at that point and it fails to dereference it > while trying to execute: > ret = cpufreq_driver->verify(new_policy); > (Hint verify is at offset 0x1c/28) > > So I suspect some race as this platform with bL switcher tries to > unregister and re-register the cpufreq driver during the boot. > > I need to spend more time on this as reverting the initial PM QoS patch > to cpufreq.c makes the issue disappear. Is this easily reproducible ? cpufreq_driver == NULL shouldn't be the case, it get updated only once while registering/unregistering cpufreq drivers. That is the last thing which can go wrong from my point of view :)
On Thu, Oct 17, 2019 at 03:27:25PM +0530, Viresh Kumar wrote: > On 16-10-19, 15:23, Sudeep Holla wrote: > > Thanks for the spinning these patches so quickly. > > > > I did give it a spin, but unfortunately it doesn't fix the bug I reported. > > So I looked at my bug report in detail and looks like the cpufreq_driver > > variable is set to NULL at that point and it fails to dereference it > > while trying to execute: > > ret = cpufreq_driver->verify(new_policy); > > (Hint verify is at offset 0x1c/28) > > > > So I suspect some race as this platform with bL switcher tries to > > unregister and re-register the cpufreq driver during the boot. > > > > I need to spend more time on this as reverting the initial PM QoS patch > > to cpufreq.c makes the issue disappear. > > Is this easily reproducible ? cpufreq_driver == NULL shouldn't be the case, it > get updated only once while registering/unregistering cpufreq drivers. That is > the last thing which can go wrong from my point of view :) > Yes, if I boot my TC2 with bL switcher enabled, it always crashes on boot. -- Regards, Sudeep
On Thu, Oct 17, 2019 at 12:00 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Oct 17, 2019 at 03:27:25PM +0530, Viresh Kumar wrote: > > On 16-10-19, 15:23, Sudeep Holla wrote: > > > Thanks for the spinning these patches so quickly. > > > > > > I did give it a spin, but unfortunately it doesn't fix the bug I reported. > > > So I looked at my bug report in detail and looks like the cpufreq_driver > > > variable is set to NULL at that point and it fails to dereference it > > > while trying to execute: > > > ret = cpufreq_driver->verify(new_policy); > > > (Hint verify is at offset 0x1c/28) > > > > > > So I suspect some race as this platform with bL switcher tries to > > > unregister and re-register the cpufreq driver during the boot. > > > > > > I need to spend more time on this as reverting the initial PM QoS patch > > > to cpufreq.c makes the issue disappear. I guess you mean commit 67d874c3b2c6 ("cpufreq: Register notifiers with the PM QoS framework")? That would make sense, because it added the cpufreq_notifier_min() and cpufreq_notifier_max() that trigger handle_update() via schedule_work(). [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure that the new min is less than the new max, because the QoS doesn't do that.] > > Is this easily reproducible ? cpufreq_driver == NULL shouldn't be the case, it > > get updated only once while registering/unregistering cpufreq drivers. That is > > the last thing which can go wrong from my point of view :) > > > > Yes, if I boot my TC2 with bL switcher enabled, it always crashes on boot. It does look like handle_update() races with cpufreq_unregister_driver() and cpufreq_remove_dev (called from there indirectly) does look racy.
On Thu, Oct 17, 2019 at 06:34:28PM +0200, Rafael J. Wysocki wrote: > On Thu, Oct 17, 2019 at 12:00 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Thu, Oct 17, 2019 at 03:27:25PM +0530, Viresh Kumar wrote: > > > On 16-10-19, 15:23, Sudeep Holla wrote: > > > > Thanks for the spinning these patches so quickly. > > > > > > > > I did give it a spin, but unfortunately it doesn't fix the bug I reported. > > > > So I looked at my bug report in detail and looks like the cpufreq_driver > > > > variable is set to NULL at that point and it fails to dereference it > > > > while trying to execute: > > > > ret = cpufreq_driver->verify(new_policy); > > > > (Hint verify is at offset 0x1c/28) > > > > > > > > So I suspect some race as this platform with bL switcher tries to > > > > unregister and re-register the cpufreq driver during the boot. > > > > > > > > I need to spend more time on this as reverting the initial PM QoS patch > > > > to cpufreq.c makes the issue disappear. > > I guess you mean commit 67d874c3b2c6 ("cpufreq: Register notifiers > with the PM QoS framework")? > Correct. > That would make sense, because it added the cpufreq_notifier_min() and > cpufreq_notifier_max() that trigger handle_update() via > schedule_work(). > Yes, it was not clear as I didn't trace to handle_update earlier. After looking at depth today afternoon, I arrived at the same conclusion. > [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure > that the new min is less than the new max, because the QoS doesn't do > that.] > > > > Is this easily reproducible ? cpufreq_driver == NULL shouldn't be the case, it > > > get updated only once while registering/unregistering cpufreq drivers. That is > > > the last thing which can go wrong from my point of view :) > > > > > > > Yes, if I boot my TC2 with bL switcher enabled, it always crashes on boot. > > It does look like handle_update() races with > cpufreq_unregister_driver() and cpufreq_remove_dev (called from there > indirectly) does look racy. Indeed, we just crossed the mails. I just found that and posted a patch. -- Regards, Sudeep
On Wed, Oct 16, 2019 at 03:23:43PM +0100, Sudeep Holla wrote: > On Wed, Oct 16, 2019 at 12:37:58PM +0200, Rafael J. Wysocki wrote: > > Hi All, > > > > The motivation for this series is to address the problem discussed here: > > > > https://lore.kernel.org/linux-pm/5ad2624194baa2f53acc1f1e627eb7684c577a19.1562210705.git.viresh.kumar@linaro.org/T/#md2d89e95906b8c91c15f582146173dce2e86e99f > > > > and also reported here: > > > > https://lore.kernel.org/linux-pm/20191015155735.GA29105@bogus/ > > > > Plus, generally speaking, using the policy CPU as a proxy for the policy > > with respect to PM QoS does not feel particularly straightforward to me > > and adds extra complexity. > > > > Anyway, the first patch adds frequency QoS that is based on "raw" PM QoS (kind > > of in analogy with device PM QoS) and is just about min and max frequency > > requests (no direct relationship to devices). > > > > The second patch switches over cpufreq and its users to the new frequency QoS. > > [The Fixes: tag has been tentatively added to it.] > > > > The third one removes frequency request types from device PM QoS. > > > > Unfortunately, the patches are rather big, but also they are quite > > straightforward. > > > > I didn't have the time to test this series, so giving it a go would be much > > appreciated. > > Thanks for the spinning these patches so quickly. > For the record, I thought of providing the crash that this series fixes. After applying [1] which fixes the boot issue I was seeing on TC2, I started seeing the below crash, which this series fixes. FWIW, Tested-by: Sudeep Holla <sudeep.holla@arm.com> -- Unable to handle kernel paging request at virtual address 31b2c303 pgd = 772b96e1 [31b2c303] *pgd=a4050003, *pmd=00000000 Internal error: Oops: 206 [#1] SMP THUMB2 Modules linked in: CPU: 1 PID: 518 Comm: bash Not tainted 5.4.0-rc3-00062-g6e3a7fd7a87e-dirty #123 Hardware name: ARM-Versatile Express PC is at blocking_notifier_chain_unregister+0x2a/0x78 LR is at blocking_notifier_chain_unregister+0x1b/0x78 Flags: NzCv IRQs on FIQs off Mode SVC_32 ISA Thumb Segment user Control: 70c5387d Table: a57b08c0 DAC: 55555555 Process bash (pid: 518, stack limit = 0x018ebe57) (blocking_notifier_chain_unregister) from (dev_pm_qos_remove_notifier+0x5d/0xb4) (dev_pm_qos_remove_notifier) from (cpufreq_policy_free+0x77/0xc8) (cpufreq_policy_free) from (subsys_interface_unregister+0x4f/0x80) (subsys_interface_unregister) from (cpufreq_unregister_driver+0x29/0x6c) (cpufreq_unregister_driver) from (bL_cpufreq_switcher_notifier+0x41/0x4c) (bL_cpufreq_switcher_notifier) from (notifier_call_chain+0x3d/0x58) (notifier_call_chain) from (blocking_notifier_call_chain+0x29/0x38) (blocking_notifier_call_chain) from (bL_activation_notify+0x13/0x40) (bL_activation_notify) from (bL_switcher_active_store+0x59/0x190) (bL_switcher_active_store) from (kernfs_fop_write+0x85/0x12c) (kernfs_fop_write) from (__vfs_write+0x21/0x130) (__vfs_write) from (vfs_write+0x6b/0xfc) (vfs_write) from (ksys_write+0x6d/0x90) (ksys_write) from (ret_fast_syscall+0x1/0x5a)
On 17-10-19, 18:34, Rafael J. Wysocki wrote: > [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure > that the new min is less than the new max, because the QoS doesn't do > that.] The ->verify() callback does that for us I believe.
On Fri, Oct 18, 2019 at 7:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-10-19, 18:34, Rafael J. Wysocki wrote: > > [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure > > that the new min is less than the new max, because the QoS doesn't do > > that.] > > The ->verify() callback does that for us I believe. It does in practice AFAICS, but in theory it may assume the right ordering between the min and the max and just test the boundaries, may it not?
On 18-10-19, 10:24, Rafael J. Wysocki wrote: > On Fri, Oct 18, 2019 at 7:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 17-10-19, 18:34, Rafael J. Wysocki wrote: > > > [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure > > > that the new min is less than the new max, because the QoS doesn't do > > > that.] > > > > The ->verify() callback does that for us I believe. > > It does in practice AFAICS, but in theory it may assume the right > ordering between the min and the max and just test the boundaries, may > it not? I think cpufreq_verify_within_limits() gets called for sure from within ->verify() for all platforms and this explicitly checks if (policy->min > policy->max) policy->min = policy->max;
On Fri, Oct 18, 2019 at 10:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 18-10-19, 10:24, Rafael J. Wysocki wrote: > > On Fri, Oct 18, 2019 at 7:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 17-10-19, 18:34, Rafael J. Wysocki wrote: > > > > [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure > > > > that the new min is less than the new max, because the QoS doesn't do > > > > that.] > > > > > > The ->verify() callback does that for us I believe. > > > > It does in practice AFAICS, but in theory it may assume the right > > ordering between the min and the max and just test the boundaries, may > > it not? > > I think cpufreq_verify_within_limits() gets called for sure from > within ->verify() for all platforms That's why I mean by "in practice". :-)
On 18-10-19, 10:30, Rafael J. Wysocki wrote: > On Fri, Oct 18, 2019 at 10:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 18-10-19, 10:24, Rafael J. Wysocki wrote: > > > On Fri, Oct 18, 2019 at 7:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > On 17-10-19, 18:34, Rafael J. Wysocki wrote: > > > > > [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure > > > > > that the new min is less than the new max, because the QoS doesn't do > > > > > that.] > > > > > > > > The ->verify() callback does that for us I believe. > > > > > > It does in practice AFAICS, but in theory it may assume the right > > > ordering between the min and the max and just test the boundaries, may > > > it not? > > > > I think cpufreq_verify_within_limits() gets called for sure from > > within ->verify() for all platforms > > That's why I mean by "in practice". :-) Hmm, I am not sure if we should really add another min <= max check in cpufreq_set_policy() as in practice it will never hit :)
On Fri, Oct 18, 2019 at 11:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 18-10-19, 10:30, Rafael J. Wysocki wrote: > > On Fri, Oct 18, 2019 at 10:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 18-10-19, 10:24, Rafael J. Wysocki wrote: > > > > On Fri, Oct 18, 2019 at 7:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > > > On 17-10-19, 18:34, Rafael J. Wysocki wrote: > > > > > > [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure > > > > > > that the new min is less than the new max, because the QoS doesn't do > > > > > > that.] > > > > > > > > > > The ->verify() callback does that for us I believe. > > > > > > > > It does in practice AFAICS, but in theory it may assume the right > > > > ordering between the min and the max and just test the boundaries, may > > > > it not? > > > > > > I think cpufreq_verify_within_limits() gets called for sure from > > > within ->verify() for all platforms > > > > That's why I mean by "in practice". :-) > > Hmm, I am not sure if we should really add another min <= max check in > cpufreq_set_policy() as in practice it will never hit :) Fair enough, but adding a comment regarding that in there would be prudent IMO. > > -- > viresh
On 18-10-19, 11:26, Rafael J. Wysocki wrote: > On Fri, Oct 18, 2019 at 11:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 18-10-19, 10:30, Rafael J. Wysocki wrote: > > > On Fri, Oct 18, 2019 at 10:27 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > On 18-10-19, 10:24, Rafael J. Wysocki wrote: > > > > > On Fri, Oct 18, 2019 at 7:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > > > > > On 17-10-19, 18:34, Rafael J. Wysocki wrote: > > > > > > > [BTW, Viresh, it looks like cpufreq_set_policy() should still ensure > > > > > > > that the new min is less than the new max, because the QoS doesn't do > > > > > > > that.] > > > > > > > > > > > > The ->verify() callback does that for us I believe. > > > > > > > > > > It does in practice AFAICS, but in theory it may assume the right > > > > > ordering between the min and the max and just test the boundaries, may > > > > > it not? > > > > > > > > I think cpufreq_verify_within_limits() gets called for sure from > > > > within ->verify() for all platforms > > > > > > That's why I mean by "in practice". :-) > > > > Hmm, I am not sure if we should really add another min <= max check in > > cpufreq_set_policy() as in practice it will never hit :) > > Fair enough, but adding a comment regarding that in there would be prudent IMO. will do.