On Fri, Aug 13, 2010 at 8:12 PM, David Dillow <[email protected]> wrote: > On Tue, 2010-08-10 at 09:55 +0200, Bart Van Assche wrote: >> On Tue, Aug 3, 2010 at 5:44 PM, David Dillow <[email protected]> wrote: >> > >> > On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote: >> > [ ... ] >> > > I'm not sure it is a good idea to allow that all transmit buffers get >> > > allocated for sending CMD_RSP information units and that none remain >> > > for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT >> > > requests. Wouldn't that be a protocol violation ? >> > >> > You mean SRP_CMD? We don't do that, we clamp the outstanding requests we >> > generate so we'll always have a buffer to reply to SRP_CRED_REQ et al. >> > >> > If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer >> > available for the reply. >> > >> > Now, if the target sends us multiple SRP_CRED_REQ without waiting for a >> > reply, then we could use up buffers and not be able to send requests, >> > but that would be a protocol violation on the target's side. >> >> (resending as plain text) >> >> I have been looking further into this. There is a slight asymmetry in >> the current transmit buffer allocation code: one buffer element is >> reserved on the target for task management requests but not on the >> initiator. So - at least in theory - it is possible that all elements >> of the initiator transmit ring are allocated for SRP_CMD requests at >> the time an SRP_TSK_MGMT request should be sent. Wouldn't it be more >> symmetric to change __srp_get_tx_iu() as follows (diff against the >> current for-next branch) ? > > This is not needed in the code as it currently stands. We cannot have > all elements of the tx ring allocated for SRP_CMD requests, because we > don't send one if we don't have a credit for it, and we save a credit > for the SRP_TSK_MGMT request. Hence, there will always be a buffer > available for the management request. > > We don't need it for when we start replying to target requests in the > future, because we already reserve that buffer by limiting our view of > credits to one less than the actual ring size.
It is not needed as long as the target respects the limit of at most one outstanding request, as required by the SRP (draft) standard. The test in question is a single if-statement that provides some additional robustness and no measurable overhead. So I don't see why it should be removed. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
