Messages in this thread | | | Date | Wed, 22 Apr 2009 13:14:02 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 01/10] sysfs: don't use global workqueue in sysfs_schedule_callback() |
| |
On Fri, 17 Apr 2009 12:04:25 -0700 Greg Kroah-Hartman <gregkh@suse.de> wrote:
> From: Alex Chiang <achiang@hp.com> > > A sysfs attribute using sysfs_schedule_callback() to commit suicide > may end up calling device_unregister(), which will eventually call > a driver's ->remove function. > > Drivers may call flush_scheduled_work() in their shutdown routines, > in which case lockdep will complain with something like the following: > > ============================================= > [ INFO: possible recursive locking detected ] > 2.6.29-rc8-kk #1 > --------------------------------------------- > events/4/56 is trying to acquire lock: > (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 > > but task is already holding lock: > (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 > > other info that might help us debug this: > 3 locks held by events/4/56: > #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 > #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 > #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40 > > stack backtrace: > Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1 > Call Trace: > [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260 > [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40 > [<ffffffff8026f148>] lock_acquire+0x58/0x80 > [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 > [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0 > [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 > [<ffffffff80258070>] flush_scheduled_work+0x10/0x20 > [<ffffffffa0144065>] e1000_remove+0x55/0xfe [e1000e] > [<ffffffff8033ee30>] ? sysfs_schedule_callback_work+0x0/0x50 > [<ffffffff803bfeb2>] pci_device_remove+0x32/0x70 > [<ffffffff80441da9>] __device_release_driver+0x59/0x90 > [<ffffffff80441edb>] device_release_driver+0x2b/0x40 > [<ffffffff804419d6>] bus_remove_device+0xa6/0x120 > [<ffffffff8043e46b>] device_del+0x12b/0x190 > [<ffffffff8043e4f6>] device_unregister+0x26/0x70 > [<ffffffff803ba969>] pci_stop_dev+0x49/0x60 > [<ffffffff803baab0>] pci_remove_bus_device+0x40/0xc0 > [<ffffffff803c10d9>] remove_callback+0x29/0x40 > [<ffffffff8033ee4f>] sysfs_schedule_callback_work+0x1f/0x50 > [<ffffffff8025769a>] run_workqueue+0x15a/0x230 > [<ffffffff80257648>] ? run_workqueue+0x108/0x230 > [<ffffffff8025846f>] worker_thread+0x9f/0x100 > [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40 > [<ffffffff802583d0>] ? worker_thread+0x0/0x100 > [<ffffffff8025b89d>] kthread+0x4d/0x80 > [<ffffffff8020d4ba>] child_rip+0xa/0x20 > [<ffffffff8020cebc>] ? restore_args+0x0/0x30 > [<ffffffff8025b850>] ? kthread+0x0/0x80 > [<ffffffff8020d4b0>] ? child_rip+0x0/0x20 > > Although we know that the device_unregister path will never acquire > a lock that a driver might try to acquire in its ->remove, in general > we should never attempt to flush a workqueue from within the same > workqueue, and lockdep rightly complains. > > So as long as sysfs attributes cannot commit suicide directly and we > are stuck with this callback mechanism, put the sysfs callbacks on > their own workqueue instead of the global one. > > This has the side benefit that if a suicidal sysfs attribute kicks > off a long chain of ->remove callbacks, we no longer induce a long > delay on the global queue.
I still don't know why I merged
: commit 2355b70fd59cb5be7de2052a9edeee7afb7ff099 : Author: Lai Jiangshan <laijs@cn.fujitsu.com> : Date: Thu Apr 2 16:58:24 2009 -0700 : : workqueue: avoid recursion in run_workqueue()
there was nothing wrong with permitting limited recursion into run_workqueue(). It never deadlocked and the three-deep-recursion warning never triggered.
> + if (sysfs_workqueue == NULL) { > + sysfs_workqueue = create_workqueue("sysfsd"); > + if (sysfs_workqueue == NULL) { > + module_put(owner); > + return -ENOMEM; > + } > + }
This will create a kernel thread per CPU. Surely create_singlethread_workqueue() will suffice?
| |