buglwIP - A Lightweight TCP/IP stack - Bugs: bug #11400, ARP multi-packet-queue modifies...

 
 

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

bug #11400: ARP multi-packet-queue modifies TCP unsent/unacked segment packet pbuf chain into packet queue

Submitted by:  Leon Woestenberg <likewise>
Submitted on:  Sun 26 Dec 2004 03:52:15 PM UTC  
 
Category:  ARP Severity:  1 - Wish
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

( Jump to the original submission)

Fri 04 May 2007 09:35:18 PM UTC, comment #50: 

I've checked in a modified version of etharp.c and pbuf.c so that pbuf_copy() does not allocate the target pbuf and can copy to chained pbufs also (PBUF_POOL).

I'd be happy to have some test comments.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 17 Apr 2007 12:55:27 PM UTC, comment #49: 

OK, closing it as fixed.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 17 Apr 2007 11:03:00 AM UTC, comment #48: 

No more to add from me.

Jonathan Larmour <jifl>
Project Member
Tue 17 Apr 2007 09:07:33 AM UTC, comment #47: 

Can we close this or is there still something to add?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 11 Apr 2007 06:56:58 PM UTC, comment #46: 

I've checked in the new patch:
- removed pbuf_take(), pbuf_queue() and pbuf_dequeue() since they were only used for arp-queueing
- call pbuf_copy() for every pbuf being queued except PBUF_ROM
- use a PBUF_RAM instead of PBUF_POOL for the copy (previous versions (pbuf_take) always tried PBUF_POOL first)

Still, I'm leaving this open, though read-for-test since I'd like some feedback.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sat 07 Apr 2007 03:57:10 AM UTC, comment #45: 

From what Kieran both quoted and said himself, step 2 is certainly right. And steps 4 & 5 are allowed - hence "on return 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."

I'm not sure quite what you mean by "a copy flag is not needed" because it would be needed for UDP if we wanted to do anything in future to avoid a copy using the existing pbuf types. In fact with TCP it's even more silly - the user may have set the copy flag to tcp_write() so we know the payload data is not going to change - however the ARP layer doesn't know this as it doesn't get passed that information, so when queuing, the ARP layer will copy the data regardless.

Although Kieran says we should set aside the issue of optimising this for now, I'll just mention that I think a solution to this would be my suggestion on lwip-devel of a PBUF_RAM_NOCOPY pbuf type - TCP data to be copied could be created as this type instead of PBUF_RAM as it is at present in tcp_enqueue(). And pbuf_ref() called on it in tcp_output() before ip_output() so subsequent lower layers don't really release the memory when they pbuf_free() it.

Anyway, that's an idea for the future, not now - Kieran recommended solving the current issue first, and then work on optimisations later. I just wanted to raise the possibility of that as a medium term solution, but it's probably best not to further discuss its merits in this bug now to avoid confusing the issue.

Jonathan Larmour <jifl>
Project Member
Thu 05 Apr 2007 07:51:23 PM UTC, comment #44: 

In my opinion, comment #34 had nothing to do with PBUF_RAM types. If we have a queue of only RAM types, ref would be enough. But OK, when the user can re-use a PBUF_RAM after giving it to udp_send(), then it must also be copied.

re to #43:

> An external application shouldn't be using PBUF_RAM buffers to refer to its own memory


Of course not to to refer to it's own memory. But I can imagine an app like this:

- udp_recv callback is called
- app generates a unique packet into a PBUF_RAM
- calls udp_send() with that
- modifies the contents
- calls udp_send() again

So, is step 2 wrong? And wether step 4 & 5 are allowed or not, we have to decide.

By the way, it's a different thing with tcp than udp: udp_send() gets a pbuf while tcp_write() is given a buffer pointer. we have the pbuf types because we decided earlier if we copy or ref the data into a pbuf. A copy flag is not needed! I still think that it is dangerous to modify any ref-counted object that is referenced more than once!

But as a first start, I propose to take over the newest patch pasted here and call pbuf_copy() on any pbuf except PBUF_ROM

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 05 Apr 2007 11:44:39 AM UTC, comment #43: 

