Re: remove cpu hotplug bustification in cpufreq.

Re: remove cpu hotplug bustification in cpufreq.

[Chuck Ebbert]

In-Reply-To: <1153738326.3043.13.camel@laptopd505.fenrus.org>

On Mon, 24 Jul 2006 12:52:05 +0200, Arjan van de Ven wrote:
> 
> > What kind of _crap_ is this cpufreq thing?
> 
> this is why lockdep got highly upset with it, and why Davej proposed to
> remove the locking entirely for now until it can be put back in a
> correct way....

I thought just the 'ondemand' governor was a problem?

That thing has been broken since day 1 AFAICT.  There are lots of
reports of problems with it on the list.

-- 
Chuck

Re: remove cpu hotplug bustification in cpufreq.

[Linus Torvalds]

On Mon, 24 Jul 2006, Chuck Ebbert wrote:
> 
> I thought just the 'ondemand' governor was a problem?

The ondemand governor seems to be singled out not because it has unique 
problems, but because it seems to be used by Fedora Core for some strange 
reason.

I would judge that any bugs in cpufreq_ondemand.c are likely equally 
evident in cpufreq_conservative.c, for example. I think the two have the 
same background, and seem to have the same broken locking.

> That thing has been broken since day 1 AFAICT.  There are lots of
> reports of problems with it on the list.

See above. I seriously doubt this is ondemand-specific. The whole cpufreq 
locking seems to be very screwed up.

The current -git tree will complain about some of the more obvious 
problems. If you see a "Lukewarm IQ" message, it's a sign of somebody 
re-taking a cpu lock that is already held.

			Linus

Re: remove cpu hotplug bustification in cpufreq.

[Erik Mouw]

On Mon, Jul 24, 2006 at 05:59:23PM -0700, Linus Torvalds wrote:
> On Mon, 24 Jul 2006, Chuck Ebbert wrote:
> > 
> > I thought just the 'ondemand' governor was a problem?
> 
> The ondemand governor seems to be singled out not because it has unique 
> problems, but because it seems to be used by Fedora Core for some strange 
> reason.
> 
> I would judge that any bugs in cpufreq_ondemand.c are likely equally 
> evident in cpufreq_conservative.c, for example. I think the two have the 
> same background, and seem to have the same broken locking.

The "conservative" governor switches less often, so the locking
condition just happens less often.

After some strange lockups with 2.6.18-rc* I switched from "ondemand"
to "conservative" and now my laptop survives the night. Sheer luck, I
guess. I'd rather switch to "powersave" or "performance".


Erik

-- 
+-- Erik Mouw -- www.harddisk-recovery.com -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands

Re: remove cpu hotplug bustification in cpufreq.

[Ingo Molnar]

* Linus Torvalds <torvalds@osdl.org> wrote:

> The current -git tree will complain about some of the more obvious 
> problems. If you see a "Lukewarm IQ" message, it's a sign of somebody 
> re-taking a cpu lock that is already held.

