From: Gautham R Shenoy <hidden> Date: 2016-11-07 02:31:57
Hi Denis,
On Fri, Nov 04, 2016 at 07:08:38AM -0400, Denis Kirjanov wrote:
You can provide the config option with which this bug was found in the
change log. I suppose you had enabled CONFIG_DEBUG_PREEMPT.
On Monday, November 7, 2016, Gautham R Shenoy [off-list ref]
wrote:
Hi Denis,
On Fri, Nov 04, 2016 at 07:08:38AM -0400, Denis Kirjanov wrote:
You can provide the config option with which this bug was found in the
change log. I suppose you had enabled CONFIG_DEBUG_PREEMPT.
that's why I put the comment
quoted
[ 67.700897] BUG: using smp_processor_id() in preemptible
[00000000] code: cat/7343
quoted
[ 67.700988] caller is .powernv_cpufreq_throttle_check+0x2c/0x710
[ 67.700998] CPU: 13 PID: 7343 Comm: cat Not tainted
@@ -647,8 +647,15 @@ static int powernv_cpufreq_target_index(struct
cpufreq_policy *policy,
quoted
if (unlikely(rebooting) && new_index != get_nominal_index())
return 0;
- if (!throttled)
+ if (!throttled) {
+ /*
+ * we don't want to be preempted while
+ * checking if the CPU frequency has been throttled
+ */
+ preempt_disable();
powernv_cpufreq_throttle_check(NULL);
+ preempt_enable();
+ }
cur_msec = jiffies_to_msecs(get_jiffies_64());
--
1.8.3.1
That should be below the "---".
When did this break?
I think that problem exists since commit
09a972d1620934142d30cfda455ffe429af751c4 ("cpufreq: powernv: Report
cpu frequency throttling") but I can be wrong
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-11-08 00:21:23
Denis Kirjanov [off-list ref] writes:
On 11/7/16, Michael Ellerman [off-list ref] wrote:
quoted
Denis Kirjanov [off-list ref] writes:
quoted
[ 67.700897] BUG: using smp_processor_id() in preemptible [00000000] code: cat/7343
[ 67.700988] caller is .powernv_cpufreq_throttle_check+0x2c/0x710
When did this break?
I think that problem exists since commit
09a972d1620934142d30cfda455ffe429af751c4 ("cpufreq: powernv: Report
cpu frequency throttling") but I can be wrong
Yep that looks right to me, thanks.
Gautham, how bad is the bug here, do we want to send this to stable?
cheers
From: Gautham R Shenoy <hidden> Date: 2016-11-08 03:17:32
Hi Michael,
On Tue, Nov 08, 2016 at 11:21:23AM +1100, Michael Ellerman wrote:
Denis Kirjanov [off-list ref] writes:
quoted
On 11/7/16, Michael Ellerman [off-list ref] wrote:
quoted
Denis Kirjanov [off-list ref] writes:
quoted
[ 67.700897] BUG: using smp_processor_id() in preemptible [00000000] code: cat/7343
[ 67.700988] caller is .powernv_cpufreq_throttle_check+0x2c/0x710
When did this break?
I think that problem exists since commit
09a972d1620934142d30cfda455ffe429af751c4 ("cpufreq: powernv: Report
cpu frequency throttling") but I can be wrong
Yep that looks right to me, thanks.
Gautham, how bad is the bug here, do we want to send this to stable?
Not so bad that it needs to be sent to stable immediately.
On CONFIG_PREEMPT enabled systems, if task executing
powernv_cpufreq_throttle_check can be switched to a CPU on a different
chip, then we will end up incorrectly attributing the throttle
statistics of the new chip to the chip where it previously ran. These
throttle statistics aren't used inside the kernel for any computation,
but are reported to the user via /sysfs for them to account for any
variance during a benchmark run.
It would be good to include this in the 4.9 fixes though.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-11-08 08:27:34
Gautham R Shenoy [off-list ref] writes:
On Tue, Nov 08, 2016 at 11:21:23AM +1100, Michael Ellerman wrote:
quoted
Denis Kirjanov [off-list ref] writes:
quoted
On 11/7/16, Michael Ellerman [off-list ref] wrote:
quoted
Denis Kirjanov [off-list ref] writes:
quoted
[ 67.700897] BUG: using smp_processor_id() in preemptible [00000000] code: cat/7343
[ 67.700988] caller is .powernv_cpufreq_throttle_check+0x2c/0x710
When did this break?
I think that problem exists since commit
09a972d1620934142d30cfda455ffe429af751c4 ("cpufreq: powernv: Report
cpu frequency throttling") but I can be wrong
Yep that looks right to me, thanks.
Gautham, how bad is the bug here, do we want to send this to stable?
Not so bad that it needs to be sent to stable immediately.
On CONFIG_PREEMPT enabled systems, if task executing
powernv_cpufreq_throttle_check can be switched to a CPU on a different
chip, then we will end up incorrectly attributing the throttle
statistics of the new chip to the chip where it previously ran. These
throttle statistics aren't used inside the kernel for any computation,
but are reported to the user via /sysfs for them to account for any
variance during a benchmark run.
OK.
It would be good to include this in the 4.9 fixes though.
Normally I'd say by about rc5 that if it's not important enough for
stable then it can wait for next (or it fixes a bug introduced in the
current release).
But I just realised it's actually in drivers/cpufreq/powernv-cpufreq.c,
which goes via Raphael, so it's up to him.
And he's not on CC :/
Denis can you resend and CC the cpufreq folks:
rjw@rjwysocki.net
viresh.kumar@linaro.org
linux-pm@vger.kernel.org
And add:
Fixes: 09a972d16209 ("cpufreq: powernv: Report cpu frequency throttling")
And fixup the subject to be:
cpufreq, powernv: Disable preemption while checking CPU throttling state
Thanks.
cheers