lwIP - 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
Submitter: | Leon Woestenberg <likewise> | ||
Submitted: | Sun 26 Dec 2004 03:52:15 PM UTC | ||
Category: | ARP | Severity: | 1 - Wish |
Item Group: | Change Request | Status: | Fixed |
Privacy: | Public | Assigned to: | 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: |
Simon Goldschmidt <goldsimon> |
Tue 17 Apr 2007 12:55:27 PM UTC, comment #49: OK, closing it as fixed. |
Simon Goldschmidt <goldsimon> |
Tue 17 Apr 2007 11:03:00 AM UTC, comment #48: No more to add from me. |
Jonathan Larmour <jifl> |
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> |
Wed 11 Apr 2007 06:56:58 PM UTC, comment #46: I've checked in the new patch:
|
Simon Goldschmidt <goldsimon> |
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."
|
Jonathan Larmour <jifl> |
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.
|
Simon Goldschmidt <goldsimon> |
Thu 05 Apr 2007 11:44:39 AM UTC, comment #43: From the changelog when this feature was introduced:
|
Kieran Mansley <kieranm> |
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?
|
Jonathan Larmour <jifl> |
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 :-)
|
Simon Goldschmidt <goldsimon> |
Wed 04 Apr 2007 02:23:57 PM UTC, comment #40: From visual inspection:
|
Jonathan Larmour <jifl> |
Tue 03 Apr 2007 08:41:11 PM UTC, comment #39: Hohoho! here it is. |
Simon Goldschmidt <goldsimon> |
Tue 03 Apr 2007 03:07:57 PM UTC, comment #38: Simon, I think you forgot to attach the patch :-). |
Jonathan Larmour <jifl> |
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> |
Wed 28 Mar 2007 01:03:20 PM UTC, comment #36:
|
Simon Goldschmidt <goldsimon> |
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.
|
Jonathan Larmour <jifl> |
Mon 26 Mar 2007 05:08:26 PM UTC, comment #34: Ok, I've looked through the code a little.
|
Simon Goldschmidt <goldsimon> |
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> |
Mon 26 Mar 2007 01:22:25 PM UTC, comment #32: Simon, what would you like to do? |
Jonathan Larmour <jifl> |
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.
|
Kieran Mansley <kieranm> |
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.
|
Jonathan Larmour <jifl> |
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> |
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).
|
Jonathan Larmour <jifl> |
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.
|
Kieran Mansley <kieranm> |
Mon 26 Mar 2007 11:57:02 AM UTC, comment #26: Re comment 25:
|
Jonathan Larmour <jifl> |
Mon 26 Mar 2007 11:42:11 AM UTC, comment #25: re comment #24:
|
Simon Goldschmidt <goldsimon> |
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.
|
Jonathan Larmour <jifl> |
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> |
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):
|
Kieran Mansley <kieranm> |
Fri 23 Mar 2007 07:51:10 AM UTC, comment #21: Re comment #20
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.
|
Simon Goldschmidt <goldsimon> |
Fri 23 Mar 2007 05:12:10 AM UTC, comment #20: Re Simon's comment #15
|
Jonathan Larmour <jifl> |
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.
|
Simon Goldschmidt <goldsimon> |
Thu 22 Mar 2007 01:37:35 PM UTC, comment #18: Blast. They do, don't they.
|
Kieran Mansley <kieranm> |
Thu 22 Mar 2007 01:06:53 PM UTC, comment #17: I'm on it, although Jonathan seems to be back, too ;-)
|
Simon Goldschmidt <goldsimon> |
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.
|
Kieran Mansley <kieranm> |
Thu 22 Mar 2007 11:17:25 AM UTC, comment #15: (to comment #12):
|
Simon Goldschmidt <goldsimon> |
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.
|
Kieran Mansley <kieranm> |
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.
|
Jonathan Larmour <jifl> |
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.
|
Jonathan Larmour <jifl> |
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.
|
Simon Goldschmidt <goldsimon> |
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...
|
Simon Goldschmidt <goldsimon> |
Fri 09 Mar 2007 05:00:53 AM UTC, comment #9: Memory corruption was introduced in the following commit:
|
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.
|
Simon Goldschmidt <goldsimon> |
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> |
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...
|
Simon Goldschmidt <goldsimon> |
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> |
Mon 26 Feb 2007 03:43:02 PM UTC, comment #4: I have tracker access, neat, thanks :).
|
Jonathan Larmour <jifl> |
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> |
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> |
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> |
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.
|
Leon Woestenberg <likewise> |
Depends on the following items: None found
Items that depend on this one: None found
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 13 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2007-04-17 | goldsimon | Status | Ready For Test | Fixed | |
Open/Closed | Open | Closed | |||
2007-04-11 | goldsimon | Status | In 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 to | jifl | 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 | Status | Postponed | In Progress | |
2007-02-26 | jifl | Severity | 3 - Normal | 1 - Wish | |
Item Group | Faulty Behaviour | Change Request | |||
Assigned to | leonwoestenberg | jifl | |||
2005-01-03 | likewise | Status | None | Postponed |
Powered by Savane 3.13-f8d8.
Corresponding source code
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.