[ofa-general] [RFC PATCH] IB/srp: respect target credit limit

[David Dillow]

On Tue, 2007-12-18 at 10:16 -0500, Dave Dillow wrote:
> On Mon, Dec 17, 2007 at 09:58:17PM -0800, Roland Dreier wrote:
> >  > The SRP initiator will currently send requests, even if we have no
> >  > credits available. The results of sending extra requests is
> >  > vendor-specific, but on some devices, overrunning our credits will cost
> >  > us 85% of peak performance -- e.g. 100 MB/s vs 720 MB/s. Other devices
> >  > may just drop them.
> > 
> > I guess this happens because a target sometimes completes commands
> > with a value of 0 for req_lim delta in the response?
> 
> I didn't instrument the delta in the responses, I just noticed that
> req_lim_zero was growing very, very quickly when the performance dropped.
> I added a sysfs entry to give a view of req_lim, and I saw it get to -3
> several times, so I expect we're actually getting some negative deltas back.

My guess about negative deltas was incorrect -- here's the breakdown of
8192 requests (trying to keep 40 in flight, but max credits is ~33):

rsp delta 0		2369
rsp delta 1		3905
rsp delta 2		1503
rsp delta 3		379
rsp delta 4		36

This was with a steady stream of 1MB I/O requests.

> >  > +	.max_host_blocked		= 1,
> > 
> > Documentation of what max_host_blocked does is pretty scant.  What
> > does this change do?

I've dropped this change -- when looking at the code that handles
scsi_host->host_blocked, I missed that it gets reset after each SCSI
command completes, so the mid-layer doesn't wait until the queue clears
to start dispatching commands again.

max_host_blocked/host_blocked only really come into play stall-wise when
we return HOST_BUSY and there are no commands in flight. Then, it will
block attempts to run the queue until scsi_host->host_blocked counts
down to zero. The default means it will take 7 commands to clear this
state, but we should not enter it in the first place. It also should not
be a behavior change from the existing code.

I'll send the new patch under separate cover -- with the queue_len
parameter split out, and an additional patch to let us easily grow to
1MB I/Os and more.

Dave