Messages in this thread | | | Date | Thu, 5 Feb 2015 22:14:36 +0100 | From | Bruno Prémont <> | Subject | Re: Linux 3.19-rc5 |
| |
On Thu, 29 January 2015 Linus Torvalds wrote: > On Thu, Jan 29, 2015 at 5:12 PM, Linus Torvalds wrote: > > > > The WARN() was already changed to a WARN_ONCE(). > > Oh, but I notice that the "__set_current_state(TASK_RUNNING) ends up > always happening. > > So I think the right fix is to: > > - warn once like we do > > - but *not* do that __set_current_state() which was always total crap anyway > > Why do I say "total crap"? Because of two independent issues: > > (a) it actually changes behavior for a debug vs non-debug kernel, > which is a really bad idea to begin with > > (b) it's really wrong. The whole "nested sleep" case was never a > major bug to begin with, just a possible inefficiency where constant > nested sleeps would possibly make the outer sleep not sleep. But that > "could possibly make" case was the unlikely case, and the debug patch > made it happen *all* the time by explicitly setting things running. > > So I think the proper patch is the attached. > > The comment is also crap. The comment says > > "Blocking primitives will set (and therefore destroy) current->state [...]" > > but the reality is that they *may* set it, and only in the unlikely > slow-path where they actually block. > > So doing this in "__may_sleep()" is just bogus and horrible horrible > crap. It turns the "harmless ugliness" into a real *harmful* bug. The > key word of "__may_sleep()" is that "MAY" part. It's a debug thing to > make relatively rare cases show up. > > PeterZ, please don't make "debugging" patches like this. Ever again. > Because this was just stupid, and it took me too long to realize that > despite the warning being shut up, the debug patch was still actively > doing bad bad things. > > Ingo, maybe you'd want to apply this through the scheduler tree, the > way you already did the WARN_ONCE() thing. > > Bruno, does this finally actually fix your pccard thing?
Tested the variant that was applied by running rc7 and it fixes the endless loop. The loop is now replaced by a single WARN() trace - I guess expected:
[ 3.083647] ------------[ cut here ]------------ [ 3.087477] WARNING: CPU: 0 PID: 67 at /usr/src/linux-git/kernel/sched/core.c:7300 __might_sleep+0x79/0x90() [ 3.091357] do not call blocking ops when !TASK_RUNNING; state=1 set at [<c1442040>] pccardd+0xa0/0x3e0 [ 3.095232] Modules linked in: [ 3.099020] CPU: 0 PID: 67 Comm: pccardd Not tainted 3.19.0-rc7-00003-g67288c4 #17 [ 3.102760] Hardware name: Acer TravelMate 660/TravelMate 660, BIOS 3A19 01/14/2004 [ 3.106504] c212def4 c212def4 c212deb4 c16caf23 c212dee4 c10416fd c1907334 c212df10 [ 3.110315] 00000043 c1907380 00001c84 c105a099 c105a099 c1442040 00000001 f5f54bc0 [ 3.114143] c212defc c104176e 00000009 c212def4 c1907334 c212df10 c212df30 c105a099 [ 3.117960] Call Trace: [ 3.121703] [<c16caf23>] dump_stack+0x16/0x18 [ 3.125447] [<c10416fd>] warn_slowpath_common+0x7d/0xc0 [ 3.129172] [<c105a099>] ? __might_sleep+0x79/0x90 [ 3.132868] [<c105a099>] ? __might_sleep+0x79/0x90 [ 3.136500] [<c1442040>] ? pccardd+0xa0/0x3e0 [ 3.140092] [<c104176e>] warn_slowpath_fmt+0x2e/0x30 [ 3.143657] [<c105a099>] __might_sleep+0x79/0x90 [ 3.147209] [<c1442040>] ? pccardd+0xa0/0x3e0 [ 3.150747] [<c1442040>] ? pccardd+0xa0/0x3e0 [ 3.154256] [<c16cf447>] mutex_lock+0x17/0x2a [ 3.157734] [<c1442089>] pccardd+0xe9/0x3e0 [ 3.161207] [<c1441fa0>] ? pcmcia_socket_uevent+0x30/0x30 [ 3.164660] [<c1056990>] kthread+0xa0/0xc0 [ 3.168059] [<c16d1040>] ret_from_kernel_thread+0x20/0x30 [ 3.171436] [<c10568f0>] ? kthread_worker_fn+0x140/0x140 [ 3.174796] ---[ end trace c3f708b642e3d8f0 ]---
From my reading of the thread fixing pccardd/sched TASK_RUNNING usage/check is another issue left for the future.
Thanks, Bruno
> Linus
| |