buglwIP - A Lightweight TCP/IP stack - Bugs: bug #51925, Replace the OOSEQ max bytes and...

 
 

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

bug #51925: Replace the OOSEQ max bytes and pbufs constants with functions.

Submitter:  Douglas <ourairquality>
Submitted:  Sun 03 Sep 2017 09:30:13 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Thu 21 Sep 2017 08:37:54 AM UTC, comment #6: 

Pushed slightly changed:
- the *_LIMIT defines take the pcb, not the pcb's ooseq
- reverted your || -> && changes to make the defines work independently (which never worked before)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 06 Sep 2017 01:20:45 PM UTC, comment #5: 

The configuration does not use pbuf pools rather MEMP_MEM_MALLOC. The RX buffers are allocated from a special pool, from a NMI handler that can not call back into most of the library code, these later have a pbuf wrapper allocated as a reference - so these are not using pbuf_alloc for their primary allocation and the PBUF_POOL_IS_EMPTY does not help.

The PBUF_POOL_IS_EMPTY callback is interesting, but not effective on this configuration. What would be needed is a callback when the RX pool is filling. It is possible to copy the RX pool pbuf as an option, but that is not the default. So the callback would need to trigger on the RX pool size, or trigger on memory pressure. This could be explored separately, and probably in code specific to this configuration.

A revised patch is attached. It attempts backwards compatibility, defining new names for the functions, and defaulting them to return the prior constants. It also passes the tcp segment to these to off load the walking of the list to the callee, so that configurations returning a simple constant do not have that burden.

(file #41744)

Douglas <ourairquality>
Tue 05 Sep 2017 07:12:07 PM UTC, comment #4: 

Also, keep in mind that keeping backwards compatibility is a really strong requirement for us! We're open to throw things over, but not without compelling arguments!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 05 Sep 2017 02:51:40 PM UTC, comment #3: 

I forgot about this, but there is already support to free OOSEQ packets upon PBUF_POOL exhaustion. See PBUF_POOL_IS_EMPTY (called from pbuf_alloc).  Is this not working in your configuration?

Joel Cunningham <jcunningham>
Group Member
Tue 05 Sep 2017 02:24:54 PM UTC, comment #2: 

Thank you for taking a look. Here is how this is being used, see the functions ooseq_max_bytes() and ooseq_max_pbufs():
https://github.com/ourairquality/esp-open-rtos/blob/893f715852d1b831289056fc59903d3422d07611/lwip/esp_interface.c#L168

The current constant limit per pcb does not limit the total number of ooseq buffers because there can be multiple pcbs. The esp8266 sdk has a hard limit, a small pool, and if that is exhausted then that blocks further input. So this change allows the number of ooseq buffers for a particular pcb to be limited. It supports a greedy allocation strategy, allowing ooseq buffers to grow until the limit is reached.

Similarly for memory usage, things turn ugly if memory is exhausted.

The pcb could be passed in as an alternative, and that would allow the callee to optionally loop over the list to add up the current total number of buffers and/or memory usage.

Douglas <ourairquality>
Tue 05 Sep 2017 02:02:45 PM UTC, comment #1: 

I took a look at your patch and I'm having trouble fully understanding how one is supposed to implement TCP_OOSEQ_MAX_BYTES and TCP_OOSEQ_MAX_PBUFS since no examples were provided.  They could return constants to maintain the current behavior, but I want to understand the advantage this change provides

> For example, if the system only wants to allow the total number of rx pbufs queued on all the ooseq lists to grow by one and a call is make indicating a pcb already has two then it can return three for this call, but might return one for another call - supporting a greedy allocation strategy.


PCB isn't an input into the new functions, so how do you do this check?

Joel Cunningham <jcunningham>
Group Member
Sun 03 Sep 2017 09:30:13 AM UTC, original submission:  

Some systems need to take into account an RX buffer pool size when advising an appropriate number of RX pbufs to queue on the ooseq list. For some systems there is a practical hard limit beyond which the rx pool becomes exhausted blocking reception of further buffers until some are freed.

It also helps to be able to consider the available dynamic memory when advising an appropriate maximum number of bytes to buffer on the ooseq list.

These decisions can also benefit from knowing the number already allocated on a particular pcb, so this is passed to these functions. For example, if the system only wants to allow the total number of rx pbufs queued on all the ooseq lists to grow by one and a call is make indicating a pcb already has two then it can return three for this call, but might return one for another call - supporting a greedy allocation strategy.

Found I needed this on the esp8266 which has a limited and fixed rx pool. While the pbufs could be copied on input to lwip that wastes the rx pool. Perhaps OOSEQ should not even be contemplated on such a limited device, but when combination with SACK it makes a big difference on a wifi link with high loss, and if there is only one pcb working hard then it is practical to keep a few buffers on the oosq list, even being able to buffer just one buffer on the ooseq list makes a big difference.

Douglas <ourairquality>

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2017-09-21 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2017-09-06 ourairquality Attached File- Added 0001-Replace-the-OOSEQ-max-bytes-and-pbufs-constants-with.patch, #41744
    2017-09-03 ourairquality Attached File- Added 0001-Replace-the-OOSEQ-max-bytes-and-pbufs-constants-with.patch, #41720

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code