From the changelog when this feature was introduced:
(0.6.1)

  ++ New features:

  * The packet buffer implementation has been enhanced to support
    zero-copy and copy-on-demand for packet buffers which have their
    payloads in application-managed memory.
    Implemented by David Haas.

    Use PBUF_REF to make a pbuf refer to RAM. lwIP will use zero-copy
    if an outgoing packet can be directly sent on the link, or perform
    a copy-on-demand when necessary.

    The application can safely assume the packet is sent, and the RAM
    is available to the application directly after calling udp_send()
    or similar function.

An external application shouldn't be using PBUF_RAM buffers to refer to its own memory; it should instead use PBUF_REF (if the memory contents can change) or PBUF_ROM (if it can guarantee it will be preserved as long as lwip needs it).  PBUF_RAM should only be used to refer to buffers whose payload points to a region of memory allocated by lwIP.  However, this might not be relevant because on return 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. 

We could add complexity/efficiency by working out if it's a TCP or UDP packet, and then not doing as much copying in the TCP case, but for the first pass I think that is unnecessary.  Let's get it working first.

Another alternative would be to extend the API to add the copy/no-copy flag for UDP so that we can tell if the application is going to try and reuse the buffer when the udp_send() call returns, but again I think we should get it working first and worry about making it more efficient later.

Kieran Mansley <kieranm>
Project Member
Thu 05 Apr 2007 10:39:51 AM UTC, comment #42: 

Re copying PBUF_RAMs: that's what I was discussing in comment #35 (and was what I thought you had raised in comment #34). As far as I know, users are allowed to reuse data including pbufs without needing to check their ref count. That's why there's a "copy" flag to netconn_write/tcp_write, for example. I assume UDP or other protocols (ICMP, and whatever other packets artificially constructed by the user etc.) are analogous. Although at the same time, we don't need to copy a PBUF_RAM if it's part of a TCP seg - we know that is controlled by lwIP. Is there any way we can detect and avoid a pbuf_copy for this?

For UDP and other protocols, we theoretically could change the API contract here and say that users can't reuse pbufs (or their payload) until their ref count returns to 1 (from a higher value set within the stack). I don't think that's right personally, but it's Kieran's call really.

Jonathan Larmour <jifl>
Project Member
Wed 04 Apr 2007 06:46:02 PM UTC, comment #41: 

You're right about the returned pbuf. I think I was a little confused of the names and because of the two chains, which made it complicated :-)

And I'm sorry I completely forgot about the PBUF_POOL/_RAM issue.
Using a PBUF_RAM, the whole function gets simpler, since the target pbuf is not a chain.

>I thought that after all we would also need to copy RAM pbufs


Hm, I don't know if we have to copy PBUF_RAM types (and IF yes, we would have to copy everyting but PBUF_ROM). The contents of a pbuf may generally not be changed after giving it to the stack as long as it is referenced more than once (until the stack does not reference it any more). And as long as that's true, we're safe. The problem with PBUF_REF is that it points to other data that may be changed at any time after sending.

If the following should be OK for an application:

- create pbuf p
- copy data into it
- udp_send(p)
- change contents of p->payload

then we need to copy everything except PBUF_ROM. But my experience with refcounting frameworks showed me you're not allowed to change an object you give someone else to reference.
But as I don't know how lwip should handle this, I can't make the decision.

Finally, I'm attaching the patch with a new, smaller version of pbuf_copy(). I only try PBUF_RAM now. In my opinion, that's enough and we don't have to revert to PBUF_POOL.