testing on my latest-rawhide laptop (kernel-2.6.17-1.2445.fc6 and later 
rpms have this change) seems to have pushed the problem over to another 
lock:

  S06cpuspeed/1580 is trying to acquire lock:
   (&policy->lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24

  but task is already holding lock:
   (cpu_bitmask_lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24

  which lock already depends on the new lock.

and we also get the:

  Lukewarm IQ detected in hotplug locking

message :-| Find the full bootlog below. And i dont understand the 
cpufreq code well enough to fix this. In fact, does anyone understand 
it? :-/

	Ingo

------------>
Linux version 2.6.17-1.2445.fc6 (brewbuilder@hs20-bc2-2.build.redhat.com) (gcc version 4.1.1 20060718 (Red Hat 4.1.1-9)) #1 SMP Mon Jul 24 22:43:31 EDT 2006
BIOS-provided physical RAM map:
 BIOS-e820: 0000000000000000 - 000000000009f800 (usable)
 BIOS-e820: 000000000009f800 - 00000000000a0000 (reserved)
 BIOS-e820: 00000000000ce000 - 00000000000d0000 (reserved)
 BIOS-e820: 00000000000dc000 - 0000000000100000 (reserved)
 BIOS-e820: 0000000000100000 - 000000003dee0000 (usable)
 BIOS-e820: 000000003dee0000 - 000000003deec000 (ACPI data)
 BIOS-e820: 000000003deec000 - 000000003df00000 (ACPI NVS)
 BIOS-e820: 000000003df00000 - 0000000040000000 (reserved)
 BIOS-e820: 00000000ff800000 - 00000000ffc00000 (reserved)
 BIOS-e820: 00000000fffffc00 - 0000000100000000 (reserved)
94MB HIGHMEM available.
895MB LOWMEM available.
Using x86 segment limits to approximate NX protection
On node 0 totalpages: 253664
  DMA zone: 4096 pages, LIFO batch:0
  Normal zone: 225279 pages, LIFO batch:31
  HighMem zone: 24289 pages, LIFO batch:3
DMI present.
Using APIC driver default
ACPI: RSDP (v000 HP                                    ) @ 0x000f6560
ACPI: RSDT (v001 HP     3084     0x06040000  LTP 0x00000000) @ 0x3dee66d9
ACPI: FADT (v001 HP     3084     0x06040000 PTL  0x00000050) @ 0x3deebed2
ACPI: BOOT (v001 HP     3084     0x06040000  LTP 0x00000001) @ 0x3deebfd8
ACPI: SSDT (v001 HP     3084     0x00000001 INTL 0x20030224) @ 0x3dee6b10
ACPI: SSDT (v001 HP     3084     0x00002000 INTL 0x20030224) @ 0x3dee670d
ACPI: DSDT (v001 HP     3084     0x06040000 MSFT 0x0100000e) @ 0x00000000
ACPI: PM-Timer IO Port: 0x1008
Allocating PCI resources starting at 50000000 (gap: 40000000:bf800000)
Detected 1596.087 MHz processor.
Built 1 zonelists.  Total pages: 253664
Kernel command line: ro root=/dev/VolGroup00/LogVol00 quiet selinux=0 3
Local APIC disabled by BIOS -- you can enable it with "lapic"
mapped APIC to ffffd000 (017d3000)
Enabling fast FPU save and restore... done.
Enabling unmasked SIMD FPU exception support... done.
Initializing CPU#0
CPU 0 irqstacks, hard=c0802000 soft=c07e2000
PID hash table entries: 4096 (order: 12, 16384 bytes)
Console: colour VGA+ 80x25
Lock dependency validator: Copyright (c) 2006 Red Hat, Inc., Ingo Molnar
... MAX_LOCKDEP_SUBCLASSES:    8
... MAX_LOCK_DEPTH:          30
... MAX_LOCKDEP_KEYS:        2048
... CLASSHASH_SIZE:           1024
... MAX_LOCKDEP_ENTRIES:     8192
... MAX_LOCKDEP_CHAINS:      8192
... CHAINHASH_SIZE:          4096
 memory used by lock dependency info: 696 kB
 per task-struct memory footprint: 1200 bytes
Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
Memory: 997324k/1014656k available (2092k kernel code, 16724k reserved, 1113k data, 240k init, 97156k highmem)
Checking if this processor honours the WP bit even in supervisor mode... Ok.
Calibrating delay using timer specific routine.. 3194.92 BogoMIPS (lpj=6389854)
Security Framework v1.0.0 initialized
SELinux:  Disabled at boot.
Capability LSM initialized
Mount-cache hash table entries: 512
CPU: After generic identify, caps: afe9f9bf 00000000 00000000 00000000 00000180 00000000 00000000
CPU: After vendor identify, caps: afe9f9bf 00000000 00000000 00000000 00000180 00000000 00000000
CPU: L1 I cache: 32K, L1 D cache: 32K
CPU: L2 cache: 2048K
CPU: After all inits, caps: afe9f1bf 00000000 00000000 00000040 00000180 00000000 00000000
Intel machine check architecture supported.
Intel machine check reporting enabled on CPU#0.
Checking 'hlt' instruction... OK.
SMP alternatives: switching to UP code
Freeing SMP alternatives: 16k freed
ACPI: Core revision 20060707
ACPI: setting ELCR to 0200 (from 0c20)
CPU0: Intel(R) Pentium(R) M processor 1.60GHz stepping 06
SMP motherboard not detected.
Local APIC not detected. Using dummy APIC emulation.
Brought up 1 CPUs
sizeof(vma)=84 bytes
sizeof(page)=32 bytes
sizeof(inode)=568 bytes
sizeof(dentry)=160 bytes
sizeof(ext3inode)=804 bytes
sizeof(buffer_head)=52 bytes
sizeof(skbuff)=172 bytes
checking if image is initramfs... it is
Freeing initrd memory: 1970k freed
PM: Adding info for No Bus:platform
NET: Registered protocol family 16
ACPI: bus type pci registered
PCI: PCI BIOS revision 2.10 entry at 0xfd9a2, last bus=2
Setting up standard PCI resources
ACPI: Interpreter enabled
ACPI: Using PIC for interrupt routing
PM: Adding info for acpi:acpi
ACPI: PCI Root Bridge [PCI0] (0000:00)
PCI: Probing PCI hardware (bus 00)
PM: Adding info for No Bus:pci0000:00
Boot video device is 0000:00:02.0
PCI quirk: region 1000-107f claimed by ICH4 ACPI/GPIO/TCO
PCI quirk: region 1180-11bf claimed by ICH4 GPIO
PCI: Ignoring BAR0-3 of IDE controller 0000:00:1f.1
PCI: Transparent bridge - 0000:00:1e.0
PCI: Bus #03 (-#06) is hidden behind transparent bridge #02 (-#02) (try 'pci=assign-busses')
Please report the result to linux-kernel to fix this permanently
ACPI: PCI Interrupt Routing Table [\_SB_.PCI0._PRT]
PM: Adding info for pci:0000:00:00.0
PM: Adding info for pci:0000:00:00.1
PM: Adding info for pci:0000:00:00.3
PM: Adding info for pci:0000:00:02.0
PM: Adding info for pci:0000:00:02.1
PM: Adding info for pci:0000:00:1d.0
PM: Adding info for pci:0000:00:1d.1
PM: Adding info for pci:0000:00:1d.2
PM: Adding info for pci:0000:00:1d.7
PM: Adding info for pci:0000:00:1e.0
PM: Adding info for pci:0000:00:1f.0
PM: Adding info for pci:0000:00:1f.1
PM: Adding info for pci:0000:00:1f.3
PM: Adding info for pci:0000:00:1f.5
PM: Adding info for pci:0000:00:1f.6
PM: Adding info for pci:0000:02:00.0
PM: Adding info for pci:0000:02:05.0
PM: Adding info for pci:0000:02:07.0
ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.PCIB._PRT]
ACPI: PCI Interrupt Link [LNKA] (IRQs *10)
ACPI: PCI Interrupt Link [LNKB] (IRQs *5)
ACPI: PCI Interrupt Link [LNKC] (IRQs 11) *0, disabled.
ACPI: PCI Interrupt Link [LNKD] (IRQs *11)
ACPI: PCI Interrupt Link [LNKE] (IRQs 11) *0, disabled.
ACPI: PCI Interrupt Link [LNKF] (IRQs *11)
ACPI: PCI Interrupt Link [LNKG] (IRQs 11) *0, disabled.
ACPI: PCI Interrupt Link [LNKH] (IRQs *11)
ACPI: Embedded Controller [H_EC] (gpe 29) interrupt mode.
Linux Plug and Play Support v0.97 (c) Adam Belay
pnp: PnP ACPI init
PM: Adding info for No Bus:pnp0
PM: Adding info for pnp:00:00
PM: Adding info for pnp:00:01
PM: Adding info for pnp:00:02
PM: Adding info for pnp:00:03
PM: Adding info for pnp:00:04
PM: Adding info for pnp:00:05
PM: Adding info for pnp:00:06
pnp: PnP ACPI: found 7 devices
usbcore: registered new driver usbfs
usbcore: registered new driver hub
PCI: Using ACPI for IRQ routing
PCI: If a device doesn't work, try "pci=routeirq".  If it helps, post a report
PCI: Ignore bogus resource 6 [0:0] of 0000:00:02.0
PCI: Bus 3, cardbus bridge: 0000:02:05.0
  IO window: 00003400-000034ff
  IO window: 00003800-000038ff
  PREFETCH window: 50000000-51ffffff
  MEM window: 54000000-55ffffff
PCI: Bridge: 0000:00:1e.0
  IO window: 3000-3fff
  MEM window: e0200000-e02fffff
  PREFETCH window: 50000000-51ffffff
PCI: Setting latency timer of device 0000:00:1e.0 to 64
ACPI: PCI Interrupt Link [LNKE] enabled at IRQ 11
PCI: setting IRQ 11 as level-triggered
ACPI: PCI Interrupt 0000:02:05.0[A] -> Link [LNKE] -> GSI 11 (level, low) -> IRQ 11
NET: Registered protocol family 2
IP route cache hash table entries: 32768 (order: 5, 131072 bytes)
TCP established hash table entries: 131072 (order: 10, 4194304 bytes)
TCP bind hash table entries: 65536 (order: 9, 2097152 bytes)
TCP: Hash tables configured (established 131072 bind 65536)
TCP reno registered
PM: Adding info for platform:pcspkr
Simple Boot Flag at 0x36 set to 0x1
apm: BIOS version 1.2 Flags 0x03 (Driver version 1.16ac)
apm: overridden by ACPI.
audit: initializing netlink socket (disabled)
audit(1153861090.720:1): initialized
highmem bounce pool size: 64 pages
Total HugeTLB memory allocated, 0
VFS: Disk quotas dquot_6.5.1
Dquot-cache hash table entries: 1024 (order 0, 4096 bytes)
Initializing Cryptographic API
io scheduler noop registered
io scheduler anticipatory registered
io scheduler deadline registered
io scheduler cfq registered (default)
pci_hotplug: PCI Hot Plug PCI Core version: 0.5
PM: Adding info for platform:vesafb.0
ACPI: CPU0 (power states: C1[C1] C2[C2])
ACPI: Thermal Zone [THRM] (63 C)
PM: Adding info for No Bus:pnp1
isapnp: Scanning for PnP cards...
isapnp: No Plug & Play device found
Real Time Clock Driver v1.12ac
Non-volatile memory driver v1.2
Linux agpgart interface v0.101 (c) Dave Jones
agpgart: Detected an Intel 855 Chipset.
agpgart: Detected 32636K stolen memory.
agpgart: AGP aperture is 128M @ 0xe8000000
Serial: 8250/16550 driver $Revision: 1.90 $ 4 ports, IRQ sharing enabled
PM: Adding info for platform:serial8250
ACPI: PCI Interrupt Link [LNKB] enabled at IRQ 5
PCI: setting IRQ 5 as level-triggered
ACPI: PCI Interrupt 0000:00:1f.6[B] -> Link [LNKB] -> GSI 5 (level, low) -> IRQ 5
ACPI: PCI interrupt for device 0000:00:1f.6 disabled
PM: Adding info for No Bus:isa
RAMDISK driver initialized: 16 RAM disks of 16384K size 4096 blocksize
Uniform Multi-Platform E-IDE driver Revision: 7.00alpha2
ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
ICH4: IDE controller at PCI slot 0000:00:1f.1
PCI: Enabling device 0000:00:1f.1 (0005 -> 0007)
ACPI: PCI Interrupt Link [LNKC] enabled at IRQ 11
ACPI: PCI Interrupt 0000:00:1f.1[A] -> Link [LNKC] -> GSI 11 (level, low) -> IRQ 11
ICH4: chipset revision 3
ICH4: not 100% native mode: will probe irqs later
    ide0: BM-DMA at 0x1810-0x1817, BIOS settings: hda:DMA, hdb:pio
    ide1: BM-DMA at 0x1818-0x181f, BIOS settings: hdc:DMA, hdd:pio
Probing IDE interface ide0...
hda: HITACHI_DK23FA-40, ATA DISK drive
PM: Adding info for No Bus:ide0
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
PM: Adding info for ide:0.0
Probing IDE interface ide1...
hdc: HL-DT-STCD-RW/DVD DRIVE GCC-4243N, ATAPI CD/DVD-ROM drive
PM: Adding info for No Bus:ide1
ide1 at 0x170-0x177,0x376 on irq 15
PM: Adding info for ide:1.0
hda: max request size: 128KiB
hda: 78140160 sectors (40007 MB) w/2048KiB Cache, CHS=65535/16/63, UDMA(100)
hda: cache flushes supported
 hda: hda1 hda2
ide-floppy driver 0.99.newide
Yenta: CardBus bridge found at 0000:02:05.0 [103c:3084]
Yenta: Using CSCINT to route CSC interrupts to PCI
Yenta: Routing CardBus interrupts to PCI
Yenta TI: socket 0000:02:05.0, mfunc 0x01111112, devctl 0x64
Yenta: ISA IRQ mask 0x04d8, PCI irq 11
Socket status: 30000010
Yenta: Raising subordinate bus# of parent bus (#02) from #02 to #06
pcmcia: parent PCI bridge I/O window: 0x3000 - 0x3fff
cs: IO port probe 0x3000-0x3fff: clean.
pcmcia: parent PCI bridge Memory window: 0xe0200000 - 0xe02fffff
pcmcia: parent PCI bridge Memory window: 0x50000000 - 0x51ffffff
usbcore: registered new driver libusual
usbcore: registered new driver hiddev
usbcore: registered new driver usbhid
drivers/usb/input/hid-core.c: v2.6:USB HID core driver
PNP: PS/2 Controller [PNP0303:PS2K,PNP0f13:PSM1] at 0x60,0x64 irq 1,12
PM: Adding info for platform:i8042
serio: i8042 AUX port at 0x60,0x64 irq 12
serio: i8042 KBD port at 0x60,0x64 irq 1
PM: Adding info for serio:serio0
mice: PS/2 mouse device common for all mice
md: md driver 0.90.3 MAX_MD_DEVS=256, MD_SB_DISKS=27
md: bitmap version 4.39
TCP bic registered
Initializing IPsec netlink socket
NET: Registered protocol family 1
NET: Registered protocol family 17
PM: Adding info for serio:serio1
Using IPI No-Shortcut mode
ACPI: (supports S0 S3 S4 S5)
Freeing unused kernel memory: 240k freed
Write protecting the kernel read-only data: 395k
Time: tsc clocksource has been installed.
Time: acpi_pm clocksource has been installed.
input: AT Translated Set 2 keyboard as /class/input/input0
device-mapper: ioctl: 4.7.0-ioctl (2006-06-24) initialised: dm-devel@redhat.com
pccard: PCMCIA card inserted into slot 0
cs: memory probe 0xe0200000-0xe02fffff: excluding 0xe0200000-0xe020ffff
pcmcia: registering new device pcmcia0.0
PM: Adding info for pcmcia:0.0
Synaptics Touchpad, model: 1, fw: 5.9, id: 0x236eb3, caps: 0x904713/0x10008
input: SynPS/2 Synaptics TouchPad as /class/input/input1
EXT3-fs: INFO: recovery required on readonly filesystem.
EXT3-fs: write access will be enabled during recovery.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: dm-0: orphan cleanup on readonly fs
ext3_orphan_cleanup: deleting unreferenced inode 5763820
ext3_orphan_cleanup: deleting unreferenced inode 5765620
ext3_orphan_cleanup: deleting unreferenced inode 5762038
EXT3-fs: dm-0: 3 orphan inodes deleted
EXT3-fs: recovery complete.
EXT3-fs: mounted filesystem with ordered data mode.
orinoco 0.15 (David Gibson <hermes@gibson.dropbear.id.au>, Pavel Roskin <proski@gnu.org>, et al)
orinoco_cs 0.15 (David Gibson <hermes@gibson.dropbear.id.au>, Pavel Roskin <proski@gnu.org>, et al)
eth0: Hardware identity 0001:0001:0004:0002
eth0: Station identity  001f:0001:0008:000a
eth0: Firmware determined as Lucent/Agere 8.10
eth0: Ad-hoc demo mode supported
eth0: IEEE standard IBSS ad-hoc mode supported
eth0: WEP supported, 104-bit key
eth0: MAC address 00:02:A5:2D:84:BE
eth0: Station name "HERMES I"
eth0: ready
eth0: orinoco_cs at 0.0, irq 3, io 0x3100-0x313f
hdc: ATAPI 24X DVD-ROM CD-R/RW drive, 2048kB Cache, DMA
Uniform CD-ROM driver Revision: 3.20
ieee1394: Initialized config rom entry `ip1394'
ACPI: PCI Interrupt Link [LNKF] enabled at IRQ 11
ACPI: PCI Interrupt 0000:02:07.0[A] -> Link [LNKF] -> GSI 11 (level, low) -> IRQ 11
PM: Adding info for ieee1394:fw-host0
ohci1394: fw-host0: OHCI-1394 1.1 (PCI): IRQ=[11]  MMIO=[e0204000-e02047ff]  Max Packet=[2048]  IR/IT contexts=[4/8]
8139cp: 10/100 PCI Ethernet driver v1.2 (Mar 22, 2004)
8139cp 0000:02:00.0: This (id 10ec:8139 rev 10) is not an 8139C+ compatible chip
8139cp 0000:02:00.0: Try the "8139too" driver instead.
ACPI: PCI Interrupt 0000:00:1f.3[B] -> Link [LNKB] -> GSI 5 (level, low) -> IRQ 5
PM: Adding info for No Bus:i2c-0
ACPI: PCI Interrupt Link [LNKH] enabled at IRQ 11
ACPI: PCI Interrupt 0000:00:1d.7[D] -> Link [LNKH] -> GSI 11 (level, low) -> IRQ 11
PCI: Setting latency timer of device 0000:00:1d.7 to 64
ehci_hcd 0000:00:1d.7: EHCI Host Controller
ehci_hcd 0000:00:1d.7: new USB bus registered, assigned bus number 1
ehci_hcd 0000:00:1d.7: debug port 1
PCI: cache line size of 32 is not supported by device 0000:00:1d.7
ehci_hcd 0000:00:1d.7: irq 11, io mem 0xe0100000
ehci_hcd 0000:00:1d.7: USB 2.0 started, EHCI 1.00, driver 10 Dec 2004
PM: Adding info for usb:usb1
PM: Adding info for No Bus:usbdev1.1_ep00
usb usb1: configuration #1 chosen from 1 choice
PM: Adding info for usb:1-0:1.0
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 6 ports detected
USB Universal Host Controller Interface driver v3.0
8139too Fast Ethernet driver 0.9.27
PM: Adding info for No Bus:usbdev1.1_ep81
PM: Adding info for No Bus:usbdev1.1
ACPI: PCI Interrupt Link [LNKA] enabled at IRQ 10
PCI: setting IRQ 10 as level-triggered
ACPI: PCI Interrupt 0000:00:1d.0[A] -> Link [LNKA] -> GSI 10 (level, low) -> IRQ 10
PCI: Setting latency timer of device 0000:00:1d.0 to 64
uhci_hcd 0000:00:1d.0: UHCI Host Controller
uhci_hcd 0000:00:1d.0: new USB bus registered, assigned bus number 2
uhci_hcd 0000:00:1d.0: irq 10, io base 0x00001820
PM: Adding info for usb:usb2
PM: Adding info for No Bus:usbdev2.1_ep00
usb usb2: configuration #1 chosen from 1 choice
PM: Adding info for usb:2-0:1.0
hub 2-0:1.0: USB hub found
hub 2-0:1.0: 2 ports detected
input: PC Speaker as /class/input/input2
PM: Adding info for No Bus:usbdev2.1_ep81
PM: Adding info for No Bus:usbdev2.1
ACPI: PCI Interrupt Link [LNKD] enabled at IRQ 11
ACPI: PCI Interrupt 0000:00:1d.1[B] -> Link [LNKD] -> GSI 11 (level, low) -> IRQ 11
PCI: Setting latency timer of device 0000:00:1d.1 to 64
uhci_hcd 0000:00:1d.1: UHCI Host Controller
uhci_hcd 0000:00:1d.1: new USB bus registered, assigned bus number 3
uhci_hcd 0000:00:1d.1: irq 11, io base 0x00001840
PM: Adding info for usb:usb3
PM: Adding info for No Bus:usbdev3.1_ep00
usb usb3: configuration #1 chosen from 1 choice
PM: Adding info for usb:3-0:1.0
hub 3-0:1.0: USB hub found
hub 3-0:1.0: 2 ports detected
PM: Adding info for No Bus:usbdev3.1_ep81
PM: Adding info for No Bus:usbdev3.1
ACPI: PCI Interrupt 0000:00:1d.2[C] -> Link [LNKC] -> GSI 11 (level, low) -> IRQ 11
PCI: Setting latency timer of device 0000:00:1d.2 to 64
uhci_hcd 0000:00:1d.2: UHCI Host Controller
uhci_hcd 0000:00:1d.2: new USB bus registered, assigned bus number 4
uhci_hcd 0000:00:1d.2: irq 11, io base 0x00001860
PM: Adding info for usb:usb4
PM: Adding info for No Bus:usbdev4.1_ep00
usb usb4: configuration #1 chosen from 1 choice
PM: Adding info for usb:4-0:1.0
hub 4-0:1.0: USB hub found
hub 4-0:1.0: 2 ports detected
PM: Adding info for No Bus:usbdev4.1_ep81
PM: Adding info for No Bus:usbdev4.1
ACPI: PCI Interrupt 0000:02:00.0[A] -> Link [LNKA] -> GSI 10 (level, low) -> IRQ 10
eth1: RealTek RTL8139 at 0xf89a4800, 00:c0:9f:63:51:51, IRQ 10
eth1:  Identified 8139 chip type 'RTL-8100B/8139D'
usb 2-2: new low speed USB device using uhci_hcd and address 2
ACPI: PCI Interrupt 0000:00:1f.5[B] -> Link [LNKB] -> GSI 5 (level, low) -> IRQ 5
PCI: Setting latency timer of device 0000:00:1f.5 to 64
cs: IO port probe 0x100-0x3af: clean.
cs: IO port probe 0x3e0-0x4ff: excluding 0x4d0-0x4d7
cs: IO port probe 0x820-0x8ff: clean.
cs: IO port probe 0xc00-0xcf7: clean.
cs: IO port probe 0xa00-0xaff: clean.
Intel 82802 RNG detected
PM: Adding info for usb:2-2
PM: Adding info for No Bus:usbdev2.2_ep00
usb 2-2: configuration #1 chosen from 1 choice
PM: Adding info for usb:2-2:1.0
input: HID 0461:4d03 as /class/input/input3
input: USB HID v1.00 Mouse [HID 0461:4d03] on usb-0000:00:1d.0-2
PM: Adding info for No Bus:usbdev2.2_ep81
PM: Adding info for No Bus:usbdev2.2
intel8x0_measure_ac97_clock: measured 53458 usecs
intel8x0: clocking to 48000
PM: Adding info for ac97:0-0:unknown codec
PCI: Enabling device 0000:00:1f.6 (0000 -> 0001)
ACPI: PCI Interrupt 0000:00:1f.6[B] -> Link [LNKB] -> GSI 5 (level, low) -> IRQ 5
PCI: Setting latency timer of device 0000:00:1f.6 to 64
PM: Adding info for ieee1394:00c09f00002e3893
ieee1394: Host added: ID:BUS[0-00:1023]  GUID[00c09f00002e3893]
PM: Adding info for ieee1394:00c09f00002e3893-0
MC'97 0 converters and GPIO not ready (0x1)
PM: Adding info for ac97:1-0:unknown codec
floppy0: no floppy controllers found
lp: driver loaded but no devices found
ACPI: AC Adapter [ACAD] (on-line)
ACPI: Battery Slot [BAT0] (battery present)
ACPI: Power Button (FF) [PWRF]
ACPI: Lid Switch [LID0]
ACPI: Power Button (CM) [PWRB]
ibm_acpi: ec object not found
ACPI: Video Device [GFX0] (multi-head: yes  rom: yes  post: no)
md: Autodetecting RAID arrays.
md: autorun ...
md: ... autorun DONE.
device-mapper: multipath: version 1.0.4 loaded
loop: loaded (max 8 devices)
EXT3 FS on dm-0, internal journal
kjournald starting.  Commit interval 5 seconds
EXT3 FS on hda1, internal journal
EXT3-fs: mounted filesystem with ordered data mode.
Adding 2031608k swap on /dev/VolGroup00/LogVol01.  Priority:-1 extents:1 across:2031608k

=======================================================
[ INFO: possible circular locking dependency detected ]
-------------------------------------------------------
S06cpuspeed/1580 is trying to acquire lock:
 (&policy->lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24

but task is already holding lock:
 (cpu_bitmask_lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (cpu_bitmask_lock){--..}:
       [<c043bf43>] lock_acquire+0x4b/0x6c
       [<c060748a>] __mutex_lock_slowpath+0xbc/0x20a
       [<c06075f9>] mutex_lock+0x21/0x24
       [<c043f25a>] lock_cpu_hotplug+0x64/0x6f
       [<c05a22f8>] __cpufreq_driver_target+0x15/0x6d
       [<c05a3258>] cpufreq_governor_userspace+0x199/0x1cc
       [<c05a1d9f>] __cpufreq_governor+0x57/0xd8
       [<c05a1fb7>] __cpufreq_set_policy+0x197/0x1a9
       [<c05a1ff6>] cpufreq_set_policy+0x2d/0x6f
       [<c05a29d1>] cpufreq_add_dev+0x31a/0x496
       [<c054bf9c>] sysdev_driver_register+0x5c/0x9b
       [<c05a1c98>] cpufreq_register_driver+0x9e/0x14e
       [<c07ab22b>] centrino_init+0x9b/0xa2
       [<c0400454>] init+0x180/0x2fe
       [<c0402005>] kernel_thread_helper+0x5/0xb

-> #1 (userspace_mutex){--..}:
       [<c043bf43>] lock_acquire+0x4b/0x6c
       [<c060748a>] __mutex_lock_slowpath+0xbc/0x20a
       [<c06075f9>] mutex_lock+0x21/0x24
       [<c05a3112>] cpufreq_governor_userspace+0x53/0x1cc
       [<c05a1d9f>] __cpufreq_governor+0x57/0xd8
       [<c05a1f5d>] __cpufreq_set_policy+0x13d/0x1a9
       [<c05a1ff6>] cpufreq_set_policy+0x2d/0x6f
       [<c05a29d1>] cpufreq_add_dev+0x31a/0x496
       [<c054bf9c>] sysdev_driver_register+0x5c/0x9b
       [<c05a1c98>] cpufreq_register_driver+0x9e/0x14e
       [<c07ab22b>] centrino_init+0x9b/0xa2
       [<c0400454>] init+0x180/0x2fe
       [<c0402005>] kernel_thread_helper+0x5/0xb

-> #0 (&policy->lock){--..}:
       [<c043bf43>] lock_acquire+0x4b/0x6c
       [<c060748a>] __mutex_lock_slowpath+0xbc/0x20a
       [<c06075f9>] mutex_lock+0x21/0x24
       [<c05a2158>] store_scaling_governor+0x120/0x15b
       [<c05a17f0>] store+0x37/0x48
       [<c04a8cd4>] sysfs_write_file+0xab/0xd1
       [<c0471f33>] vfs_write+0xab/0x157
       [<c0472578>] sys_write+0x3b/0x60
       [<c0403faf>] syscall_call+0x7/0xb

other info that might help us debug this:

1 lock held by S06cpuspeed/1580:
 #0:  (cpu_bitmask_lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24

stack backtrace:
 [<c04051ea>] show_trace_log_lvl+0x54/0xfd
 [<c04057a6>] show_trace+0xd/0x10
 [<c04058bf>] dump_stack+0x19/0x1b
 [<c043b030>] print_circular_bug_tail+0x59/0x64
 [<c043b843>] __lock_acquire+0x808/0x997
 [<c043bf43>] lock_acquire+0x4b/0x6c
 [<c060748a>] __mutex_lock_slowpath+0xbc/0x20a
 [<c06075f9>] mutex_lock+0x21/0x24
 [<c05a2158>] store_scaling_governor+0x120/0x15b
 [<c05a17f0>] store+0x37/0x48
 [<c04a8cd4>] sysfs_write_file+0xab/0xd1
 [<c0471f33>] vfs_write+0xab/0x157
 [<c0472578>] sys_write+0x3b/0x60
 [<c0403faf>] syscall_call+0x7/0xb
Lukewarm IQ detected in hotplug locking
BUG: warning at kernel/cpu.c:38/lock_cpu_hotplug() (Not tainted)
 [<c04051ea>] show_trace_log_lvl+0x54/0xfd
 [<c04057a6>] show_trace+0xd/0x10
 [<c04058bf>] dump_stack+0x19/0x1b
 [<c043f23f>] lock_cpu_hotplug+0x49/0x6f
 [<c043384f>] __create_workqueue+0x52/0x116
 [<f8af2337>] cpufreq_governor_dbs+0x9f/0x2d7 [cpufreq_ondemand]
 [<c05a1d9f>] __cpufreq_governor+0x57/0xd8
 [<c05a1f5d>] __cpufreq_set_policy+0x13d/0x1a9
 [<c05a2165>] store_scaling_governor+0x12d/0x15b
 [<c05a17f0>] store+0x37/0x48
 [<c04a8cd4>] sysfs_write_file+0xab/0xd1
 [<c0471f33>] vfs_write+0xab/0x157
 [<c0472578>] sys_write+0x3b/0x60
 [<c0403faf>] syscall_call+0x7/0xb
Lukewarm IQ detected in hotplug locking
BUG: warning at kernel/cpu.c:38/lock_cpu_hotplug() (Not tainted)
 [<c04051ea>] show_trace_log_lvl+0x54/0xfd
 [<c04057a6>] show_trace+0xd/0x10
 [<c04058bf>] dump_stack+0x19/0x1b
 [<c043f23f>] lock_cpu_hotplug+0x49/0x6f
 [<f8af250d>] cpufreq_governor_dbs+0x275/0x2d7 [cpufreq_ondemand]
 [<c05a1d9f>] __cpufreq_governor+0x57/0xd8
 [<c05a1fb7>] __cpufreq_set_policy+0x197/0x1a9
 [<c05a2165>] store_scaling_governor+0x12d/0x15b
 [<c05a17f0>] store+0x37/0x48
 [<c04a8cd4>] sysfs_write_file+0xab/0xd1
 [<c0471f33>] vfs_write+0xab/0x157
 [<c0472578>] sys_write+0x3b/0x60
 [<c0403faf>] syscall_call+0x7/0xb
ip_tables: (C) 2000-2006 Netfilter Core Team
Netfilter messages via NETLINK v0.30.
ip_conntrack version 2.4 (7927 buckets, 63416 max) - 228 bytes per conntrack
eth1: New link status: Connected (0001)
audit(1153853928.095:2): audit_pid=1809 old=0 by auid=4294967295
DLM (built Jul 24 2006 22:44:14) installed
Bluetooth: Core ver 2.10
PM: Adding info for platform:bluetooth
NET: Registered protocol family 31
Bluetooth: HCI device and connection manager initialized
Bluetooth: HCI socket layer initialized
Bluetooth: L2CAP ver 2.8
Bluetooth: L2CAP socket layer initialized
Bluetooth: RFCOMM socket layer initialized
Bluetooth: RFCOMM TTY layer initialized
Bluetooth: RFCOMM ver 1.8
Bluetooth: HIDP (Human Interface Emulation) ver 1.1
NET: Registered protocol family 10
lo: Disabled Privacy Extensions
IPv6 over IPv4 tunneling driver
PM: Removing info for ac97:1-0:unknown codec
ACPI: PCI interrupt for device 0000:00:1f.6 disabled
eth1: no IPv6 routers present

Re: remove cpu hotplug bustification in cpufreq.

[Arjan van de Ven]

On Tue, 2006-07-25 at 20:54 +0200, Ingo Molnar wrote:
> * Linus Torvalds <torvalds@osdl.org> wrote:
> 
> > The current -git tree will complain about some of the more obvious 
> > problems. If you see a "Lukewarm IQ" message, it's a sign of somebody 
> > re-taking a cpu lock that is already held.
> 
> testing on my latest-rawhide laptop (kernel-2.6.17-1.2445.fc6 and later 
> rpms have this change) seems to have pushed the problem over to another 
> lock:
> 
>   S06cpuspeed/1580 is trying to acquire lock:
>    (&policy->lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24
> 
>   but task is already holding lock:
>    (cpu_bitmask_lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24
> 
>   which lock already depends on the new lock.


so cpufreq_set_policy() takes policy->lock, and then calls into the
userspace governer code
(__cpufreq_set_policy->cpufreq_governor->cpufreq_governor_userspace)
which calls __cpufreq_driver_target... which does lock_cpu_hotplug().


now on the other side:
store_scaling_governor() has the following code:

        lock_cpu_hotplug();

        /* Do not use cpufreq_set_policy here or the user_policy.max
           will be wrongly overridden */
        mutex_lock(&policy->lock);

so that's the entirely opposite lock order, and a classic AB-BA
deadlock.

Greetings,
     Arjan -- who's just cleaned Linus' wall to prepare it for more head
banging

Re: remove cpu hotplug bustification in cpufreq.

[Dave Jones]

On Tue, Jul 25, 2006 at 08:54:49PM +0200, Ingo Molnar wrote:
 > 
 > * Linus Torvalds <torvalds@osdl.org> wrote:
 > 
 > > The current -git tree will complain about some of the more obvious 
 > > problems. If you see a "Lukewarm IQ" message, it's a sign of somebody 
 > > re-taking a cpu lock that is already held.
 > testing on my latest-rawhide laptop (kernel-2.6.17-1.2445.fc6 and later 
 > rpms have this change) seems to have pushed the problem over to another 
 > lock:
 > 
 >   S06cpuspeed/1580 is trying to acquire lock:
 >    (&policy->lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24
 > 
 >   but task is already holding lock:
 >    (cpu_bitmask_lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24
 > 
 >   which lock already depends on the new lock.
 > 
 > and we also get the:
 > 
 >   Lukewarm IQ detected in hotplug locking
 > 
 > message :-| Find the full bootlog below. And i dont understand the 
 > cpufreq code well enough to fix this. In fact, does anyone understand 
 > it? :-/

Things used to be fairly simple until hotplug cpu came along :-/
Each day, I'm getting more of the opinion that my patch just ripping
out this garbage is the right solution.

		Dave

-- 
http://www.codemonkey.org.uk

Re: remove cpu hotplug bustification in cpufreq.

[Linus Torvalds]

On Tue, 25 Jul 2006, Arjan van de Ven wrote:
>
> so cpufreq_set_policy() takes policy->lock, and then calls into the
> userspace governer code
> (__cpufreq_set_policy->cpufreq_governor->cpufreq_governor_userspace)
> which calls __cpufreq_driver_target... which does lock_cpu_hotplug().

Yeah. I think the target should _not_ take the lock_cpu_hotplug(), since 
the call chain (much much earlier) should have done it. 

Ie we should probably do it at the "cpufreq_set_policy()" level.

>      Arjan -- who's just cleaned Linus' wall to prepare it for more head
> banging

It's not actually "my wall". I'll happily share it with anybody else.

Please. Take my wall.

		Linus

Re: remove cpu hotplug bustification in cpufreq.

[Linus Torvalds]

On Tue, 25 Jul 2006, Dave Jones wrote:
> 
> Things used to be fairly simple until hotplug cpu came along :-/
> Each day, I'm getting more of the opinion that my patch just ripping
> out this garbage is the right solution.

I think your patch is fine, but the cpu_hotplug_lock() should probably be 
taken by the callers much higher up in the stack, and then we could just 
have the rule that by the time any real cpufreq code is actually entered, 
we should hold the lock already.

		Linus

[patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Arjan van de Ven]

On Tue, 2006-07-25 at 13:57 -0700, Linus Torvalds wrote:
> 
> On Tue, 25 Jul 2006, Arjan van de Ven wrote:
> >
> > so cpufreq_set_policy() takes policy->lock, and then calls into the
> > userspace governer code
> > (__cpufreq_set_policy->cpufreq_governor->cpufreq_governor_userspace)
> > which calls __cpufreq_driver_target... which does lock_cpu_hotplug().
> 
> Yeah. I think the target should _not_ take the lock_cpu_hotplug(), since 
> the call chain (much much earlier) should have done it. 
> 
> Ie we should probably do it at the "cpufreq_set_policy()" level.
> 
> >      Arjan -- who's just cleaned Linus' wall to prepare it for more head
> > banging
> 
> It's not actually "my wall". I'll happily share it with anybody else.
> 
> Please. Take my wall.

I had to clean the wall again after making the following patch, but at
least something useful came out of it:



Subject: Reorganize the cpufreq cpu hotplug locking to not be totally bizare
From: Arjan van de Ven <arjan@linux.intel.com>

The patch below moves the cpu hotplugging higher up in the cpufreq
layering; this is needed to avoid recursive taking of the cpu hotplug
lock and to otherwise detangle the mess.

The new rules are:
1. you must do lock_cpu_hotplug() around the following functions:
   __cpufreq_driver_target
   __cpufreq_governor (for CPUFREQ_GOV_LIMITS operation only)
   __cpufreq_set_policy
2. governer methods (.governer) must NOT take the lock_cpu_hotplug()
   lock in any way; they are called with the lock taken already
3. if your governer spawns a thread that does things, like calling
   __cpufreq_driver_target, your thread must honor rule #1.
4. the policy lock and other cpufreq internal locks nest within 
   the lock_cpu_hotplug() lock. 

I'm not entirely happy about how the __cpufreq_governor rule ended up
(conditional locking rule depending on the argument) but basically all
callers pass this as a constant so it's not too horrible.

The patch also removes the cpufreq_governor() function since during the
locking audit it turned out to be entirely unused (so no need to fix it)

The patch works on my testbox, but it could use more testing 
(otoh... it can't be much worse than the current code)

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

Index: linux-2.6.18-rc2-git5/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-2.6.18-rc2-git5.orig/drivers/cpufreq/cpufreq.c
+++ linux-2.6.18-rc2-git5/drivers/cpufreq/cpufreq.c
@@ -364,10 +364,12 @@ static ssize_t store_##file_name					\
 	if (ret != 1)							\
 		return -EINVAL;						\
 									\
+	lock_cpu_hotplug();						\
 	mutex_lock(&policy->lock);					\
 	ret = __cpufreq_set_policy(policy, &new_policy);		\
 	policy->user_policy.object = policy->object;			\
 	mutex_unlock(&policy->lock);					\
+	unlock_cpu_hotplug();						\
 									\
 	return ret ? ret : count;					\
 }
@@ -1197,20 +1199,18 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
  *********************************************************************/
 
 
+/* Must be called with lock_cpu_hotplug held */
 int __cpufreq_driver_target(struct cpufreq_policy *policy,
 			    unsigned int target_freq,
 			    unsigned int relation)
 {
 	int retval = -EINVAL;
 
-	lock_cpu_hotplug();
 	dprintk("target for CPU %u: %u kHz, relation %u\n", policy->cpu,
 		target_freq, relation);
 	if (cpu_online(policy->cpu) && cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
-	unlock_cpu_hotplug();
-
 	return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
@@ -1225,17 +1225,23 @@ int cpufreq_driver_target(struct cpufreq
 	if (!policy)
 		return -EINVAL;
 
+	lock_cpu_hotplug();
 	mutex_lock(&policy->lock);
 
 	ret = __cpufreq_driver_target(policy, target_freq, relation);
 
 	mutex_unlock(&policy->lock);
+	unlock_cpu_hotplug();
 
 	cpufreq_cpu_put(policy);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_target);
 
+/*
+ * Locking: Must be called with the lock_cpu_hotplug() lock held
+ * when "event" is CPUFREQ_GOV_LIMITS
+ */
 
 static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event)
 {
@@ -1257,24 +1263,6 @@ static int __cpufreq_governor(struct cpu
 }
 
 
-int cpufreq_governor(unsigned int cpu, unsigned int event)
-{
-	int ret = 0;
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-
-	if (!policy)
-		return -EINVAL;
-
-	mutex_lock(&policy->lock);
-	ret = __cpufreq_governor(policy, event);
-	mutex_unlock(&policy->lock);
-
-	cpufreq_cpu_put(policy);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(cpufreq_governor);
-
-
 int cpufreq_register_governor(struct cpufreq_governor *governor)
 {
 	struct cpufreq_governor *t;
@@ -1342,6 +1330,9 @@ int cpufreq_get_policy(struct cpufreq_po
 EXPORT_SYMBOL(cpufreq_get_policy);
 
 
+/*
+ * Locking: Must be called with the lock_cpu_hotplug() lock held
+ */
 static int __cpufreq_set_policy(struct cpufreq_policy *data, struct cpufreq_policy *policy)
 {
 	int ret = 0;
@@ -1436,6 +1427,8 @@ int cpufreq_set_policy(struct cpufreq_po
 	if (!data)
 		return -EINVAL;
 
+	lock_cpu_hotplug();
+
 	/* lock this CPU */
 	mutex_lock(&data->lock);
 
@@ -1446,6 +1439,8 @@ int cpufreq_set_policy(struct cpufreq_po
 	data->user_policy.governor = data->governor;
 
 	mutex_unlock(&data->lock);
+
+	unlock_cpu_hotplug();
 	cpufreq_cpu_put(data);
 
 	return ret;
@@ -1469,6 +1464,7 @@ int cpufreq_update_policy(unsigned int c
 	if (!data)
 		return -ENODEV;
 
+	lock_cpu_hotplug();
 	mutex_lock(&data->lock);
 
 	dprintk("updating policy for CPU %u\n", cpu);
@@ -1494,7 +1490,7 @@ int cpufreq_update_policy(unsigned int c
 	ret = __cpufreq_set_policy(data, &policy);
 
 	mutex_unlock(&data->lock);
-
+	unlock_cpu_hotplug();
 	cpufreq_cpu_put(data);
 	return ret;
 }
Index: linux-2.6.18-rc2-git5/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-2.6.18-rc2-git5.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-2.6.18-rc2-git5/drivers/cpufreq/cpufreq_conservative.c
@@ -525,7 +525,6 @@ static int cpufreq_governor_dbs(struct c
 		break;
 
 	case CPUFREQ_GOV_LIMITS:
-		lock_cpu_hotplug();
 		mutex_lock(&dbs_mutex);
 		if (policy->max < this_dbs_info->cur_policy->cur)
 			__cpufreq_driver_target(
@@ -536,7 +535,6 @@ static int cpufreq_governor_dbs(struct c
 					this_dbs_info->cur_policy,
 				       	policy->min, CPUFREQ_RELATION_L);
 		mutex_unlock(&dbs_mutex);
-		unlock_cpu_hotplug();
 		break;
 	}
 	return 0;
Index: linux-2.6.18-rc2-git5/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-2.6.18-rc2-git5.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-2.6.18-rc2-git5/drivers/cpufreq/cpufreq_ondemand.c
@@ -309,7 +309,9 @@ static void do_dbs_timer(void *data)
 	if (!dbs_info->enable)
 		return;
 
+	lock_cpu_hotplug();
 	dbs_check_cpu(dbs_info);
+	unlock_cpu_hotplug();
 	queue_delayed_work_on(cpu, kondemand_wq, &dbs_info->work,
 			usecs_to_jiffies(dbs_tuners_ins.sampling_rate));
 }
@@ -412,7 +414,6 @@ static int cpufreq_governor_dbs(struct c
 		break;
 
 	case CPUFREQ_GOV_LIMITS:
-		lock_cpu_hotplug();
 		mutex_lock(&dbs_mutex);
 		if (policy->max < this_dbs_info->cur_policy->cur)
 			__cpufreq_driver_target(this_dbs_info->cur_policy,
@@ -423,7 +424,6 @@ static int cpufreq_governor_dbs(struct c
 			                        policy->min,
 			                        CPUFREQ_RELATION_L);
 		mutex_unlock(&dbs_mutex);
-		unlock_cpu_hotplug();
 		break;
 	}
 	return 0;
Index: linux-2.6.18-rc2-git5/include/linux/cpufreq.h
===================================================================
--- linux-2.6.18-rc2-git5.orig/include/linux/cpufreq.h
+++ linux-2.6.18-rc2-git5/include/linux/cpufreq.h
@@ -172,9 +172,6 @@ extern int __cpufreq_driver_target(struc
 				   unsigned int relation);
 
 
-/* pass an event to the cpufreq governor */
-int cpufreq_governor(unsigned int cpu, unsigned int event);
-
 int cpufreq_register_governor(struct cpufreq_governor *governor);
 void cpufreq_unregister_governor(struct cpufreq_governor *governor);
 
Index: linux-2.6.18-rc2-git5/drivers/cpufreq/cpufreq_userspace.c
===================================================================
--- linux-2.6.18-rc2-git5.orig/drivers/cpufreq/cpufreq_userspace.c
+++ linux-2.6.18-rc2-git5/drivers/cpufreq/cpufreq_userspace.c
@@ -18,6 +18,7 @@
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/cpufreq.h>
+#include <linux/cpu.h>
 #include <linux/types.h>
 #include <linux/fs.h>
 #include <linux/sysfs.h>
@@ -70,6 +71,7 @@ static int cpufreq_set(unsigned int freq
 
 	dprintk("cpufreq_set for cpu %u, freq %u kHz\n", policy->cpu, freq);
 
+	lock_cpu_hotplug();
 	mutex_lock(&userspace_mutex);
 	if (!cpu_is_managed[policy->cpu])
 		goto err;
@@ -92,6 +94,7 @@ static int cpufreq_set(unsigned int freq
 
  err:
 	mutex_unlock(&userspace_mutex);
+	unlock_cpu_hotplug();
 	return ret;
 }
 

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Dave Jones]

On Wed, Jul 26, 2006 at 03:40:07PM +0200, Arjan van de Ven wrote:

 > Subject: Reorganize the cpufreq cpu hotplug locking to not be totally bizare
 > From: Arjan van de Ven <arjan@linux.intel.com>
 > 
 > The patch below moves the cpu hotplugging higher up in the cpufreq
 > layering; this is needed to avoid recursive taking of the cpu hotplug
 > lock and to otherwise detangle the mess.
 > 
 > The new rules are:
 > 1. you must do lock_cpu_hotplug() around the following functions:
 >    __cpufreq_driver_target
 >    __cpufreq_governor (for CPUFREQ_GOV_LIMITS operation only)
 >    __cpufreq_set_policy
 > 2. governer methods (.governer) must NOT take the lock_cpu_hotplug()
 >    lock in any way; they are called with the lock taken already
 > 3. if your governer spawns a thread that does things, like calling
 >    __cpufreq_driver_target, your thread must honor rule #1.
 > 4. the policy lock and other cpufreq internal locks nest within 
 >    the lock_cpu_hotplug() lock. 
 > 
 > I'm not entirely happy about how the __cpufreq_governor rule ended up
 > (conditional locking rule depending on the argument) but basically all
 > callers pass this as a constant so it's not too horrible.
 > 
 > The patch also removes the cpufreq_governor() function since during the
 > locking audit it turned out to be entirely unused (so no need to fix it)
 > 
 > The patch works on my testbox, but it could use more testing 
 > (otoh... it can't be much worse than the current code)
 > 
 > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

Looks sensible to me.   Assuming it passes testing..

Signed-off-by: Dave Jones <davej@redhat.com>

Linus ?

		Dave

-- 
http://www.codemonkey.org.uk

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Linus Torvalds]

On Wed, 26 Jul 2006, Dave Jones wrote:
> 
> Looks sensible to me.   Assuming it passes testing..

It looked sensible to me too, although it still shows some "Lukewarm IQ" 
notices for the ondemand driver:

	Lukewarm IQ detected in hotplug locking
	BUG: warning at kernel/cpu.c:38/lock_cpu_hotplug()
	 [<c0103d07>] show_trace+0xd/0x10
	 [<c01042ec>] dump_stack+0x19/0x1b
	 [<c013778d>] lock_cpu_hotplug+0x43/0x69
	 [<c012f9df>] __create_workqueue+0x52/0x11f
	 [<df0ec34b>] cpufreq_governor_dbs+0x9f/0x2bd [cpufreq_ondemand]
	 [<c0305542>] __cpufreq_governor+0x57/0xd8
	 [<c0305700>] __cpufreq_set_policy+0x13d/0x1a9
	 [<c0305906>] store_scaling_governor+0x12d/0x155
	 [<c0304fbd>] store+0x34/0x45
	 [<c01998fc>] sysfs_write_file+0x99/0xbf
	 [<c0164953>] vfs_write+0xab/0x157
	 [<c0164f8c>] sys_write+0x3b/0x60
	 [<c0102d41>] sysenter_past_esp+0x56/0x79

but is sure looks better than it used to. Which is why I already applied 
it ;)

		Linus

Re: remove cpu hotplug bustification in cpufreq.

[Russell King]

On Tue, Jul 25, 2006 at 04:46:24PM -0400, Dave Jones wrote:
> On Tue, Jul 25, 2006 at 08:54:49PM +0200, Ingo Molnar wrote:
>  > 
>  > * Linus Torvalds <torvalds@osdl.org> wrote:
>  > 
>  > > The current -git tree will complain about some of the more obvious 
>  > > problems. If you see a "Lukewarm IQ" message, it's a sign of somebody 
>  > > re-taking a cpu lock that is already held.
>  > testing on my latest-rawhide laptop (kernel-2.6.17-1.2445.fc6 and later 
>  > rpms have this change) seems to have pushed the problem over to another 
>  > lock:
>  > 
>  >   S06cpuspeed/1580 is trying to acquire lock:
>  >    (&policy->lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24
>  > 
>  >   but task is already holding lock:
>  >    (cpu_bitmask_lock){--..}, at: [<c06075f9>] mutex_lock+0x21/0x24
>  > 
>  >   which lock already depends on the new lock.
>  > 
>  > and we also get the:
>  > 
>  >   Lukewarm IQ detected in hotplug locking
>  > 
>  > message :-| Find the full bootlog below. And i dont understand the 
>  > cpufreq code well enough to fix this. In fact, does anyone understand 
>  > it? :-/
> 
> Things used to be fairly simple until hotplug cpu came along :-/
> Each day, I'm getting more of the opinion that my patch just ripping
> out this garbage is the right solution.

Not sure if I'm reading your sentiment correctly, but...

It came from the ARM tree, where it had one specific problem to solve -
how to change the CPU core frequency in a safe way.

"Safe way" means informing drivers that they need to quiesce their DMA,
_carefully_ reprogramming the SDRAM timings so we don't lose system
memory, etc.  Locking was (iirc) semaphore based because we used notifier
lists and the like which could sleep (and drivers did and continue to
sleep in the callbacks).

It then got battered into working for x86, which brought a new realm of
locking issues.

Maybe the right answer back in years gone by was to be hard-nosed about
it and said "hands off, this is ARM only, invent your own".

Therefore, if you are intending throwing out cpufreq, I'd like to replace
it with the ARM-only version instead - we _require_ this infrastructure
for the correct operation of some ARM platforms.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: remove cpu hotplug bustification in cpufreq.

[Dave Jones]

On Wed, Jul 26, 2006 at 06:12:57PM +0100, Russell King wrote:

 > > Things used to be fairly simple until hotplug cpu came along :-/
 > > Each day, I'm getting more of the opinion that my patch just ripping
 > > out this garbage is the right solution.
 > 
 > Not sure if I'm reading your sentiment correctly, but...

You didn't.
The garbage I referred to was 'cpu hotplug locking'.

		Dave

-- 
http://www.codemonkey.org.uk

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Arjan van de Ven]

On Wed, 2006-07-26 at 10:09 -0700, Linus Torvalds wrote:

> It looked sensible to me too, although it still shows some "Lukewarm IQ" 
> notices for the ondemand driver:
> 
> 	Lukewarm IQ detected in hotplug locking
> 	BUG: warning at kernel/cpu.c:38/lock_cpu_hotplug()
> 	 [<c0103d07>] show_trace+0xd/0x10
> 	 [<c01042ec>] dump_stack+0x19/0x1b
> 	 [<c013778d>] lock_cpu_hotplug+0x43/0x69
> 	 [<c012f9df>] __create_workqueue+0x52/0x11f
> 	 [<df0ec34b>] cpufreq_governor_dbs+0x9f/0x2bd [cpufreq_ondemand]

ok so this is messy. ondemand wants to create a workqueue, but this
function is called (after my patch) with the lock_cpu_hotplug() already
held, and all the workqueue functions take the lock_cpu_hotplug
themselves (looking at that code I'm not 100% convinced it's quite right
how it does that, but that's a separate matter). Before my first patch
it wasn't doing that, but that situation was even nastier; there is a
lock that is taken in the caller, and the lock_cpu_hotplug() lock
*really* wants to nest outside that lock ;( 

As a quick hack I made non-lock_cpu_hotplug()'ing versions of the 3 key
workqueue functions (patch below). It works, it's correct, it's just so
ugly that I'm almost too ashamed to post it. I haven't found a better
solution yet though... time to take a step back I suppose.

Index: linux-2.6.18-rc2-git5/include/linux/workqueue.h
===================================================================
--- linux-2.6.18-rc2-git5.orig/include/linux/workqueue.h
+++ linux-2.6.18-rc2-git5/include/linux/workqueue.h
@@ -55,17 +55,20 @@ struct execute_work {
 	} while (0)
 
 extern struct workqueue_struct *__create_workqueue(const char *name,
-						    int singlethread);
-#define create_workqueue(name) __create_workqueue((name), 0)
-#define create_singlethread_workqueue(name) __create_workqueue((name), 1)
+						    int singlethread, int locked);
+#define create_workqueue(name) __create_workqueue((name), 0, 0)
+#define create_workqueue_cpulocked(name) __create_workqueue((name), 0, 1)
+#define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0)
 
 extern void destroy_workqueue(struct workqueue_struct *wq);
+extern void destroy_workqueue_cpulock(struct workqueue_struct *wq);
 
 extern int FASTCALL(queue_work(struct workqueue_struct *wq, struct work_struct *work));
 extern int FASTCALL(queue_delayed_work(struct workqueue_struct *wq, struct work_struct *work, unsigned long delay));
 extern int queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
 	struct work_struct *work, unsigned long delay);
 extern void FASTCALL(flush_workqueue(struct workqueue_struct *wq));
+extern void FASTCALL(__flush_workqueue(struct workqueue_struct *wq));
 
 extern int FASTCALL(schedule_work(struct work_struct *work));
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
Index: linux-2.6.18-rc2-git5/kernel/workqueue.c
===================================================================
--- linux-2.6.18-rc2-git5.orig/kernel/workqueue.c
+++ linux-2.6.18-rc2-git5/kernel/workqueue.c
@@ -307,6 +307,22 @@ void fastcall flush_workqueue(struct wor
 }
 EXPORT_SYMBOL_GPL(flush_workqueue);
 
+void fastcall __flush_workqueue(struct workqueue_struct *wq)
+{
+	might_sleep();
+
+	if (is_single_threaded(wq)) {
+		/* Always use first cpu's area. */
+		flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, singlethread_cpu));
+	} else {
+		int cpu;
+
+		for_each_online_cpu(cpu)
+			flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu));
+	}
+}
+EXPORT_SYMBOL_GPL(__flush_workqueue);
+
 static struct task_struct *create_workqueue_thread(struct workqueue_struct *wq,
 						   int cpu)
 {
@@ -333,7 +349,7 @@ static struct task_struct *create_workqu
 }
 
 struct workqueue_struct *__create_workqueue(const char *name,
-					    int singlethread)
+					    int singlethread, int locked)
 {
 	int cpu, destroy = 0;
 	struct workqueue_struct *wq;
@@ -351,7 +367,8 @@ struct workqueue_struct *__create_workqu
 
 	wq->name = name;
 	/* We don't need the distraction of CPUs appearing and vanishing. */
-	lock_cpu_hotplug();
+	if (!locked)
+		lock_cpu_hotplug();
 	if (singlethread) {
 		INIT_LIST_HEAD(&wq->list);
 		p = create_workqueue_thread(wq, singlethread_cpu);
@@ -372,7 +389,8 @@ struct workqueue_struct *__create_workqu
 				destroy = 1;
 		}
 	}
-	unlock_cpu_hotplug();
+	if (!locked)
+		unlock_cpu_hotplug();
 
 	/*
 	 * Was there any error during startup? If yes then clean up:
@@ -400,14 +418,17 @@ static void cleanup_workqueue_thread(str
 		kthread_stop(p);
 }
 
-void destroy_workqueue(struct workqueue_struct *wq)
+static void __destroy_workqueue(struct workqueue_struct *wq, int locked)
 {
 	int cpu;
 
-	flush_workqueue(wq);
 
 	/* We don't need the distraction of CPUs appearing and vanishing. */
-	lock_cpu_hotplug();
+	if (!locked)
+		lock_cpu_hotplug();
+
+	__flush_workqueue(wq);
+
 	if (is_single_threaded(wq))
 		cleanup_workqueue_thread(wq, singlethread_cpu);
 	else {
@@ -417,11 +438,22 @@ void destroy_workqueue(struct workqueue_
 		list_del(&wq->list);
 		spin_unlock(&workqueue_lock);
 	}
-	unlock_cpu_hotplug();
+	if (!locked)
+		unlock_cpu_hotplug();
 	free_percpu(wq->cpu_wq);
 	kfree(wq);
 }
+
+void destroy_workqueue(struct workqueue_struct *wq)
+{
+	__destroy_workqueue(wq, 0);
+}
 EXPORT_SYMBOL_GPL(destroy_workqueue);
+void destroy_workqueue_cpulock(struct workqueue_struct *wq)
+{
+	__destroy_workqueue(wq, 1);
+}
+EXPORT_SYMBOL_GPL(destroy_workqueue_cpulock);
 
 static struct workqueue_struct *keventd_wq;
 

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Linus Torvalds]

On Wed, 26 Jul 2006, Arjan van de Ven wrote:
> 
> As a quick hack I made non-lock_cpu_hotplug()'ing versions of the 3 key
> workqueue functions (patch below). It works, it's correct, it's just so
> ugly that I'm almost too ashamed to post it. I haven't found a better
> solution yet though... time to take a step back I suppose.

That really is _way_ too ugly for words.

For 2.6.18, we may just have to leave the recursive locking in place, and 
just remove the warning. With the recursive lock, if/when somebody needs 
to take that lock early, the code can just do so, and then the inner 
lock-taker ends up being a no-op.

Of course, that's why people want recursive locks in the first place, and 
it's also why we've (largely successfully) have avoided them - it allows 
for people being way too lazy about locking, and allows for really broken 
schenarios like this.

I wonder if we could just make the workqueue code just run with preemption 
disabled - that should also automatically protect against any CPU hotplug 
events on the local CPU (and I think "local CPU" is all that the wq code 
cares about, no?)

		Linus

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Srivatsa Vaddagiri]

On Wed, Jul 26, 2006 at 09:42:34PM +0200, Arjan van de Ven wrote:
> As a quick hack I made non-lock_cpu_hotplug()'ing versions of the 3 key
> workqueue functions (patch below). It works, it's correct, it's just so
> ugly that I'm almost too ashamed to post it. I haven't found a better
> solution yet though... time to take a step back I suppose.

My worry is that such special cases might be needed in more places as we
discover further or as code evolves. Fundamentally looks like the locked and 
unlocked paths of the kernel cannot be separated so well because of interaction 
between subsystems. /me thinks rwsem seems to be a sane thing to go after.

-- 
Regards,
vatsa

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Srivatsa Vaddagiri]

On Wed, Jul 26, 2006 at 01:22:24PM -0700, Linus Torvalds wrote:
> I wonder if we could just make the workqueue code just run with preemption 
> disabled - that should also automatically protect against any CPU hotplug 
> events on the local CPU (and I think "local CPU" is all that the wq code 
> cares about, no?)

__create_workqueue(), destroy_workqueue() and flush_workqueue() are all 
taking CPU hotplug lock currently. AFAICS they all can block and so
disabling preemption wont work. What am I missing?

-- 
Regards,
vatsa

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Arjan van de Ven]

On Wed, 2006-07-26 at 13:42 -0700, Srivatsa Vaddagiri wrote:
> On Wed, Jul 26, 2006 at 09:42:34PM +0200, Arjan van de Ven wrote:
> > As a quick hack I made non-lock_cpu_hotplug()'ing versions of the 3 key
> > workqueue functions (patch below). It works, it's correct, it's just so
> > ugly that I'm almost too ashamed to post it. I haven't found a better
> > solution yet though... time to take a step back I suppose.
> 
> My worry is that such special cases might be needed in more places as we
> discover further or as code evolves. Fundamentally looks like the locked and 
> unlocked paths of the kernel cannot be separated so well because of interaction 
> between subsystems. /me thinks rwsem seems to be a sane thing to go after.

rwsems unfortunately help you zilch; an rwsem is just a mutex with a
performance tweak, but from the deadlock perspective it's really a
mutex.

I'm really starting to feel that the hotplug lock would have been better
of being a refcount (with a waitqueue for zero) than a lock. While
"refcount+waitqueue" sort of IS a lock, the semantics make more sense
imo...

Greetings,
   Arjan van de Ven

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Ashok Raj]

On Wed, Jul 26, 2006 at 01:22:24PM -0700, Linus Torvalds wrote:
> 
> Of course, that's why people want recursive locks in the first place, and 
> it's also why we've (largely successfully) have avoided them - it allows 
> for people being way too lazy about locking, and allows for really broken 
> schenarios like this.
> 
> I wonder if we could just make the workqueue code just run with preemption 
> disabled - that should also automatically protect against any CPU hotplug 

Its probably ok for this case.

before introducing the ugly recursion we did try the preempt_disable()
for cpufreq, and it worked for most all governers with preempt_disable(), 
but powernowk8 called set_cpus_allowed() in the callback path that 
threw out a scheduling while in atomic BUG().

http://lkml.org/lkml/2005/10/31/239


> events on the local CPU (and I think "local CPU" is all that the wq code 
> cares about, no?)
> 
> 		Linus

-- 
Cheers,
Ashok Raj
- Open Source Technology Center

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Srivatsa Vaddagiri]

On Wed, Jul 26, 2006 at 11:03:06PM +0200, Arjan van de Ven wrote:
> rwsems unfortunately help you zilch; an rwsem is just a mutex with a
> performance tweak, but from the deadlock perspective it's really a
> mutex.

ah ..didnt realize that. 

> I'm really starting to feel that the hotplug lock would have been better
> of being a refcount (with a waitqueue for zero) than a lock. While
> "refcount+waitqueue" sort of IS a lock, the semantics make more sense
> imo...

I think that will work, although you now need to deal with a global or per-cpu 
refcount now. Later is more cache-friendly, but dont think we have ready 
refcounting APIs for that?

-- 
Regards,
vatsa

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Linus Torvalds]

On Wed, 26 Jul 2006, Srivatsa Vaddagiri wrote:
> 
> __create_workqueue(), destroy_workqueue() and flush_workqueue() are all 
> taking CPU hotplug lock currently. AFAICS they all can block and so
> disabling preemption wont work. What am I missing?

Well, at least __create_workqueue() should hopefully be fixable in the 
sense that you can do the blocking ops first, and then do the final part 
of actually instantiating it with preemption disabled.

But I agree with Arjan - I think the fundamental problem is that cpu 
hotplug locking is just is fundamentally badly designed as-is. There's 
really very little point to making it a _lock_ per se, since most people 
really want more of a "I'm using this CPU, don't try to remove it right 
now" thing which is more of a ref-counting-like issue.

I think we're hopefully ok for now in the AB-BA deadlock department (and I 
should probably remove the recursion warning), and will need to revisit 
things again after 2.6.18 to try to see if we can fix it properly.

Do we have lockdep warnings remaining? I'd hope that Arjan's earlier patch 
(the one I already applied) got all of those, and that we at least thus 
would have some basis for the belief that we got the ABBA deadlocks fixed.

			Linus

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Rafael J. Wysocki]

On Wednesday 26 July 2006 23:03, Arjan van de Ven wrote:
> On Wed, 2006-07-26 at 13:42 -0700, Srivatsa Vaddagiri wrote:
> > On Wed, Jul 26, 2006 at 09:42:34PM +0200, Arjan van de Ven wrote:
> > > As a quick hack I made non-lock_cpu_hotplug()'ing versions of the 3 key
> > > workqueue functions (patch below). It works, it's correct, it's just so
> > > ugly that I'm almost too ashamed to post it. I haven't found a better
> > > solution yet though... time to take a step back I suppose.
> > 
> > My worry is that such special cases might be needed in more places as we
> > discover further or as code evolves. Fundamentally looks like the locked and 
> > unlocked paths of the kernel cannot be separated so well because of interaction 
> > between subsystems. /me thinks rwsem seems to be a sane thing to go after.
> 
> rwsems unfortunately help you zilch; an rwsem is just a mutex with a
> performance tweak, but from the deadlock perspective it's really a
> mutex.
> 
> I'm really starting to feel that the hotplug lock would have been better
> of being a refcount (with a waitqueue for zero) than a lock. While
> "refcount+waitqueue" sort of IS a lock, the semantics make more sense
> imo...

Related, it looks like we need to disable the cpu hotplug for suspend, either
to disk or to RAM, so that the userspace won't enable the CPUs we have
taken down before freezing tasks.  I have a patch for that, which is appended
(experimental, untested), and I hope it won't interfere with what you're
now doing.

Or perhaps I can get the same result in a better way?

Please advise.

Greetings,
Rafael


---
 include/linux/cpu.h     |    3 ++
 include/linux/suspend.h |    4 +--
 kernel/cpu.c            |   49 +++++++++++++++++++++++++++++++++---------------
 kernel/power/disk.c     |    6 ++++-
 kernel/power/main.c     |    9 ++------
 kernel/power/smp.c      |   39 +++++++++++++++++++++++++++-----------
 kernel/power/user.c     |   13 +++++++-----
 7 files changed, 83 insertions(+), 40 deletions(-)

Index: linux-2.6.18-rc2/include/linux/suspend.h
===================================================================
--- linux-2.6.18-rc2.orig/include/linux/suspend.h	2006-07-26 08:52:42.000000000 +0200
+++ linux-2.6.18-rc2/include/linux/suspend.h	2006-07-26 09:17:16.000000000 +0200
@@ -58,10 +58,10 @@ static inline int software_suspend(void)
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_SUSPEND_SMP
-extern void disable_nonboot_cpus(void);
+extern int disable_nonboot_cpus(void);
 extern void enable_nonboot_cpus(void);
 #else
-static inline void disable_nonboot_cpus(void) {}
+static inline int disable_nonboot_cpus(void) { return 0; }
 static inline void enable_nonboot_cpus(void) {}
 #endif
 
Index: linux-2.6.18-rc2/kernel/power/smp.c
===================================================================
--- linux-2.6.18-rc2.orig/kernel/power/smp.c	2006-07-26 08:52:42.000000000 +0200
+++ linux-2.6.18-rc2/kernel/power/smp.c	2006-07-26 23:16:34.000000000 +0200
@@ -20,33 +20,50 @@
 /* This is protected by pm_sem semaphore */
 static cpumask_t frozen_cpus;
 
-void disable_nonboot_cpus(void)
+int disable_nonboot_cpus(void)
 {
-	int cpu, error;
+	int cpu, error = 0;
 
-	error = 0;
+	/* We take down all of the non-boot CPUs in one shot to avoid races
+	 * with the userspace trying to use the CPU hotplug at the same time
+	 */
+	lock_cpu_hotplug();
 	cpus_clear(frozen_cpus);
 	printk("Freezing cpus ...\n");
 	for_each_online_cpu(cpu) {
 		if (cpu == 0)
 			continue;
-		error = cpu_down(cpu);
+		error = __cpu_down(cpu);
 		if (!error) {
 			cpu_set(cpu, frozen_cpus);
 			printk("CPU%d is down\n", cpu);
-			continue;
+		} else {
+			printk(KERN_ERR "Error taking cpu %d down: %d\n",
+				cpu, error);
+			break;
 		}
-		printk("Error taking cpu %d down: %d\n", cpu, error);
 	}
-	BUG_ON(raw_smp_processor_id() != 0);
-	if (error)
-		panic("cpus not sleeping");
+	if (!error) {
+		BUG_ON(num_online_cpus() > 1);
+		BUG_ON(raw_smp_processor_id() != 0);
+		/* Make sure cpu_up() and cpu_down() won't work from now on */
+		cpu_hotplug_disabled = 1;
+	} else {
+		printk(KERN_ERR "cpus are not sleeping");
+	}
+	unlock_cpu_hotplug();
+	return error;
 }
 
 void enable_nonboot_cpus(void)
 {
 	int cpu, error;
 
+	/* Enable the CPU hotplug so that the userspace can use it */
+	lock_cpu_hotplug();
+	cpu_hotplug_disabled = 0;
+	unlock_cpu_hotplug();
+
 	printk("Thawing cpus ...\n");
 	for_each_cpu_mask(cpu, frozen_cpus) {
 		error = cpu_up(cpu);
@@ -54,8 +71,8 @@ void enable_nonboot_cpus(void)
 			printk("CPU%d is up\n", cpu);
 			continue;
 		}
-		printk("Error taking cpu %d up: %d\n", cpu, error);
-		panic("Not enough cpus");
+		printk(KERN_WARNING "Error taking cpu %d up: %d\n",
+			cpu, error);
 	}
 	cpus_clear(frozen_cpus);
 }
Index: linux-2.6.18-rc2/kernel/power/disk.c
===================================================================
--- linux-2.6.18-rc2.orig/kernel/power/disk.c	2006-07-26 08:52:49.000000000 +0200
+++ linux-2.6.18-rc2/kernel/power/disk.c	2006-07-26 08:57:01.000000000 +0200
@@ -72,7 +72,10 @@ static int prepare_processes(void)
 	int error;
 
 	pm_prepare_console();
-	disable_nonboot_cpus();
+
+	error = disable_nonboot_cpus();
+	if (error)
+		goto enable_cpus;
 
 	if (freeze_processes()) {
 		error = -EBUSY;
@@ -84,6 +87,7 @@ static int prepare_processes(void)
 		return 0;
 thaw:
 	thaw_processes();
+enable_cpus:
 	enable_nonboot_cpus();
 	pm_restore_console();
 	return error;
Index: linux-2.6.18-rc2/kernel/power/main.c
===================================================================
--- linux-2.6.18-rc2.orig/kernel/power/main.c	2006-07-26 08:52:42.000000000 +0200
+++ linux-2.6.18-rc2/kernel/power/main.c	2006-07-26 08:57:01.000000000 +0200
@@ -51,7 +51,7 @@ void pm_set_ops(struct pm_ops * ops)
 
 static int suspend_prepare(suspend_state_t state)
 {
-	int error = 0;
+	int error;
 	unsigned int free_pages;
 
 	if (!pm_ops || !pm_ops->enter)
@@ -59,12 +59,9 @@ static int suspend_prepare(suspend_state
 
 	pm_prepare_console();
 
-	disable_nonboot_cpus();
-
-	if (num_online_cpus() != 1) {
-		error = -EPERM;
+	error = disable_nonboot_cpus();
+	if (error)
 		goto Enable_cpu;
-	}
 
 	if (freeze_processes()) {
 		error = -EAGAIN;
Index: linux-2.6.18-rc2/kernel/power/user.c
===================================================================
--- linux-2.6.18-rc2.orig/kernel/power/user.c	2006-07-26 08:52:49.000000000 +0200
+++ linux-2.6.18-rc2/kernel/power/user.c	2006-07-26 08:57:33.000000000 +0200
@@ -139,12 +139,15 @@ static int snapshot_ioctl(struct inode *
 		if (data->frozen)
 			break;
 		down(&pm_sem);
-		disable_nonboot_cpus();
-		if (freeze_processes()) {
-			thaw_processes();
-			enable_nonboot_cpus();
-			error = -EBUSY;
+		error = disable_nonboot_cpus();
+		if (!error) {
+			error = freeze_processes();
+			if (error) {
+				thaw_processes();
+				error = -EBUSY;
+			}
 		}
+		enable_nonboot_cpus();
 		up(&pm_sem);
 		if (!error)
 			data->frozen = 1;
Index: linux-2.6.18-rc2/include/linux/cpu.h
===================================================================
--- linux-2.6.18-rc2.orig/include/linux/cpu.h	2006-07-16 11:38:15.000000000 +0200
+++ linux-2.6.18-rc2/include/linux/cpu.h	2006-07-26 23:00:12.000000000 +0200
@@ -50,6 +50,8 @@ static inline void unregister_cpu_notifi
 #endif
 extern int current_in_cpu_hotplug(void);
 
+extern int cpu_hotplug_disabled;
+
 int cpu_up(unsigned int cpu);
 
 #else
@@ -81,6 +83,7 @@ extern int lock_cpu_hotplug_interruptibl
 }
 #define register_hotcpu_notifier(nb)	register_cpu_notifier(nb)
 #define unregister_hotcpu_notifier(nb)	unregister_cpu_notifier(nb)
+int __cpu_down(unsigned int cpu);
 int cpu_down(unsigned int cpu);
 #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
 #else
Index: linux-2.6.18-rc2/kernel/cpu.c
===================================================================
--- linux-2.6.18-rc2.orig/kernel/cpu.c	2006-07-16 11:38:16.000000000 +0200
+++ linux-2.6.18-rc2/kernel/cpu.c	2006-07-26 22:59:31.000000000 +0200
@@ -20,6 +20,12 @@ static DEFINE_MUTEX(cpucontrol);
 
 static __cpuinitdata BLOCKING_NOTIFIER_HEAD(cpu_chain);
 
+/* If set, cpu_up and cpu_down will not work.
+ * Should always be manipulated with the cpucontrol mutex locked.
+ * Currently only swsusp uses it.
+ */
+int cpu_hotplug_disabled;
+
 #ifdef CONFIG_HOTPLUG_CPU
 static struct task_struct *lock_cpu_hotplug_owner;
 static int lock_cpu_hotplug_depth;
@@ -116,32 +122,25 @@ static int take_cpu_down(void *unused)
 	return 0;
 }
 
-int cpu_down(unsigned int cpu)
+/* Requires cpucontrol mutex to be locked */
+int __cpu_down(unsigned int cpu)
 {
 	int err;
 	struct task_struct *p;
 	cpumask_t old_allowed, tmp;
 
-	if ((err = lock_cpu_hotplug_interruptible()) != 0)
-		return err;
-
-	if (num_online_cpus() == 1) {
-		err = -EBUSY;
-		goto out;
-	}
+	if (num_online_cpus() == 1)
+		return -EBUSY;
 
-	if (!cpu_online(cpu)) {
-		err = -EINVAL;
-		goto out;
-	}
+	if (!cpu_online(cpu))
+		return -EINVAL;
 
 	err = blocking_notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE,
 						(void *)(long)cpu);
 	if (err == NOTIFY_BAD) {
 		printk("%s: attempt to take down CPU %u failed\n",
 				__FUNCTION__, cpu);
-		err = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	/* Ensure that we are not runnable on dying cpu */
@@ -186,7 +185,22 @@ out_thread:
 	err = kthread_stop(p);
 out_allowed:
 	set_cpus_allowed(current, old_allowed);
-out:
+	return err;
+}
+
+int cpu_down(unsigned int cpu)
+{
+	int err;
+
+	err = lock_cpu_hotplug_interruptible();
+	if (err)
+		return err;
+
+	if (cpu_hotplug_disabled)
+		err = -EPERM;
+	else
+		err = __cpu_down(cpu);
+
 	unlock_cpu_hotplug();
 	return err;
 }
@@ -200,6 +214,11 @@ int __devinit cpu_up(unsigned int cpu)
 	if ((ret = lock_cpu_hotplug_interruptible()) != 0)
 		return ret;
 
+	if (cpu_hotplug_disabled) {
+		ret = -EPERM;
+		goto out;
+	}
+
 	if (cpu_online(cpu) || !cpu_present(cpu)) {
 		ret = -EINVAL;
 		goto out;

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Andrew Morton]

On Wed, 26 Jul 2006 23:03:06 +0200
Arjan van de Ven <arjan@linux.intel.com> wrote:

> I'm really starting to feel that the hotplug lock would have been better
> of being a refcount (with a waitqueue for zero) than a lock. While
> "refcount+waitqueue" sort of IS a lock, the semantics make more sense
> imo...

The mistake in the above paragraph is its use of the term "the hotplug
lock".

Think.  We don't want to lock CPUs.  We don't want to block plug/unplug
events.

What we _do_ want is for subsystems to be able to guarantee the stability
of their per-cpu data and the coherency of that data with cpu_online_map
and cpu_present_map.

We should delete lock_cpu_hotplug() and start again.  Perhaps we can do
that post-2.6.18 if we can cobble the current stuff into some semi-working
state.  But I doubt if it's very important really - we have heaps of code
in there which is already racy wrt hotplug and adding a little more isn't
likely to hurt.

I count 187 instances of for_each_online_cpu(), and most of them are racy. 
There's just no way we can fix all these with lock_cpu_hotplug().  It
simply doesn't have a future.

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Arjan van de Ven]

> Do we have lockdep warnings remaining? I'd hope that Arjan's earlier patch 
> (the one I already applied) got all of those, and that we at least thus 
> would have some basis for the belief that we got the ABBA deadlocks fixed.

I've not seen lockdep warnings with my patch; I tend to run with lockdep
on always nowadays ;)

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Sanjoy Mahajan]

From: Andrew Morton <akpm@osdl.org>
> We should delete lock_cpu_hotplug() and start again.

Here is another example of possible lock_cpu_hotplug() problems.  Is
it worth tracking down, or should I just ignore the messages until a
proper solution is figured out?  The only problem is that it means S3
suspend doesn't work.

The hardware is a Thinkpad T60, T2400 dual-core, compiled with SMP and
PREEMPT, hotpluggable CPUs, and it has a SATA drive.  Kernel is
2.6.18-rc1.  Suspend (UP) worked with 2.6.15-25-386 from Ubuntu using
the same sleep.sh script.  The messages below, including the large
lockdep backtrace, occur after running sleep.sh (run by Fn-F4):


[  546.652000] Stopping tasks: ====================================================================================
[  566.848000]  stopping tasks timed out after 20 seconds (8 tasks remaining):
[  566.848000]   rt-test-0
[  566.848000]   rt-test-1
[  566.848000]   rt-test-2
[  566.848000]   rt-test-3
[  566.848000]   rt-test-4
[  566.848000]   rt-test-5
[  566.848000]   rt-test-6
[  566.848000]   rt-test-7

The lockdep code also reported problems:

[  538.292000] ACPI: PCI interrupt for device 0000:02:00.0 disabled
[  546.144000] Freezing cpus ...
[  546.172000] 
[  546.172000] =======================================================
[  546.172000] [ INFO: possible circular locking dependency detected ]
[  546.172000] -------------------------------------------------------
[  546.172000] sleep.sh/15184 is trying to acquire lock:
[  546.172000]  (&policy->lock){--..}, at: [<c0310645>] mutex_lock+0x25/0x30
[  546.172000] 
[  546.172000] but task is already holding lock:
[  546.172000]  ((cpu_chain).rwsem){----}, at: [<c0133267>] blocking_notifier_call_chain+0x17/0x40
[  546.172000] 
[  546.172000] which lock already depends on the new lock.
[  546.172000] 
[  546.172000] 
[  546.172000] the existing dependency chain (in reverse order) is:
[  546.172000] 
[  546.172000] -> #2 ((cpu_chain).rwsem){----}:
[  546.172000]        [<c0142479>] lock_acquire+0x69/0x90
[  546.172000]        [<c013e08f>] down_read+0x4f/0x60
[  546.172000]        [<c0133267>] blocking_notifier_call_chain+0x17/0x40
[  546.172000]        [<c0147496>] cpu_up+0x76/0x110
[  546.172000]        [<c01005e5>] init+0x295/0x370
[  546.172000]        [<c0101005>] kernel_thread_helper+0x5/0x10
[  546.176000] 
[  546.176000] -> #1 (cpucontrol){--..}:
[  546.176000]        [<c0142479>] lock_acquire+0x69/0x90
[  546.176000]        [<c03103de>] __mutex_lock_slowpath+0x7e/0x2c0
[  546.176000]        [<c0310645>] mutex_lock+0x25/0x30
[  546.176000]        [<c01473c9>] __lock_cpu_hotplug+0x29/0x70
[  546.176000]        [<c014753a>] lock_cpu_hotplug+0xa/0x10
[  546.176000]        [<c02ae6cf>] __cpufreq_driver_target+0xf/0x60
[  546.176000]        [<c02b0218>] cpufreq_governor_performance+0x38/0x40
[  546.176000]        [<c02aee1c>] __cpufreq_governor+0x9c/0x1c0
[  546.176000]        [<c02af1d3>] __cpufreq_set_policy+0x103/0x150
[  546.180000]        [<c02af52e>] cpufreq_set_policy+0x4e/0x90
[  546.180000]        [<c02af871>] cpufreq_add_dev+0x301/0x5a0
[  546.180000]        [<c026685b>] sysdev_driver_register+0x7b/0xc0
[  546.180000]        [<c02af018>] cpufreq_register_driver+0x78/0x130
[  546.180000]        [<f899f04b>] 0xf899f04b
[  546.180000]        [<c014ae93>] sys_init_module+0xa3/0x210
[  546.180000]        [<c010339d>] sysenter_past_esp+0x56/0x8d
[  546.180000] 
[  546.180000] -> #0 (&policy->lock){--..}:
[  546.180000]        [<c0142479>] lock_acquire+0x69/0x90
[  546.180000]        [<c03103de>] __mutex_lock_slowpath+0x7e/0x2c0
[  546.184000]        [<c0310645>] mutex_lock+0x25/0x30
[  546.184000]        [<c02aecf2>] cpufreq_driver_target+0x32/0x70
[  546.184000]        [<c02afd54>] cpufreq_cpu_callback+0x64/0xb0
[  546.184000]        [<c01330e0>] notifier_call_chain+0x30/0x50
[  546.184000]        [<c0133275>] blocking_notifier_call_chain+0x25/0x40
[  546.184000]        [<c01475ca>] cpu_down+0x8a/0x2a0
[  546.184000]        [<c014cb51>] disable_nonboot_cpus+0x51/0xd0
[  546.184000]        [<c014bef7>] enter_state+0x67/0x1b0
[  546.184000]        [<c014c0df>] state_store+0x9f/0xb0
[  546.184000]        [<c01afb4e>] subsys_attr_store+0x2e/0x30
[  546.188000]        [<c01b0345>] sysfs_write_file+0xb5/0x100
[  546.188000]        [<c0171c67>] vfs_write+0xa7/0x190
[  546.188000]        [<c0172697>] sys_write+0x47/0x70
[  546.188000]        [<c010339d>] sysenter_past_esp+0x56/0x8d
[  546.188000] 
[  546.188000] other info that might help us debug this:
[  546.188000] 
[  546.188000] 2 locks held by sleep.sh/15184:
[  546.188000]  #0:  (cpucontrol){--..}, at: [<c0310355>] mutex_lock_interruptible+0x25/0x30
[  546.188000]  #1:  ((cpu_chain).rwsem){----}, at: [<c0133267>] blocking_notifier_call_chain+0x17/0x40
[  546.188000] 
[  546.188000] stack backtrace:
[  546.188000]  [<c0105c5b>] show_trace+0x1b/0x20
[  546.188000]  [<c0105c84>] dump_stack+0x24/0x30
[  546.188000]  [<c013fe41>] print_circular_bug_tail+0x61/0x70
[  546.188000]  [<c0141b77>] __lock_acquire+0x867/0xde0
[  546.188000]  [<c0142479>] lock_acquire+0x69/0x90
[  546.188000]  [<c03103de>] __mutex_lock_slowpath+0x7e/0x2c0
[  546.188000]  [<c0310645>] mutex_lock+0x25/0x30
[  546.188000]  [<c02aecf2>] cpufreq_driver_target+0x32/0x70
[  546.188000]  [<c02afd54>] cpufreq_cpu_callback+0x64/0xb0
[  546.188000]  [<c01330e0>] notifier_call_chain+0x30/0x50
[  546.192000]  [<c0133275>] blocking_notifier_call_chain+0x25/0x40
[  546.192000]  [<c01475ca>] cpu_down+0x8a/0x2a0
[  546.192000]  [<c014cb51>] disable_nonboot_cpus+0x51/0xd0
[  546.192000]  [<c014bef7>] enter_state+0x67/0x1b0
[  546.192000]  [<c014c0df>] state_store+0x9f/0xb0
[  546.192000]  [<c01afb4e>] subsys_attr_store+0x2e/0x30
[  546.192000]  [<c01b0345>] sysfs_write_file+0xb5/0x100
[  546.192000]  [<c0171c67>] vfs_write+0xa7/0x190
[  546.192000]  [<c0172697>] sys_write+0x47/0x70
[  546.192000]  [<c010339d>] sysenter_past_esp+0x56/0x8d
[  546.204000] Breaking affinity for irq 0
[  546.308000] CPU 1 is now offline
[  546.308000] lockdep: not fixing up alternatives.
[  546.652000] CPU1 is down
[  546.652000] Stopping tasks: ====================================================================================
[  566.848000]  stopping tasks timed out after 20 seconds (8 tasks remaining):
[  566.848000]   rt-test-0
[  566.848000]   rt-test-1
[  566.848000]   rt-test-2
[  566.848000]   rt-test-3
[  566.848000]   rt-test-4
[  566.848000]   rt-test-5
[  566.848000]   rt-test-6
[  566.848000]   rt-test-7
[  566.848000] Restarting tasks...<6> Strange, rt-test-0 not stopped
[  566.848000]  Strange, rt-test-1 not stopped
[  566.848000]  Strange, rt-test-2 not stopped
[  566.848000]  Strange, rt-test-3 not stopped
[  566.848000]  Strange, rt-test-4 not stopped
[  566.848000]  Strange, rt-test-5 not stopped
[  566.848000]  Strange, rt-test-6 not stopped
[  566.848000]  Strange, rt-test-7 not stopped
[  568.112000]  done
[  568.112000] Thawing cpus ...
[  568.464000] lockdep: not fixing up alternatives.
[  568.464000] Booting processor 1/1 eip 3000
[  568.472000] Initializing CPU#1
etc.

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Arjan van de Ven]

On Wed, 2006-07-26 at 23:35 +0100, Sanjoy Mahajan wrote:
> From: Andrew Morton <akpm@osdl.org>
> > We should delete lock_cpu_hotplug() and start again.
> 
> Here is another example of possible lock_cpu_hotplug() problems.  Is
> it worth tracking down, or should I just ignore the messages until a
> proper solution is figured out?  The only problem is that it means S3
> suspend doesn't work.
> 
> The hardware is a Thinkpad T60, T2400 dual-core, compiled with SMP and
> PREEMPT, hotpluggable CPUs, and it has a SATA drive.  Kernel is
> 2.6.18-rc1.  Suspend (UP) worked with 2.6.15-25-386 from Ubuntu using
> the same sleep.sh script.  The messages below, including the large
> lockdep backtrace, occur after running sleep.sh (run by Fn-F4):
> 
> 
> [  546.652000] Stopping tasks: ====================================================================================
> [  566.848000]  stopping tasks timed out after 20 seconds (8 tasks remaining):
> [  566.848000]   rt-test-0
> [  566.848000]   rt-test-1
> [  566.848000]   rt-test-2
> [  566.848000]   rt-test-3
> [  566.848000]   rt-test-4
> [  566.848000]   rt-test-5
> [  566.848000]   rt-test-6
> [  566.848000]   rt-test-7
> 
this got fixed in -rc2

> The lockdep code also reported problems:
> 
> [  538.292000] ACPI: PCI interrupt for device 0000:02:00.0 disabled
> [  546.144000] Freezing cpus ...
> [  546.172000] 
> [  546.172000] =======================================================
> [  546.172000] [ INFO: possible circular locking dependency detected ]
> [  546.172000] -------------------------------------------------------
> [  546.172000] sleep.sh/15184 is trying to acquire lock:
> [  546.172000]  (&policy->lock){--..}, at: [<c0310645>] mutex_lock+0x25/0x30
> [  546.172000] 
> [  546.172000] but task is already holding lock:
> [  546.172000]  ((cpu_chain).rwsem){----}, at: [<c0133267>] blocking_notifier_call_chain+0x17/0x40
> [  546.172000] 

and afaik my patches in current git should fix all this up
if not please send a trace ;)

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Ingo Molnar]

* Linus Torvalds <torvalds@osdl.org> wrote:

> But I agree with Arjan - I think the fundamental problem is that cpu 
> hotplug locking is just is fundamentally badly designed as-is. There's 
> really very little point to making it a _lock_ per se, since most 
> people really want more of a "I'm using this CPU, don't try to remove 
> it right now" thing which is more of a ref-counting-like issue.

we'd also need a facility to wait on that refcount - i.e. a waitqueue. 
Which means we'd have a "refcount + waitqueue", which is equivalent to a 
"recursive, sleeping read-lock", where the write-side could be used as a 
simple facility to "wait for all readers to go away and block new 
readers from entering the critical sections". [which type of lock Linux 
does not have right now. rwsems come the closest but they dont recurse.]

Also, the hotplug lock is global right now which is pretty unscalable, 
so the rw-mutex should also be per-CPU, and the hotplug locking API 
should be changed to something like:

	cpu = cpu_hotplug_lock();
	...
	cpu_hotplug_unlock(cpu);

To enable a task to schedule away (and potentially migrate to another 
CPU) with the per-CPU lock held but still be able to unlock the right 
per-cpu lock. [this above approach is quite similar to how we do 
sleeping per-cpu locks in -rt.]

	Ingo

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Ashok Raj]

On Thu, Jul 27, 2006 at 03:40:49AM +0200, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@osdl.org> wrote:
> 
> > But I agree with Arjan - I think the fundamental problem is that cpu 
> > hotplug locking is just is fundamentally badly designed as-is. There's 
> > really very little point to making it a _lock_ per se, since most 
> > people really want more of a "I'm using this CPU, don't try to remove 
> > it right now" thing which is more of a ref-counting-like issue.
> 
> we'd also need a facility to wait on that refcount - i.e. a waitqueue. 
> Which means we'd have a "refcount + waitqueue", which is equivalent to a 
> "recursive, sleeping read-lock", where the write-side could be used as a 
> simple facility to "wait for all readers to go away and block new 
> readers from entering the critical sections". [which type of lock Linux 
> does not have right now. rwsems come the closest but they dont recurse.]

sounds like some varient of conditional variables, caveat might be that
new readers permitted if in the same call thread/cpu? 
(i.e recurive inside the same context?)

for e.g with the cpufreq kind of usage today, 

cpu_down()
   waits for ref to drop to zero;
   // now signalled by last reader when refcnt drops to 0
   do pre-remove-notify---> cpufreq 
	// this attempt to acquire read lock again shoudnt be blocked right
        // even though we have officially started waiting for cnt to drop 0?

problem was with the kondemand() when a remove_wq() caused a flush_wq()
that started to yeild and run the other wq thread. Now the depth control
that checked if the locking_task == current wasnt true that caused the 
dead lock again.

> 
> Also, the hotplug lock is global right now which is pretty unscalable, 
> so the rw-mutex should also be per-CPU, and the hotplug locking API 
> should be changed to something like:
> 
> 	cpu = cpu_hotplug_lock();

so this is sort of like the get_cpu()/put_cpu() interface that does 
preempt_disable() + get current cpu.


-- 
Cheers,
Ashok Raj
- Open Source Technology Center

RE: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Langsdorf, Mark]

 
> before introducing the ugly recursion we did try the 
> preempt_disable() for cpufreq, and it worked for most all 
> governers with preempt_disable(), but powernowk8 called 
> set_cpus_allowed() in the callback path that threw out a 
> scheduling while in atomic BUG().
> 
> http://lkml.org/lkml/2005/10/31/239

Is there some other preferred call we could be making 
instead of set_cpus_allowed() ?  We need to be able to
program the MSRs on that specific core, and as far as
I know, the only way to do that is to guarantee that
we are scheduled on that particular core.  

If there's a better way to hop to a specific core, I'll
gladly rewrite the code in question.

-Mark Langsdorf
AMD, Inc.

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Andi Kleen]

"Langsdorf, Mark" <mark.langsdorf@amd.com> writes:
> 
> If there's a better way to hop to a specific core, I'll
> gladly rewrite the code in question.

You could use smp_call_function_single() 

(i386 version might be only in -mm* so far)

-Andi

RE: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Langsdorf, Mark]

> > If there's a better way to hop to a specific core, I'll 
> gladly rewrite 
> > the code in question.
> 
> You could use smp_call_function_single() 
> 
> (i386 version might be only in -mm* so far)

I'll wait and submit for 2.6.20, I guess.

-Mark Langsdorf
AMD, Inc.

Re: [patch] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

[Ingo Molnar]

* Ashok Raj <ashok.raj@intel.com> wrote:

> On Thu, Jul 27, 2006 at 03:40:49AM +0200, Ingo Molnar wrote:
> > 
> > * Linus Torvalds <torvalds@osdl.org> wrote:
> > 
> > > But I agree with Arjan - I think the fundamental problem is that cpu 
> > > hotplug locking is just is fundamentally badly designed as-is. There's 
> > > really very little point to making it a _lock_ per se, since most 
> > > people really want more of a "I'm using this CPU, don't try to remove 
> > > it right now" thing which is more of a ref-counting-like issue.
> > 
> > we'd also need a facility to wait on that refcount - i.e. a waitqueue. 
> > Which means we'd have a "refcount + waitqueue", which is equivalent to a 
> > "recursive, sleeping read-lock", where the write-side could be used as a 
> > simple facility to "wait for all readers to go away and block new 
> > readers from entering the critical sections". [which type of lock Linux 
> > does not have right now. rwsems come the closest but they dont recurse.]
> 
> sounds like some varient of conditional variables, caveat might be 
> that new readers permitted if in the same call thread/cpu?

well, i'd just call it a recursive rwsem. (sure, you can express it via 
condition variables, but just about any locking method can be expressed 
via them.)

> > Also, the hotplug lock is global right now which is pretty unscalable, 
> > so the rw-mutex should also be per-CPU, and the hotplug locking API 
> > should be changed to something like:
> > 
> > 	cpu = cpu_hotplug_lock();
> 
> so this is sort of like the get_cpu()/put_cpu() interface that does 
> preempt_disable() + get current cpu.

the API is similar - behavior is different in that the 'per-cpu lock' 
i'm talking about _does_ allow preemption and migration to another CPU.

	Ingo

Re: remove cpu hotplug bustification in cpufreq.

[Arjan van de Ven]

On Sun, 2006-07-23 at 11:12 -0700, Linus Torvalds wrote:
> 
> On Sun, 23 Jul 2006, Linus Torvalds wrote:
> > 
> > Does this work? Hey, it works for me once. It's pretty simple, and had 
> > better not have any recursion issues.
> 
> GAAH!!
> 
> What kind of _crap_ is this cpufreq thing?


this is why lockdep got highly upset with it, and why Davej proposed to
remove the locking entirely for now until it can be put back in a
correct way....

Re: remove cpu hotplug bustification in cpufreq.

[Patrick McFarland]

On Sunday 23 July 2006 14:12, Linus Torvalds wrote:
> On Sun, 23 Jul 2006, Linus Torvalds wrote:
> [ Linus bangs his head against the wall until tears of blood course down
>   his face ]

I know how you feel.

> cpufreq (or at least ondemand) must DIE! And the people who wrote that
> crap should have red-hot pokers jammed into some very uncomfortable
> places.

You know what else must die? powernowd... which does exactly what the 
conservative governor does, but takes about a meg of memory to do it, and it 
doesn't even provide stuff like changing behavior based on ac/battery state 
or lm_sensors feedback.

-- 
Patrick McFarland || www.AdTerrasPerAspera.com
"Computer games don't affect kids; I mean if Pac-Man affected us as kids,
we'd all be running around in darkened rooms, munching magic pills and
listening to repetitive electronic music." -- Kristian Wilson, Nintendo,
Inc, 1989

Re: remove cpu hotplug bustification in cpufreq.

[Linus Torvalds]

On Sun, 23 Jul 2006, Linus Torvalds wrote:
> 
> Does this work? Hey, it works for me once. It's pretty simple, and had 
> better not have any recursion issues.

GAAH!!

What kind of _crap_ is this cpufreq thing?

Lookie here:

	S06cpuspeed   D DD94A324  2180 10241  10215                     (NOTLB)
	Call Trace:
	 [<c03c411d>] __mutex_lock_slowpath+0x4d/0x7b
	 [<c03c415a>] .text.lock.mutex+0xf/0x14
	 [<c0137651>] lock_cpu_hotplug+0xd/0xf
	 [<c012f9df>] __create_workqueue+0x52/0x11f
	 [<df0cd336>] cpufreq_governor_dbs+0x9e/0x2c5 [cpufreq_ondemand]
	 [<c0305d2a>] __cpufreq_governor+0x57/0xd8
	 [<c0305ee8>] __cpufreq_set_policy+0x13d/0x1a9
	 [<c03060e4>] store_scaling_governor+0x12d/0x155
	 [<c03057a5>] store+0x34/0x45
	 [<c0199a6c>] sysfs_write_file+0x99/0xbf
	 [<c0164ac3>] vfs_write+0xab/0x157
	 [<c01650fc>] sys_write+0x3b/0x60
	 [<c0102d41>] sysenter_past_esp+0x56/0x79

where it takes the cpu_hotplug lock in "store_scaling_governor()", and 
then calls __cpufreq_set_policy(), and then that ondemand thing WILL TAKE 
IT AGAIN!

What a piece of crap. Why, why, why?

[ Linus bangs his head against the wall until tears of blood course down 
  his face ]

I will here-by re-introduce the recursion thing for lock_cpu_hotplug, but 
I will make it say some very rude things about idiots who create code like 
this. 

cpufreq (or at least ondemand) must DIE! And the people who wrote that 
crap should have red-hot pokers jammed into some very uncomfortable 
places.

		Linus

Re: remove cpu hotplug bustification in cpufreq.

[Linus Torvalds]

On Sun, 23 Jul 2006, Arjan van de Ven wrote:
> 
> with rwsems being 100% fair... how is that going to make a difference?
> Other than just making the deadlock harder to trigger because the writer
> needs to come in just at the right time...

Fair enough, I forgot about that particular braindamage. It would be _soo_ 
nice to have a proper rwsem like we used to.

Ok, here's a final try.

Its actually simpler than either of the previous ones, and it just does 
two different locks: a "cpu_add_remove_lock" which is entirely internal to 
kernel/cpu.c, and is held over the entirety of a cpu_add()/cpu_down() 
sequence, so that we will only ever add one CPU at a time.

The other lock is the "cpu_bitmask_lock", and the only thing that protects 
is the actual changing of the present bitmask. It's always nested inside 
of the "cpu_add_remove_lock" (if that is taken at all, of course).

The latter one is the one that "lock_cpu_hotplug()" actually takes, so 
anybody who does "lock_cpu_hotplug()" will only protect against the bitmap 
itself changing, not against the bigger issue of "cpu hotplug events are 
happening".

Does this work? Hey, it works for me once. It's pretty simple, and had 
better not have any recursion issues. It guarantees that actual cpu 
hotplug events are single-threaded wrt each other, and it does not allow 
any recursive taking of "lock_cpu_hotplug()", but since cpu_add() and 
cpu_down() no longer hold that particular lock when they to the call-outs 
for the cpu events, it should hopefully not be needed any more either.

Not very well tested, but it did suspend and resume twice for me.

Does anybody see any problems with this?

		Linus
---
 include/linux/cpu.h |    6 ------
 kernel/cpu.c        |   54 ++++++++++++---------------------------------------
 2 files changed, 13 insertions(+), 47 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 44a11f1..8fb344a 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -48,7 +48,6 @@ static inline void unregister_cpu_notifi
 {
 }
 #endif
-extern int current_in_cpu_hotplug(void);
 
 int cpu_up(unsigned int cpu);
 
@@ -61,10 +60,6 @@ static inline int register_cpu_notifier(
 static inline void unregister_cpu_notifier(struct notifier_block *nb)
 {
 }
-static inline int current_in_cpu_hotplug(void)
-{
-	return 0;
-}
 
 #endif /* CONFIG_SMP */
 extern struct sysdev_class cpu_sysdev_class;
@@ -73,7 +68,6 @@ #ifdef CONFIG_HOTPLUG_CPU
 /* Stop CPUs going up and down. */
 extern void lock_cpu_hotplug(void);
 extern void unlock_cpu_hotplug(void);
-extern int lock_cpu_hotplug_interruptible(void);
 #define hotcpu_notifier(fn, pri) {				\
 	static struct notifier_block fn##_nb =			\
 		{ .notifier_call = fn, .priority = pri };	\
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 70fbf2e..4157055 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -16,7 +16,8 @@ #include <linux/stop_machine.h>
 #include <linux/mutex.h>
 
 /* This protects CPUs going up and down... */
-static DEFINE_MUTEX(cpucontrol);
+static DEFINE_MUTEX(cpu_add_remove_lock);
+static DEFINE_MUTEX(cpu_bitmask_lock);
 
 static __cpuinitdata BLOCKING_NOTIFIER_HEAD(cpu_chain);
 
@@ -24,48 +25,18 @@ #ifdef CONFIG_HOTPLUG_CPU
 static struct task_struct *lock_cpu_hotplug_owner;
 static int lock_cpu_hotplug_depth;
 
-static int __lock_cpu_hotplug(int interruptible)
-{
-	int ret = 0;
-
-	if (lock_cpu_hotplug_owner != current) {
-		if (interruptible)
-			ret = mutex_lock_interruptible(&cpucontrol);
-		else
-			mutex_lock(&cpucontrol);
-	}
-
-	/*
-	 * Set only if we succeed in locking
-	 */
-	if (!ret) {
-		lock_cpu_hotplug_depth++;
-		lock_cpu_hotplug_owner = current;
-	}
-
-	return ret;
-}
-
 void lock_cpu_hotplug(void)
 {
-	__lock_cpu_hotplug(0);
+	mutex_lock(&cpu_bitmask_lock);
 }
 EXPORT_SYMBOL_GPL(lock_cpu_hotplug);
 
 void unlock_cpu_hotplug(void)
 {
-	if (--lock_cpu_hotplug_depth == 0) {
-		lock_cpu_hotplug_owner = NULL;
-		mutex_unlock(&cpucontrol);
-	}
+	mutex_unlock(&cpu_bitmask_lock);
 }
 EXPORT_SYMBOL_GPL(unlock_cpu_hotplug);
 
-int lock_cpu_hotplug_interruptible(void)
-{
-	return __lock_cpu_hotplug(1);
-}
-EXPORT_SYMBOL_GPL(lock_cpu_hotplug_interruptible);
 #endif	/* CONFIG_HOTPLUG_CPU */
 
 /* Need to know about CPUs going up/down? */
@@ -122,9 +93,7 @@ int cpu_down(unsigned int cpu)
 	struct task_struct *p;
 	cpumask_t old_allowed, tmp;
 
-	if ((err = lock_cpu_hotplug_interruptible()) != 0)
-		return err;
-
+	mutex_lock(&cpu_add_remove_lock);
 	if (num_online_cpus() == 1) {
 		err = -EBUSY;
 		goto out;
@@ -150,7 +119,10 @@ int cpu_down(unsigned int cpu)
 	cpu_clear(cpu, tmp);
 	set_cpus_allowed(current, tmp);
 
+	mutex_lock(&cpu_bitmask_lock);
 	p = __stop_machine_run(take_cpu_down, NULL, cpu);
+	mutex_unlock(&cpu_bitmask_lock);
+
 	if (IS_ERR(p)) {
 		/* CPU didn't die: tell everyone.  Can't complain. */
 		if (blocking_notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED,
@@ -187,7 +159,7 @@ out_thread:
 out_allowed:
 	set_cpus_allowed(current, old_allowed);
 out:
-	unlock_cpu_hotplug();
+	mutex_unlock(&cpu_add_remove_lock);
 	return err;
 }
 #endif /*CONFIG_HOTPLUG_CPU*/
@@ -197,9 +169,7 @@ int __devinit cpu_up(unsigned int cpu)
 	int ret;
 	void *hcpu = (void *)(long)cpu;
 
-	if ((ret = lock_cpu_hotplug_interruptible()) != 0)
-		return ret;
-
+	mutex_lock(&cpu_add_remove_lock);
 	if (cpu_online(cpu) || !cpu_present(cpu)) {
 		ret = -EINVAL;
 		goto out;
@@ -214,7 +184,9 @@ int __devinit cpu_up(unsigned int cpu)
 	}
 
 	/* Arch-specific enabling code. */
+	mutex_lock(&cpu_bitmask_lock);
 	ret = __cpu_up(cpu);
+	mutex_unlock(&cpu_bitmask_lock);
 	if (ret != 0)
 		goto out_notify;
 	BUG_ON(!cpu_online(cpu));
@@ -227,6 +199,6 @@ out_notify:
 		blocking_notifier_call_chain(&cpu_chain,
 				CPU_UP_CANCELED, hcpu);
 out:
-	unlock_cpu_hotplug();
+	mutex_unlock(&cpu_add_remove_lock);
 	return ret;
 }

Re: remove cpu hotplug bustification in cpufreq.

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> > I'll try them out. If they don't work, we should just delete the 
> > lock and go totally back to square 1.
> 
> rwsem conversion has the potential to merely hide the problem.  Ingo, 
> does lockdep detect recursive down_read()?

yeah, it does - there are a couple of testcases for it too, in the 
testsuite.

	Ingo

Re: remove cpu hotplug bustification in cpufreq.

[Arjan van de Ven]

Hi,

> > Well, I just got Ashok's trial patches which turns the thing into a rwsem 
> > as I outlined earlier.
> 
> Mark my words ;)
> 
> > I'll try them out. If they don't work, we should just delete the lock and 
> > go totally back to square 1.
> 
> rwsem conversion has the potential to merely hide the problem.  Ingo, does
> lockdep detect recursive down_read()?

lockdep detects and warns about those.

I think we're about equally skeptical about this; I'm extremely hesitant
about any place in the kernel that uses rwsems for anything other than a
performance tweak. I've ended up with a mental model of rwsems that
basically comes down to "you need to be able to replace it with a mutex
without breaking correctness". Now of course that model is somewhat of
an oversimplification, but not by that much...

Greetings,
   Arjan van de Ven

Re: remove cpu hotplug bustification in cpufreq.

[Andrew Morton]

On Sat, 22 Jul 2006 18:15:32 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> 
> 
> On Sat, 22 Jul 2006, Andrew Morton wrote:
> > 
> > It was just wrong in conception.  We should not and probably cannot fix it.
> > Let's just delete it all, then implement version 2.
> 
> Well, I just got Ashok's trial patches which turns the thing into a rwsem 
> as I outlined earlier.

Mark my words ;)

> I'll try them out. If they don't work, we should just delete the lock and 
> go totally back to square 1.

rwsem conversion has the potential to merely hide the problem.  Ingo, does
lockdep detect recursive down_read()?

Re: remove cpu hotplug bustification in cpufreq.

[Arjan van de Ven]

On Sat, 2006-07-22 at 18:15 -0700, Linus Torvalds wrote:
> 
> On Sat, 22 Jul 2006, Andrew Morton wrote:
> > 
> > It was just wrong in conception.  We should not and probably cannot fix it.
> > Let's just delete it all, then implement version 2.
> 
> Well, I just got Ashok's trial patches which turns the thing into a rwsem 
> as I outlined earlier.

with rwsems being 100% fair... how is that going to make a difference?
Other than just making the deadlock harder to trigger because the writer
needs to come in just at the right time...

Re: remove cpu hotplug bustification in cpufreq.

[Linus Torvalds]

On Sat, 22 Jul 2006, Andrew Morton wrote:
> 
> It was just wrong in conception.  We should not and probably cannot fix it.
> Let's just delete it all, then implement version 2.

Well, I just got Ashok's trial patches which turns the thing into a rwsem 
as I outlined earlier.

I'll try them out. If they don't work, we should just delete the lock and 
go totally back to square 1.

		Linus

Re: remove cpu hotplug bustification in cpufreq.

[Andrew Morton]

On Sat, 22 Jul 2006 17:10:36 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> 
> 
> On Sat, 22 Jul 2006, Dave Jones wrote:
> >
> > This stuff makes my head hurt. Someone who is motivated
> > enough to fix up hotplug-cpu can fix this up later.
> > In the meantime, this patch should cure the lockdep
> > warnings that seem to trigger very easily.
> 
> It doesn't seem to fix all problems. On CPU unplug, I still get deadlocks 
> through some workqueue:
> 
>  [<c03af64a>] __mutex_lock_slowpath+0x4d/0x7b
>  [<c03af687>] .text.lock.mutex+0xf/0x14
>  [<c0137591>] __lock_cpu_hotplug+0x36/0x56
>  [<c01375ca>] lock_cpu_hotplug+0xa/0xc
>  [<c012f7c2>] flush_workqueue+0x2d/0x61
>  [<c012f83b>] flush_scheduled_work+0xd/0xf
>  ...
> 
> and the nasty part is that this can apparently hit _any_ process that 
> wants to flush workqueues (in one particular case, it was through 
> tty_release() -> release_dev() in drivers/char/tty.c).

If one was sufficiently morbidly curious, one would ask for the rest of the
trace, to identify the deadlock.  But it's just not interesting.

Let's just delete the whole thing.

> The whole CPU hotplug locking seems to be broken.

It was just wrong in conception.  We should not and probably cannot fix it.
Let's just delete it all, then implement version 2.

Which is: subsystems are locally responsible for locking their per-cpu
resources.  No global lock.  Subsystems have two options:

a) If the resource can be accessed while running atomically, lock it
   with preempt_disable().  Because preempt_disable() holds off hotunplug.

b) If the kernel wants to sleep while requiring that the per-cpu
   resource remain stable, lock it down with mutex_lock() in the normal
   operating code and also lock it down with mutex_lock() in the
   subsystem's cpu hotplug notifier callback.

The precise handling of b) needs some thought and will depend upon how the
subsystem tells itself about the availability of each CPU's data.  A simple
implementation would be:

mainline_code()
{
	mutex_lock(lock);
	fiddle_with(data[some_cpu_id]);
	mutex_unlock(lock);
}

static int foo_cpu_callback(struct notifier_block *nfb, unsigned long action,
				void *hcpu)
{
	unsigned int cpu = (unsigned long)hcpu;

	switch (action) {
	case CPU_DOWN_PREPARE:
		mutex_lock(lock);
		break;
	case CPU_DOWN_FAILED:
		mutex_unlock(lock);
		break;
	case CPU_DOWN_DEAD:
		mutex_unlock(lock);
		break;
	}
	return NOTIFY_OK;
}

Leaving the lock held in this manner isn't nice IMO, but it should work OK.

Note that there is what appears to be a bug in cpu_down().  This:

	if (cpu_online(cpu))
		goto out_thread;

should do a CPU_DOWN_FAILED callout.  Or, better, it should go
BUG(), because this shouldn't happen.

Re: remove cpu hotplug bustification in cpufreq.

[Linus Torvalds]

On Sat, 22 Jul 2006, Dave Jones wrote:
>
> This stuff makes my head hurt. Someone who is motivated
> enough to fix up hotplug-cpu can fix this up later.
> In the meantime, this patch should cure the lockdep
> warnings that seem to trigger very easily.

It doesn't seem to fix all problems. On CPU unplug, I still get deadlocks 
through some workqueue:

 [<c03af64a>] __mutex_lock_slowpath+0x4d/0x7b
 [<c03af687>] .text.lock.mutex+0xf/0x14
 [<c0137591>] __lock_cpu_hotplug+0x36/0x56
 [<c01375ca>] lock_cpu_hotplug+0xa/0xc
 [<c012f7c2>] flush_workqueue+0x2d/0x61
 [<c012f83b>] flush_scheduled_work+0xd/0xf
 ...

and the nasty part is that this can apparently hit _any_ process that 
wants to flush workqueues (in one particular case, it was through 
tty_release() -> release_dev() in drivers/char/tty.c).

The whole CPU hotplug locking seems to be broken.

		Linus

remove cpu hotplug bustification in cpufreq.

[Dave Jones]

This stuff makes my head hurt. Someone who is motivated
enough to fix up hotplug-cpu can fix this up later.
In the meantime, this patch should cure the lockdep
warnings that seem to trigger very easily.

Signed-off-by: Dave Jones <davej@redhat.com>

--- linux-2.6.17.noarch/drivers/cpufreq/cpufreq_ondemand.c~	2006-07-22 15:35:04.000000000 -0400
+++ linux-2.6.17.noarch/drivers/cpufreq/cpufreq_ondemand.c	2006-07-22 15:35:16.000000000 -0400
@@ -408,7 +408,6 @@ static int cpufreq_governor_dbs(struct c
 		break;
 
 	case CPUFREQ_GOV_LIMITS:
-		lock_cpu_hotplug();
 		mutex_lock(&dbs_mutex);
 		if (policy->max < this_dbs_info->cur_policy->cur)
 			__cpufreq_driver_target(this_dbs_info->cur_policy,
@@ -419,7 +418,6 @@ static int cpufreq_governor_dbs(struct c
 			                        policy->min,
 			                        CPUFREQ_RELATION_L);
 		mutex_unlock(&dbs_mutex);
-		unlock_cpu_hotplug();
 		break;
 	}
 	return 0;
--- linux-2.6.17.noarch/drivers/cpufreq/cpufreq_stats.c~	2006-07-22 15:35:40.000000000 -0400
+++ linux-2.6.17.noarch/drivers/cpufreq/cpufreq_stats.c	2006-07-22 15:35:54.000000000 -0400
@@ -350,12 +350,10 @@ __init cpufreq_stats_init(void)
 	}
 
 	register_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
-	lock_cpu_hotplug();
 	for_each_online_cpu(cpu) {
 		cpufreq_stat_cpu_callback(&cpufreq_stat_cpu_notifier, CPU_ONLINE,
 			(void *)(long)cpu);
 	}
-	unlock_cpu_hotplug();
 	return 0;
 }
 static void
@@ -368,12 +366,10 @@ __exit cpufreq_stats_exit(void)
 	cpufreq_unregister_notifier(&notifier_trans_block,
 			CPUFREQ_TRANSITION_NOTIFIER);
 	unregister_hotcpu_notifier(&cpufreq_stat_cpu_notifier);
-	lock_cpu_hotplug();
 	for_each_online_cpu(cpu) {
 		cpufreq_stat_cpu_callback(&cpufreq_stat_cpu_notifier, CPU_DEAD,
 			(void *)(long)cpu);
 	}
-	unlock_cpu_hotplug();
 }
 
 MODULE_AUTHOR ("Zou Nan hai <nanhai.zou@intel.com>");
--- linux-2.6.17.noarch/drivers/cpufreq/cpufreq.c~	2006-07-22 15:36:05.000000000 -0400
+++ linux-2.6.17.noarch/drivers/cpufreq/cpufreq.c	2006-07-22 15:36:20.000000000 -0400
@@ -423,8 +423,6 @@ static ssize_t store_scaling_governor (s
 	if (cpufreq_parse_governor(str_governor, &new_policy.policy, &new_policy.governor))
 		return -EINVAL;
 
-	lock_cpu_hotplug();
-
 	/* Do not use cpufreq_set_policy here or the user_policy.max
 	   will be wrongly overridden */
 	mutex_lock(&policy->lock);
@@ -434,8 +432,6 @@ static ssize_t store_scaling_governor (s
 	policy->user_policy.governor = policy->governor;
 	mutex_unlock(&policy->lock);
 
-	unlock_cpu_hotplug();
-
 	return ret ? ret : count;
 }
 
@@ -1203,14 +1199,11 @@ int __cpufreq_driver_target(struct cpufr
 {
 	int retval = -EINVAL;
 
-	lock_cpu_hotplug();
 	dprintk("target for CPU %u: %u kHz, relation %u\n", policy->cpu,
 		target_freq, relation);
 	if (cpu_online(policy->cpu) && cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
-	unlock_cpu_hotplug();
-
 	return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
--- linux-2.6.17.noarch/drivers/cpufreq/cpufreq_conservative.c~	2006-07-22 15:36:46.000000000 -0400
+++ linux-2.6.17.noarch/drivers/cpufreq/cpufreq_conservative.c	2006-07-22 15:36:57.000000000 -0400
@@ -423,14 +423,12 @@ static void dbs_check_cpu(int cpu)
 static void do_dbs_timer(void *data)
 { 
 	int i;
-	lock_cpu_hotplug();
 	mutex_lock(&dbs_mutex);
 	for_each_online_cpu(i)
 		dbs_check_cpu(i);
 	schedule_delayed_work(&dbs_work, 
 			usecs_to_jiffies(dbs_tuners_ins.sampling_rate));
 	mutex_unlock(&dbs_mutex);
-	unlock_cpu_hotplug();
 } 
 
 static inline void dbs_timer_init(void)
@@ -525,7 +523,6 @@ static int cpufreq_governor_dbs(struct c
 		break;
 
 	case CPUFREQ_GOV_LIMITS:
-		lock_cpu_hotplug();
 		mutex_lock(&dbs_mutex);
 		if (policy->max < this_dbs_info->cur_policy->cur)
 			__cpufreq_driver_target(
@@ -536,7 +533,6 @@ static int cpufreq_governor_dbs(struct c
 					this_dbs_info->cur_policy,
 				       	policy->min, CPUFREQ_RELATION_L);
 		mutex_unlock(&dbs_mutex);
-		unlock_cpu_hotplug();
 		break;
 	}
 	return 0;

-- 
http://www.codemonkey.org.uk