Re: PATCH: scsi device queue depth adjustability patch

Doug Ledford
2002-10-02T22:18:37Z

On Wed, Oct 02, 2002 at 05:41:12PM -0400, James Bottomley wrote:
> dledford@redhat.com said:
> > This patch makes it possible to adjust the queue depth of a scsi
> > device after it has been in use some time and you have a better idea
> > of what the optimal queue depth should be. For the most part this
> > should work, but my 2.5.40 test machine is blowing chunks on the
> > serverworks IDE support right now so it isn't tested :-(
>
> I note that there's a lot more than dynamic queue depth adjustment in this
> patch (PPA inquiry, slave attach etc.).

The slave attach is required for the queue depth adjustment stuff to work.
The goal is to remove the select_queue_depths() function entirely and make
slave_attach() -> adjust_queue_depth() the replacement. The PPR message
ability flag was just a simple bit that goes along with slave attach.
There's more to be done there, but that's in there as a sample.

> How do HBA's that don't support this now work?

The mid layer calls into the HBA driver's select_queue_depths() function
and the driver sets the queue depth on each device from there.

> You've taken any dependency on
> scsi_host.cmd_per_lun out of the code (thus rendering it useless) so every HBA
> driver now is forced to use an initial queue depth adjustment just to start
> tagged command queueing.

This is true both before and after my patch. If the HBA doesn't implement
the select_queue_depths function then it will never get tagged queueing.
Besides, cmd_per_lun is *not* for tagged queueing, it's the queue depth
the scsi mid layer is suppossed to keep for *non* tagged devices.

> Can't we at least start with cmd_per_lun as the
> default depth?

The current mid layer never has enabled a default depth, all scsi drivers
must enable tagged queueing for each device before it happens. I'm just
changing how that's done from a specific select_queue_depths() call in
point that does one and only one thing to a generic slave_attach() call in
point that sets various post-INQUIRY data drive specific items, queue
depth and acceptable speed negotiation message protocols being the two
examples in my patch.

> I'm not entirely happy with the idea that we control the queue depth by
> adjusting the number of the device's allocated commands.

It's the only safe way to do it. You can't count on being able to
allocate commands because you may not have available mem, and you don't
want extra laying around because that wastes mem, and it allows my next
changes to go in which includes switching to linked lists for commands and
that way we can have a free list for each lun and when there is a command
on the free list, then you know you are under your queue depth count
because if all commands are used, then none are on the free list.

> I know the patch
> goes to great lengths to move these kmallocs out of the critical path, but
> there are certain environments (multi-initiator) where the queue depth can be
> nastily and randomly variable.

Go look at the QUEUE_FULL handler in the aic7xxx_old driver. This is how
most/all reasonably well written drivers handle queue depth adjustments.
Trust me, they don't go around adjusting the depth all the time. Most of
the time there will be one initial adjustment, then maybe one more
adjustment as we lock it down to the max upper limit when one exists, the
rest of the time we just handle the occasional random queue depth
QUEUE_FULL messages as exactly that and only temporarily freeze the queue
to let the drive get some work done.

> If the allocations were more lazy (wait a
> while before freeing a struct Scsi_Cmnd to see if the queue depth goes up
> again for instance) this would address some of these concerns (perhaps just
> moving to a slab allocator for command blocks would do it?)