(file #12399)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 04 Apr 2007 02:23:57 PM UTC, comment #40: 

From visual inspection:
pbuf_copy returns q. It should return r. I think q could be renamed to something like p_copy, and r to p_copy_head to make their purposes clearer.

An assert should trigger at the point it says:
       /* this should not happen! */
I don't think we should control programming errors - that's what asserts are for, and if we're too defensive, that adds code bloat to lwip.
So it should instead it should just be:
LWIP_ASSERT( "pbuf_copy: Reached unexpected end of copy", q != NULL );

Next, there's an assumption that if the current chunk of p doesn't fit into the current chunk of q, then after we copy part of p to the current q and move q to the next q, the rest of p will fit into that next q. Instead there should probably be something like a "continue" to restart the loop (and the offset into p preserved) at the end of the "if(p->len > rem_len)" block.

As per comment #34 and comment #35, I thought that after all we would also need to copy RAM pbufs because the user may otherwise be allowed to change data in the pbuf once control is returned to them (this is a weakness of the API).

A final comment - I think a PBUF_RAM should be attempted before resorting to PBUF_POOL. PBUF_POOL packets may be much much larger than the data for this packet needs (and the size of PBUF_POOL packets is normally chosen by the user according to the requirements of receive data, not outgoing data).

Thanks,

Jifl

Jonathan Larmour <jifl>
Project Member
Tue 03 Apr 2007 08:41:11 PM UTC, comment #39: 

Hohoho! here it is.

(file #12389)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 03 Apr 2007 03:07:57 PM UTC, comment #38: 

Simon, I think you forgot to attach the patch :-).

Jonathan Larmour <jifl>
Project Member
Mon 02 Apr 2007 08:21:57 PM UTC, comment #37: 

Here's the new patch using pbuf_copy() if there is a PBUF_REF in p, or pbuf_ref() else. Please comment, or else I'll check it in in a few days :) (as the current version is buggy, I think this is rather pressing)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 28 Mar 2007 01:03:20 PM UTC, comment #36: 

>Hmmm, oh dear it looks like you're right. I hadn't thought there would be a problem because I assumed the ref count would sort it out.


It would, I think, if pbuf_ref() increased the ref-count of all packets in a chain instead of only the first one.

About including a 'copy' flag in UDP: the UDP raw API is somewhat different than the TCP: TCP is given a data and length pointer, here it is ok to give a copy flag. UDP, however, is directly given a pbuf. Instead of a copy flag, the application can directly copy the data into a PBUF_RAM. Only lwip_send() doesn't do that! (Which I think is good -regarding speed-, and the only point this doesn't work is ARP queueing!)

So, I think I will commit the pbuf_copy() stuff after having a second look over it.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 28 Mar 2007 10:16:47 AM UTC, comment #35: 

Hmmm, oh dear it looks like you're right. I hadn't thought there would be a problem because I assumed the ref count would sort it out. But the API contract with the user is not like that. This issue - whether chains survive a call - is exactly the same as with TCP, where it may get put on a queue. But in that case there is a "copy" flag (originating from netconn_write) to indicate what should be done. By the time the code path reaches ARP, that information is lost. And in the case of UDP and netconn_send, it never existed.

So unless we change the UDP raw API to also have a 'copy' flag, and then pass that around (or perhaps better, add in a new different pbuf flag e.g. PBUF_FLAG_COPY). At which point it clearly becomes far too much effort, so Simon's pbuf_copy() implementation is probably the best thing to do right now after all. We should consider changing the API for the future to allow this optimisation.

About the last question and pbuf_ref. Yes you must not call pbuf_free(b). And there's a lot of API stuff which should be pointed out more clearly IMHO!

Jonathan Larmour <jifl>
Project Member
Mon 26 Mar 2007 05:08:26 PM UTC, comment #34: 

Ok, I've looked through the code a little.

If the first pbuf is of type REF, we can simply copy it, let the new copy point to the second pbuf (if the second is also REF the same for that one).

Whether we can simply re-allocate the middle of a pbuf-chain is a question of whether we want user-given chains to survive a call to lwip functions or not.

E.g. if udp_send() was called with a 2-pbuf-chain RAM->REF and we move the REF to RAM, we change the first pbuf in the chain. If the user uses this pbuf again (e.g. just trying to re-send it while changing the contents behind the REF) it will lead to (in my view) unexpected behaviour, wouldn't it?

