lkml.org 
[lkml]   [2010]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] scsi: don't use execute_in_process_context()
From
Date
On Wed, 2010-12-15 at 20:19 +0100, Tejun Heo wrote:
> Hello,
>
> On 12/15/2010 08:10 PM, James Bottomley wrote:
> >> Yes, it would do, but we were already too far with the existing
> >> implementation and I don't agree we need more when replacing it with
> >> usual workqueue usage would remove the issue. So, when we actually
> >> need them, let's consider that or any other way to do it, please.
> >> A core API with only a few users which can be easily replaced isn't
> >> really worth keeping around. Wouldn't you agree?
> >
> > Not really ... since the fix is small and obvious.
>
> IMHO, it's a bit too subtle to be a good API. The callee is called
> under different (locking) context depending on the callsite and I've
> been already bitten enough times from implicit THIS_MODULEs. Both
> properties increase possbility of introducing problems which can be
> quite difficult to detect and reproduce.

Both have subtleties ... see below.

> > Plus now it can't be moved into SCSI because I need the unremovable
> > call chain.
>
> Yes, with the proposed change, it cannot be moved to SCSI.
>
> > Show me how you propose to fix it differently first, since we both agree
> > the initial attempt doesn't work, and we can take the discussion from
> > there.
>
> Given that the structures containing the work items are dynamically
> allocated, I would introduce a scsi_wq, unconditionally schedule
> release works on them and flush them before unloading. Please note
> that workqueues no longer require dedicated threads, so it's quite
> cheap.

A single flush won't quite work. The target is a parent of the device,
both of which release methods have execute_in_process_context()
requirements. What can happen here is that the last put of the device
will release the target (from the function). If both are moved to
workqueues, a single flush could cause the execution of the device work,
which then queues up target work (and makes it still pending). A double
flush will solve this (because I think our nesting level doesn't go
beyond 2) but it's a bit ugly ...

execute_in_process_context() doesn't have this problem because the first
call automatically executes the second inline (because it now has
context).

James




\
 
 \ /
  Last update: 2010-12-15 20:35    [from the cache]
©2003-2011 Jasper Spaans