linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH 1/2] cpufreq: serialize calls to __cpufreq_governor()
@ 2014-10-08  7:04 Viresh Kumar
  2014-10-08 12:46 ` Prarit Bhargava
  0 siblings, 1 reply; 38+ messages in thread
From: Viresh Kumar @ 2014-10-08  7:04 UTC (permalink / raw)
  To: Robert Schöne, Prarit Bhargava
  Cc: Rafael J. Wysocki, Lists linaro-kernel, linux-pm@vger.kernel.org,
	Saravana Kannan

On 25 September 2014 11:37, Robert Schöne <robert.schoene@tu-dresden.de> wrote:
> We had some iterations of patches, but the only solution that works for
> me is the patch with the coarse-grained lock that I sent at Mon, 08 Sep
> 2014 10:16:48 CEST [1]
> Viresh is pretty occupied lately, but he told me that he might do the
> tests himself when the current period of busyness is over as he is
> supplied with a test system. I'm not sure about his current status (busy
> or testing).

Hi Robert/Prarit,

The last state of my branch: cpufreq/governor-fixes you tested had
few bugs in it and so you weren't able to even tests things up.

I couldn't manage to test my patches on a multi-cluster system
(couldn't get it up yet :( ), but was able to do that on a dual-core
ARM-cortexA15 board. And could simply find the bugs there.

I have updated my branch with the changes now and it would be
great if you can confirm if they fix your issues or not.

git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/governor-fixes

--
viresh

^ permalink raw reply	[flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] cpufreq: serialize calls to __cpufreq_governor()
@ 2014-10-10 13:55 Prarit Bhargava
  2014-10-10 13:58 ` Viresh Kumar
  0 siblings, 1 reply; 38+ messages in thread
From: Prarit Bhargava @ 2014-10-10 13:55 UTC (permalink / raw)
  To: Robert Schöne
  Cc: Viresh Kumar, Rafael J. Wysocki, Lists linaro-kernel,
	linux-pm@vger.kernel.org, Saravana Kannan


My test is similar to Robert's.  With 3.17 + Viresh's 3 v2 patches,

#!/bin/bash

i=0
while [ True ]; do
	i=$((i+1))
	echo "ondemand" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor &
	echo "performance" > /sys/devices/system/cpu/cpu1/cpufreq/scaling_governor &
	echo "ondemand" > /sys/devices/system/cpu/cpu1/cpufreq/scaling_governor &
	echo "performance" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor &
	if [ $((i % 100)) = 0 ]; then
		echo $i
	fi
done

and I get...

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffff81577052>] cpufreq_governor_dbs+0x52/0x820
PGD ff5aff067 PUD ff8546067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in: sg nfsv3 rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache
cfg80211 rfkill x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm igb
iTCO_wdt iTCO_vendor_support crct10dif_pclmul crc32_pclmul ptp crc32c_intel
ioatdma pps_core ghash_clmulni_intel aesni_intel dca lrw gf128mul glue_helper
ablk_helper cryptd pcspkr sb_edac edac_core shpchp i2c_i801 lpc_ich mfd_core wmi
ipmi_si ipmi_msghandler nfsd acpi_cpufreq auth_rpcgss nfs_acl lockd grace sunrpc
xfs libcrc32c sd_mod sr_mod crc_t10dif cdrom crct10dif_common mgag200
syscopyarea sysfillrect sysimgblt i2c_algo_bit drm_kms_helper ttm isci drm
libsas ahci libahci scsi_transport_sas libata i2c_core dm_mirror dm_region_hash
dm_log dm_mod
CPU: 39 PID: 8576 Comm: doit Tainted: G        W      3.17.0+ #2
Hardware name: Intel Corporation S2600CP/S2600CP, BIOS
RMLSDP.86I.00.29.D696.1311111329 11/11/2013
task: ffff880ff3920000 ti: ffff880ff4190000 task.ti: ffff880ff4190000
RIP: 0010:[<ffffffff81577052>]  [<ffffffff81577052>] cpufreq_governor_dbs+0x52/0x820
RSP: 0018:ffff880ff4193a70  EFLAGS: 00010297
RAX: 0000000000000024 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000009
RBP: ffff880ff4193ae0 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000000 R11: ffff880ff4193716 R12: ffff880fff62b800
R13: 0000000000000001 R14: 0000000000000001 R15: ffffffff81bd3b80
FS:  00007fd0291ee740(0000) GS:ffff881012400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000ff3cdd000 CR4: 00000000001407e0
Stack:
 ffffffff81570c27 00000000fffffffe 0000000000000046 ffffffff81bd3340
 0000000000000001 0000000000000246 ffffffff81bd3348 ffffffff810e1a1d
 ffff880ff4193ae0 ffff880fff62b800 0000000000000001 ffffffff81bd3500