Another question (to me) is the way pbuf_ref() increases the ref-count: if I have 2 pbufs and I chain the writing "a->next = b" and send them using udp_send(), I simply may not call pbuf_free(b) later, is that correct?
I agree that is faster than updating the refcount of the whole chain (in pbuf_ref()), but I think that should be pointed out more clearly!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Mar 2007 01:46:40 PM UTC, comment #33: 

The RAM-saving idea is sure best. Only I need some time to re-think it. Just want to make sure this always works...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Mar 2007 01:22:25 PM UTC, comment #32: 

Simon, what would you like to do?

Jonathan Larmour <jifl>
Project Member
Mon 26 Mar 2007 01:16:45 PM UTC, comment #31: 

I don't particularly mind either way, I was just aware that the other way was simpler to implement, and in most cases ARP is not going to be queuing a lot of data so didn't think it was an important case to optimise.

If you feel strongly that it should be done this way, you're welcome to go ahead - I'll let you and Simon sort this one out!

Kieran Mansley <kieranm>
Project Member
Mon 26 Mar 2007 01:08:58 PM UTC, comment #30: 

Oh yes, that is the same thing - I thought it was describing something different, sorry.

I think this is quite an important "optimisation". If someone's using a PBUF_REF in the first place, it's probably because the  data, of which the pbuf is a part, is potentially large (at least compared to free memory) and they want/need to be particularly space efficient.

I also don't think it's that much more complex.

newp->next = oldp->next;
newp->tot_len = oldp->tot_len;
pbuf_ref(newp->next);

Adding this after Simon's done won't work if Simon is going to take the pbuf_copy() approach. Simon's work would need to be undone and replaced with something more like pbuf_take() again, making Simon's efforts somewhat pointless. After all, pbuf_copy() was only needed when you didn't want to change the original pbufs.

Jonathan Larmour <jifl>
Project Member
Mon 26 Mar 2007 12:41:24 PM UTC, comment #29: 

What you suggest is basically the second/further optimisation discussed in comment #18, and we could do this, but the added complexity didn't seem worth it.  We can always do this if it turns out to be necessary as it's an incremental addition on top of what is currently being done.

Kieran Mansley <kieranm>
Project Member
Mon 26 Mar 2007 12:26:22 PM UTC, comment #28: 

But if it's the first pbuf of the packet, you only need to copy that pbuf, and then increase the reference count of the very next pbuf to stop it, and the ones after it, being freed (pbufs in a chain implicitly having the ref count of their predecessors as well as their own ref count).

I could just be wrong of course :-)

Jonathan Larmour <jifl>
Project Member
Mon 26 Mar 2007 12:08:38 PM UTC, comment #27: 

As I understand it though there is a problem if it is the first pbuf in the packet (other way round to Simon in comment #25) as then if it is part of a queue something else which you don't have access to right now will be pointing at it.  This other pointer would need changing. 

I think I'd prefer to go with copying out any REF containing packets.  Other things might work, but there will be some tricky corner case (e.g. application making it's own REF pbufs and sending through TCP) which will break some of our assumptions and things will go wrong in strange ways. 

Assigning to Simon.

Kieran Mansley <kieranm>
Project Member
Mon 26 Mar 2007 11:57:02 AM UTC, comment #26: 

Re comment 25:

What I was saying in comment 24 was, though, that you can change the ->next pointers of the other parts of the packet.  You can insert a single PBUF_RAM pbuf in place of the PBUF_REF pbuf, and maintain the rest of the chain.

Jonathan Larmour <jifl>
Project Member
Mon 26 Mar 2007 11:42:11 AM UTC, comment #25: 

re comment #24:

I think you can't simply copy only one PBUF_REF part of a packet since that would include changing the ->next pointers of the other parts of the packet. This would only work if the PBUF_REF was the first pbuf in the packet, which it never will be since all headers (IP / TCP / UDP) are allocated from _RAM at send time.

