From: tzachid Date: Mon, 21 Nov 2005 12:29:19 +0000 (+0000) Subject: Fix a race in the SDP code (buffers were sent before RTU was received) (Integrate... X-Git-Url: https://openfabrics.org/gitweb/?a=commitdiff_plain;h=a3c2438ccaed545c0f84643613adb485d42be575;p=~shefty%2Frdma-win.git Fix a race in the SDP code (buffers were sent before RTU was received) (Integrate 546 from branch) (Rev 624) git-svn-id: svn://openib.tc.cornell.edu/gen1@180 ad392aa1-c5ef-ae45-8dd8-e69d62a5ef86 --- diff --git a/trunk/ulp/sdp/kernel/SdpBufferPool.cpp b/trunk/ulp/sdp/kernel/SdpBufferPool.cpp index 750edbd3..bceb4ac6 100644 --- a/trunk/ulp/sdp/kernel/SdpBufferPool.cpp +++ b/trunk/ulp/sdp/kernel/SdpBufferPool.cpp @@ -293,6 +293,18 @@ BufferPool::SendBuffersIfCan() AssertLocked(); NTSTATUS rc = STATUS_SUCCESS; +// Check if such code should be here ???? what if we have packets ???? + if (m_PostCreditsWhenCan == true) { + m_PostCreditsWhenCan = false; + rc = PostCredits(); + if (!NT_SUCCESS(rc)) { + SDP_PRINT(SDP_ERR, SDP_BUFFER_POOL, ("PostCredits failed rc = 0x%x\n", rc )); + goto Cleanup; + } + } + + + while ((m_QueuedPackets.Size() > 0) && (m_CurrentlySentBuffers < m_MaxConcurrentSends) && (m_rRecvBuf > 2)) { @@ -391,7 +403,7 @@ BufferPool::SendBuffer(BufferDescriptor *pBufferDescriptor) ib_send_wr_t send_wr; send_wr.p_next = NULL; - send_wr.wr_id = (uintn_t)pBufferDescriptor;//?????(uint64_t) (uintptr_t) wr; + send_wr.wr_id = (uintn_t)pBufferDescriptor; send_wr.wr_type = WR_SEND; send_wr.send_opt = IB_SEND_OPT_SIGNALED;//socket_info->send_opt; @@ -415,6 +427,7 @@ BufferPool::SendBuffer(BufferDescriptor *pBufferDescriptor) rc = IB2Status(ib_status); goto Cleanup; } + //????? Should we clear the post credits here ???????? m_CurrentlySentBuffers ++; m_rRecvBuf--; m_pSdpSocket->m_RecvBufferPool.UpdateLocaleAdvertisedBuffers(); @@ -439,6 +452,14 @@ BufferPool::PostCredits() m_PostCreditsWhenCan = true; goto Cleanup; } + + if (m_pSdpSocket->GetState() != SS_CONNECTED) { + SDP_PRINT(SDP_TRACE, SDP_BUFFER_POOL, ("this = 0x%p - Not sending credits," + " because state = %s \n",this, SdpSocket::SS2String(m_pSdpSocket->GetState() ))); + // We will have to send them once we can + m_PostCreditsWhenCan = true; + goto Cleanup; + } // Post the credit if (m_CreditdBufferDescriptor == NULL) { @@ -499,7 +520,7 @@ NTSTATUS BufferPool::PostDisConn() pBufferDescriptor->SetMid(SDP_MID_DISCONNECT); rc = AddBufferToQueuedList(pBufferDescriptor); if (!NT_SUCCESS(rc)) { - SDP_PRINT(SDP_ERR, SDP_BUFFER_POOL, ("SendBuffer failed rc = 0x%x\n", rc )); + SDP_PRINT(SDP_ERR, SDP_BUFFER_POOL, ("AddBufferToQueuedList failed rc = 0x%x\n", rc )); goto Cleanup; } diff --git a/trunk/ulp/sdp/kernel/SdpRecvPool.cpp b/trunk/ulp/sdp/kernel/SdpRecvPool.cpp index a49e7529..eafa0a3d 100644 --- a/trunk/ulp/sdp/kernel/SdpRecvPool.cpp +++ b/trunk/ulp/sdp/kernel/SdpRecvPool.cpp @@ -51,8 +51,6 @@ RecvPool::Init( pBufferDescriptor->Reset(); m_FreePackets.InsertTailList(&pBufferDescriptor->BuffersList); } - m_FirstPostingOfBuffers = 0; - return STATUS_SUCCESS; } @@ -298,13 +296,6 @@ RecvPool::ReceiveIfCan() while (m_CurrentlyPostedRecievedBuffers < m_MaxConcurrentRecieves) { // do we have a free packet ? if (m_FreePackets.Size() > 0) { - //??????????? lET'S Sleep here for some time (FIND THE RACE) ?????????? - { - if (m_FirstPostingOfBuffers ==0 ) { - m_FirstPostingOfBuffers++; - Sleep(0xfffff); - } - } // we can take a packet from the list LIST_ENTRY *item = m_FreePackets.RemoveHeadList(); pBufferDescriptor = CONTAINING_RECORD(item, BufferDescriptor , BuffersList); diff --git a/trunk/ulp/sdp/kernel/SdpRecvPool.h b/trunk/ulp/sdp/kernel/SdpRecvPool.h index 625b94c6..cfc26740 100644 --- a/trunk/ulp/sdp/kernel/SdpRecvPool.h +++ b/trunk/ulp/sdp/kernel/SdpRecvPool.h @@ -90,8 +90,6 @@ private: // This signals that the remote side will not be sending any data any more bool m_DisConnRecieved; - int m_FirstPostingOfBuffers;//????????????????? USED TO HANDLE SOME RACE - VOID AssertLocked(); }; diff --git a/trunk/ulp/sdp/kernel/SdpSocket.cpp b/trunk/ulp/sdp/kernel/SdpSocket.cpp index b6837978..de3717bb 100644 --- a/trunk/ulp/sdp/kernel/SdpSocket.cpp +++ b/trunk/ulp/sdp/kernel/SdpSocket.cpp @@ -390,7 +390,7 @@ SdpSocket::WSPRecv( UserMode, FALSE, NULL - ); + ); pBuffersEvent = NULL; if (( rc == STATUS_ALERTED ) ||( rc == STATUS_USER_APC )) { // BUGBUG: Think what to do here, we should be able to stop the @@ -1225,7 +1225,7 @@ NTSTATUS SdpSocket::CmSendRTU() cm_rtu.pfn_cm_apr_cb = cm_apr_callback; cm_rtu.pfn_cm_dreq_cb = cm_dreq_callback; - + ib_status = ib_cm_rtu( m_cm_handle_t, &cm_rtu ); if( ib_status != IB_SUCCESS ) { SDP_PRINT(SDP_ERR, SDP_SOCKET, ("ib_cm_rtu failed ib_status = 0x%d\n", ib_status )); @@ -1253,7 +1253,7 @@ NTSTATUS SdpSocket::CmSendRTU() // We now start the recieve processing - rc = m_Lock.Lock(); + rc = m_Lock.Lock(); //?????????? if (!NT_SUCCESS(rc)) { SDP_PRINT(SDP_ERR, SDP_SOCKET, ("m_Lock.Lock failed rc = 0x%x\n", rc )); goto Cleanup; @@ -1312,7 +1312,7 @@ SdpSocket::CmReqCallback(IN ib_cm_req_rec_t *p_cm_req_rec) } // Take the lock and verify the state - rc = m_Lock.Lock(); + rc = m_Lock.Lock(); //??????? if (!NT_SUCCESS(rc)) { SDP_PRINT(SDP_ERR, SDP_SOCKET, ("m_Lock.Lock failed rc = 0x%x\n", rc )); goto Cleanup; @@ -1468,6 +1468,8 @@ SdpSocket::CmReqCallback(IN ib_cm_req_rec_t *p_cm_req_rec) sdp_msg_hello_ack hello_ack_msg; CreateHelloAckHeader(&hello_ack_msg); + pNewSocket->m_RecvBufferPool.SetLocaleAdvertisedBuffers(CL_NTOH16(hello_ack_msg.bsdh.recv_bufs)); + cm_rep.p_rep_pdata = (uint8_t *) &hello_ack_msg; cm_rep.rep_length = sizeof(hello_ack_msg); @@ -1497,7 +1499,7 @@ SdpSocket::CmReqCallback(IN ib_cm_req_rec_t *p_cm_req_rec) goto ErrorLocked; } - rc = pNewSocket->m_Lock.Lock(); + rc = pNewSocket->m_Lock.Lock(); //???????? if (!NT_SUCCESS(rc)) { SDP_PRINT(SDP_ERR, SDP_SOCKET, ("pNewSocket.Init() failed rc = 0x%x\n", rc )); goto ErrorLocked; @@ -1529,7 +1531,7 @@ SdpSocket::CmReqCallback(IN ib_cm_req_rec_t *p_cm_req_rec) // Sucess - we can now release the lock and update our state pNewSocket->m_state = SS_REP_SENT; - rc = pNewSocket->m_Lock.Unlock(); // Error is ignored, since this is already an error path + rc = pNewSocket->m_Lock.Unlock(); // ???????? Error is ignored, since this is already an error path if (!NT_SUCCESS(rc)) { SDP_PRINT(SDP_ERR, SDP_SOCKET, ("pNewSocket->m_Lock.Unlock() failed rc = 0x%x\n", rc )); // BUGBUG: who is responsibale for the cleanup ??????? @@ -1575,7 +1577,7 @@ SdpSocket::CmRtuCallback(IN ib_cm_rtu_rec_t *p_cm_rtu_rec) // Take the lock and verify the state - rc = m_Lock.Lock(); + rc = m_Lock.Lock(); //????? if (!NT_SUCCESS(rc)) { SDP_PRINT(SDP_ERR, SDP_SOCKET, ("m_Lock.Lock failed rc = 0x%x\n", rc )); goto Cleanup; @@ -1597,7 +1599,7 @@ SdpSocket::CmRtuCallback(IN ib_cm_rtu_rec_t *p_cm_rtu_rec) } // Next step is to move the new socket to the SS_CONNECTED state - rc = pSocket->m_Lock.Lock(); + rc = pSocket->m_Lock.Lock(); //??????? if (!NT_SUCCESS(rc)) { SDP_PRINT(SDP_ERR, SDP_SOCKET, ("m_Lock.Lock failed rc = 0x%x\n", rc )); goto ErrorLocked; @@ -1839,6 +1841,7 @@ SdpSocket::__send_cb1( IN void *cq_context ) { SdpSocket *pSocket = (SdpSocket *) cq_context; + SDP_PRINT(SDP_DEBUG, SDP_SOCKET, ("called this = 0x%x\n", pSocket )); pSocket->m_Lock.SignalCB(SEND_CB_CALLED); } @@ -2067,19 +2070,19 @@ NTSTATUS SdpSocket::CreateQp() rc = IB2Status(ib_status); goto Cleanup; } -#if 0 +#if DBG /* Query the QP so we can get our QPN. */ - status = p_port->p_adapter->p_ifc->query_qp( - p_port->ib_mgr.h_qp, &qp_attr ); - if( status != IB_SUCCESS ) + ib_qp_attr_t qp_attr; + ib_status = ib_query_qp( + m_qp, &qp_attr ); + if( ib_status != IB_SUCCESS ) { - IPOIB_TRACE_EXIT( IPOIB_DBG_ERROR, - ("ib_query_qp returned %s\n", - p_port->p_adapter->p_ifc->get_err_str( status )) ); - return status; + SDP_PRINT(SDP_ERR, SDP_SOCKET, ("ib_query_qp failed ib_status = 0x%d\n", ib_status )); + // ignore the error, this is only needed for debug } - p_port->ib_mgr.qpn = qp_attr.num; -#endif + SDP_PRINT(SDP_TRACE, SDP_SOCKET,("QP number is %x\n", CL_NTOH32(qp_attr.num))); +#endif + const net64_t MEM_REG_SIZE = 0xFFFFFFFFFFFFFFFF; /* Register all of physical memory */ phys_create.length = MEM_REG_SIZE; diff --git a/trunk/ulp/sdp/kernel/SdpSocket.h b/trunk/ulp/sdp/kernel/SdpSocket.h index 28136ddf..15c5c9ce 100644 --- a/trunk/ulp/sdp/kernel/SdpSocket.h +++ b/trunk/ulp/sdp/kernel/SdpSocket.h @@ -63,6 +63,7 @@ class SdpSocket : public RefCountImpl { friend void cm_rtu_callback(IN ib_cm_rtu_rec_t *p_cm_rtu_rec ); friend class ConnectionList; + private: SocketStates m_state; @@ -212,7 +213,8 @@ public: VOID CloseSocketThread(); VOID DisconectSentEvent(); - + + SocketStates GetState() {return m_state;}; // Two varibales that are needed for passing REP data struct sdp_msg_hello_ack m_hello_ack; @@ -225,7 +227,8 @@ public: LIST_ENTRY m_UserFileList; #if DBG - char * SS2String(SocketStates state) { + + static char * SS2String(SocketStates state) { switch (state) { case SS_IDLE : return "SS_IDLE"; case SS_CONNECTING_QPR_SENT : return "SS_CONNECTING_QPR_SENT"; diff --git a/trunk/ulp/sdp/kernel/SdpTrace.cpp b/trunk/ulp/sdp/kernel/SdpTrace.cpp index 6a7724d4..bd7da2ab 100644 --- a/trunk/ulp/sdp/kernel/SdpTrace.cpp +++ b/trunk/ulp/sdp/kernel/SdpTrace.cpp @@ -3,10 +3,13 @@ #include "Precompile.h" +//int g_SdpDbgLevel = SDP_WARN; +//int g_SdpDbgLevel = SDP_TRACE; +int g_SdpDbgLevel = SDP_DEBUG; + BOOLEAN CheckCondition(int sev, int top, char *file, int line, char * func) { - if (sev < SDP_TRACE) return FALSE; -// if (sev < SDP_WARN) return FALSE; + if (sev < g_SdpDbgLevel) return FALSE; if (top == SDP_PERFORMANCE) return FALSE; DbgPrint ("%s: ", func); diff --git a/trunk/ulp/sdp/todo b/trunk/ulp/sdp/todo index b5bb951a..2d455f72 100644 --- a/trunk/ulp/sdp/todo +++ b/trunk/ulp/sdp/todo @@ -28,6 +28,8 @@ general: Find a better solution for threading and remove the current solution. Fix the race of sdp user file + + Fix the lock implmentation to have also a void implmentation as well as an RC implmentation USER MODE: