tasklwIP - A Lightweight TCP/IP stack - Tasks: task #6735, Provide new pbuf type:...

 
 

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

task #6735: Provide new pbuf type: PBUF_RAM_NOCOPY

Submitter:  Jonathan Larmour <jifl>
Submitted:  Tue 10 Apr 2007 02:24:22 PM UTC
   
 
Category:  None Should Start On:  Mon 09 Apr 2007 11:00:00 PM UTC
Should be Finished on:  Sat 09 Jun 2007 11:00:00 PM UTC Priority:  7 - High
Status:  Cancelled Privacy:  Public
Assigned to:  jifl Percent Complete:  50%
Open/Closed:  Closed Planned Release:  1.4.0
Effort:  0.00

Jump to the original submission

Thu 27 Mar 2008 10:25:22 PM UTC, comment #34: 

Unfortunately I will be up to my ears for the next 3 weeks or so, and even then, I need to update my own port to 1.3.0 first. If someone else wants to run with this feel free, however given Kieran's comment, really this particular task is now invalid. If pbufs can't be reused, we no longer need a separate PBUF_RAM_NOCOPY pbuf type.

But there's still a lot to do to support zero-copy transmits and receives. For receives, there are usually requirements for things such as using specific memory regions (PCI windows, or dedicated DMA regions, or on-chip RAM, etc.), as well as alignment and buffer size constraints. One port I've already done with my own zero-copy rx mods is to AT91SAM7X which uses fixed 128-byte buffers, thus requiring fixed 128-byte pool pbufs. Fortunately I could still keep the existing pool structure, but that won't be true on other systems - it will need to be possible to separate the struct pbuf, from the following pool memory itself. The simplest way I can think of is to keep the struct pbuf and the pbuf memory itself in parallel arrays. So if you have the pbuf start, then since the pbufs are of constant size and you know the base, you can easily calculate the index of this pbuf, and therefore the index of the corresponding struct pbuf.

Another requirement is that drivers need to be informed when more of that pool memory becomes available, as often the hardware has its own circular buffer lists, and these will need to be marked as available again, once a pool pbuf has been freed.

So far I only have a solution for one ethernet driver at a time. Implementing this for multiple ones (efficiently) is more tricky since of course it's possible for drivers to have different requirements. The easiest solution is probably to say they can't do it then.

Anyway, I'm closing this task as invalid, and I'll open a new task for the above.

Jonathan Larmour <jifl>
Group Member
Tue 25 Mar 2008 01:11:27 PM UTC, comment #33: 

Now that 1.3.0 is out the door we should revisit this.

I think jifl has the best handle on this problem and should go ahead with solving it.

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.

Kieran Mansley <kieranm>
Group Member
Fri 19 Oct 2007 08:27:07 PM UTC, comment #32: 

EVS Hardware Dpt wrote:

>
> After reading the whole discussion my conclusion is that you
> want to be sure that each time you call an output function
> (tcp_write, udp_send,...) the buffer is free for re-use ?


That is what was decided, after looking at both sides of the argument.
 

> In our port, we use Dma functionnality, so low_level_output
> makes a pbuf_ref(p), put it on dma chain and return, then the
> TX Isr free this pbuf. If nobody has a ref on it, it's freed,
> else the buffer is still allocated and owned by it's creator.


Although the creator cannot tell, other than polling, whether they can reuse it or not.
 

> For Udp, why not simply free the buffer directly after sending,
> either the ethernet driver has already sent the buffer and it
> is effectively freed, or the driver has still a reference on
> it and thus the buffer is not freed. Now if you want to send
> another bunch of data, simply ask for a new pBuf.
>
> The basic schema would be :
>
> while(MustSendData == TRUE)
> {
>    pbuf = pbuf_alloc()
>    fill_buf_with_data()
>    udp_send(pbuf)
>    pbuf_free(pbuf)
> }


