On Sat, 2010-08-14 at 10:21 +0200, Bart Van Assche wrote: > 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.
I didn't say remove it, I'm objecting to your changes to it. They are unnecessary because we will keep a Tx buffer around for the response to a conforming target. Keeping the guard is needed for the reasons you point out about non-conforming targets. Dave -- 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
