K64F: Memory leak in network driver

[infinnovation-dev]

Description

• Type: Bug
• Related issue: TCPSocket stream stops sending/receiving after a while #2553
• Priority: ?

---

Bug

Target
K64F

Toolchain:
GCC_ARM

mbed-os sha:
de41409

Expected behaviour
The ability to continue send large (1400-byte) UDP packets

Actual behaviour
After sending two bursts of packets, I am unable to send any more

Steps to reproduce
See demonstration program and description below.

Analysis

While investigating another issue, I was reading the code of
k64f_emac.c when I came across the following stanza:

  /* Wait until a descriptor is available for the transfer. */
  /* THIS WILL BLOCK UNTIL THERE ARE A DESCRIPTOR AVAILABLE */
  while (g_handle.txBdCurrent->control & ENET_BUFFDESCRIPTOR_TX_READY_MASK)
    osSemaphoreWait(k64f_enet->xTXDCountSem.id, osWaitForever);

This looks reasonable on the face of it, until you find that the only other reference to xTXDCountSem is its creation i.e nowhere is there a matching osSemaphoreRelease. I haven't fully examined the workings of the 8-item ring buffer, but it seems that this supposed safeguard against over-filling the ring buffer is not being exercised.

Consequently, by sending packets to the driver fast enough, and before the tx_clean thread has had a chance to free buffers already sent by the ENET device, it is possible to overwrite buffer pointers, resulting in a memory leak.

The bug is partially masked by the quite tight timings: the 120MHz CPU is hard pushed to generate packets faster than the ENET can transmit them on a 100Mbps link (given the several layers of software stack between user code and driver). It is also partially masked by the memory pool used by the ring buffer, into which the packets are copied (trying to write too many large packets fails at this stage as there is only space for 8 full-sized tx frames and the 16 rx frames already allocated).

However, I have managed to create a demonstration program which illustrates the misbehaviour.

The program repeatedly sends a burst of 8 1400-byte UDP packets and 12 20-byte packets. What we see is that after the second burst, we are unable to send any further 1400-byte packet. lwip's statistics show the heap memory pretty much all used.

Sample output:

k64f-net-leak

MEM HEAP
        avail: 36784
        used: 25096
        max: 27440
        err: 0
big=8 small=12

MEM HEAP
        avail: 36784
        used: 25096
        max: 28284
        err: 0
big=7 small=12

MEM HEAP
        avail: 36784
        used: 36432
        max: 36708
        err: 1
big=0 small=2

MEM HEAP
        avail: 36784
        used: 36432
        max: 36708
        err: 3
big=0 small=2

MEM HEAP
        avail: 36784
        used: 36432
        max: 36708
        err: 5
big=0 small=2

MEM HEAP
        avail: 36784
        used: 36432
        max: 36708
        err: 7
big=0 small=2

MEM HEAP
        avail: 36784
        used: 36432
        max: 36708
        err: 9
big=0 small=2

MEM HEAP
        avail: 36784
        used: 36432
        max: 36708
        err: 11
big=0 small=4

MEM HEAP
        avail: 36784
        used: 36432
        max: 36708
        err: 13
big=0 small=4

Note that I was not able to reproduce the failure mode with mbed-os 5.1. There don't appear to be any relevant changes to k64f_emac.c between 5.1 and 5.2, so I suspect it is some subtlety of the timing.

I have a suspicion that the bug may be the cause of #2553. The failure mode (packets not being sent) seems to fit the observed symptoms.

[0xc0170]

cc @geky @kjbracey-arm @mmahadevan108 @bogdanm

[bogdanm]

@infinnovation, can you please check if #3124 fixes this issue?

[infinnovation-dev]

No, it doesn't fix the issue. The change may be part of a solution, but my no means all of it.

One of the problems is that the test in the "while" loop doesn't work - I haven't investigated why. If it did work, it would just loop obtaining all 8 semaphore tokens and then block forever.

I can think of two possible ways to implement the buffering mechanism.

The first uses the semaphore tokens as surrogates for buffer slots. The output task then always does an osSemaphoreWait and then can use the slot without needing to check the ready bit. The cleaner task does an osSemaphoreRelease whenever it frees up a buffer slot.

The second implements a count of used buffers and a condition variable meaning "buffer not full". Fairly standard practice for a threaded buffer producer/consumer pattern.

The code as is stands seems to be a non-functional mish-mash of the two approaches.

[bogdanm]

Interesting. All I did with this change was mirror the corresponding code from the LPC1768 driver (on which the K64F one is based). It seems that the issue is more than just a missing semaphore release though.
The test in the while loop should work at least to some extent, that much we know, because that while loop was problematic for a different reason in the past and we've fixed it. I'll keep on investigating.

[kjbracey]

The Nanostack version of the driver got tightened up a lot and stress tested by @hasnainvirk. I do recall the LWIP one(s) that it was originally derived from being a bit of a mess.

He mainly used lots of flood pinging from other devices using large and fragmented datagrams.

You may want to see how that version ended up:

https://github.com/ARMmbed/sal-nanostack-driver-k64f-eth

[infinnovation-dev]

I guess there are two potential pitfalls: (1) re-using a slot which hasn't been sent yet; (2) re-using a slot which has been sent (ready bit cleared) but whose buffer has not been reclaimed. Maybe I've only fallen into pit (2) due to tight timing. If I had time I'd be tempted to dig out that 10Mbps hub from my box of not-quite-WEEE-yet...

[geky]

Wow! Thanks for putting in the work to track this down! Sorry I couldn't get to it earlier, but the effort put in here led straight to a small hive of bugs.

Other than the confusion around the semaphore usage, the biggest problem was how the cyclic-buffer dealt with tx_consume_index/tx_produce_index equality with overflow. Equal indexes meant both empty and full buffers, which meant if the buffer caught up with itself if could forget a whole buffer of pbufs.

I've put up a pr that hopefully squashes all of them, let me know if you all spot any issues: #3135

[0xc0170]

I believe issue 3135 fixed the issue, @infinnovation if that's the case, please close this