[ofw] [IPoIB] NdisMSendNetBufferListCompleteX assert

[Smith, Stan]

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 in 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:xalex at mellanox.co.il]
Sent: Tuesday, March 01, 2011 12:42 AM
To: Smith, Stan; Fab Tillier; ofw at lists.openfabrics.org
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: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Smith, Stan
Sent: Tuesday, March 01, 2011 3:42 AM
To: Fab Tillier; ofw at lists.openfabrics.org
Subject: Re: [ofw] [IPoIB] NdisMSendNetBufferListCompleteX assert

>-----Original Message-----
>From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Fab
>Tillier
>Sent: Monday, February 28, 2011 4:46 PM
>To: ofw at lists.openfabrics.org
>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
>ofw at lists.openfabrics.org
>http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
_______________________________________________
ofw mailing list
ofw at lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20110301/130c9ca6/attachment.html>