On Tue, Feb 19, 2013 at 4:07 PM, Mandeep Singh Baines <m...@chromium.org> wrote: > On Sat, Feb 16, 2013 at 9:05 AM, Oleg Nesterov <o...@redhat.com> wrote: >> On 02/16, Mandeep Singh Baines wrote: >>> >>> We don't need to call freezer_do_not_count() for in-kernel users >>> of CLONE_VFORK since exec will get called in bounded time. >>> >>> We don't want to call freezer_count() for in-kernel users because >>> they may be holding locks. freezer_count() calls try_to_freeze(). >>> We don't want to freeze an in-kernel user because it may be >>> holding locks. >> >> I can only repeat my question ;) >> >> Who? We should not do this anyway. And __call_usermodehelper() >> doesn't >> afaics. >> >> OK, its caller (process_one_work) does lock_map_acquire() for >> debugging >> purposes, this can "confuse" print_held_locks_bug(). But this thread >> is >> PF_NOFREEZE ? >> >> Previously this was needed to suppress the false positive. Now that 2/5 >> checks PF_NOFREEZE, why do we need this change? >> > > After applying the PF_NOFREEZE check, I still get the following: > > [ 1.001030] ======================================= > [ 1.001039] [ BUG: lock held while trying to freeze! ] > [ 1.001048] 3.4.0 #24 Not tainted > [ 1.001053] --------------------------------------- > [ 1.001060] kworker/u:0/5 is exiting with locks still held! > [ 1.001068] 2 locks held by kworker/u:0/5: > [ 1.001073] #0: (khelper){.+.+.+}, at: [<8103896f>] > process_one_work+0x108/0 > x2ee > [ 1.001095] #1: ((&sub_info->work)){+.+.+.}, at: [<8103896f>] > process_one_wo > rk+0x108/0x2ee > [ 1.001111] > [ 1.001113] stack backtrace: > [ 1.001119] Pid: 5, comm: kworker/u:0 Not tainted 3.4.0 #24 > [ 1.001124] Call Trace: > [ 1.001135] [<81025bd6>] ? console_unlock+0x17a/0x18b > [ 1.001146] [<8105d68e>] debug_check_no_locks_held+0x82/0x8a > [ 1.001156] [<8102493f>] do_fork+0x20d/0x2ac > [ 1.001167] [<810366cb>] ? call_usermodehelper_setup+0x8c/0x8c > [ 1.001177] [<81008310>] kernel_thread+0x7a/0x82 > [ 1.001186] [<810366cb>] ? call_usermodehelper_setup+0x8c/0x8c > [ 1.001198] [<814b45dc>] ? common_interrupt+0x3c/0x3c > [ 1.001208] [<810365e8>] __call_usermodehelper+0x3b/0x71 > [ 1.001216] [<810389ce>] process_one_work+0x167/0x2ee > [ 1.001226] [<810365ad>] ? call_usermodehelper_freeinfo+0x1e/0x1e > [ 1.001235] [<81038dbc>] worker_thread+0xbd/0x18b > [ 1.001244] [<81038cff>] ? rescuer_thread+0x184/0x184 > [ 1.001254] [<8103c636>] kthread+0x77/0x7c > [ 1.001264] [<8103c5bf>] ? kthread_freezable_should_stop+0x4a/0x4a > [ 1.001273] [<814b45e2>] kernel_thread_helper+0x6/0x10 >
D'oh. I had the logic in my patch inverted. Ignore the trace. > Regards, > Mandeep > >>> @@ -722,9 +722,11 @@ static int wait_for_vfork_done(struct task_struct >>> *child, >>> { >>> int killed; >>> >>> - freezer_do_not_count(); >>> + if (!(current->flags & PF_KTHREAD)) >>> + freezer_do_not_count(); >> >> If I missed something and we really need this, imho this needs a comment. >> >> Oleg. >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/