Sure. That was explicitly considered - we could just say that a pbuf can't be reused. But people were saying that it's sometimes useful to make minor modifications to an existing buffer, rather than refilling it afresh every time. (Admittedly personally, I'd have thought a PBUF_REF pointing to an existing behaviour would be fine for that case, but whatever).
 

> Now for udp, if you don't want to free pbuf everytime, why not
> create a Macro like PBUF_CAN_REUSE that check that ref_count
> is 1, so this buffer is free for reuse. So the schema would
> be:
>
> pbuf = pbuf_alloc()
>
> while(MustSendData == TRUE)
> {
>    if(PBUF_CAN_REUSE(pbuf))
>    {
>       fill_buf_with_data()
>       udp_send(pbuf)
>    }
> }


Well, you could do that right now - no special macro required. But polling is a highly inefficient way to do this, and bad in a multi-threaded system. But I don't think this half way house makes sense - applications can't stall depending on whether pbufs can be reused or not.

Or at the very least, there would want to be some sort of new callback to indicate a pbuf can be reused, e.g. udp_sent().
 

> I like the current state of stack, and I think we must not
> change to something with lot of copies just to make sure that
> we can use it without prior knownledge. In our design we are
> able to get 950Mbps throughput and certainly don't want to go
> below that.


Exactly, that's the point of resolving this. Right now, since it's been decided that pbufs can be reused, this cannot be done safely so I would guess the majority of ethernet drivers would always need to copy every packet which is ridiculous. You're saying we shouldn't change the stack, but arguably any driver that isn't immediately sending any packet (and blocking till its complete) is in error. So adding a PBUF_RAM_NOCOPY type is one way to preserve the API and allow efficient use. I would like to hope that before long most people would use PBUF_RAM_NOCOPY for pretty much everything and not PBUF_RAM. I'm sure reusing pbufs is something that very few people do, but apparently that's the way API has to be.

Personally in my driver I am doing what you did - just pbuf_ref and pbuf_free, but then I'm guaranteeing I don't reuse the buffer (and insisting my users don't either). But I want lwIP fixed properly because otherwise we have anomalies like drivers queuing up data in pbufs that had been marked PBUF_REF, which is an explicit indication from the user that the data is volatile.

There are other potential ways to solve this while preserving the "you can change the pbuf and data after sending" API behaviour. One might be to allow registering a callback for when a pbuf has been pbuf_free()d such that its reference count drops to 0. However this doesn't map well to higher level APIs.

Jifl

Jonathan Larmour <jifl>
Group Member
Fri 19 Oct 2007 01:43:51 PM UTC, comment #31: 

After reading the whole discussion my conclusion is that you want to be sure that each time you call an output function (tcp_write, udp_send,...) the buffer is free for re-use ?

In our port, we use Dma functionnality, so low_level_output makes a pbuf_ref(p), put it on dma chain and return, then the TX Isr free this pbuf. If nobody has a ref on it, it's freed, else the buffer is still allocated and owned by it's creator.

For tcp_write, it seems obvious that if you want to be sure you can reuse a buffer, either you send it with the Copy flag, or you wait the associated data_sent callback.

For Udp, why not simply free the buffer directly after sending, either the ethernet driver has already sent the buffer and it is effectively freed, or the driver has still a reference on it and thus the buffer is not freed. Now if you want to send another bunch of data, simply ask for a new pBuf.

The basic schema would be :

while(MustSendData == TRUE)
{
   pbuf = pbuf_alloc()
   fill_buf_with_data()
   udp_send(pbuf)
   pbuf_free(pbuf)
}

Now for udp, if you don't want to free pbuf everytime, why not create a Macro like PBUF_CAN_REUSE that check that ref_count is 1, so this buffer is free for reuse. So the schema would be:

pbuf = pbuf_alloc()

while(MustSendData == TRUE)
{
   if(PBUF_CAN_REUSE(pbuf))
   {
      fill_buf_with_data()
      udp_send(pbuf)
   }
}

pbuf_free(pbuf)

A nice side effect would be that we can wait for previous data sent before sending another one.

I like the current state of stack, and I think we must not change to something with lot of copies just to make sure that we can use it without prior knownledge. In our design we are able to get 950Mbps throughput and certainly don't want to go below that.

Fred.

EVS Hardware Dpt <vse>
Sun 12 Aug 2007 04:36:15 PM UTC, comment #30: 

In the patch attached here, I included a macro PBUF_NEEDS_COPY(_p).

Jonathan Larmour <jifl>
Group Member
Sun 12 Aug 2007 02:54:18 PM UTC, comment #29: 

Then maybe we should provide a function that checks whether a pbuf to be enqueued must be copied or not, like 'q = pbuf_to_enqueue(p)', so that this knowledge is wrapped in another function, not in every driver.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 12 Aug 2007 01:30:30 PM UTC, comment #28: 


> We could otherwise set a flag in the netif struct and do
> the work in the stack to simplify driver development,


Although as I said, for some chips, the decision on whether the packet can be sent immediately or not might be taken in the driver depending on available buffer space at the time. So setting a flag on such a property wouldn't always be sensible either.

Jonathan Larmour <jifl>
Group Member
Sat 11 Aug 2007 09:06:34 AM UTC, comment #27: 


> That's not true if the driver is non-blocking


Of course it's not. But we currently don't know whether the driver is blocking or non-blocking i.e. whether it needs a packet longer than the stack has it or not. Thus the only place where we know whether to copy the packet or not is in the driver! We could otherwise set a flag in the netif struct and do the work in the stack to simplify driver development, but copying on entering the stack (from the app) definitively leads to unnecessary copying for me!

Simon Goldschmidt <goldsimon>
Group administrator
Fri 10 Aug 2007 08:53:32 PM UTC, comment #26: 


> I think that's wasteful, and this is part of the reason for the existence of PBUF_REF - only copy it if you need to. Otherwise you may as well simplify the API and say users must always copy such data into a PBUF_RAM.


Yes, that's true and would be wasteful.

> I think in some cases (especially for UDP) you can expect some packets to be sent directly and they never need to be copied.


That's not true if the driver is non-blocking; control may have returned to the application before the packet is even sent.

Jared Grubb <jgrubb>
Group Member
Fri 10 Aug 2007 07:23:01 PM UTC, comment #25: 


> If an application submits a PBUF_REF, then it would get copied
> immediately in TCP/UDP.


I think that's wasteful, and this is part of the reason for the existence of PBUF_REF - only copy it if you need to. Otherwise you may as well simplify the API and say users must always copy such data into a PBUF_RAM.

>> This also avoids penalty for polled drivers that may not need
>> separate copying in the driver at all.
>
> I personally think the driver should never copy. The stack
> should take care of all of that when it is necessary.


Only because you're essentially making it "always copy" but in a different place. I think in some cases (especially for UDP) you can expect some packets to be sent directly and they never need to be copied. Take the popular CS8900a chipset - that has a packet buffer in the device - no DMA. If the packet buffer is empty you can copy it straight in. If the packet buffer is full you need to buffer it up (or perhaps drop the packet if you prefer). There are times when you don't have to copy. For ethernet devices that use DMA you will usually need to, but that's not all there is out there.



Jonathan Larmour <jifl>
Group Member
Fri 10 Aug 2007 06:48:36 PM UTC, comment #24: 


> But I don't think that's workable for PBUF_REF. You may only find deep down that you need to queue it or copy it, and where it not for that you may not need to. I think we should keep the existing behaviour of only copying pbufs when they need to be queued (although it will be incumbent on the stack to then mark the copy PBUF_RAM_NOCOPY so it isn't copied a further time).


I propose one policy: All functions in lwIP expect that zero-copy is valid on pbufs handed to it, except in a handful of exact places on the app/stack boundary. If a function does something that breaks the zero-copy expectation to another function, then it must fix it before passing the pbuf on.

If an application submits a PBUF_REF, then it would get copied immediately in TCP/UDP. If lwIP creates a PBUF_REF at some point, then lwIP knows the zero-copy requirements already, and there's no need to ever copy it later, right?

What condition would this fail? Where would "pbuf_copy_required==FALSE" at the app/stack boundary but later change to TRUE in the driver?

> This also avoids penalty for polled drivers that may not need separate copying in the driver at all.

I personally think the driver should never copy. The stack should take care of all of that when it is necessary.

Jared Grubb <jgrubb>
Group Member
Fri 10 Aug 2007 03:35:11 PM UTC, comment #23: 


> In my opinion, the best place to copy is at the "top" of the
> stack right away and never again. This is how BSD IPv6 works.
> This makes it much easier on the driver.


But I don't think that's workable for PBUF_REF. You may only find deep down that you need to queue it or copy it, and where it not for that you may not need to. I think we should keep the existing behaviour of only copying pbufs when they need to be queued (although it will be incumbent on the stack to then mark the copy PBUF_RAM_NOCOPY so it isn't copied a further time).

This also avoids penalty for polled drivers that may not need separate copying in the driver at all.

Jonathan Larmour <jifl>
Group Member
Fri 10 Aug 2007 03:20:03 PM UTC, comment #22: 


> But that would mean copying also for MACs that would support zero-copy sending (like mine, currently!). I'd rather leave it with PBUF_REF and let the driver copy everything but PBUF_RAM_NOCOPY. Which of course gives the problem with chained pbufs containing e.g. PBUF_REF and PBUF_RAM_NOCOPY, but still, some MACs would support zero-copy sending for UDP sockets.


I am suggesting that drivers, ARP, and IP protocols are completely zero-copy -- and dont even check for it. TCP and UDP will be zero-copy as well, except they will initially accept pbuf's from the user that require a copy. The TCP/UDP output functions will check if any of the pbuf's submitted require a copy, and will do a copy only if necessary.

If your application never asks for a copy (for example, uses a no-copy pbuf type), then no copies are ever made.

In my opinion, the best place to copy is at the "top" of the stack right away and never again. This is how BSD IPv6 works. This makes it much easier on the driver.

Jared Grubb <jgrubb>
Group Member
Fri 10 Aug 2007 02:10:19 PM UTC, comment #21: 


> Why can't PBUF_RAM_NOCOPY be freed by the allocating code as
> well, making the driver reference the pbuf before enqueueing it
> for sending?


But if the user is unable to reuse the pbuf or payload, what would be the point? It would just waste code.

> I don't see any indication for letting the stack (or driver) free
> the pbuf when looking at the name (NOCOPY)... And it would
> certainly be more straightforward like the other pbuf types that
> way...


Do you mean you haven't seen that in the patch? If so, yes you're right - that patch is only the first part of the work. As per comment #7 there's more to come, but that will wait till after 1.3.

Jonathan Larmour <jifl>
Group Member
Fri 10 Aug 2007 08:21:06 AM UTC, comment #20: 

Jonathan,
Why can't PBUF_RAM_NOCOPY be freed by the allocating code as well, making the driver reference the pbuf before enqueueing it for sending? I don't see any indication for letting the stack (or driver) free the pbuf when looking at the name (NOCOPY)... And it would certainly be more straightforward like the other pbuf types that way...

> For us, this would make sense too.... copy as early as possible
> (if necessary) and make the stack itself 100% zero-copy.


But that would mean copying also for MACs that would support zero-copy sending (like mine, currently!). I'd rather leave it with PBUF_REF and let the driver copy everything but PBUF_RAM_NOCOPY. Which of course gives the problem with chained pbufs containing e.g. PBUF_REF and PBUF_RAM_NOCOPY, but still, some MACs would support zero-copy sending for UDP sockets.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 09 Aug 2007 06:26:09 PM UTC, comment #19: 


> I'd like to know how other stacks (that are advertising zero-copy) solve this... Although this seems a problem only for the sockets layer, a netconn app can use the new pbuf type.


I was reading the KAME IPv6 (the BSD IPv6 code) implementation book, and I remember it saying packets are copied upon coming into the socket API.

For us, this would make sense too.... copy as early as possible (if necessary) and make the stack itself 100% zero-copy. Then, we must only have special checks along the interface to the application. Network layer and lower (IP, ARP, driver, etc) would always assume zero-copy, whereas transport layer makes the conversions.

Jared Grubb <jgrubb>
Group Member
Thu 09 Aug 2007 04:16:26 PM UTC, comment #18: 


> So if implementing PBUF_RAM_NOCOPY (and using it in TCP and all
> other stack-internal uses of PBUF_RAM), this would still leave
> us with the fact that the present zero-copy sending for UDP
> sockets would not work any more! At least for most hardware
> (as DMA MACs are rather widely used).


But this isn't a surprise is it? If you can't guarantee a packet has truly been sent when handed off to the driver, then you can't allow the application to potentially change the buffer. Maybe you could argue that the UDP socket send call should not return until the packet has definitely left the hardware, but I think that would create bad inefficiencies of its own.

> I'd like to know how other stacks (that are advertising
> zero-copy) solve this... Although this seems a problem only for
> the sockets layer, a netconn app can use the new pbuf type.


Indeed, you may have noticed me commenting before that a socket API won't be as efficient as the netconn API ;-). Netconn can pass incoming packet data directly as received from the hardware, and, with PBUF_RAM_NOCOPY, send directly too. Any alignment constraints for data are currently left as the application's issue. In due course we should probably address things like alignment requirements later, but there's immediate need straight away.

But data passed through the socket API is harder. Here's some links to what BSD OS people have tried to do:
http://freebsd-man.page2go2.com/man9/zero_copy_9.html
http://www.cs.duke.edu/ari/trapeze/freenix/node6.html
http://people.freebsd.org/~ken/zero_copy/

Even there they have had to jump through quite exceptional hoops (only one hardware device supported now, and even that requires specially patched firmware), and much of that come from the inherent properties of the socket API they're trying to fit it into. Additionally, relying on the presence of copy-on-write capable VM is something they may be able to do, but lwIP certainly couldn't (and shouldn't).

Maybe there's something more specific that can be done, like having a setsockopt that indicates that subsequent UDP sends should use PBUF_RAM_NOCOPY, but it's all hairy and obviously non-standard, which raises the question of why use the socket API in that case anyway.

> One thing: I would always let the allocator deallocate the pbuf
> (e.g. alloc; send; free) and let the lower layers (network
> driver?) ref the pbuf and free it later, instead of the way
> described here earlier. Like Jared, I have the feeling this
> could otherwise lead to many 'bugs' reported on lwip-users and
> I also think the code should be the same for all pbuf types.


It is a valid alternative solution, and I agreed with Jared too in comment #13. As per comment #15, it seems Kieran would prefer to go with the PBUF_RAM_NOCOPY approach. Certainly we have to do something because the way the stack works now is not good: choose one of unsafe (unless you're using a polled driver) or extra unnecessary copies.

The API contract for PBUF_RAM_NOCOPY is explicitly that the application doesn't free it. Where it gets freed inside the stack or the driver is up to us, but the current interface between the stack and driver implies it should be freed in the driver. I don't think this will be too hard for users to understand, and only affects things if they choose to use PBUF_RAM_NOCOPY in the first place.

Jifl

Jonathan Larmour <jifl>
Group Member
Thu 09 Aug 2007 03:01:08 PM UTC, comment #17: 

So if implementing PBUF_RAM_NOCOPY (and using it in TCP and all other stack-internal uses of PBUF_RAM), this would still leave us with the fact that the present zero-copy sending for UDP sockets would not work any more! At least for most hardware (as DMA MACs are rather widely used).

I'd like to know how other stacks (that are advertising zero-copy) solve this... Although this seems a problem only for the sockets layer, a netconn app can use the new pbuf type.

One thing: I would always let the allocator deallocate the pbuf (e.g. alloc; send; free) and let the lower layers (network driver?) ref the pbuf and free it later, instead of the way described here earlier. Like Jared, I have the feeling this could otherwise lead to many 'bugs' reported on lwip-users and I also think the code should be the same for all pbuf types.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 01 Aug 2007 02:26:00 PM UTC, comment #16: 

Kieran Mansley wrote:

>
> In my opinion, if the application was going to use the buffers
> in that way (i.e. never modify the contents, just free it after
> sending) they could just use the PBUF_ROM type (or the
> suggested new PBUF_RAM_NOCOPY if the stack should do the free)
> and the stack would have the guarantees it needs.


That's true. It may then be the case that if the most common use at present is for people to just free pbufs after transmission, then in practice users will mostly want to use PBUF_RAM_NOCOPY instead of PBUF_RAM in future (although we may want to provide a way in the sequential API to indicate this since netbuf_alloc() only allocates a normal PBUF_RAM).

And hopefully in future we can consider reworking TCP so that copied data in tcp_enqueue goes into a PBUF_RAM_NOCOPY instead of PBUF_RAM. If so, maybe we can get to the position where most data isn't copied by the driver after all.

> Also, just preventing users from re-using PBUFs doesn't really
> solve the problem.  If they free it, then allocate another one
> for the next send, they might get the same one back and so it
> be identical to re-use.


The driver would have to have pbuf_ref()'d it, so a pbuf_free() wouldn't have really freed it anyway.

> lwIP can support zero-copy if the application allows it.


At present I'm not sure we can safely even do single copy at present. If a driver at present doesn't copy all data, there are problems if the user reuses a PBUF_RAM sent by UDP, or changes its payload after send. If a driver does copy all data, then overall there will be unnecessary multiple copies e.g. for TCP.

> Either way, this will be a 1.4 not 1.3 problem.


Ack. I'll keep going with PBUF_RAM_NOCOPY then, but wait till after 1.3.

Jonathan Larmour <jifl>
Group Member
Wed 01 Aug 2007 08:03:01 AM UTC, comment #15: 

In my opinion, if the application was going to use the buffers in that way (i.e. never modify the contents, just free it after sending) they could just use the PBUF_ROM type (or the suggested new PBUF_RAM_NOCOPY if the stack should do the free) and the stack would have the guarantees it needs.  We should assume that if they've used PBUF_REF, they have done so for a reason.  The current API allows the application to choose both ways, so I'd rather not restrict it if we don't have to.

Also, just preventing users from re-using PBUFs doesn't really solve the problem.  If they free it, then allocate another one for the next send, they might get the same one back and so it be identical to re-use.  We can't expect the application to cope with that level of subtlety.

lwIP can support zero-copy if the application allows it.  I think where we need API changes are between the stack and the driver (so that information about what needs copying if not sent synchronously can be passed) not between the application and the stack.  Either way, this will be a 1.4 not 1.3 problem.

Kieran Mansley <kieranm>
Group Member
Tue 31 Jul 2007 06:52:36 PM UTC, comment #14: 


>  However see comment #43 of bug #11400.


Thanks, I hadn't seen that background.

> Even if we don't want to include any changes for 1.3, it would be good to have something written to tell users that reusing pbufs after transmission is deprecated and will be unsupported in 1.4. So it's worth resolving this soon.


Absolutely. If lwIP cannot support a zero-copy environment efficiently, I think this must be a high-priority task for the very near future (whether it's 1.3.0 or not). This is an embedded stack after all; extra rules on the application in order to ensure efficient memory usage and tight code are par for the course.

By the way, we have begun a documentation wiki (I'm sure you saw the messages). I am trying to be verbose and get lots of discussion on issues like this one so that we can document it well. You guys may all know what's going on, but for a newcomer like me, there is a big learning curve!

Jared Grubb <jgrubb>
Group Member
Tue 31 Jul 2007 06:14:40 PM UTC, comment #13: 

In comment #11, Jared Grubb wrote:

> In an embedded environment, I would think "copy only when necessary" is the
> right behavior -- you have limited CPU time and limited RAM. Since we agree a
> copy has to happen somehere the question is: Where should the copy happen?


And making sure the copy only happens at most once.

> If the driver always copies data, then it will be copying every IP, ARP,
> ICMP, UDP, and TCP packet it has to send -- even when an upper layer's next
> step is pbuf_free(). That is a lot of wasted memory and a lot of wasted
> space.


Well, not entirely: if copying a PBUF_RAM or a PBUF_REF, then an upper layer could then copy it into a PBUF_RAM_NOCOPY, so a lower level driver knows it does not need to copy it another time. So we ensure that a copy only happens once.

> In my opinion, though, a good design has the RX & TX chains completely
> decoupled and complementary, with the RX chain only doing pbuf_alloc (the free
> happens elsewhere) and the TX chain only doing pbuf_ref and pbuf_free (and
> allocation happens elsewhere).


Well, ideally pbuf_ref for TX, but that's the issue at hand!
 

> What do we lose by saying: "driver is zero-copy; application must not reuse a
> buffer after sending"? This seems to be the simpler design choice across the
> board -- and the most efficient from an embedded standpoint.


I agree, and I think I did say this at one point. However see comment #43 of bug #11400. I would be very much in favour of going back on that and just decreeing that you can't alter PBUF_RAM payloads or the struct pbuf of any of the sorts of pbufs after transmission, except to pbuf_free it. That would simplify a lot of things. I think with modern hardware there are going to be very few cases which we could exploit as described in that comment #43 of bug #14400 - sending can and should happen in the background which means it will be rare that something can be considered sent immediately.

This doesn't help PBUF_REFs much - we have no control over external data, and the user would never have any sensible way to know when they would be allowed to change it. So for example, lwip_sendto would essentially always cause a copy, but at least there's hope for raw or sequential API users.

Kieran, since we're in "don't mind breaking things for 1.3" mode, could we reconsider the API behaviour as described in #43 of bug #11400?

Even if we don't want to include any changes for 1.3, it would be good to have something written to tell users that reusing pbufs after transmission is deprecated and will be unsupported in 1.4. So it's worth resolving this soon.

I'm just keen to get a solution, because what's there now doesn't allow safe zero-copy.

Jonathan Larmour <jifl>
Group Member
Tue 31 Jul 2007 05:41:53 PM UTC, comment #12: 


> If you think the driver will be transmitting that fast, you could always instead ensure the driver doesn't return until it is sent.


You could do that, but there's no reason to stall the program waiting for this to happen. In a driver, you can immediately return from the linkoutput, but stall any other linkoutputs (via semaphore) until the previous is finished.

Suppose you do give the user two valid ways of sending a packet:

// Method A
p = pbuf_alloc(PBUF_RAM, ...);
fill_p_with_data(p);
some_output(p);
pbuf_free(p);

// Method B
p = pbuf_alloc(PBUF_RAM_NOCOPY, ...);
fill_p_with_data(p);
some_output(p);

This makes it easy to make a critical mistake. If you do A but forget the free, or you do B and add a free, then you have a major error in your application. A nasty side effect is that it's not obvious from looking at the code that you even made a mistake.

By requiring an application to always free a pbuf that it itself allocs (or that it gets on return from the stack), you can see runtime errors in the code itself. Even better: the application programmer only has to remember that simple rule.

Jared Grubb <jgrubb>
Group Member
Tue 31 Jul 2007 05:21:02 PM UTC, comment #11: 


> It's possible that zero copy can work if you happen to have fast enough ethernet hardware, and depending on the driver design (e.g. how much it can queue), and the application. But there are no guarantees in general. It's likely that the more Frederic and Simon improve the sockets.c performance :-), the more likely it is that this problem will show up. It should already be fairly easy for it to show up with the netconn API.


In an embedded environment, I would think "copy only when necessary" is the right behavior -- you have limited CPU time and limited RAM. Since we agree a copy has to happen somehere the question is: Where should the copy happen?

If the driver always copies data, then it will be copying every IP, ARP, ICMP, UDP, and TCP packet it has to send -- even when an upper layer's next step is pbuf_free(). That is a lot of wasted memory and a lot of wasted space.

Alternatively, you can have the driver scan through the pbuf chain and conditionally copy some pbuf's and not others... but this is added complexity and requires the transmit portion of the driver to be able to allocate memory for transmission. This might be ok, since the RX part of the driver must have allocation  capability, so borrowing a bit of the RX buffer pool could be ok. In my opinion, though, a good design has the RX & TX chains completely decoupled and complementary, with the RX chain only doing pbuf_alloc (the free happens elsewhere) and the TX chain only doing pbuf_ref and pbuf_free (and allocation happens elsewhere).

The only protocol that may specifically need some copy mechanism is TCP (because of retransmissions), but that's already built in.

What do we lose by saying: "driver is zero-copy; application must not reuse a buffer after sending"? This seems to be the simpler design choice across the board -- and the most efficient from an embedded standpoint.

Jared Grubb <jgrubb>
Group Member
Tue 31 Jul 2007 01:19:04 AM UTC, comment #10: 

Re Frederic's comment #9:
Oh, I knew Kieran had been marking some items as for 1.3, but I hadn't realised that there was a feature freeze now - I must have missed that, and there still seemed quite a bit of activity.

Even though it by rights shouldn't change existing behaviour, if it has to wait, it has to wait. In that case, I'll hold off committing, and making the second patch.

Still, if people have comments, they're still welcome.

> if adding this new option don't make mandatory to change
> ethernet drivers, so, an option is not necessary to my point of
> view... I see than mainly like an optimization possible for
> netconn api/raw > api users, I am wrong?)


Certainly that was my original intention - it's not used till someone creates a pbuf of that type. However, I remember Simon bringing up some situations within the stack when it would be useful for the stack itself to create them. That's for the future though.

Jared wrote in comment #8:

> I wrote my Ethernet driver completely zero-copy and I haven't
> had any issues yet -- what part of lwIP code expects to be able
> to alter a pbuf after it is submitted? (Maybe I haven't enabled
> that module yet!)


One problem is the user. As Kieran wrote in bug 11400: "the API does currently seem to give the application the right to do what it likes with the pbuf, so I think we will have to copy if we can't send it immediately."

The most obvious example is if someone sends a UDP packet. So e.g. they call lwip_sendto in sockets.c, which makes a PBUF_REF (via calling netbuf_ref). It then sends it with netconn_send, which eventually goes through the stack until the linkoutput calls into the driver. That returns immediately back up through the stack, but if the driver hasn't really sent the packet data, but has queued it or is half-way through sending it, the user has the freedom to start changing the original data they passed in.

But the same holds for PBUF_RAM UDP packets with the netconn API - you can e.g. reuse or chain a netbuf after having just sent data from it.

> A brief glance through the code seems to
> suggest that "some_output (p); pbuf_free(p);" is a common
> sequence of code. To require a driver to allocate a new buffer,
> copy the data, and then transmit seems to be a waste when the
> next statement upon return is often a pbuf_free anyway.


If you think the driver will be transmitting that fast, you could always instead ensure the driver doesn't return until it is sent.

But otherwise, there is a race condition. The idea is that if your usage pattern is something like:
p = pbuf_alloc(PBUF_RAM, ...);
fill_p_with_data(p);
some_output(p);
pbuf_free(p);

then instead since you know you're going to free the data afterwards, this would become
p = pbuf_alloc(PBUF_RAM_NOCOPY, ...);
fill_p_with_data(p);
some_output(p);

So does this make the purpose clearer? It's possible that zero copy can work if you happen to have fast enough ethernet hardware, and depending on the driver design (e.g. how much it can queue), and the application. But there are no guarantees in general. It's likely that the more Frederic and Simon improve the sockets.c performance :-), the more likely it is that this problem will show up. It should already be fairly easy for it to show up with the netconn API.

TCP is a bit different because there's a char* and length, and a copy flag passed in, instead of a pbuf. The copy flag can be 1, in which case it's copied. (Although the hardware driver won't know that it's safe to use it without copying). The only situation in which the copy flag can be 0 is if you're never prepared to change the data.

Jonathan Larmour <jifl>
Group Member
Mon 30 Jul 2007 11:24:07 PM UTC, comment #9: 


>Note that I have some trouble with testing here because I can't test current CVS - my port is still stuck in the 1.1 dark ages, and that's not likely to change till 1.3 comes out (or at least the code base has stabilised in preparation for 1.3 - I'm not in a position to play with unstable code and changing APIs, so will have to wait at least for a pre-1.3 feature freeze). I will be backporting this change to my 1.1 code base though.


I think we are already in "pre-1.3.0 freeze" step. Kieran added a new field in the "bug" tracker to indicate if a task/bug/patch is planned for the 1.3.0 release (the field is "Planned Release", incredible, no? :) ). But currently, this task is not planned for 1.3.0...

>So any comments before I commit?


Yes, please, don't commit it in 1.3.0 if you can't fully test it.

>Let me know if you think the existence of the PBUF_RAM_NOCOPY type of pbuf should be controlled by an option.


I didn't have yet see all the details of your patch file, but if adding this new option don't make mandatory to change ethernet drivers, so, an option is not necessary to my point of view... I see than mainly like an optimization possible for netconn api/raw api users, I am wrong?)

>and I know Simon is itching to use NOCOPY pbufs in generic core code as it is,


Yes, but if we continue with such changes, it will be difficult to terminate 1.3.0... It could be a "never-ending story" !!! :)





Frédéric Bernon <fbernon>
Group Member
Mon 30 Jul 2007 09:59:35 PM UTC, comment #8: 

(background: relative newcomer to lwIP)

Reply to #7, Jonathan:

> With this change the notional abstraction for the netif layer output function is now very clear that it will behave as if it sent the packet immediately (e.g. like a polled driver). In practice this may mean copying the payload if it needs to.


I wrote my Ethernet driver completely zero-copy and I haven't had any issues yet -- what part of lwIP code expects to be able to alter a pbuf after it is submitted? (Maybe I haven't enabled that module yet!) A brief glance through the code seems to suggest that "some_output (p); pbuf_free(p);" is a common sequence of code. To require a driver to allocate a new buffer, copy the data, and then transmit seems to be a waste when the next statement upon return is often a pbuf_free anyway.

