On Tue, Aug 3, 2010 at 5:05 PM, David Dillow <[email protected]> wrote: > On Tue, 2010-08-03 at 13:40 +0200, Bart Van Assche wrote: >> On Mon, Aug 2, 2010 at 11:44 PM, David Dillow <[email protected]> wrote: >> > On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote: >> > [ ... ] >> > >> > There seems to be an accounting change from using tx_head/tx_tail to >> > using tx_req and tx_rsp. I'm not sure that really makes sense -- >> > splitting that into its own patch would allow for an easier review and >> > may make it obvious if it is the right thing to do. >> >> As I wrote in [PATCH 0/4], both initator-to-target requests and >> initiator-to-target responses are now allocated from the same send >> queue. Since tx_head - tx_tail is now the sum of the number of >> requests and the number of responses allocated on the send queue, new >> variables had to be introduced in order to count how many requests and >> how many responses had been allocated on the send queue. > > I understood that you were allocating from the same queue of buffers, > I'm just not sure you need to keep track separately. We already hold > SRP_RSP_SQ_SIZE buffers back from sending requests, so we will always > have those (well, one) available for sending a response. > > If we use too many for responses, that's not great, but it isn't > completely fatal -- srp_queuecommand() will bounce the command back to > the mid-layer with a BUSY status, and while the error handling (task > mgmt) will go to the big guns and reset the connection sooner, we will > recover. > > If there isn't a buffer available to send the response, it'll get > dropped on the floor, just like in your patch. > > I just don't see a reason for the accounting change.
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 ? Note: because of the invariant tx_head - tx_tail == tx_req + tx_rsp it is possible to eliminate at least one of these last two variables. I have not yet done this because I prefer readability over saving a tiny amount of memory. 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
