Alex, One could fix the g_NBL accounting in ipoib_driver.cpp by counting the number of NBLs being completed, subtract 1 from the total (NdisMSendNetBufferListsCompleteX does ++) and then add total to g_NBL; not pretty although correct and debug only.
The completion logic is, if NDIS_STATUS_SUCCESS, then allow only one NBL to be completed; if status != NDIS_STATUS_SUCCESS, then allow 1 or more NBLs to be completed. A comment, per Fab's recommendation, would be good. Stan. From: [email protected] [mailto:[email protected]] On Behalf Of Smith, Stan Sent: Tuesday, March 01, 2011 5:41 PM To: Alex Naslednikov; Fab Tillier; [email protected] Subject: Re: [ofw] [IPoIB] NdisMSendNetBufferListCompleteX assert Alex, I like your patch, in that a single call to NDIS is made. I do not see the path to keeping g_NBL correct without a for loop? Below is a slight modification, such that 'Completing NBL....' message is not output two times and the NDIS error code is output in hex to make it easier to find in ndis.h #if DBG if (NET_BUFFER_LIST_STATUS(NetBufferLists) != NDIS_STATUS_SUCCESS) { IPOIB_PRINT( TRACE_LEVEL_INFORMATION, IPOIB_DBG_ALL, ("NBL completed with error %#x to NDIS\n", NET_BUFFER_LIST_STATUS(NetBufferLists))); } else { ASSERT(NET_BUFFER_LIST_NEXT_NBL(NetBufferLists) == NULL); IPOIB_PRINT( TRACE_LEVEL_VERBOSE, IPOIB_DBG_SEND, ("Completing NBL=%x, g_NBL=%d, g_NBL_completed=%d \n", NetBufferLists, g_NBL, g_NBL_complete) ); } #endif From: Alex Naslednikov [mailto:[email protected]] Sent: Tuesday, March 01, 2011 12:42 AM To: Smith, Stan; Fab Tillier; [email protected] Subject: RE: [IPoIB] NdisMSendNetBufferListCompleteX assert Hello Stan, You are right - we have the following code for the regular flow (prior calling to ipoib_port_send()): // Important issue, break the connection between the different nbls NET_BUFFER_LIST_NEXT_NBL(curr_net_buffer_list) = NULL; Of course, when we get into bad flow, the assertion mentioned by Fab is no longer valid. Thus, to avoid big changes, we can simply modify the code of NdisMSendNetBufferListsCompleteX in a following manner: #if DBG if (NET_BUFFER_LIST_STATUS(NetBufferLists) != NDIS_STATUS_SUCCESS) { IPOIB_PRINT( TRACE_LEVEL_INFORMATION, IPOIB_DBG_ALL, ("NBL completed with error %d to NDIS\n", NET_BUFFER_LIST_STATUS(NetBufferLists))); } else { ASSERT(NET_BUFFER_LIST_NEXT_NBL(NetBufferLists) == NULL); } IPOIB_PRINT( TRACE_LEVEL_VERBOSE, IPOIB_DBG_SEND, ("Completing NBL=%x, g_NBL=%d, g_NBL_completed=%d \n", NetBufferLists, g_NBL, g_NBL_complete) ); #endif In addition, we should take into account that g_NBL was increased by an actual numbers of NBLs inside the list. Thus, g_NBL_completed should be increased accordingly (and not only once) The other possibility was proposed by Stan. In this case the change will be even smaller, but we will call to NDIS several times instead of one call only. I am ok with both of them From: [email protected] [mailto:[email protected]] On Behalf Of Smith, Stan Sent: Tuesday, March 01, 2011 3:42 AM To: Fab Tillier; [email protected] Subject: Re: [ofw] [IPoIB] NdisMSendNetBufferListCompleteX assert >-----Original Message----- >From: [email protected] >[mailto:[email protected]] On Behalf Of Fab >Tillier >Sent: Monday, February 28, 2011 4:46 PM >To: [email protected] >Subject: [ofw] [IPoIB] NdisMSendNetBufferListCompleteX assert > >The assertion at line 1122 in ipoib_port.h seems wrong since the following >code dereferences the NBL. >Is it wrong, or there to cause an assert because of a send error? > >If the latter, a comment would help make the intent clear, as the code looks >wrong as is. > >-Fab NdisMSendNetBufferListsCompleteX() is completing a 'single' NBL (NetworkBufferList); not a list of NBLs. The code is correctly used in ipoib_port.cpp @ ipoib_port_send(). Although it's usage in ipoib_driver.cpp @ ipoib_send_net_buffer_list() after label 'compl_status:' is incorrect. Appears the comment is wrong w.r.t. NdisMSendNetBufferListsCompleteX() and the for() loop should be extended down to encompass the NdisMSendNetBufferListsCompleteX() call with a single NBL. The dispatch check should be done once prior to entering the for()loop. Tzachi, Alex - do you agree? Stan. >_______________________________________________ >ofw mailing list >[email protected] >http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
_______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