Nope, this is a non concern because we don't call adjust_queue_depth on
every queue full (or at least we shouldn't, there isn't any documentation
yet to tell people that, but that was the purpose of making the
aic7xxx_old driver implement the new method since it serves as an example
and it does exactly as I'm talking about).

> > Side note: I left the control of queue depth setting solely in the
> > hands of the low level drivers since they are the *only* ones that
> > can get an accurate queue depth reading at the time of any given
> > QUEUE_FULL message (see what the aic7xxx_old driver has to do in the
> > QUEUE_FULL handler to find out how many commands the drive has seen
> > at this exact point in time vs. how many we may have queued up to the
> > card, the difference in numbers can be significant). For that
> > reason, the adjust_queue_depth call was made to defer the action
> > until later so that it was interrupt context safe.
>
> I appreciate that the HBA driver is the most exact counter of the queue depth,
> but would it make a significant difference if the adjustments were done
> globally in the mid-layer?

The mid layer simply does not have access to the info needed to do what
you are talking about. Let me give you an example from the aic7xxx
driver. On this card we have a sequencer that handles the scsi protocol
for us. When we get a command from the mid layer it goes either to the
card's QINFIFO (queue of commands that the sequencer hasn't started yet)
or to the device's waiting queue (queue of commands that we can't send to
the sequencer yet because of some reason). Once it is on the card and the
sequencer starts the process of selecting the target device, the command
is moved to the waiting_q on the card (queue of commands already started
but for which the target device has not yet responded). Once the device
is selected and we get to send the command, then we usually go directly
into status phase and get our QUEUE_FULL. At this point the sequencer
pauses all operations on the scsi bus and interrupts the kernel to handle
the QUEUE_FULL. Prior to this command being selected, there may have been
any number of commands place in the cards QOUTFIFO (our queue of already
completed SCSI commands which the kernel interrupt driver hasn't processed
yet). So when the kernel driver gets the QUEUE_FULL interrupt, it must
first plug this device up. It then must remove any commands it finds in
the QINFIFO and in the waiting_q on the card and requeue them back into
the device's waiting queue. Each command it moves like this decrements
the device's active command count since when we place a command on the
QINFIFO that's when we increment the active count. Then we have to pull
any possible completed commands out of the QOUTFIFO and place them on the
done queue (commands we have queued to go back to the mid layer), also
decrementing the active count each time. Finally, we decrement the active
count for the device one more time to accomodate our QUEUE_FULL command.
Only after we have done *all* of this crap do we have an accurate queue
depth count for the command that returned a QUEUE_FULL. As you can see,
lots of this is very hardware specific. Controller cards that don't have
a sequencer like control engine won't be starting commands off in a lazy
fasion like this and so don't need all this stuff. That's why the only
place where we can get an accurate count of the queue depth is in the low
level driver. So, that code *must* stay there since getting an accurate
count is how we avoid the random flip flops of queue depth that you were
worried about earlier.

Now, what can be moved to the mid layer, and is on my list to do, is a
generic interface for coming up with the proper action after a QUEUE_FULL.
Currently, each driver not only determines the real depth, but then also
does it's own magic to tell if it's a random event or a hard limit. It
would be easy to add something like scsi_notify_queue_full(sdev, count);
where scsi_notify_queue_full() would keep track of the last queue full
depth, etc. Let the low level driver come up with the accurate count,
then they can use this function to determine what to do about it. On any
change to queue depth, the function can return the amount of commands to
add/subtract, then the low level driver can adjust it's own internal
structure counts and also call scsi_adjust_queue_depth() to have the mid
layer do likewise. BTW, I'll change the adjust_queue_depth code to make
it immediately adjust the depth down when possible and do lazy increases
so that hitting a hard limit will free up resources immediately, but that
will go with making the commands linked list based so that it can simply
do a while(sdev->queue_depth > sdev->new_queue_depth &&
list_not_empty(sdev->free_list)) { kfree(get_list_head(sdev->free_list));
sdev->queue_depth--; }

> The great advantage is that we would gain dynamic
> queue depth adjustments without having to add specific code to every driver,

You're never going to get around this and have a reasonably reliable
method :-(

> at the cost of not always being entirely accurate about the depth. Is there a
> good argument that this really, really must be done at the LLD level given the
> cost in terms of LLD modifications? You could still add hooks for those HBAs
> that really want to do it themselves.

Well, all the device drivers that implement queue depth adjustments at all
already do it themselves, so my proposed method is better than what we
currently have since it at least allows them to move the decide
disposition stuff into a library call or they can do it themselves.

--
Doug Ledford <dledford@redhat.com> 919-754-3700 x44233
Red Hat, Inc.
1801 Varsity Dr.
Raleigh, NC 27606