buglwIP - A Lightweight TCP/IP stack - Bugs: bug #50064, Zero-copy RX: ARP reply fails with...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #50064: Zero-copy RX: ARP reply fails with PBUF_REF

Submitted by:  Daniel Elstner <danielk>
Submitted on:  Tue 17 Jan 2017 10:27:19 AM UTC  
 
Category: ARPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Dirk Ziegelmeier <dziegel>
Open/Closed: ClosedPlanned Release: 2.0.2
lwIP version: 2.0.0

(Jump to the original submission Jump to the original submission)

Wed 18 Jan 2017 01:04:05 PM UTC, comment #10:

Thanks for reporting and testing!

Dirk Ziegelmeier <dziegel>
Project MemberIn charge of this item.
Wed 18 Jan 2017 12:49:05 PM UTC, comment #9:

Now it works, great. Thanks a lot!

Daniel Elstner <danielk>
Wed 18 Jan 2017 12:44:55 PM UTC, comment #8:

Sorry, used the wrong destination HW addr.

http://git.savannah.gnu.org/cgit/lwip.git/commit/?id=645ca84704db70f6dd29aefdec7bfbf6d9fc01f4

Dirk Ziegelmeier <dziegel>
Project MemberIn charge of this item.
Wed 18 Jan 2017 12:34:44 PM UTC, comment #7:

Just tested it and it doesn't work. Looking at Wireshark logs I can see ARP replies, but the Ethernet destination address is set to 00:00:00:00:00:00 so they end up nowhere.

Daniel Elstner <danielk>
Wed 18 Jan 2017 11:25:47 AM UTC, comment #6:

I also vote to allocate a new PBUF

Dirk Ziegelmeier <dziegel>
Project MemberIn charge of this item.
Wed 18 Jan 2017 10:50:39 AM UTC, comment #5:

No, its OK, etharp_raw() also has the AUTOIP special case built-in.

Dirk Ziegelmeier <dziegel>
Project MemberIn charge of this item.
Wed 18 Jan 2017 10:11:32 AM UTC, comment #4:

Doesn't the fix break the LWIP_AUTOIP speciality of sending the response as broadcast?

I'd vote for allocating a new pbuf for the response instead of adding a speciality for PBUF_REF.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 18 Jan 2017 09:16:03 AM UTC, comment #3:

I pushed a fix, can you please test if it works for you, Daniel?

Dirk Ziegelmeier <dziegel>
Project MemberIn charge of this item.
Wed 18 Jan 2017 08:51:00 AM UTC, comment #2:

Alternative 1: Removie the special case of reusing the received pbuf in etharp_input() and call etharp_raw() to create ARP response. This is less performant, but saves some code bytes. Is there a reason for optimizing the ARP reply case?

Alternative 2: Use an if clause to detect PBUF_REFs and only use etharp_raw() in this case, keeping the performance optimization for the PBUF_POOL case.

Any opinions?

Dirk Ziegelmeier <dziegel>
Project MemberIn charge of this item.
Tue 17 Jan 2017 11:16:41 AM UTC, comment #1:

I messed it up in commit 31778193da09a4394749ed3d5cc20aa35c418b1f

Work on bug #48868: Cleanup etharp.c not to use struct eth_hdr: Cleanup etharp_input() to use ethernet_output()

Dirk Ziegelmeier <dziegel>
Project MemberIn charge of this item.
Tue 17 Jan 2017 10:27:19 AM UTC, original submission:

After implementing zero-copy DMA in my Ethernet driver with custom PBUF_REF pbufs, ARP replies stopped working.

It turns out that etharp_input() re-uses the incoming pbuf for ARP replies. However, this leads to an error later in ethernet_output() because pbuf_header() refuses to adjust the header size for PBUF_REF pbufs.

As a quick hack, I replaced the pbuf_header() call in ethernet_output() by pbuf_header_force() and it worked. However, this essentially removes the error checking for pbufs from any other source, so it is not a proper solution. Also, it relies on the Ethernet driver to implicitly copy PBUF_REF pbufs to avoid potentially deadlocking the input queue.

So I think it would be best if etharp_input() itself would create the new pbuf. Either always (simplicity), or only for PBUF_REF if the small allocation is a concern.

Daniel Elstner <danielk>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by dziegel (Posted a comment)
  • -unavailable- added by danielk (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 18 Jan 2017 01:04:05 PM UTCdziegelStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Wed 18 Jan 2017 11:25:47 AM UTCdziegelAssigned toNone=>dziegel
    Wed 18 Jan 2017 09:16:03 AM UTCdziegelStatusNone=>Ready For Test
      Planned ReleaseNone=>2.0.2
    Tue 17 Jan 2017 11:19:25 AM UTCdziegelSummaryARP reply fails with PBUF_REF=>Zero-copy RX: ARP reply fails with PBUF_REF

    Back to the top


    Powered by Savane 3.1-cleanup1