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