buglwIP - A Lightweight TCP/IP stack - Bugs: bug #49914, lwip_sendmsg uses PBUF_REF pbufs

 
 

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

bug #49914: lwip_sendmsg uses PBUF_REF pbufs

Submitter:  Dirk Ziegelmeier <dziegel>
Submitted:  Thu 22 Dec 2016 08:13:42 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Change Request Status:  Invalid
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  2.0.0

Jump to the original submission

Sun 15 Jan 2017 04:16:20 PM UTC, comment #13: 

I think Simon's ideas about rewriting the PBUF allocation is a good solution for this. There has to be some way to delegate the PBUF allocation to the application, there may be too many different requirements to implement them all in lwip, especially the alignment stuff I described.

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 09 Jan 2017 11:11:39 PM UTC, comment #12: 

I'm curious to see how the Linux driver for that hardware handles the alignment.  I'm somewhat familiar with Linux socket buffers and did a little digging.

Looks like there is a default alignment applied in alloc_skb() (__alloc_skb): http://lxr.free-electrons.com/source/net/core/skbuff.c#L203

There is also evidence here that the driver can't specify arbitrary alignments for TX buffers and must handle a mis-match by making a copy:

http://www.gossamer-threads.com/lists/linux/kernel/1108227

Any chance you can look at the Linux driver?

Maybe we could have a feature where PBUF_RAM alignment could be specified during compilation.  This might be tricky if there are multiple netifs with different requirements

Joel Cunningham <jcunningham>
Group Member
Sat 07 Jan 2017 07:52:35 PM UTC, comment #11: 

Even DMA-enabled netifs have more problems. I wrote a driver for the Synopsys Designware MAC (in Linux its called "stmmac"). It is capable of scatter-gather DMA, but has alignment requirements for the buffer (I guess resulting from burst-access boundaries). So even for this MAC, I was unable to use zero-copy TX because the RAM PBUFs did not satisfy the alignment requirements.
To make this work, there most flexible way would be to allocate TX PBUFs via a callback from the application...
Zero-copy TX can be quite challenging.

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 27 Dec 2016 08:44:14 PM UTC, comment #10: 

The extra expense is not only for sockets: DMA enabled netifs must handle PBUF_REF from any source correctly, so why not for sockets, too?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 27 Dec 2016 08:36:57 PM UTC, comment #9: 

Dirk,

My products/systems currently fit the case you are describing as "large systems with OS and socket API that have non-DMA HW". 

Without going into extreme detail, my products aren't functioning like the simple case of direct copy to Ethernet HW buffers you describe.  The link layer does in fact operate asynchronously with a queue and the transmit path can't currently handle scatter/gather or pbuf types, so I have copy the entire packet from the pbuf chain into a contiguous allocation.  This is all done in the netif output function, so from LwIP's perspective, the output is synchronous.

Thinking about this some more, it's probably not very common to have the case you describe where large OS with sockets writing out directly to HW buffers without some link layer queue.  Given that, the question comes down to: Do we want to keep the extra flexibility provided by the PBUF_REF case at the expense of all asynchronous netif developers?

Joel Cunningham <jcunningham>
Group Member
Fri 23 Dec 2016 09:02:08 PM UTC, comment #8: 

Zero-copy depends on the application:

1)
- application uses raw API
- application creates a PBUF and generates/assembles its data into the PBUF
- application hands PBUF to lwIP
- lwIP prepends UDP/IP/ethernet header
- A scatter-gather capable DMA netif increases refcount on pbuf and enqueues it in the ETH HW DMA chain
- ETH HW sends pbuf
- netif decreases refcount on PBUF, pbuf is released

2)
- application uses socket API
- application has data to send in a contiguous buffer anyway
- application calls lwip_send()
- lwIP creates a ref PBUF with pointer to application data
- lwIP prepends PBUF with UDP/IP/ethernet header
- netif synchronously copies data into ETH HW buffer for sending (not DMA capable)


Problems:
- Systems with large amount of resources usually have an OS -> mostly socket API is used. But these systems often have a DMA capable ETH HW -> copy needed in driver for DMA

- Systems with low amount of resources usually have no OS -> raw API is used. These systems often do NOT have DMA capable ETH HW -> copy needed in driver for into ETH HW buffer


-> the only systems that can really do zero-copy TX are:

1) large systems with OS and socket API that have non-DMA HW (are there systems like that?)

2) systems where RAW API is used (no matter if OS is running or not) and DMA-capable HW. Whether to use RAW API or socket API depends on application demands, portability requirements and programming effort


Comments to this summary? Is this correct or am I missing something?

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 23 Dec 2016 04:32:21 PM UTC, comment #7: 

Hm yes, I was suggesting a whole-chain copy because of Simon's earlier point here: messing with next-pointers of an in-use chain is not necessarily risk-free - in fact, I don't think it can be done safely for the header=RAM,data=REF case without making assumptions about the way that, say, udp_send() is used. So I'd say optimizing that part is probably not worth it, given the likely small gain it would yield anyway. Of course the nice thing about hiding this stuff behind an API is that it's easy to change later anyway.. :)

David van Moolenbroek <dcvmoole>
Fri 23 Dec 2016 03:53:24 PM UTC, comment #6: 

David,

Thanks for correcting my memory of the TCP retransmit/output behavior. I had provided some discussion in task #7896 about the pitfalls of not handling this case (which I had seen with another embedded stack) and had remembered incorrectly for LwIP. Given that LwIP is handling the reference count, I agree the netif should not have to copy any PBUF_RAM pbufs

