Re: CVS commit: src/sys/dev

[Christian Limpach <cl%NetBSD.org@localhost>]

Hi,

On Sat, Apr 24, 2004 at 05:10:56PM -0400, Roland C. Dowdeswell wrote:
> On 1082825674 seconds since the Beginning of the UNIX epoch
> Christian Limpach wrote:
> >Doesn't this mess up the ordering of the buffers on the queue when diskstart
> >doesn't process a buf and we have to put it back on the queue with BUFQ_PUT?
> 
> I don't think that there are many situations where this can actually
> happen.  dk_start() is called at splbio(), so this change should
> mostly affect what happens when dk_start() is called on the same
> device further down the stack.  In this case, we are guaranteed
> that the buffers will be put back in order in failure cases.  (In
> fact, in cgd.c we should not actually get to another dk_start on
> the same device until we actually know that the buffer will not be
> put back on the queue---but that's another issue.)

My concern is not so much another dk_start() grabbing a 2nd buffer
and running it while a previous dk_start() is still in progres but
the fact that BUFQ_PUT() will put the buffer at the end of the queue
when dk_start() has to put the buffer back because diskstart couldn't
allocate the resources to run the buffer.  That seems rather unfair
and could lead to bad performance when the queue is rather long.

FWIW, my driver has 64 slots for requests which can be in progress
concurrently and it occasionally has to return from the diskstart
routine when no free slots are available.

I agree that it's not a requirement that we preserve ordering but
with the change we seem to mess up the ordering specifically at those
times when we're short on resources already.

If you agree that it would make sense to care about ordering, then
it would probably be easiest to require the diskstart() routine to
remove the buffer from the queue.

> >Additionally, the change prevents a *start routine from processing and
> >dequeueing multiple bufs and then return -1 to make dk_start exit.
> >(see arch/xen/xen/xbd.c rev 1.1)
> 
> When I wrote the interface, this was not something I was considering.
> I'll have a look at it and think about it a little.  :-)

;-) I was well aware that I was not using the interface as intended (and
prominently XXXed the places in the code), but it doesn't seem
unreasonable that a driver wants to operate on more than one buffer
at a time.  In my driver's case starting a request is somewhat
expensive and thus it's preferable to prepare as many requests
as possible and then issue one start command.  Additionally this
gives more leeway to the underlying "hardware" to do disksort()-like
operations.

I'm now delaying start commands while there are still buffers on the
queue, relying on the fact that dk_start() will keep calling diskstart()
as long as there are buffers on the queue.  That's good enough (the
additional function call is negligible) except for the reordering issue
from above.

For my driver's use:
- a "buffer processed, out of resources now" return code from
diskstart() would be good since I know at the end of diskstart()
if there's a free slot for another request.  This would also take
care of the reordering problem in my case.
- a diststart_finished() callback after the while() loop in dk_start()
would make it unnecessary that my diskstart() routine knows about the
buffer queue at all.

    christian