Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761753AbZCXRd0 (ORCPT ); Tue, 24 Mar 2009 13:33:26 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1763948AbZCXRcy (ORCPT ); Tue, 24 Mar 2009 13:32:54 -0400 Received: from g4t0015.houston.hp.com ([15.201.24.18]:18625 "EHLO g4t0015.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761366AbZCXRcq (ORCPT ); Tue, 24 Mar 2009 13:32:46 -0400 Date: Tue, 24 Mar 2009 11:32:43 -0600 From: Alex Chiang To: Oleg Nesterov , jeffrey.t.kirsher@intel.com Cc: Ingo Molnar , Peter Zijlstra , Andrew Morton , Johannes Berg , jbarnes@virtuousgeek.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, kaneshige.kenji@jp.fujitsu.com Subject: Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove Message-ID: <20090324173243.GC17297@ldl.fc.hp.com> References: <20090320204327.12275.43010.stgit@bob.kio> <20090320205636.12275.1825.stgit@bob.kio> <49C74FCC.7070308@jp.fujitsu.com> <20090324032304.GB6175@ldl.fc.hp.com> <20090324092525.GE6605@elte.hu> <20090324161259.GA5964@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090324161259.GA5964@redhat.com> User-Agent: Mutt/1.5.17+20080114 (2008-01-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6977 Lines: 140 (Adding some e1000e folks to answer questions about their driver) * Oleg Nesterov : > On 03/24, Ingo Molnar wrote: > > > > > > * Kenji Kaneshige : > > > > > > Kenji Kaneshige reported the below lockdep problem when testing > > > my patch on one of his machines. > > > > > > > I still have the following kernel error messages in testing with your > > > > latest set of patches (Jesse's linux-next). The test case is removing > > > > e1000e device or its parent bridge by "echo 1 > /sys/bus/pci/devices/ > > > > .../remove". > > > > > > > > [ 537.379995] ============================================= > > > > [ 537.380124] [ INFO: possible recursive locking detected ] > > > > [ 537.380128] 2.6.29-rc8-kk #1 > > > > [ 537.380128] --------------------------------------------- > > > > [ 537.380128] events/4/56 is trying to acquire lock: > > > > [ 537.380128] (events){--..}, at: [] flush_workqueue+0x0/0xa0 > > > > [ 537.380128] > > > > [ 537.380128] but task is already holding lock: > > > > [ 537.380128] (events){--..}, at: [] run_workqueue+0x108/0x230 > > > > [ 537.380128] > > > > [ 537.380128] other info that might help us debug this: > > > > [ 537.380128] 3 locks held by events/4/56: > > > > [ 537.380128] #0: (events){--..}, at: [] run_workqueue+0x108/0x230 > > > > [ 537.380128] #1: (&ss->work){--..}, at: [] run_workqueue+0x108/0x230 > > > > [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [] remove_callback+0x21/0x40 > > > > [ 537.380128] > > > > [ 537.380128] stack backtrace: > > > > [ 537.380128] Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1 > > > > [ 537.380128] Call Trace: > > > > [ 537.380128] [] validate_chain+0xb7d/0x1260 > > > > [ 537.380128] [] __lock_acquire+0x42e/0xa40 > > > > [ 537.380128] [] lock_acquire+0x58/0x80 > > > > [ 537.380128] [] ? flush_workqueue+0x0/0xa0 > > > > [ 537.380128] [] flush_workqueue+0x4d/0xa0 > > > > [ 537.380128] [] ? flush_workqueue+0x0/0xa0 > > > > [ 537.383380] [] flush_scheduled_work+0x10/0x20 > > > > [ 537.383380] [] e1000_remove+0x55/0xfe [e1000e] > > > > [ 537.383380] [] ? sysfs_schedule_callback_work+0x0/0x50 > > > > [ 537.383380] [] pci_device_remove+0x32/0x70 > > > > [ 537.383380] [] __device_release_driver+0x59/0x90 > > > > [ 537.383380] [] device_release_driver+0x2b/0x40 > > > > [ 537.383380] [] bus_remove_device+0xa6/0x120 > > > > [ 537.384382] [] device_del+0x12b/0x190 > > > > [ 537.384382] [] device_unregister+0x26/0x70 > > > > [ 537.384382] [] pci_stop_dev+0x49/0x60 > > > > [ 537.384382] [] pci_remove_bus_device+0x40/0xc0 > > > > [ 537.384382] [] remove_callback+0x29/0x40 > > > > [ 537.384382] [] sysfs_schedule_callback_work+0x1f/0x50 > > > > [ 537.384382] [] run_workqueue+0x15a/0x230 > > > > [ 537.384382] [] ? run_workqueue+0x108/0x230 > > > > [ 537.384382] [] worker_thread+0x9f/0x100 > > > > [ 537.384382] [] ? autoremove_wake_function+0x0/0x40 > > > > [ 537.384382] [] ? worker_thread+0x0/0x100 > > > > [ 537.384382] [] kthread+0x4d/0x80 > > > > [ 537.384382] [] child_rip+0xa/0x20 > > > > [ 537.386380] [] ? restore_args+0x0/0x30 > > > > [ 537.386380] [] ? kthread+0x0/0x80 > > > > [ 537.386380] [] ? child_rip+0x0/0x20 > > > > > > > > I think the cause of this error message is flush_workqueue() > > > > from the work of keventd. When removing device using > > > > "/sys/bus/pci/devices/.../ remove", pci_remove_bus_device() is > > > > executed by the keventd's work through > > > > device_schedule_callback(), and it invokes e1000e's remove > > > > callback. And then, e1000e's remove callback invokes > > > > flush_workqueue(). Actually, the kernel error messages are not > > > > displayed when I changed e1000e driver to not call > > > > flush_workqueue(). In my understanding, flush_workqueue() from > > > > the work must be avoided because it can cause a deadlock. > > > > Please note that this is not a problem of e1000e driver. > > > > Drivers can use flush_workqueue(), of course. > > > > > > I agree with this analysis; the reason we're seeing this lockdep > > > warning is because the sysfs attributed scheduled a removal for > > > itself using device_schedule_callback(). This is necessary > > > because sysfs attributes can't remove themselves due to other > > > locking issues. > > > > > > My question is -- is it a bug to call flush_workqueue during > > > run_workqueue? > > > > Yes, it generally is. > > > > > Conceptually, I don't think it should be a bug; it should be a > > > nop, since run_workqueue _is_ flushing the work queue. > > As it was already said, we can deadlock. As answered in another email, I agree that generically, it can deadlock. I think this particular warning is a false positive though, because pci_remove_bus_device() definitely cannot acquire any locks that lower level device drivers want to acquire. If that were true, then hotplug would never work, and you would also see hangs every time you shut your machine off. I can't answer the e1000e questions below but... > Can't e1000_remove() avoid flush_scheduled_work() ? (and it should > be always avoided when possible). > > Of course, I don't understand this code. But afaics e1000_remove() > can just cancel its own works (in struct e1000_adapter), no? > > cancel_work_sync(work) from run_workqueue() should be OK even if > this work is queued on the same wq. If it is queued on the same CPU > cancel_work_sync() won't block because we are ->current_work. > > > Btw. Again, I don't understand the code, but this looks suspicious: > > e1000_remove: > > set_bit(__E1000_DOWN, &adapter->state); > del_timer_sync(&adapter->watchdog_timer); > flush_scheduled_work(); > > What if e1000_watchdog_task() is running, has already checked > !test_bit(__E1000_DOWN, &adapter->state), but didn't call > mod_timer(&adapter->phy_info_timer) yet? Even if e1000e is doing something funny, the conclusion I'm coming to is that the sysfs_schedule_callback implementation needs to change, because there are simply too many drivers that call flush_scheduled_work. Thanks. /ac -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/