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/04/2006 21:25:06
The following reply was made to PR kern/34293; it has been noted by GNATS.

From: Michael van Elst <mlelstv@serpens.de>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org,
	netbsd-bugs@NetBSD.org
Subject: Re: PR/34293 CVS commit: src/sys/dev
Date: Mon, 4 Sep 2006 23:23:37 +0200

 On Mon, Sep 04, 2006 at 08:55:06PM +0000, Manuel Bouyer wrote:
 
 >  > vndstrategy isn't called from interrupt context.
 >  
 >  It is, if called from another driver (e.g. Xen block device).
 
 I believe that this is not allowed. The strategy routine is
 part of the top half of the driver, it operates in process
 context.
 
 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).
 
 
 >  > But if that is
 >  > a problem, the feedback loop can be put into both vndread()/vndwrite().
 >  I'm not sure we're allowed to tsleep() here either.
 
 vndread()/vndwrite() definitely do already tsleep() in physio() exactly
 when they are waiting for buffers. However, I was mistaken that these
 routines were passed when accessing the block device but they are
 only used by the raw device.
 
 
 >  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.
 
 
 >  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. You are right that sleeps
 should be avoided when possible for performance reasons.
 
 However, the patch improves performance significantly by avoiding
 the buffer drain.
 
 
 >  > Preventing vnd from consuming all buffers cannot be bad for
 >  > 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. The buffers queue up
 for vnd and the vndthread waits for free buffers. Deadlock.
 
 
 
 Greetings,
 -- 
                                 Michael van Elst
 Internet: mlelstv@serpens.de
                                 "A potential Snark may lurk in every tree."