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. That is not the case however -- it also
depends on the rate at which a target sends target-to-initiator
requests. At least in theory, if a (not completely conforming) target
sends requests fast enough, and if the above if-statement would be
removed, it becomes possible that tx_head - tx_tail is larger than
SRP_SQ_SIZE. This means that it becomes possible that
__srp_get_tx_iu() returns a pointer to an information unit that is
still in use and hence that data corruption is caused. Since the above
if-statement does not cause a measurable performance penalty and
because of the risks of removing that if-statement, I prefer to keep
it.
> > - if (target->req_lim < min) {
> > + if (target->req_lim <= rsv) {
> > ++target->zero_req_lim;
> > return NULL;
> > }
>
>
> > @@ -1626,9 +1632,9 @@ static struct scsi_host_template srp_template = {
> > .eh_abort_handler = srp_abort,
> > .eh_device_reset_handler = srp_reset_device,
> > .eh_host_reset_handler = srp_reset_host,
> > - .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);
Bart.
--
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
