buglwIP - A Lightweight TCP/IP stack - Bugs: bug #46290, Optimize passing contiguous nocopy...

 
 

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

bug #46290: Optimize passing contiguous nocopy buffers to tcp_write

Submitted by:  Ambroz Bizjak <abizjak>
Submitted on:  Sun 25 Oct 2015 08:36:19 AM UTC  
 
Category:  None Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

( Jump to the original submission)

Thu 24 Nov 2016 03:02:55 PM UTC, comment #6: 

Fantastic! Thank you so much :)

David van Moolenbroek <dcvmoole>
Thu 24 Nov 2016 10:28:33 AM UTC, comment #5: 

I've pushed the patch, but removed the opt.h setting making the new code active always.

Thanks for the improvement.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sat 22 Oct 2016 03:11:19 PM UTC, comment #4: 

Attached is my edited version of the same patch. My changes compared to Ambroz's version:
- rebased on top of lwip-current, incorporating recent (e.g. doxygen) changes;
- removed the pbuf_last() addition, as that should really be a separate patch;
- added a few extra asserts, both as integrity checks and for code flow clarification;
- added and expanded comments to reflect the changed functionality.

I hope that it will be reconsidered for merging. I'd be happy to make additional changes should that help. Otherwise this patch might serve as an updated starting point for anyone else interested in this feature.

As a random data point: on my setup and with my own test set, the patch reduces the maximum pbuf chain length per TCP packet from 256 to 5.

(file #38795)

David van Moolenbroek <dcvmoole>
Mon 02 Nov 2015 09:51:28 PM UTC, comment #3: 

Hello,

Please consider the general case when the application is sending data out of a (ring) buffer and cannot assume anything regarding in how large chunks it receives data into the buffer.

The natural thing for the application to do then is to call tcp_write(no COPY) for each new chunk of data it gets. It could try to optimize this by waiting a bit expecting new data, but there's no guarantee about the outcome. The data really could be arriving slowly in small pieces. If this happens, you using memory very inefficiently (one pbuf for each chunk of data) and consequently the throughput is low.

...But the worst part is that you risk exhausting the PBUF pool and effectively starving other connections (this is why I flagged it as faulty behavior :). Please note that tcp_write() only checks the queue length limit when allocating a new segments, and not when appending to the existing last segment; I do not know if this is intentionally or by mistake.

With this patch, as long as you are sending out of a ring buffer, you know for a fact that there will be at most one ROM pbuf used per TCP segment, except for possibly one segment using two due to the ring buffer wrapping. I think it follows that a connection will use at most TCP_SND_QUEUELEN/2+1 ROM pbufs (/2 due to each segment also having a header pbuf which is also counted into the TCP_SND_QUEUELEN, but this is a PBUF_RAM pbuf).

I think the extra code is worth the benefit. This feature seems to me like the equivalent of the OVERSIZE feature but for the no-COPY case, and the logic is simpler than OVERSIZE.

Ambroz Bizjak <abizjak>
Thu 29 Oct 2015 09:24:03 PM UTC, comment #2: 

In my opinion, this is not a scenario that needs optimization! I'd rather optimize the application than making the tcp code even more complicated.

Oh, and this is no faulty behaviour at all!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sun 25 Oct 2015 08:38:39 AM UTC, comment #1: 

Sorry I meant "When the application is using tcp_write() without TCP_WRITE_FLAG_COPY..."

Ambroz Bizjak <abizjak>
Sun 25 Oct 2015 08:36:19 AM UTC, original submission:  

When the application is using tcp_write() without TCP_WRITE_FLAG_MORE to send data out of a single (ring) buffer, tcp_write() will end up allocating a PBUF_ROM pbuf for each call. This can lead to lots of these pbufs being allocated when the application writes data in little chunks, and possibly we run out of pbufs in the PBUF pool (MEMP_NUM_PBUF). There is no reasonable limit on the number of pbufs that are needed in the worst case. This is an important issue because one "bad" connection can starve other connections.

I've implemented a little bit of smartness in tcp_write() which I think fixes this issue.

  • TCP_EXTEND_ROM_PBUFS: If enabled, a buffer passed to tcp_write() that
  • is a continuation of the previous passed buffer will be combined into the
  • existing pbuf when possible, instead of allocating a new pbuf and adding
  • it to the chain.
  • This only works when tcp_write() is called without TCP_WRITE_FLAG_COPY.
  • It greatly reduces the worst case number of needed pbufs for TCP
  • transmission when the application sends data from a single buffer
  • (to a little more than TCP_SND_QUEUELEN per connection).
Ambroz Bizjak <abizjak>

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Updated the item)
  • -email is unavailable- added by dcvmoole (Updated the item)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by abizjak (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-11-24 goldsimon StatusNone => Fixed
        Open/ClosedOpen => Closed
    2016-11-24 goldsimon SummaryOptimize passing contiguous buffers to tcp_write => Optimize passing contiguous nocopy buffers to tcp_write
    2016-10-23 dziegel StatusWorks For Me => None
        Open/ClosedClosed => Open
    2016-10-22 dcvmoole Attached File- => Added 0001-Add-TCP_EXTEND_ROM_PBUFS-setting-by-Ambroz-Bizjak.patch, #38795
    2015-10-29 goldsimon Item GroupFaulty Behaviour => Change Request
        StatusNone => Works For Me
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2015-10-25 abizjak Attached File- => Added lwip-extend-rom-pbuf.patch, #35284

    Back to the top


    Powered by Savane 3.5