On Sat, 2010-08-14 at 09:35 +0200, Bart Van Assche wrote:
> On Fri, Aug 13, 2010 at 8:24 PM, David Dillow <d...@thedillows.org> wrote:
> >
> > > +
> > > + rsv = (req_type == SRP_REQ_NORMAL) ? SRP_TSK_MGMT_RSV : 0;
> > >
> > > srp_send_completion(target->send_cq, target);
> > >
> > > - if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
> > > + if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE - rsv)
> > > return NULL;
> >
> > This isn't needed per my previous message. It is just a guard to make
> > sure we don't use more buffers than are on the ring. The real limiter is
> > the number of credits. Perhaps swapping it with the req_limit check
> > below and adding a WARN_ONCE() may be a good idea, since I don't think
> > we should ever hit it.
>
> Replacing the above if-statement by a WARN_ON_ONCE() statement would
> make sense if the value of tx_head - tx_tail did only depend on the
> implementation of ib_srp.
I didn't say replace the guard, I said add a message so we knew we hit
this case. The "isn't needed" was referring to the change to the line.
WARN_ON_ONCE may not be the best thing to use, but perhaps a
rate-limited shost_printk would be good. We really should never hit the
limit on tx buffers, unless we are dealing with a non-conforming target
as you point out.
> > > @@ -1626,9 +1632,9 @@ static struct scsi_host_template srp_template = {
> > > - .can_queue = SRP_SQ_SIZE,
> > > + .can_queue = SRP_REQ_SQ_SIZE -
> > > SRP_TSK_MGMT_RSV,
> >
> > I don't like the naming here and the duplicate math in several places.
> > Pick a name for this and use it. Since this is just the template and is
> > overridden when we actually connect to a host, I'm not sure there is a
> > much of a reason to change things here anyways. Same for the next two...
>
> I do not agree that the can_queue value filled in in the template is
> thrown away later on. As you can see in the ib_srp login code, the
> can_queue value is preserved when target->req_lim is larger than the
> can_queue value from the template:
>
> target->scsi_host->can_queue = min(target->req_lim,
> target->scsi_host->can_queue);
Yep, I missed that. That's what I get working from memory. We could just
change the setting of can_queue after connection to ignore the value in
scsi_host and clamp the the appropriate size. Probably not my best idea
-- perhaps the SCSI mid-layer wants to clamp that for some reason.
Either way, it needs a good name.
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
