lkml.org 
[lkml]   [2009]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
From
Date
On Tue, 2009-03-24 at 11:23 -0600, Alex Chiang wrote:

> > There is no bug -- it's a false positive in a way. I've pointed this out
> > in the original thread, see
> > http://thread.gmane.org/gmane.linux.kernel/550877/focus=550932
>
> I'm actually a bit confused now.

Sorry.

> Peter explained why flushing a workqueue from the same queue is
> bad, and in general I agree, but what do you mean by "false
> positive"?

Well, even though generally flushing it from within is bad, the actual
thing lockdep reports is bogus -- it's reporting a nested locking.

> By the way, this scenario:
>
> code path 1:
> my_function() -> lock(L1); ...; flush_workqueue(); ...
>
> code path 2:
> run_workqueue() -> my_work() -> ...; lock(L1); ...
>
> is _not_ what is happening here.

Indeed.

> So what you really have going on is:
>
> sysfs callback -> add remove callback to global workqueue
> remove callback fires off (pci_remove_bus_device) and we do...
> device_unregister
> driver's ->remove method called
> driver's ->remove method calls flush_scheduled_work
>
> Yes, after read the thread I agree that generically calling
> flush_workqueue in the middle of run_workqueue is bad, but the
> lockdep warning that Kenji showed us really won't deadlock.

Exactly that is what I meant by "false positive".

> This is because pci_remove_bus_device() will not acquire any lock
> L1 that an individual device driver will attempt to acquire in
> the remove path. If that were the case, we would deadlock every
> time you rmmod'ed a device driver's module or every time you shut
> your machine down.
>
> I think from my end, there are 2 things I need to do:
>
> a) make sysfs_schedule_callback() use its own work queue
> instead of global work queue, because too many drivers
> call flush_scheduled_work in their remove path
>
> b) give sysfs attributes the ability to commit suicide
>
> (a) is short term work, 2.6.30 timeframe, since it doesn't
> involve any large conceptual changes.
>
> (b) is picking up Tejun Heo's existing work, but that was a bit
> controversial last time, and I'm not sure it will make it during
> this merge window.
>
> Question for the lockdep folks though -- given what I described,
> do you agree that the warning we saw was a false positive? Or am
> I off in left field?

I think we're not sure yet -- it seems Lai Jiangshan described a
scenario in which flushing from within the work actually _can_ deadlock.

johannes
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2009-03-24 21:25    [W:0.104 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site