buglwIP - A Lightweight TCP/IP stack - Bugs: bug #46289, tcp_write fails with ERR_MEM if...

 
 

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

bug #46289: tcp_write fails with ERR_MEM if TCP_SND_QUEUELEN is reached

Submitter:  Ambroz Bizjak <abizjak>
Submitted:  Sun 25 Oct 2015 08:23:14 AM UTC
   
 
Category:  TCP Severity:  1 - Wish
Item Group:  Feature Request Status:  Invalid
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Sun 06 Mar 2016 11:46:55 PM UTC, comment #13: 

Yes, sorry, PBUF_POOL is meant to be used for RX packets :-)

From what I can tell, you are doing things well. I am not sure the PBUF_POOL problem when used with both TX and RX paths is documented somewhere, especially for our users without heap (i.e. without PBUF_RAM). What is really missing in lwIP is a PBUF_POOL_RX and a PBUF_POOL_TX to prevent this condition to happen.

Sylvain Rochet <gradator>
Group Member
Sat 05 Mar 2016 01:59:14 PM UTC, comment #12: 


> This allows me to pass packets from the RX ring buffer of the MAC driver directly into the stack.


Yes I know you're gonna say the "right" way is to implement vectored RX :)

Ambroz Bizjak <abizjak>
Sat 05 Mar 2016 01:56:17 PM UTC, comment #11: 

Hey,

I was being very careful with the buffer management. From what I can tell, PBUF_POOL is meant to be used for RX packets (not TX). But I reversed it, using it for TX in the various places that previously used PBUF_RAM, and for RX, I use "temporary" pbufs (PBUF_REF) [1]. I am 100% sure this is safe because they are never queued by the stack (in my the current configuration). This allows me to pass packets from the RX ring buffer of the MAC driver directly into the stack.

> The problem is, if your TX and RX path share the same pool, you can reach a condition where all your buffers are used, most commonly with unACKed TCP data but you are not able anymore to receive TCP ACK anymore to be able to free some buffers.


I am fully aware of that. A solution is to configure/correct stuff so that this can never happen. In my case, PBUF_POOL pbufs will only be used for "temporaty" pbufs that are created before being send out for transmission. The netif driver will free them immediately, so all I need is sufficient space in the pool for the longest possible pbuf. In more complex cases one could rely on limits such as the max number of pbufs queued for transmission by the driver, the max number of received pbufs waiting for reassembly...

Note, another change I've done is implemented a new PBUF_TCP type of pbufs that are used specifically for sending TCP headers. It uses a pool of pbufs that are specifically sized for this. Of course, I've taken great care to configure the limits (e.g. TCP_SND_QUEUELEN) so that we never run out of pbufs in this pool

Generally I really strive for predictability and assurances in the buffer allocations. It should be possible to configure the stack such that you never run out of space for anything, and specifically such that one connection cannot starve other connections of memory.

[1] https://github.com/ambrop72/aprinter/blob/web-interface/aprinter/net/LwipNetwork.h#L1241

Ambroz Bizjak <abizjak>
Sat 05 Mar 2016 01:12:18 PM UTC, comment #10: 

He is merging changes from the lwIP master branch, by breaking all commit authorship so it isn't very clear at first sight. The question is, how long someone could keep up doing that given the high commit rate on lwIP. I even have trouble myself following everything that's happening.

Sylvain Rochet <gradator>
Group Member
Sat 05 Mar 2016 01:02:43 PM UTC, comment #9: 

Forking lwIP is not a good idea in the first place, as you risk getting new bugs/not fixing old bugs that get fixed in our sources, but I can't keep you from doing so and won't try :-)

Simon Goldschmidt <goldsimon>
Group administrator
Sat 05 Mar 2016 12:46:22 PM UTC, comment #8: 

That's not a very good idea, PBUF_POOL is meant to be used for TX packets only.

The problem is, if your TX and RX path share the same pool, you can reach a condition where all your buffers are used, most commonly with unACKed TCP data but you are not able anymore to receive TCP ACK anymore to be able to free some buffers.

