patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9272, arp/ndp: allow overriding the...

 
 

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

patch #9272: arp/ndp: allow overriding the decision to copy pbufs

Submitter:  David van Moolenbroek <dcvmoole>
Submitted:  Mon 27 Feb 2017 04:41:11 PM UTC
   
 
Category:  ARP Priority:  3 - Low
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Tue 28 Feb 2017 05:42:41 PM UTC, comment #5: 

Thank you :)

David van Moolenbroek <dcvmoole>
Tue 28 Feb 2017 08:39:49 AM UTC, comment #4: 
Simon Goldschmidt <goldsimon>
Group administrator
Tue 28 Feb 2017 07:43:12 AM UTC, comment #3: 

Task #6735 is not much newer, being 9 years old now. And even back then, Kieran said in comment #33:
"I'm OK with saying that lwIP doesn't support re-using of the pbufs. I think that this is rare enough use of the API that it is justifiable to simplify the lwIP code by not allowing it."
This is exactly what I'm thinking. Only we'll have to clearly point it out that a pbuf must not be reused when the transmit function returns ERR_OK. Without that, we shouldn't make a change.
But as I said, I think adding a define for this (whether it's overridable or not) is a good thing to clarify the code and to provide it to netif drivers.
I'll push it in a minute ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 27 Feb 2017 09:30:28 PM UTC, comment #2: 

That would be fine with me as well! The last comments from task #6735, which are more recent than bug #11400, suggested to me that at least at the time it was considered acceptable to reuse the pbuf, so I wasn't sure if I'd break anything changing that.

For PBUF_REF, I think it's worth noting that the fragmentation code generates PBUF_REF buffers which, as far as I can tell, are also zero-copy safe. In fact it appears that any zero-copy unsafe PBUF_REFs would have to come from outside the lwIP core, which I'd say is another reason to allow this to be configured, at least for now..

David van Moolenbroek <dcvmoole>
Mon 27 Feb 2017 08:48:50 PM UTC, comment #1: 

Hm, to me the correct thing to do would be to only copy PBUF_REF.
Back in 2007, the result of discussion in bug #11400 seemed to be that we need to copy everything but PBUF_ROM because an application was allowed to reuse a pbuf after passing it to the stack.
I'm not so sure about that any more: I think it would be best to say an application must not use a pbuf passed to a TX function if that function returned ERR_OK. That would save us a lot of problems here.
I'm OK with your patch though as it just defers the decision and actually improves the documentation (i.e. "why does everything but PBUF_ROM have to be copied").

Simon Goldschmidt <goldsimon>
Group administrator
Mon 27 Feb 2017 04:41:11 PM UTC, original submission:  

I realize that this is a contentious topic, especially because the long-term goal is a larger redesign, but after about a decade I'm guessing that that might take a while longer. ;) Therefore I'd like to propose a tiny change in this area that has substantial benefits for me right now at least [*]. I'd love to hear what you think.

To recap the current situation, at least as I understand it: in zero-copy setups, drivers may asynchronously transmit buffers provided to them, simply increasing the reference count of those buffers until the transmission is complete. This avoids the need to make copies of the buffers. Whether avoiding the copy is possible at all depends on the buffer type. For PBUF_ROM, it is always possible. For PBUF_RAM/PBUF_ROM/PBUF_REF there are two factors to consider: lwIP itself, and the application that uses lwIP. lwIP itself internally supports zero-copy use of those types of buffers, as it should, because otherwise zero-copy would not have any benefit at all. Applications that use lwIP must be careful with their own use (and in particular, reuse) of such buffers so as to not mess up the ability to do such corruption-free zero-copy transmissions. As such, any zero-copy setup needs an application/driver agreement on which types of buffers need to be copied on low-level output for reliable zero-copy transmission. lwIP itself does not know anything about that agreement. So far so good.

My issue, then, is with the current arp/ndp queuing code, which in essence is very much like the driver transmission code: it may hold a packet for a short time while waiting for something external to happen. The queuing code currently always assumes the worst case, making copies of any packets that do not consist entirely of PBUF_ROM buffers. As a default policy, that is perfectly fine. However, my proposal here is to allow the user to be able to override this decision, specifically by allowing zero-copy implementations to specify their own aforementioned application/driver agreement in a single macro. That way, lwIP can avoid the extra copy unless necessary. The attached patch basically moves two lines around in order to allow for this.

As a possible theoretical extension, the same new macro (PBUF_NEEDS_COPY) may be used at the driver level for the same purpose. As such, this is the simplest implementation of my suggestion in bug #49914 comment #5. It should then probably documented in doxygen as well; for now I'm leaving that to a future patch if it is deemed useful at all.

[*] To be exact: it is not any speed increase that is important to me here, but rather the elimination of the only operation-critical place that requires relatively large (~1500 byte) contiguous memory allocations. In a nutshell, this issue is in my way of eliminating the need for a 1500+byte memory pool which, to account for the worst case, takes up about 75KB of memory all by itself right now. It seems silly having to spend that much memory just to account for allocations that are entirely unnecessary in my setup..

===

lwIP aims to support zero-copy TX, and thus, must internally handle
all cases that pbufs are referenced rather than copied upon low-level
output.  However, in the current situation, the arp/ndp packet queuing
routines conservatively copy entire packets, even when unnecessary in
cases where lwIP is used in a zero-copy compliant manner.  This patch
moves the decision whether to copy into a centralized macro, allowing
zero-copy compliant applications to override the macro to avoid the
unnecessary copies.  The macro defaults to the safe behavior, though.

David van Moolenbroek <dcvmoole>

 

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

Attached Files

 

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 dcvmoole (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-02-28 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2017-02-27 dcvmoole Attached File- Added 0001-arp-ndp-allow-overriding-the-decision-to-copy-pbufs.patch, #39837

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code