Subject: Re: PR/34293 CVS commit: src/sys/dev
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Michael van Elst <mlelstv@serpens.de>
List: netbsd-bugs
Date: 09/07/2006 06:30:02
The following reply was made to PR kern/34293; it has been noted by GNATS.

From: Michael van Elst <mlelstv@serpens.de>
To: Manuel Bouyer <bouyer@antioche.eu.org>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@NetBSD.org,
	gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org
Subject: Re: PR/34293 CVS commit: src/sys/dev
Date: Thu, 7 Sep 2006 08:27:45 +0200

 On Wed, Sep 06, 2006 at 11:23:02PM +0200, Manuel Bouyer wrote:
 
 Hi Manuel,
 
 > > I believe that this is not allowed. The strategy routine is
 > > part of the top half of the driver, it operates in process
 > > context.
 > 
 > There was a discussion on tech-kern when I introduced the Xen
 > backend driver, and the conclusion was that it's valid to call
 > a strategy routine from interrupt context, and drivers should deal
 > with that.
 
 I now read that discussion (and one from 1996 when ccd had problems
 and one more when cgd had problems).
 
 So, in 4.4BSD the strategy functions operated in process context,
 and a few of them even used this to handle ressource shortages
 on the lower parts of the driver. The original vn driver does
 call VOP_STRATEGY itself, so did our vnd driver until you added
 the thread.
 
 Since strategy routines are not supposed to sleep for performance
 reasons and most therefore didn't, it was concluded that strategy
 routines have to be interrupt safe.
 
 I believe this change in the interface model was a bad decision,
 and it was done to make life for ccd, cgd and later xbd simpler.
 
 
 > > Saying that, I scanned other drivers and found several that may
 > > sleep in their strategy() routine. Most promiment scsistrategy()
 > > does this (by virtue of calling scsipi_command which waits for
 > > a free scsi transfer context in scsipi_get_xs).
 > 
 > This is a pseudo-strategy, part of the SCSI-specific ioctls implementation.
 > It's internal details, it's not exported to other drivers.
 
 I noticed. There are some left that do spin-locking (possible to do 
 in interrupts) and an ancient broken VAX driver that still shows
 that the BSD design let strategy routines operate in process context
 and sleep.
 
 
 > > >  This patch broke vnd for all Xen users, where it worked fine before.
 > > 
 > > I agree. The patch probably leaves a lot space for improvement. But
 > > the real bug is calling the strategy() routine from interrupt context.
 > 
 > You're redefining the strategy() interface here.
 
 No, some people did that 2 years ago (I believe Jason was the first caller,
 but others agreed). This change was done to accomodate cgd.
 
 
 > > >  I understand all of this. But 1) the patch is broken (calling tsleep in
 > > >  a strategy routine) 2) whenever it's not a problem for other drivers is
 > > >  a matter of opinion. If has probably performances inpacts.
 > > 
 > > Calling tsleep() in strategy is allowed.
 > 
 > Where did you get this ?
 
 That is 4.4BSD.
 
 
 > > >  > performances on systems with multiple drives.
 > > >  
 > > >  No, I meant if all buffers end up in a single device queue.
 > > 
 > > That's what happening without the patch.
 > 
 > And that's what happens in other driver's queue too.
 
 True. But with other drivers this does not end in a deadlock as
 they don't do the circle to the VOP layer. It still is a bad
 thing, maybe that SoC project to add some 'congestion control'
 to the filesystems develops a solution to that. The vnd driver
 itself however should protect itself against this deadlock
 that it generates itself. N.B. there is a second deadlock
 for io buffers, that only gets exhibited when memory runs
 out, but when the buffer deadlock is solved, this will show
 up.
 
 I am not sure about ccd, it calls VOP_STRATEGY() which then
 calls DEV_STRATEGY() but has some code path that may sleep.
 
 
 Greetings,
 -- 
                                 Michael van Elst
 Internet: mlelstv@serpens.de
                                 "A potential Snark may lurk in every tree."