When I wrote the driver, I had to guess whether zero-copy would work (since I didn’t see anything in the doc or in the comments), and so far it has worked for me.

Jared

Jared Grubb <jgrubb>
Group Member
Mon 30 Jul 2007 07:23:23 PM UTC, comment #7: 

Hopefully the patch in the attachment is straightforward. It's a lot smaller than I thought it would be.

The next stage of this task will update the maintained ports and ppp.

Let me know if you think the existence of the PBUF_RAM_NOCOPY type of pbuf should be controlled by an option. The obvious issue being that the underlying driver will need to support freeing the pbuf on transmission. But maybe this just comes under the long list of things people need to change in ports and drivers for 1.3, and I know Simon is itching to use NOCOPY pbufs in generic core code as it is, so perhaps we should just make it a requirement for ethernet drivers.

With this change the notional abstraction for the netif layer output function is now very clear that it will behave as if it sent the packet immediately (e.g. like a polled driver). In practice this may mean copying the payload if it needs to. The upshot being that the payload is allowed to be changed by higher layers (whether that be the stack or the user) once the netif->output function returns. The exception of course being any pbuf marked as nocopy (PBUF_RAM_NOCOPY or PBUF_ROM) which is the user's guarantee that that will not happen, so the driver can reference the pbuf and its payload and for PBUF_RAM_NOCOPY,  makes the call to free it.

