On Tue, 2010-08-03 at 13:40 +0200, Bart Van Assche wrote: > On Mon, Aug 2, 2010 at 11:44 PM, David Dillow <d...@thedillows.org> 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. > > SRP_AER_REQ isn't handled, and it wouldn't be much to add it. That would > > improve our standards compatibility. > > The message "Unhandled SRP opcode" is printed for SRP_AER_REQ > information units. Support for SRP_AER_REQ can still be added in a > follow-up patch. Adding the few lines to this patch to implement a stub for it is preferable. Credits may be returned via the AER message as well, so it fits in the justification you give for this patch. 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