Regarding the PBUF_POOL: I think it's a good idea to reserve this for receiving in order not to block memory by tx needed to receive ACK frames, for example. Different sized pools including some low-water-mark for RX would be better (in fact, I'm using those pools, also not with the low-water-mark, for now. But I think we already had that on lwip-users...)
But as long as it's that way, I'm sticking to PBUF_RAM for this solution.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Mar 2007 11:31:24 AM UTC, comment #24: 

The reason I was saying about PBUF_REFs not coming from TCP was that it was only TCP, not UDP which could also have the packet on a different queue. So I believe we should be able to just reuse and queue the pbufs directly after all.

Also if we're copying, we should only need to copy the PBUF_REFs from the chain, not the whole chain i.e. the whole packet. If nothing else this would be good because the whole pbuf may be large, so is far less likely to permit allocation with a single pbuf_alloc, which has to allocate one continuous chunk.

Ideally I think PBUF_POOL should be kept for incoming packets only where possible. Using a 1500 byte buffer for 60 byte data isn't so good.

Sorry I failed to see the existing etharp_q_entry stuff - I could have sworn I updated my CVS.

About the memory leak (point 3 in comment 21) .... if the packet has been passed to the ARP layer for it to later free, then it shouldn't be calling pbuf_ref() on it at all. So I think you might be right that there's a problem here, but the fix should be simple - remove the pbuf_ref in etharp_query. I confess I'm not 100% certain and would have to think about it a bit more.

About Simon's point 4 in comment 21, you should never be in a position where an individual pbuf in a queue gets deleted, as pbuf_queue() (currently) will have pbuf_ref()d every packet (other than the head).

Jonathan Larmour <jifl>
Project Member
Mon 26 Mar 2007 11:29:43 AM UTC, comment #23: 

I think that's about it. And indeed, I'm nearly there already, I have exactly this on my PC at home. I'm going to test it and add a patch here as soon as I find the time.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Mar 2007 10:43:26 AM UTC, comment #22: 

To summarise (please correct where wrong, some of this is based on what others have written, rather than my own investigations):

 - TCP won't result in a PBUF_REF pbuf getting as far as ARP as it's either copied (into a PBUF_RAM), or PBUF_ROM is used.  ROM is OK because it will exist until ACKed, which is much more than the ARP requirements.

 - UDP may result in a PBUF_REF pbuf getting to the ARP layer, and in this case it is necessary to do something about it because as soon as the call returns to the app it can reuse the memory.  If this wasn't the case a PBUF_ROM would have been used (as that's essentially the difference between them).

 - As a result we need to preserve any PBUF_REF pbufs, and the easiest thing to do seems to be to copy them.  It shouldn't happen often, so overhead isn't great.

I'm still happy to take this approach (from comment #18):
"How about we have two paths: if there are any REF pbufs in the chain, we do pbuf_copy() as per your new patch. If it doesn't have any REF pbufs, we can just do pbuf_ref() and put it in the arp queue. Makes the code a bit more complex, but more efficient (both less memory and faster) for the case where not using PBUF_REF, which will be quite common."

Any objections?  If not, I suggest Simon takes this (and the associated purge of the related but now unnecessary code) as he's nearly there already.

Kieran Mansley <kieranm>
Project Member
Fri 23 Mar 2007 07:51:10 AM UTC, comment #21: 

Re comment #20

1. About copying REF pbufs: That was not my idea, it's the way it was for a long time now, using pbuf_take(). If we wouldn't need this, things would get easier. I didn't look into this direction, yet. At least udp sockets send their data using PBUF_REFs without asking the user. This is normally no problem since the only delay for sending out UDP datagrams is the network hardware (-> AND arp queueing).

>I do not see a way for a PBUF_REF to get sent to the ARP layer from TCP.


I'm not sure what you're saying with this. You mean TCP layer does not 'create' PBUF_REFs and send them to ARP layer?

2.

>"that's the way it's solved right now (in CVS HEAD)"

etharp.c version 1.102 (2 weeks old) enqueues structs etharp_q_entry (taken from a new memp-pool) instead of enqueueing the pbufs directly. In this way, the pbufs are not changed.

