IB/srp: Preparation for transmit ring response allocation

Quoted 9 lines

+
+     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.

Quoted 5 lines

-     if (target->req_lim < min) {
+     if (target->req_lim <= rsv) {
              ++target->zero_req_lim;
              return NULL;
      }

drivers/infiniband/ulp/srp/ib_srp.c static struct scsi_host_template srp_template = { · line 1635

Quoted 6 lines

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

@@ -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...

Quoted 10 lines · David Dillow

drivers/infiniband/ulp/srp/ib_srp.c static void srp_send_completion(struct ib_cq *cq, void *target_ptr) · line 1003

Quoted 9 lines

drivers/infiniband/ulp/srp/ib_srp.c +3 -1

+
+     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;

Quoted 5 lines

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.

Quoted 8 lines

drivers/infiniband/ulp/srp/ib_srp.c static struct scsi_host_template srp_template = { · line 1632

Quoted 3 lines

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

@@ -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);

Quoted 17 lines · Bart Van Assche

Quoted 10 lines · David Dillow

Quoted 9 lines

+
+     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;

Quoted 5 lines

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.

Quoted 3 lines

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.