Sylvain Rochet <gradator>
Group Member
Sat 05 Mar 2016 12:31:41 PM UTC, comment #7: 

@Simon, what I mean is that I've removed all uses (relevant to me) of mem_malloc, especially pbuf_alloc(PBUF_RAM). The PBUF_RAM type is gone and all uses were changed to pbuf_alloc(PBUF_POOL).

Ambroz Bizjak <abizjak>
Sat 05 Mar 2016 12:22:45 PM UTC, comment #6: 

Great! Actually you can use lwIP without a heap allocator, too. Although I don't think it matters for you... ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 04 Mar 2016 09:54:21 PM UTC, comment #5: 

That would be https://github.com/ambrop72/aprinter/tree/web-interface/lwip actually (wrong branch), just in case anyone is interested. One significant change is that the heap allocator is gone.

Ambroz Bizjak <abizjak>
Fri 04 Mar 2016 09:52:49 PM UTC, comment #4: 

Hey,
I guess I don't really care about this anymore, I've already started maintaining my own fork of lwIP[1], stripped down and working like I want it to.

[1] https://github.com/ambrop72/aprinter/tree/master/lwip

Ambroz Bizjak <abizjak>
Fri 04 Mar 2016 08:49:47 PM UTC, comment #3: 

Updating category/item group/severity: this is not faulty behaviour, it is intended behaviour for *lw*IP.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 02 Nov 2015 10:15:45 PM UTC, comment #2: 

Hey,
I think it can be done that way. But, I'm a bit afraid that this would make the two functions very tightly coupled, and one could change the tcp_write() logic while forgetting to make the appropriate change in the tcp_write_partly() calculating the available space, breaking it.

So I have a counter-proposal. We make the partial writes an optional feature enabled with e.g. LWIP_TCP_PARTIAL_WRITE. With partial writes disabled, the pretty much stays as it is right now, so just a tcp_write() function. With partial writes enabled, the current tcp_write() code actually becomes the new tcp_write_partly(), which works the same as my tcp_write_ext() (flag enabled partial mode). And in this case we also have a tcp_write() wrapper on top of tcp_write_partly().

Finally I think I we can get away without a TCP_WRITE_FLAG_PARTIAL flag if we just look at whether written_len is not NULL.

Ambroz Bizjak <abizjak>
Thu 29 Oct 2015 09:26:16 PM UTC, comment #1: 

The idea is good, but the implementation could be improved. I'd prefer havin an additional "tcp_write_partly()" that checks before calling "tcp_write()" so that the majority of users not using this new code don't get increased code size.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 25 Oct 2015 08:23:14 AM UTC, original submission:  

If tcp_write() is called many times with small chunks of data and tcp_output() calls in between, the maximum allowed number of queued pbufs (TCP_SND_QUEUELEN) will be reached and tcp_write() calls will start failing with ERR_MEM.

I'm aware that the application can react to this by retrying tcp_write() after receiving a tcp_sent callback. However this is not optimal, because the failing tcp_write() call could have written some data - perhaps only the last few bytes of the given buffer didn't fit. Additionally, it is not possible for the application to distinguish between hitting the TCP_SND_QUEUELEN limit from otherwise running out of memory (other places where ERR_MEM is returned) which is more serious and may warrant closing the connection.

I provide a patch which implements a partial-write feature in tcp_write (actually I rename that to tcp_write_ext()). When using this TCP_WRITE_FLAG_PARTIAL, tcp_write_ext() will not consider hitting the TCP_SND_QUEUELEN an error, but it will write whatever was possible until the limit is reached. The number of bytes written will be made available to the user.

Ambroz Bizjak <abizjak>

 

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

Attached Files
file #35283:  lwip-partial-write.patch added by abizjak (6KiB - text/x-patch - Implementation of partial TCP writes.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by abizjak (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-03-05 goldsimon StatusNone Invalid
        Open/ClosedOpen Closed
    2016-03-04 goldsimon CategoryIPv4 TCP
        Severity3 - Normal 1 - Wish
        Item GroupFaulty Behaviour Feature Request
    2015-10-25 abizjak Attached File- Added lwip-partial-write.patch, #35283

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code