3. The memory leak I'm implying comes if the head of the pbuf is a PBUF_REF. Then, pbuf_take() will change this pbuf into PBUF_POOL or _RAM which later (through pbuf_ref) has a refcount of 2, although only ARP knows about it. So after ARP frees it, it will be left with a refcount of 1, although it's unreferenced. (At the same time, the PBUF_REF was freed but it's still referenced by an upper layer.)

Now I don't know if this ever happens, since the first pbuf in a packet (the IP-header) would be a REF type (probably does not happen?). But then it should be taken care of (for the future) by checking it.

4. (todo:) The ARP_QUEUEING as it is only references the first pbuf in a queue since pbuf_ref() only calls ref++ for the first pbuf in a queue -> the others could be freed before sending.

SO: as long as we could say PBUF_REF types are still valid later, we could throw away the pbuf_copy/pbuf_take code and simply call pbuf_ref() for the whole queue. But at least regarding UDP sockets, I think we can't. And since we mustn't delete pbufs in given to us since they are probably referenced by others, the only solution I see is to make a copy of the whole packet if it contains a PBUF_REF.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 23 Mar 2007 05:12:10 AM UTC, comment #20: 

Re Simon's comment #15

I may be missing something here, but I'm a bit worried we're all getting our knickers in a twist unnecessarily. Simon was worried about this data also existing in the TCP send queue. Looking at tcp_enqueue(), the data must either already have been marked by the user as not needing to be copied, or it will be copied into a PBUF_RAM. I do not see a way for a PBUF_REF to get sent to the ARP layer from TCP. I'm perfectly willing to be corrected on this :-).

Secondly, Simon says "that's the way it's solved right now (in CVS HEAD)". What are you referring to here? Using a list of small structures is not what's in CVS.

Finally, you imply there would be a memory leak because of the pbuf_ref. But that's what the ARP timer is for - it will pbuf_free the queue (via its head) when the timeout has expired for that entry.

Jonathan Larmour <jifl>
Project Member
Thu 22 Mar 2007 02:06:37 PM UTC, comment #19: 

Using pbuf_copy() only if there is a PBUF_REF in the queue would be best, I think.

Copying only if the first pbuf is a _REF could also do the trick but I don't know the full code in detail, so I don't know if modifying an existing pbuf queue would do harm to other areas of the stack.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 22 Mar 2007 01:37:35 PM UTC, comment #18: 

Blast.  They do, don't they.

How about we have two paths: if there are any REF pbufs in the chain, we do pbuf_copy() as per your new patch.  If it doesn't have any REF pbufs, we can just do pbuf_ref() and put it in the arp queue.  Makes the code a bit more complex, but more efficient (both less memory and faster) for the case where not using PBUF_REF, which will be quite common.

Then we can get rid of pbuf_take(), pbuf_queue() and pbuf_dequeue() altogether.

We could be even more complex and efficient by only doing the copy if the first pbuf in chain is a PBUF_REF.  Any other PBUF_REFs would get the pbuf_take() treatment, and everything else would get a pbuf_ref().  However, I'm not sure this further optimisation is worth the extra complexity.

Kieran Mansley <kieranm>
Project Member
Thu 22 Mar 2007 01:06:53 PM UTC, comment #17: 

I'm on it, although Jonathan seems to be back, too ;-)

How would you just change PBUF_REF to just be PBUF_RAM? Since they come from different pools (or the heap), and are freed depending on their type, I see no way to do it for now!
(That's why I introduced the pbuf_copy(), anyway)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 22 Mar 2007 12:02:28 PM UTC, comment #16: 

Right, so we need to work out why the current implementation isn't working (because it should) rather than just copying the whole lot.

Sounds to me like pbuf_take() has some bugs in it.  The way I'd like it to work is:
 - rather than allocate a new pbuf structure for any REF types, just change the existing pbuf structure to be a RAM type and copy the data.  This should avoid the problem where something else has a reference to the pbuf structure.
 - this should also sort out the pbuf_ref() problem, as the ref count should still reflect all those people who have a handle on this pbuf.  I'd like to see the pbuf_ref() call move into pbuf_take() as it is essentially part of that operation.

Also, pbuf_queue() is no longer used and could be purged.

Any volunteers?

Kieran Mansley <kieranm>
Project Member
Thu 22 Mar 2007 11:17:25 AM UTC, comment #15: 

(to comment #12):

I agree that copying to PBUF_RAM is not a good idea. Using PBUF_POOL would be my favourite (and I would change it to that before checking in).

Regarding the original method: that's the way it's solved right now (in CVS HEAD). But I'm not sure this solves all problems:

1. If the first pbuf in a chain given to pbuf_take() is a PBUF_REF, you will change that pbuf into a PBUF_POOL (or PBUF_RAM) and free the old PBUF_REF. But tcp might be referencing  that initial pbuf directly (in the send-queue) and so it now might point to invalid data!

2. Calling pbuf_ref later on this new pbuf queue, the first pbuf will get a refcount of 2 but only ARP knows of it -> memory leak.

The reason I suggested this new patch is that it is known nowhere in the stack how long packets are being queued. By copying queued packets into new pbufs, one could minimize the effect ARP queueing has on the rest of the stack.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 22 Mar 2007 08:55:42 AM UTC, comment #14: 

I'm with Jonathan and Leon on this one: creating a little wrapper struct to hold a list of pbufs would be preferable, but I'm also happy enough with just queuing one packet (the workaround as it stands at the moment) which is why this is marked with such low severity.

Jonathan - are you still interested in this?  It's currently assigned to you.

Kieran Mansley <kieranm>
Project Member
Thu 22 Mar 2007 05:44:04 AM UTC, comment #13: 

Oh, on rereading my last comment, I realised it might have sounded unintentionally rude. All I meant is that this is just my opinion, and if others think copying to a PBUF_RAM is the best way forward, then that's ok.

Thanks for your work on this Simon. Sorry I've been off the ball for a while - sent abroad at short notice.

Jonathan Larmour <jifl>
Project Member
Thu 22 Mar 2007 04:00:23 AM UTC, comment #12: 

My opinion is that I don't think this approach is a good idea. It could mean a lot of potentially large pbufs having to be allocated from the PBUF_RAM pool, with very little determinism to their sizes. Determinism is why lwip has memp's, which intentionally limit resources.

But I'm not advocating allocating these copied pbufs from a distinct memory pool. Instead it seems much more efficient to create a small structure which points to each pbuf being queued. Thus you only need to allocate space for this structure (which can come from a memp pool) and can still leave the original pbufs untouched. This is the same approach advocated by Leon in the very first post in this bug.

Jonathan Larmour <jifl>
Project Member
Wed 21 Mar 2007 09:38:28 PM UTC, comment #11: 

I think the current state is still not free from modifying a present queue of pbufs.

To solve this, I changed the function pbuf_take() (only used by ARP) into pbuf_copy() which copies a chained pbuf into one big PBUF_RAM. This way, we don't change the original pbufs in any way. It may be slower, but it's only used at connection start.

Please see attached file.

(file #12228)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 09 Mar 2007 07:33:30 AM UTC, comment #10: 

Hey, someone tested it! Thanks for the comment, that happens if you use cut&paste...
I've corrected it in CVS HEAD

But I've got another (code-reading) problem with that patch (I want to test that on the weekend some time): in etharp.c, If you enqueue pbufs, pbuf_take() is called (which is now the only call to that function, by the way). There, PBUF_REFs are copied to new PBUF_RAMs. I think we still corrupt a given pbuf chain in that case.

Maybe a solution would be to copy enqueued pbufs to a new location no matter what type they are?

We don't event need pbuf_take any more, do we? Also, we don't need pbuf_queue() and pbuf_dequeue() any more since it was #if ARP_QUEUEING

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 09 Mar 2007 05:00:53 AM UTC, comment #9: 

Memory corruption was introduced in the following commit:
http://lists.gnu.org/archive/html/lwip-commits/2007-03/msg00003.html
I have attached the fix.

(file #12158)

Dmitry Potapov <dpotapov>
Wed 07 Mar 2007 03:40:06 PM UTC, comment #8: 

Hm, seems like the pbuf corruption did not come frome the ARP queueing but from the pbuf_alloc() patch I applied to get rid of a signed/unsigned assigment warning. Seems like I have to revert that.

There is some work left getting rid of all the warnings, though.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 07 Mar 2007 01:07:09 PM UTC, comment #7: 

Yes, queueing just one packet is not such a bad workaround, and seems to be working for most people.

Kieran Mansley <kieranm>
Project Member
Wed 07 Mar 2007 01:02:19 PM UTC, comment #6: 

Bad news: My patch does not seem to work. It somehow still breaks pbufs... This seems to need some attention...

I think it would be best to go back to the old code (only one packet in queue) and leave the new code in the file (surrounded by #if NEW_ARP_QUEUEING or something to be able to switch between the two solutions).

OK?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Feb 2007 03:46:20 PM UTC, comment #5: 

Hoho, goldsimon beat me to it. I'll keep the bug assigned to me, so I can help out and review the patch at least.

Jonathan Larmour <jifl>
Project Member
Mon 26 Feb 2007 03:43:02 PM UTC, comment #4: 

I have tracker access, neat, thanks :).

Although I probably won't be able to do it straight away, I'm happy to take this one. I've updated the Severity and Item Group in line with its updated status now that the present code does at least work.

Jonathan Larmour <jifl>
Project Member
Mon 26 Feb 2007 03:29:11 PM UTC, comment #3: 

I'm working on that, in my free time so it could take a week or two until I have the time to test it... But I'll submit a patch.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Feb 2007 03:10:49 PM UTC, comment #2: 

Any volunteers to take this further?  If no one steps forward I'll close it as "won't fix" as we have what seems to be an acceptable work around.

Kieran Mansley <kieranm>
Project Member
Mon 03 Jan 2005 06:05:35 PM UTC, comment #1: 

Bug is reported to dissappear when the ARP queueing is limited to one packet per entry, so I limited the implementation to that. That fixes the bug, but leaves future work to see if a multi-packet queue can be implemented neatly.

Leon Woestenberg <likewise>
Project Member
Sun 26 Dec 2004 03:52:15 PM UTC, original submission:  

When multiple packets are queued on one ARP entry, the first packet has a next pointer pointing to the next packet.

However, if this was a queued TCP segment, we are modifying it, without TCP knowing about this.

Possible solutions: disallow multiple packets on the ARP queue, or modify TCP to know about packet queues.

Optimal solution is probably to design a seperate struct for ARP queues, much like the 'struct tcp_seg' linked list.

Leon Woestenberg <likewise>
Project Member

 

Attached Files
file #12399:  arp_q3_pbuf.patch added by goldsimon (10KiB - text/x-patch)
file #12389:  arp_q2.patch added by goldsimon (17KiB - text/x-patch)
file #12228:  lwip_pbuf_copy.patch added by goldsimon (7KiB - text/x-patch)
file #12158:  BUG-11400-2.patch added by dpotapov (643B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dpotapov (Updated the item)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  •  

    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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-04-17 goldsimon StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2007-04-11 goldsimon StatusIn Progress => Ready For Test
    2007-04-04 goldsimon Attached File- => Added arp_q3_pbuf.patch, #12399
    2007-04-03 goldsimon Attached File- => Added arp_q2.patch, #12389
    2007-03-26 kieranm Assigned tojifl => goldsimon
    2007-03-21 goldsimon Attached File- => Added lwip_pbuf_copy.patch, #12228
    2007-03-09 dpotapov Attached File- => Added BUG-11400-2.patch, #12158
    2007-02-28 kieranm StatusPostponed => In Progress
    2007-02-26 jifl Severity3 - Normal => 1 - Wish
        Item GroupFaulty Behaviour => Change Request
        Assigned toleonwoestenberg => jifl
    2005-01-03 likewise StatusNone => Postponed

    Back to the top


    Powered by Savane 3.5