I like the idea of a convenience function that handles the complexity for the driver.  Maybe something like pbuf_clone() could be used to handle both the check and copying. This would be called when the netif needs to keep the pbuf chain past execution of the output function. This would increment the reference count on PBUF_RAM and make copies PBUF_REF (or copy the entire chain in the REF case if we prefer that behavior)




Joel Cunningham <jcunningham>
Group Member
Fri 23 Dec 2016 11:16:15 AM UTC, comment #5: 

My 2c here, which probably just rehashes much of what's being discussed elsewhere already, but with a concrete proposal..

Joel's case makes clear that copying anywhere except at the netif/driver layer may lead to unnecessary copying, so it makes sense to perform those copies only at that layer. At the same time, zero-copy implementations in that netif/driver layer will want to eliminate as much copying as is safe. With these two points combined, it is obvious that it should be crystal clear to driver writers when a pbuf should be copied (if immediate transmission is not possible) as opposed to just increasing its refcount. Simon pointed this out in the other thread as well. In any case I don't think I need to emphasize how difficult it is to debug cases where a wrong assumption is made.. :)

As such, I would propose adding a function like,

  /* pbuf2 == pbuf if referenced
   * pbuf2 != pbuf if copied
   pbuf2 == NULL if out of memory /
  pbuf2 = netif_zerocopy_ref_or_copy(pbuf);

or perhaps simply

  if (netif_zerocopy_must_copy(pbuf)) { ..do the rest yourself..

The actual implementation of these functions could simply check if there is any PBUF_REF pbuf in the chain, and if so, (instruct to) make a full copy of the whole chain. Driver programmers then have an API to program against, and the implementation can be changed later as needed.

I would argue that TCP is a slightly different case. Making copies of entire TCP packets up front on the off chance that they will be retransmitted later would be horribly expensive and pretty much negate the main benefits of zero-copy at all. Right now, TCP does in fact check the reference count of segments (right at the start of tcp_output_segment()) and simply does not perform retransmission if it's greater than one. There should hopefully be no other cases where already-transmitted packets are modified, but if the current check is not adequate to prevent problems with zero-copy, I would consider that a bug.

David van Moolenbroek <dcvmoole>
Fri 23 Dec 2016 09:00:19 AM UTC, comment #4: 

OK, I reverted it for now. This seems to need some more discussion.

Simon and I talked about it at work, and we concluded data should be copied.

But we obviously need to think more about how this should be handled.

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 23 Dec 2016 07:54:39 AM UTC, comment #3: 

Good point on the PBUF_RAM which is normally in a chain as a header.  I think for writing a netif that did asynchronous output, it would be a good practice to copy the entire chain and not just any PBUF_REF pbufs. We've discussed this before, but a TCP segment is still referenced after output.  During retransmission, the TCP header (contained in a PBUF_RAM) is modified without checking the reference count.  Given this behavior, it seems unsafe for a netif to hold a reference to a pbuf after the output function has completed.  Maybe any netif developers that have written asynchronous output can share their approach.

I would prefer to preserve the current PBUF_REF behavior, but I think more importantly, the UDP sendmsg and sendto paths should behave the same.  So if using PBUF_REFs in this manner is no longer considered a desirable use case, I would encourage also modifying lwip_sendto

Simon and I did discuss this issue in the original bug (I should have used a patch/task :P) for adding sendmsg support and the conclusion was that since sendmsg followed the same practice as sendto, it was ok.

https://savannah.nongnu.org/bugs/?func=detailitem&item_id=44805

Joel Cunningham <jcunningham>
Group Member
Fri 23 Dec 2016 07:01:10 AM UTC, comment #2: 

I can understand your position Joel. And I think we should support zero copy in this case.

However, the part where "the netif output function is responsible for copying the pbuf/data" is unclear. Even PBUF_RAM get changed eventually, at least from TCP.

I guess a clear definition that DMA-enabled drivers must memcpy PBUF_REF pbufs before sending would be best. However, there are additional problems here as you have a PBUF_RAM header and PBUF_REF data. Is it allowed for the driver to change the next pointer of the header pbuf when this pbuf is still referenced by someone else?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 23 Dec 2016 05:20:31 AM UTC, comment #1: 

Dirk,

The sendmsg implementation follows the normal UDP sendto case, which also uses PBUF_REF and does not copy the data.  The function netbuf_ref (called from lwip_sendto) does the same thing sendmsg is doing; allocate a PBUF_REF and reference the data

To my knowledge, the netif output function is responsible for copying the pbuf/data if it needs to perform an asynchronous output of the pbuf.  This seems like the preferable approach to me because other netifs that perform synchronous output aren't penalized with this extra memory copy

I'd prefer this fix be reverted as it will reduce the performance in my use case and is not consistent with the UDP sendto case

Joel Cunningham <jcunningham>
Group Member
Thu 22 Dec 2016 08:13:42 PM UTC, original submission:  

... and thus does not create a copy of the data to be sent in RAW/UDP case.
This may have problems with DMA netifs, where the output function may return before the data was sent out. The caller may free/reuse the buffer before data was actually sent.

Dirk Ziegelmeier <dziegel>
Group administrator

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2017-01-15 dziegel StatusNone Invalid
        Open/ClosedOpen Closed
    2016-12-23 dziegel Item GroupFaulty Behaviour Change Request
        StatusFixed None
        Open/ClosedClosed Open
    2016-12-22 dziegel StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code