buglwIP - A Lightweight TCP/IP stack - Bugs: bug #34427, In RAW/NO_SYS mode, tcp_output may...

 
 

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

bug #34427: In RAW/NO_SYS mode, tcp_output may send out large number of ARQ REQ if destination ARP entry has expired

Submitter:  Min Xu <maximalmin>
Submitted:  Thu 29 Sep 2011 12:45:27 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Change Request Status:  Duplicate
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.0

Jump to the original submission

Sun 13 Nov 2011 04:15:08 PM UTC, comment #8: 


> I'm not sure if it fixes the problem of this bug


I think it does (as comment #7 also says).

Closing this as Duplicate (see bug #34682 and bug #34681).


> At the moment I'm not sure [..] how resending the ARP requests
> reduces the number of ARP requests sent.


By not re-sending from the output function (when we already sent a request and are waiting for the response) but from the timer function, the number of requests doesn't depend on the number of packets sent but is predictable.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 29 Oct 2011 09:09:18 AM UTC, comment #7: 

I have split the patch in comment #5 for two bugs reports

https://savannah.nongnu.org/bugs/index.php?34682 which fixes this bug in question directly.

and,
https://savannah.nongnu.org/bugs/index.php?34681 which protects each queue from exhaustion by other arp entries.

by patching both, The ARP layer is protected from flooding & exhaustion.

Amir Shalem <amirshalem>
Fri 28 Oct 2011 12:29:57 PM UTC, comment #6: 

The patch in comment #5 is a bit confusing.  It seems to be addressing a number of different issues, and I'm not sure if it fixes the problem of this bug, which is specifically about sending large numbers of ARPs when the destination entry has expired.

I suggest splitting the patch in comment #5 and raising a new bug for each change that is being made.  E.g. changing the ARP_TMR_INTERVAL, reducing MAXAGE from 20 minutes to 120 seconds, resending ARP requests, and so on.

At the moment I'm not sure what problem changing the constants fixes, or how resending the ARP requests reduces the number of ARP requests sent.

Kieran Mansley <kieranm>
Group Member
Sat 22 Oct 2011 10:17:17 AM UTC, comment #5: 

I'm attaching a patch for this bug,
I fix two bugs in ARP layer:

  • re-sending arp requests, every 1second, until entry is stable or purge out (ARP_MAXPENDING)

   arp requests are sent only when there's a new timer, or on etharp_tmr

  • maximum packets in queue, up to 3 packets, until entry is stable

   in case someone is sending lots of packets without the entry resolved,
   makes sure we don't run out of MEMP_ARP_QUEUE for sending packets to other IPs.

patch included.

(file #24184)

Amir Shalem <amirshalem>
Thu 29 Sep 2011 09:07:43 PM UTC, comment #4: 

Forgot the attachment

(file #24051)

Min Xu <maximalmin>
Thu 29 Sep 2011 09:03:17 PM UTC, comment #3: 

I ran a test with arp queuing turned on (ETHARP_TRUST_IP_MAC = 0, ARP_QUEUEING = 1), it doesn't appear to resolve the issue of single vs multiple ARP.  However, it does behave differently in that immediately after the ARP flood, the sequence is continuous from the last transmission by the DSP (before ARP flood, desktop relative-ACKd 10221, after ARP flood, DSP started transmit at 10221).   BUT.. after a short while, the DSP jumped a bunch of sequence number (at packet 141 in the attached pcap file), skipping 17520 bytes (12 packets of 1460 byte payload)...   I didn't observe this issue in the build where ARP QUEUING is turned off.

As a side note, the DSP main loop polls the drive receive function (to see if there are packets already received by DMA/Interrupt) with a priority of 4096 -- it polls the receive function 4096 times, until the polling function returns false, indicating there are no packets [whole or broken..].  So if the LwIP stack spends a lot of time in the tcp_output routine (to write out to the ethernet, arp or tcp packets) it won't be able to processing incoming ARP response and/or ACK / DupACK messages until it's done.  So for performance reason, I think if it will send out ARP REQ for each of the packet that it will build, calculate crc etc for, it's better to return early and let the receive function run for a little while, that's why I think having tcp_segment_output forward the transmit status of ERR_ARPREQ would probably be a sufficient solution, but I do agree further limiting the ARP REQ going out will help, perhaps by having a different meaning to the age field in ARP table, one nibble being a timer count, one being an "age" count...


Min Xu <maximalmin>
Thu 29 Sep 2011 08:23:52 PM UTC, comment #2: 

Just a note: winsock delays send() until an ARP request succeeds or times out (which it does in 3 seconds, it is not retransmitted).

I'd like to test unix/linux, too, but decreasing the ARP timer to 3 seconds and not sending more than 1 request per IP in 3 seconds is probably the way I'll fix this (aside from any TCP/ARP interop improvements).

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Sep 2011 06:03:22 AM UTC, comment #1: 

We should at least limit the number of ARP requests to 1 per second or so (I'll check the RFCs for a value), to prevent ARP flooding (a general problem, not only for TCP). Unfortunately, we only have a 5-second timer in etharp.c (which is too long for retransmissions, I guess), so we might need to decrease that timer.

As an optimization, making TCP aware of the ARP status (in a more general form) seems like a good idea to improve both throughput and resource usage (prevent packets being buffered by TCP and ARP). However, this would mean the TCP code would have to poll for success (in tcp_fasttmr), which would mean quite an overhead.

Doesn't enabling ARP_QUEUEING help you (if the number of ARP requests per second was limited)? That way, all original packets would be sent when the response comes in.

BTW: The problem of ARP entries timing out although in frequent use is solved in git already, so your problem only arises when you have a long time of silence on a TCP connection without closing it - a new TCP connection doesn't lead to ARP flooding since only the SYN is transmitted.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Sep 2011 12:45:27 AM UTC, original submission:  

This is related to bug #34426.  So the next two paragraphs are quoted from that bug report.

We have developed a driver for an integrated EMAC module on the TI DSP that can achieve a high throughput using RAW / NO_SYS mode. In the following description, DSP application is the software that is based on the LwIP stack and the Desktop application is a Win32 application.

In order to maximize that throughput, we increased the receive buffer size on the desktop application to 65535 (max). When the DSP application sends out data, it first check the send buffer size available (by calling tcp_sndbuf). When the returned value is large, the DSP application calls tcp_write (with TCP_WRITE_FLAG_COPY) with a large payload. The problem however, is that when the arp entry for the desktop has expired (either because ETHARP_TRUST_IP_MAC was defined as 0, --OR-- MORE importantly, when there is a lack of network activity for a while, and the desktop then initiates another transfer), each of the packet that would've been sent out is then transmitted as an ARP REQ (for desktop's IP address).

What I like to have changed is that in RAW/NO_SYS (no threads) mode, tcp_output should be able to check the eventual return code of etharp_output and that etharp_output should return a new status code like ERR_ARPREQ to indicate that the packet was not sent, but instead an ARP REQ was sent.  That way, tcp_output will leave the packet untouched (e.g. not remove it from unsent list) and return early.  Then the main program loop will get a chance to POLL the driver and process incoming packets and update the ARP table as necessary, instead of a multitude of ARP REQ being sent out.

To accomplish this, I believe tcp_segment_output needs to be modified to return a value, and that as indicated earlier, a new status value should be added, and returned at the end of the etharp_output function.

-----

On a somewhat unrelated note, when I was implementing the driver to have a large receive buffer that's friendly to the CACHE (128 byte aligned), I found it necessary to:

1: not use pbuf mempool: because cache needs to be turned on for performance, and because the cache is 128 bytes aligned, and because the cache lines containing the receive buffer must be invalidated since data is DMA transferred outside of CPU

2: pbuf structs (actually PBUF_ROM is the type I used on received packet) must add a new field to store the original packet buffer because ICMP PORT NOT REACHABLE calls pbuf_header to traverse back the header in udp, and pbuf_header would normally fail on PBUF_ROM types.  -- this is the same as suggested by Marc Wells  http://lists.gnu.org/archive/html/lwip-users/2003-03/msg00085.html

3: I upgraded to ver 1.4.0 (from 1.3.1) because I thought there was a PBUF_CUSTOM type that allows me to call a routine when pbuf_free actually free'd the buffer, but it turns out this code is written on certain compilation condition and that it's not normally invoked on any pbuf types.  The reason I need this is because when the driver has given a "packet" to the stack, the data in the payload must not change.  However, on high throughput, it is possible that the underlying receive buffer "slot" gets used again.  So to remedy that, I mark the slot as used when ever a PBUF_ROM structure is linked to it in the driver.  And when the pbuf_free is called, this slot is then marked as free and allowed to be given to the underlying EMAC module in its receive DMA.

So I believe that it would be nice if the LwIP stack has a conditional flag that would add these two fields (if others find that an additional 2 words would cause extra bloat in) pbuf_struct.

Best Regards

Min Xu <maximalmin>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #24184:  arp-queuing.patch added by amirshalem (6KiB - application/octet-stream - arp queuing patch)
file #24051:  example-arp-queuing.pcap added by maximalmin (147KiB - application/cap)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by amirshalem (Updated the item)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by maximalmin (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-11-13 goldsimon StatusNone Duplicate
        Open/ClosedOpen Closed
    2011-10-22 amirshalem Attached File- Added arp-queuing.patch, #24184
    2011-09-29 maximalmin Attached File- Added example-arp-queuing.pcap, #24051

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code