[patch] PPP receive buffer fragmentation issues

[GoogleCodeExporter]

The in-flight PPP receive buffer is allocated only once, to the largest 
fragment that is available at the time. That fragment may not be sufficient to 
receive a large PPP frame, resulting in an overflow. On overflow, the current 
receive buffer *content* is discarded, and the receive buffer is reused for the 
next frame, without re-allocation, regardless of whether a larger fragment has 
become available --- meaning it may still be insufficient.

On reception of a complete PPP frame, the used portion of the in-flight buffer 
is frozen, and the remaining portion made available; At that point, a new 
in-flight buffer is allocated, *before* the received frame has been processed, 
and thus before it has been free()ed and the memory it occupies becomes 
available again. This results in fragmentation.

This patch changes the behavior to aggressively re-allocate the in-flight 
receive buffer whenever a larger fragment becomes available, avoiding this 
failure condition. We've been running this internally for a few months now and 
are satisfied with the stability.

Original issue reported on code.google.com by jmatts...@dius.com.au on 25 Nov 2011 at 5:33

Attachments:

• tinyos-ppp-fragmentation.patch

[GoogleCodeExporter]

I noticed this behavior too, which causes you to stop receiving large packets 
after a while.  This patch seems like the real fix; what I did in r5799 was 
change the number of receive fragments to 1.  It didn't seem like there was a 
lot of parallelism happening there anyways, so the performance on the msp 
platform seemed fine after I made this change, but this seems like the "right" 
fix; I was just lazy.

Original comment by sdh...@gmail.com on 25 Nov 2011 at 5:36

[GoogleCodeExporter]

The complexity of this patch is beyond what I can support given that I have no 
funding for TinyOS development or enhancements.  The FragmentPool state machine 
is extremely complex; some of the changes appear to affect it, but the unit 
tests have not been updated to validate those changes.

Sorry.  Stick with Steve's existing fix for the problem, or merge this and let 
somebody take over maintenance.  (Or find about 30 hours of funding for me, 
that being what I'd estimate it'd take to refactor the patch and supply the 
necessary tests to validate the new functionality.)

Original comment by pabi...@gmail.com on 25 Nov 2011 at 6:58

[GoogleCodeExporter]

Okay, that's understandable, Peter.  

Since my one-character fix completely addresses the problem as far as I can 
tell, we'll go with that for 2.1.2 and leave this on the back-burner.  Another 
possibility is to just eliminate the fragment pool on the receive side, since 
I'm not sure it's doing much good -- it certainly isn't if there's only one 
fragment.  The only way you could have parallelism in the current stack is if a 
second packet beings arriving while another one is being processed, which could 
happen.  Since the application must either copy or discard the fragment buffer, 
though, it can only hang onto the buffer while it's in the receive handler and 
so I'm not sure it's really worth it.

Original comment by sdh...@gmail.com on 27 Nov 2011 at 7:43

• Added labels: Priority-Low
• Removed labels: Priority-Medium

[GoogleCodeExporter]

I'm fairly certain I ran into cases where there were multiple HDLC frames 
actively being processed on the receive side: in concept the framework was 
intended to permit an arbitrary delay before the receiver completes processing. 
 That behavior may only arise during protocol negotiation or when there are 
multiple protocols active, though, and may not happen the way you're using it 
today.

Original comment by pabi...@gmail.com on 27 Nov 2011 at 8:01

[GoogleCodeExporter]

Sorry, I hadn't noticed there was a test app sitting in fragpool. I'll see if 
we can get a bit of time here to update it. Quite possibly not, but I'll see 
what happens. I wouldn't mind having said tests even if you don't accept this 
patch.

Original comment by jmatts...@dius.com.au on 27 Nov 2011 at 9:53

[GoogleCodeExporter]

I'm closing this for now.  We can reopen it in the future but I'm satisfied 
with the workaround for 2.1.2. 

Original comment by sdh...@gmail.com on 11 Jan 2012 at 4:52

• Changed state: WontFix