Call Trace:
 [<ffffffff81570c27>] ? __cpufreq_governor+0x57/0x230
 [<ffffffff810e1a1d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff81574f27>] od_cpufreq_governor_dbs+0x17/0x20
 [<ffffffff81570c62>] __cpufreq_governor+0x92/0x230
 [<ffffffff81570fd8>] cpufreq_set_policy+0x1d8/0x2b0
 [<ffffffff8157181d>] store_scaling_governor+0xad/0xf0
 [<ffffffff81572860>] ? cpufreq_update_policy+0x1f0/0x1f0
 [<ffffffff815709a0>] ? store+0x70/0xd0
 [<ffffffff81572303>] ? cpufreq_freq_transition_begin+0xb3/0x140
 [<ffffffff810d4953>] ? __wake_up+0x23/0x50
 [<ffffffff815709bb>] store+0x8b/0xd0
 [<ffffffff812c9174>] sysfs_kf_write+0x44/0x60
 [<ffffffff812c8a67>] kernfs_fop_write+0xe7/0x170
 [<ffffffff8124239a>] vfs_write+0xba/0x1f0
 [<ffffffff81242fe8>] SyS_write+0x58/0xd0
 [<ffffffff81731769>] system_call_fastpath+0x16/0x1b
Code: ff 84 c0 0f 84 50 02 00 00 49 8b 5c 24 58 48 85 db 0f 84 59 07 00 00 41 83
fe 04 0f 84 70 02 00 00 41 83 fe 05 0f 84 3e 02 00 00 <48> 8b 03 44 89 ef ff 50
20 48 89 45 c0 48 8b 03 83 38 01 0f 84
RIP  [<ffffffff81577052>] cpufreq_governor_dbs+0x52/0x820
 RSP <ffff880ff4193a70>
CR2: 0000000000000000
---[ end trace 2d1b939e62867d1f ]---

This occurs at boot on my system, during systemd setup.  This *does not* happen
with plain upstream kernel.

Robert, OOC ... what happens when you patch in (sorry for the cut-and-paste)
the following? [warning: complete and total hack.  This "breaks" arm, but the
original patch that introduced this change broke x86]

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 24bf76f..94ac441 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2195,9 +2195,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *polic
        /* end old governor */
        if (old_gov) {
                __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-               up_write(&policy->rwsem);
                __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-               down_write(&policy->rwsem);
        }

        /* start new governor */
@@ -2206,9 +2204,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *polic
                if (!__cpufreq_governor(policy, CPUFREQ_GOV_START))
                        goto out;

-               up_write(&policy->rwsem);
                __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-               down_write(&policy->rwsem);
        }

P.