Fragmentation of a PBUF_RAM_NOCOPY pbuf poses an awkward problem - if we need to fragment, then we're creating new temporary pbufs and those are what are handed to the driver, and so the driver can't tell that it needs to free some original pbuf. So instead all we can do is create PBUF_REFs as before, which may be copied (despite the original pbuf being marked as not needing to be copied), and free the original on exit.

I'm not aware of any other "abuses" of pbuf internals like with ip_frag, but I'll have a look around - let me know if you know any.

Note that I have some trouble with testing here because I can't test current CVS - my port is still stuck in the 1.1 dark ages, and that's not likely to change till 1.3 comes out (or at least the code base has stabilised in preparation for 1.3 - I'm not in a position to play with unstable code and changing APIs, so will have to wait at least for a pre-1.3 feature freeze). I will be backporting this change to my 1.1 code base though.

So any comments before I commit?

Simon wrote:

> Wouldn't it be good to add an assertion into
> sockets.c:lwip_sendto() at the point where the pbuf is freed
> that there is no other reference to it? (i.e. p->ref == 1) If
> ref is > 1, the MAC still holds a reference (because the packet
> is still queued) and that might lead to wrong data being sent.
> If such an assert triggers, the user must switch to use
> PBUF_RAM(_NOCOPY?) instead.


An assert may be a good idea, although I note the problem cannot in fact be fixed by the user if they're using the sockets API. It is the sockets API internals which is calling netbuf_ref to make a PBUF_REF, and that's outside of user control. The model the stack must use now is that if a PBUF_REF needs to be queued in the driver, then it must be copied by the driver. It's the only safe way to do it.

Jifl


(file #13528)

Jonathan Larmour <jifl>
Group Member
Tue 03 Jul 2007 07:11:53 AM UTC, comment #6: 

Wouldn't it be good to add an assertion into sockets.c:lwip_sendto() at the point where the pbuf is freed that there is no other reference to it? (i.e. p->ref == 1) If ref is > 1, the MAC still holds a reference (because the packet is still queued) and that might lead to wrong data being sent. If such an assert triggers, the user must switch to use PBUF_RAM(_NOCOPY?) instead.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 09 Jun 2007 11:04:31 AM UTC, comment #5: 

Just so that we better find what we discussed about:
One of the 'original' postings about this issue was
"[lwip-devel] Packet queues (was Re: [bug #11400] ARPmulti-packet-queue modifies TCP ...)" on lwip-devel on the 5th April 2007. (I'm only writing this since I just had trouble to find it... :)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 Apr 2007 06:04:19 PM UTC, comment #4: 


>Protocols that send dynamic data can just send the char * and length directly, rather than having to wrap a pbuf around it. And tcp_write() already takes a 'copy' flag after all. Unless I'm misunderstanding what you're getting at?


With dynamic data I mean they have to create the data each time they're asked to send something (e.g. no static files or something). If they write to a private buffer they will have to present that buffer until it is finally sent (OK, that works already in conjuction with tcp_sent callback). But creating the data right into a pbuf you a) only need one pbuf instead of a buffer, a PBUF_REF and a PBUF_RAM for the headers. Also it is freed automatically.

It's not a must, only an idea for the future...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 Apr 2007 01:58:49 PM UTC, comment #3: 

Re comment #1: yep there's a lot of good things that become possible once we have a way to know what we need to do with a pbuf.

Re comment #2: Since TCP is stream-based, I'm not sure why people would want to use pbufs over the existing char and length. Protocols that send dynamic data can just send the char and length directly, rather than having to wrap a pbuf around it. And tcp_write() already takes a 'copy' flag after all. Unless I'm misunderstanding what you're getting at?


Jonathan Larmour <jifl>
Group Member
Mon 23 Apr 2007 10:35:35 AM UTC, comment #2: 

Also, it would be usefule to have a function like tcp_write() that accepts pre-filled pbufs. That way, an application can generate packet directly into a pbuf. For protocols that send dynamic data, this could completely eliminate copying. (together with a flag like 'copy' that indicates if the data should be copied or put on the segment-queue directly (the pbuf may not be changed by the application thereafter)).

We could provide a function tcp_pbuf_alloc() which takes care of the pbuf length so that a pbuf is not longer than one segment and it has the right type (PBUF_RAM_NOCOPY).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 23 Apr 2007 07:28:48 AM UTC, comment #1: 

Just a little comment so that we don't forget it:
Once we have that new flag, it would be good to be able to tell the stack at compile time wether the network driver needs queueing or not. If it needs queueing, UDP packets could be copied to a PBUF_RAM_NOCOPY directly instead of PBUF_REF for payload and PBUF_RAM for headers (which later would have to be copied on queueing). That would save us copying the headers, at least.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 10 Apr 2007 02:24:22 PM UTC, original submission:  

As discussed in thread "Packet queues" on ecos-devel around 5th April.

We would add a PBUF_RAM_NOCOPY variant of pbuf which users can craete. The effect of this is that the user is passing the pbuf to the stack for it to free. With a normal PBUF_RAM, it is expected that the user will free - the problem is that when the stack functions return, the user may change the pbuf or its payload, rather than free.

This new pbuf type means the stack can queue the pbuf if needed, and free it whenever it wants. Without the ability for a low level driver to hold the data for a little while during transmission, it is not possible to safely implement zero copy transmission.

This would be implemented as a different PBUF_FLAG bitfield. We would replace current equality tests of the pbuf flag type with macro tests, e.g. PBUF_IS_RAM(p), PBUF_NEEDS_COPY(p).

Jonathan Larmour <jifl>
Group Member

 

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

Attached Files
file #13528:  lwip.pbuf.nocopy.patch.txt added by jifl (11KiB - text/plain - First part of work to add nocopy of pbuf)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by vse (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by jgrubb (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jifl (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-03-27 jifl StatusNone Cancelled
        Open/ClosedOpen Closed
    2008-03-25 kieranm Planned ReleaseNone 1.4.0
    2008-03-25 kieranm Priority3 - Low 7 - High
    2007-08-29 kieranm Priority5 - Normal 3 - Low
    2007-07-31 jifl Carbon-Copy- Added kieranm
    2007-07-30 jifl Attached File- Added lwip.pbuf.nocopy.patch.txt, #13528
        Percent Complete0% 50%

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code