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: NoneSeverity: 3 - Normal
Item Group: Change RequestStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: git head

(Jump to the original submission 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
  • -unavailable- added by dziegel (Updated the item)
  • -unavailable- added by dcvmoole (Updated the item)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by abizjak (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 24 Nov 2016 10:28:33 AM UTCgoldsimonStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Thu 24 Nov 2016 10:26:15 AM UTCgoldsimonSummaryOptimize passing contiguous buffers to tcp_write=>Optimize passing contiguous nocopy buffers to tcp_write
    Sun 23 Oct 2016 05:22:28 PM UTCdziegelStatusWorks For Me=>None
      Open/ClosedClosed=>Open
    Sat 22 Oct 2016 03:11:19 PM UTCdcvmooleAttached File-=>Added 0001-Add-TCP_EXTEND_ROM_PBUFS-setting-by-Ambroz-Bizjak.patch, #38795
    Thu 29 Oct 2015 09:24:03 PM UTCgoldsimonItem GroupFaulty Behaviour=>Change Request
      StatusNone=>Works For Me
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Sun 25 Oct 2015 08:36:19 AM UTCabizjakAttached File-=>Added lwip-extend-rom-pbuf.patch, #35284

    Back to the top


    Powered by Savane 3.1-cleanup1