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.
|
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 :)
|
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
|
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.
|
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 :-)
|
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.
|
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).
|
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... ;-)
|
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.
|
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
|
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 lwIP.
|
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.
|
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.
|
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.
|