^ permalink raw reply related	[flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] cpufreq: serialize calls to __cpufreq_governor()
@ 2014-10-10 13:40 Prarit Bhargava
  2014-10-10 13:42 ` Robert Schöne
  0 siblings, 1 reply; 38+ messages in thread
From: Prarit Bhargava @ 2014-10-10 13:40 UTC (permalink / raw)
  To: Robert Schöne
  Cc: Viresh Kumar, Rafael J. Wysocki, Lists linaro-kernel,
	linux-pm@vger.kernel.org, Saravana Kannan


> In v2 my system still crashes when concurrently setting the governors
> I wasn't able to get a stack trace.
> 
> Robert 
> 

Send me your test script please.

P.

^ permalink raw reply	[flat|nested] 38+ messages in thread
* [PATCH 1/2] cpufreq: serialize calls to __cpufreq_governor()
@ 2014-09-09  4:16 Viresh Kumar
  2014-09-09  7:29 ` Robert Schöne
                   ` (2 more replies)
  0 siblings, 3 replies; 38+ messages in thread
From: Viresh Kumar @ 2014-09-09  4:16 UTC (permalink / raw)
  To: Rafael Wysocki
  Cc: linaro-kernel, linux-pm, robert.schoene, prarit, skannan,
	Viresh Kumar

This commit was earlier commited in kernel as:
19c7630 cpufreq: serialize calls to __cpufreq_governor()

and was later reverted by Srivatsa:
56d07db cpufreq: Remove temporary fix for race between CPU hotplug and sysfs-writes

When this commit was first introduced it was written for races during hotplug
and because we got some other solution to take care of the races with hotplug we
reverted it.

But (as I also said in the revert patch: https://lkml.org/lkml/2013/9/10/61)
there are more cases where this is required.

Recently Robert shown an instance where changing governors with multiple threads
leads to following warnings:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 2458 at drivers/cpufreq/cpufreq_governor.c:261 cpufreq_governor_dbs+0x6d2/0x740()
CPU: 1 PID: 2458 Comm: tee Tainted: G           OE 3.16.0-rc6+ #1
Hardware name: FUJITSU ESPRIMO P700/D3061-A1, BIOS V4.6.4.0 R1.12.0 for D3061-A1x 07/04/2011
 0000000000000009 ffff8800ae403b78 ffffffff8173b0bf 0000000000000000
 ffff8800ae403bb0 ffffffff8106c82d 0000000000000000 ffff88022fa27000
 0000000000000005 0000000000000002 ffffffff81cd5d00 ffff8800ae403bc0
Call Trace:
 [<ffffffff8173b0bf>] dump_stack+0x45/0x56
 [<ffffffff8106c82d>] warn_slowpath_common+0x7d/0xa0
 [<ffffffff8106c90a>] warn_slowpath_null+0x1a/0x20
 [<ffffffff815e4a12>] cpufreq_governor_dbs+0x6d2/0x740
 [<ffffffff810941fc>] ? notifier_call_chain+0x4c/0x70
 [<ffffffff815e2757>] od_cpufreq_governor_dbs+0x17/0x20
 [<ffffffff815dea50>] __cpufreq_governor+0xb0/0x2a0
 [<ffffffff815ded8c>] cpufreq_set_policy+0x14c/0x2f0
 [<ffffffff815df796>] store_scaling_governor+0x96/0xf0
 [<ffffffff815df100>] ? cpufreq_update_policy+0x1d0/0x1d0
 [<ffffffff815de3c9>] store+0x79/0xc0
 [<ffffffff81245bed>] sysfs_kf_write+0x3d/0x50
 [<ffffffff81245120>] kernfs_fop_write+0xe0/0x160
 [<ffffffff811d00d7>] vfs_write+0xb7/0x1f0
 [<ffffffff811d0c76>] SyS_write+0x46/0xb0
 [<ffffffff817439ff>] tracesys+0xe1/0xe6
---[ end trace a2dad7e42b22c796 ]---
BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffff815e4395>] cpufreq_governor_dbs+0x55/0x740
PGD 36a05067 PUD b47df067 PMD 0
Oops: 0000 [#1] SMP

Robert also provided a small script to reproduce it:
crash_governor.sh:
for I in `seq 1000`
do
        echo ondemand | sudo tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor
        echo userspace | sudo tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor
done

runme.sh:
for I in `seq 8`
do
        ./crash_governor.sh &
done

Just run runme.sh to crash your system :)

Introduce an additional variable which would guarantee serialization of
__cpufreq_governor() routine.

Reported-and-tested-by: Robert Schöne <robert.schoene@tu-dresden.de>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Hi Rafael,

These fixes the issues reported by Robert. There is slight change after Robert
tested my initial patch, 'bool' is replaced by 'int' for 'governor_state'.

Regardingn stable trees, I am not too sure. The first patch of this series was
earlier applied on 3.12 and then was reverted quickly in the same release.

So, the best we can do is 3.12+.

 drivers/cpufreq/cpufreq.c | 7 ++++++-
 include/linux/cpufreq.h   | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index d9fdedd..a7ceae3 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2012,13 +2012,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 		 policy->cpu, event);
 
 	mutex_lock(&cpufreq_governor_lock);
-	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
+	if (policy->governor_busy
+	    || (policy->governor_enabled && event == CPUFREQ_GOV_START)
 	    || (!policy->governor_enabled
 	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
 		mutex_unlock(&cpufreq_governor_lock);
 		return -EBUSY;
 	}
 
+	policy->governor_busy = true;
 	if (event == CPUFREQ_GOV_STOP)
 		policy->governor_enabled = false;
 	else if (event == CPUFREQ_GOV_START)
@@ -2047,6 +2049,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 			((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
 		module_put(policy->governor->owner);
 
+	mutex_lock(&cpufreq_governor_lock);
+	policy->governor_busy = false;
+	mutex_unlock(&cpufreq_governor_lock);
 	return ret;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 7d1955a..c7aa96b 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -82,6 +82,7 @@ struct cpufreq_policy {
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
 	bool			governor_enabled; /* governor start/stop flag */
+	bool			governor_busy;
 
 	struct work_struct	update; /* if update_policy() needs to be
 					 * called, but you're in IRQ context */
-- 
2.0.3.693.g996b0fd


^ permalink raw reply related	[flat|nested] 38+ messages in thread
end of thread, other threads:[~2014-10-17 12:12 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-08  7:04 [PATCH 1/2] cpufreq: serialize calls to __cpufreq_governor() Viresh Kumar
2014-10-08 12:46 ` Prarit Bhargava
2014-10-10  9:04   ` Viresh Kumar
2014-10-10 10:41     ` Robert Schöne
2014-10-10 11:14       ` Viresh Kumar
2014-10-10 11:21     ` Prarit Bhargava
2014-10-10 11:30       ` Viresh Kumar
2014-10-10 11:38         ` Prarit Bhargava
2014-10-10 11:46           ` Viresh Kumar
2014-10-10 11:48             ` Prarit Bhargava
2014-10-10 12:01               ` Robert Schöne
2014-10-10 12:39                 ` Viresh Kumar
2014-10-10 13:04                   ` Robert Schöne
2014-10-10 13:23                   ` Robert Schöne
2014-10-10 13:52                     ` Viresh Kumar
2014-10-10 14:05                       ` Robert Schöne
2014-10-14  6:58                         ` Viresh Kumar
2014-10-14 11:42                           ` Prarit Bhargava
2014-10-14 17:12                             ` Prarit Bhargava
2014-10-16 10:58                               ` Viresh Kumar
2014-10-17 12:12                                 ` Prarit Bhargava
2014-10-16 10:57                             ` Viresh Kumar
2014-10-17 12:09                               ` Prarit Bhargava
  -- strict thread matches above, loose matches on Subject: below --
2014-10-10 13:55 Prarit Bhargava
2014-10-10 13:58 ` Viresh Kumar
2014-10-10 13:40 Prarit Bhargava
2014-10-10 13:42 ` Robert Schöne
2014-09-09  4:16 Viresh Kumar
2014-09-09  7:29 ` Robert Schöne
2014-09-09  7:35   ` Viresh Kumar
     [not found]   ` <540EEA95.8030208@redhat.com>
2014-09-09 14:45     ` Viresh Kumar
2014-09-24 23:46 ` Rafael J. Wysocki
2014-09-25  6:07   ` Robert Schöne
2014-09-29  9:50   ` Viresh Kumar
2014-09-29 11:29 ` Prarit Bhargava
2014-09-29 11:38   ` Viresh Kumar
2014-09-29 11:50     ` Prarit Bhargava
2014-09-29 11:55       ` Viresh Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).