lwIP - A Lightweight TCP/IP stack - Bugs: bug #27352, Change ip_addr from struct to...
You are not allowed to post comments on this tracker with your current authentication level.
bug #27352: Change ip_addr from struct to typedef (u32_t)
Submitter: | Simon Goldschmidt <goldsimon> | ||
Submitted: | Sun 30 Aug 2009 09:11:40 PM UTC | ||
Category: | IPv4 | Severity: | 3 - Normal |
Item Group: | Change Request | Status: | Fixed |
Privacy: | Public | Assigned to: | goldsimon |
Open/Closed: | Closed | Planned Release: | None |
lwIP version: | CVS Head |
Jump to the original submission
Sat 22 May 2010 09:14:54 PM UTC, comment #63: |
Simon Goldschmidt <goldsimon> |
Thu 20 May 2010 06:15:36 PM UTC, comment #62: From reading the code, the only unaligned fields might be struct netbios_name_hdr.netbios_name_hdr or .addr. But then again, that's contrib code, not core lwIP code, so if you're not using this, you shouldn't need packing on a 32-bit platform when ETH_PAD_SIZE==2. I'll give it a try, too.
|
Simon Goldschmidt <goldsimon> |
Wed 19 May 2010 08:32:44 PM UTC, comment #61: Wait, I see your point about aligned packets. I DO have ETH_PAD_SIZE 2, so in that case no packing is required on any processor, right? |
Bill Auerbach <billauerbach> |
Wed 19 May 2010 08:31:02 PM UTC, comment #60: Well son of a bitfield. I just ran with no packing on the NIOS II and it does work! Either I thought I had tried it once and didn't try it, or, it didn't work 18 months ago and does now.
|
Bill Auerbach <billauerbach> |
Wed 19 May 2010 08:15:00 PM UTC, comment #59:
|
Simon Goldschmidt <goldsimon> |
Wed 19 May 2010 08:06:39 PM UTC, comment #58: I needed pack(1) as things break without it. So I tried pack(2) and everything I use (RAW API, UDP, TCP, DHCP, AUTOIP, ICMP, ARP) work fine. I can't imagine upper layers (netbuf, sockets) that I don't use being affected by this packing.
|
Bill Auerbach <billauerbach> |
Wed 19 May 2010 08:02:39 PM UTC, comment #57: I'm of the opinion not to add to code size or reduce performance in 1.4.0 because of IPv6, since lwIP doesn't really support IPv6 anyway. There may be far wider implications in supporting IPv6 than the ip_addr_t typedef. |
Bill Auerbach <billauerbach> |
Wed 19 May 2010 07:57:31 PM UTC, comment #56:
|
Simon Goldschmidt <goldsimon> |
Wed 19 May 2010 07:51:16 PM UTC, comment #55: As there is no warning about casting pointers from packed- to unpacked structs, I've decided to define two versions of ip_addr_t: one packed and one unpacked version - the packed version is used in protocol headers, the unpacked version is used normally throughout the stack.
|
Simon Goldschmidt <goldsimon> |
Wed 19 May 2010 07:42:00 PM UTC, comment #54: Yes, I did mean use the constant 2 if we were specifying packing of 1. It made more sense after I found this out because nothing in the headers is smaller than a u16_t. |
Bill Auerbach <billauerbach> |
Wed 19 May 2010 07:16:19 PM UTC, comment #53: I'm a bit surprised, what do we have to do to support structure packing of 2? Just make sure your structure packing macros use pack(2), not pack(1) and you're done, aren't you? I thought that's what you suggested last time we talked about it??
|
Simon Goldschmidt <goldsimon> |
Tue 18 May 2010 08:24:34 PM UTC, comment #52: I saw significant improvement packing by 2 over 1 before I found out that I didn't need packing at all, which with no packing things were even better than with packing by 2. Packing by 2 at least allows the compiler to access the ip_addr_t as 2 u16_t's over 4 u8_t's. I would at least recommend default the packing by 2 if it's used at all.
|
Bill Auerbach <billauerbach> |
Tue 18 May 2010 07:21:35 PM UTC, comment #51: As it is now, anyone can have a quick test of not packing ip_addr_t (just remove the packing directives, there's no need to convert from the struct to a typedef).
|
Simon Goldschmidt <goldsimon> |
Tue 18 May 2010 01:15:42 PM UTC, comment #50: Regarding markup, you can use the verbatim tags to mark sections of comments that are code:
|
Kieran Mansley <kieranm> |
Mon 17 May 2010 05:47:07 AM UTC, comment #49: Re comment #47:
|
Simon Goldschmidt <goldsimon> |
Sun 16 May 2010 11:09:52 PM UTC, comment #48: I see the problem with my confusion over Simon's comment #46 - rich markup has stuffed up the syntax. Don't use an asterisk twice on the same line, or Savannah thinks you want the text between them to be bold (when viewing the issue via a web browser - e-mail copy to lwip-devel is ok). Seems rather self-defeating for discussing C source code! |
David Empson <dempson> |
Sun 16 May 2010 11:04:46 PM UTC, comment #47: In this cast:
|
David Empson <dempson> |
Fri 14 May 2010 10:50:54 AM UTC, comment #46: Well, of course I can create an unaligned u32_t pointer if I want to. In fact, lwIP does this trick in some places and ensures correct alignment by adding its own defines. However, if I don't take the detour of using a void-pointer, the compiler warns me:
|
Simon Goldschmidt <goldsimon> |
Wed 12 May 2010 09:30:32 PM UTC, comment #45: Simon, there should be a problem either way. Casting a pointer could give an unaligned type no matter what the underlying type is. A typedef is just another name for the type - I bet the compiler handles either the same way. I know I've said it before, but casting pointers or addresses to pointers is not portable as it can lead to unaligned results. |
Bill Auerbach <billauerbach> |
Wed 12 May 2010 09:02:50 AM UTC, comment #44: I've just tried "typedef u32_t ip_addr_t" with my embedded compiler:
|
Simon Goldschmidt <goldsimon> |
Tue 16 Feb 2010 08:40:08 PM UTC, comment #43: Thanks for the patches. I'll need to find the time to test this on my embedded compiler at work to see if Iget alignment warnings (since I don't get them here with MSVC). It might take some time, but it's not forgotten! |
Simon Goldschmidt <goldsimon> |
Mon 15 Feb 2010 02:02:26 AM UTC, comment #42:
|
Stephane Lesage <slesage> |
Sat 13 Feb 2010 08:20:43 PM UTC, comment #41: Hi all,
|
Stephane Lesage <slesage> |
Mon 08 Feb 2010 11:02:20 AM UTC, comment #40:
|
Stephane Lesage <slesage> |
Mon 08 Feb 2010 09:36:30 AM UTC, comment #39: This discussion is getting quite confusing, and some of it is not really relevant to the bug. Could we move anything not directly related to the ip_addr definition change into a thread (or lots of threads!) on lwip-devel?
|
Kieran Mansley <kieranm> |
Mon 08 Feb 2010 06:46:28 AM UTC, comment #38: I don't really understand actually where the difference is between ip-addr and in-addr here. Did you mean you were confused by having both structs for the stack? Well, if you are only programming sockets that you shouldn't have used struct ip_addr at all... After all, in_addr is a socket-only define and in my opinion, the code gets rather ugly when using it...
|
Simon Goldschmidt <goldsimon> |
Mon 08 Feb 2010 06:41:24 AM UTC, comment #37:
|
Simon Goldschmidt <goldsimon> |
Sun 07 Feb 2010 11:34:14 PM UTC, comment #36: Hi again,
|
Stephane Lesage <slesage> |
Sat 06 Feb 2010 11:52:41 PM UTC, comment #35: I updated http://lwip.wikia.com/wiki/Porting_for_an_OS#cc.h to explain endianness macros and structure packing optimization.
|
Stephane Lesage <slesage> |
Sat 06 Feb 2010 03:49:20 PM UTC, comment #34: ntohX as macros is nice for constants, but it's not for run-time code especially if you have assembly instruction or intrinsics in my case.
|
Stephane Lesage <slesage> |
Fri 05 Feb 2010 09:45:46 PM UTC, comment #33: Another reason for these macros, and htonl and ntohl versions as macros is that both can be useful with constant data. In the NIOS, we used ntohl and htonl operations as custom opcodes. It was still nicer to use macro versions in some cases because the compiler can get the 32-bit value for free.
|
Bill Auerbach <billauerbach> |
Fri 05 Feb 2010 02:39:03 PM UTC, comment #32: Ah, for me, aligned was 32-bit since that is the minimum alignment for all datatypes we have (32-bit maximum) on nearly every platform from 8-bit upt to x64 (I think there are some DSPs though that need other alignment?).
|
Simon Goldschmidt <goldsimon> |
Fri 05 Feb 2010 02:00:12 PM UTC, comment #31:
|
Stephane Lesage <slesage> |
Fri 05 Feb 2010 10:29:26 AM UTC, comment #30:
|
Simon Goldschmidt <goldsimon> |
Fri 05 Feb 2010 08:05:19 AM UTC, comment #29:
|
Stephane Lesage <slesage> |
Fri 05 Feb 2010 03:41:03 AM UTC, comment #28:
|
Stephane Lesage <slesage> |
Thu 04 Feb 2010 08:15:07 PM UTC, comment #27: I've done some preliminary work on this:
|
Simon Goldschmidt <goldsimon> |
Tue 02 Feb 2010 08:23:59 PM UTC, comment #26: Thank for the pointer on rfc2553. I'll use pointers for now, we can then talk about performance improvement. I just wanted to get to a consensus before I make the change, since it involves touching nearly all files throughout the repository... |
Simon Goldschmidt <goldsimon> |
Tue 02 Feb 2010 07:27:24 PM UTC, comment #25: There is a representation of an IPv4 address in an IPv6 address so if you do IPv6 it can talk to IPv4 addresses. There is good information here on using IPv6 from a socket API interface: RFC 2553: http://www.rfc-editor.org/rfc/rfc2553.txt |
Bill Auerbach <billauerbach> |
Tue 02 Feb 2010 04:16:08 PM UTC, comment #24: Good point - if there is support for IPv6 it might be a mistake to not support IPv4 concurrently. I suppose ip_addr would have to contain both or be a union with a "what am I" member. Then pointers might be desirable since IIRC IPv6 addresses are 16 bytes.
|
Bill Auerbach <billauerbach> |
Tue 02 Feb 2010 04:00:30 PM UTC, comment #23: I'm thinking a bit about future here: what happens if we want to support IPv4 and v6 at the same time (and I'd very much like that - or might even need it): At the time of writing/compiling the callbacks, you don't know which kind of address you will receive, so we need some kind of wrapper structure that tells us whether the address is v4 or v6. This prevents us from directly passing an IPv4 or IPv6 address by value since the header (the place where you want to copy it from) does not contain the wrapper info.
|
Simon Goldschmidt <goldsimon> |
Mon 01 Feb 2010 08:20:00 PM UTC, comment #22: Wasn't the point memory alignment requirements? The original post was about packing the ip-addr struct because there was significant overhead to have every ip_addr access be byte-wide with ANDs, ORs and shifts. Not packing means you can't take the address of a casted ip_hdr pointer into payload in any callbacks, and your point on this was well advised. So I'm saying, don't pass the address of an ip_addr anywhere but simply pass in a typedef ip_addr into all functions. If this typedef of ip_addr becomes a struct for IPv6, fine, it will be copied by the compiler avoiding alignment problems then as well. For IPv4, just using a typedef yo u32_t passes by value which is more efficient.
|
Bill Auerbach <billauerbach> |
Mon 01 Feb 2010 07:37:50 PM UTC, comment #21: The problem is that it involves changing from a pointer to a copy everywhere: OK, we would leave out one copy but I don't think that is will be bottle neck, there are certainly other places that have more impact on performance!
|
Simon Goldschmidt <goldsimon> |
Mon 01 Feb 2010 05:16:06 PM UTC, comment #20: The difference is copying to a new variable and then passing the variable address of the variable. What is the benefit in this versus just passing the IP address in the first place? Are you meaning something like:
|
Bill Auerbach <billauerbach> |
Mon 01 Feb 2010 03:47:13 PM UTC, comment #19: Well, if it's only for IPv4 it really doesn't make a difference whether we copy a pointer or the actual address, does it?
|
Simon Goldschmidt <goldsimon> |
Mon 01 Feb 2010 03:34:50 PM UTC, comment #18: Yeah, sorry - it's Monday. It's done not only in the callbacks though. As often as it's used and seeing that a 4-byte memcpy has huge overhead I would benchmark this change before deciding on that choice. I hope it's not being chosen because of IPv6. IPv6 is far from complete and not widely used or required in small embedded devices. |
Bill Auerbach <billauerbach> |
Mon 01 Feb 2010 03:02:53 PM UTC, comment #17: Ehrm, maybe I haven't expressed that clearly enough but that's solution b) already. |
Simon Goldschmidt <goldsimon> |
Mon 01 Feb 2010 02:15:12 PM UTC, comment #16: One possibility not mentioned is to pass ip_addr as a variable and not pointer to an ip_addr. When it's a u32_t the lack of concern for alignment is obvious. For IPv6 this means the typedef has to be a struct and it's passed by value by the compiler (with a copy). I don't think anyone requiring IPv6 can be worrying too much about performance - on some processors I don't even know if IPv6 makes sense. Do IPv6 and "light weight" fit in the same sentence?
|
Bill Auerbach <billauerbach> |
Sun 31 Jan 2010 12:21:20 PM UTC, comment #15:
|
Simon Goldschmidt <goldsimon> |
Sun 31 Jan 2010 10:21:21 AM UTC, comment #14: I'd rather not have yet another option. Let's stick to one or the other. I vote for solution (a) |
Kieran Mansley <kieranm> |
Fri 29 Jan 2010 04:29:18 PM UTC, comment #13: Would it be possible to have some kind of preprocessor magic that would allow to have either (struct or typedef) depending on a config setting? Provided the speedup is worth the trouble, of course. |
Mike Kleshov <kleshov> |
Fri 29 Jan 2010 04:16:31 PM UTC, comment #12: There's no doubt the influences performance in a negative way, the only question is how to solve it (i.e. comment #6: solution a or b?). |
Simon Goldschmidt <goldsimon> |
Fri 29 Jan 2010 04:14:33 PM UTC, comment #11: I get it. So I assume it all works for me because I do have ETH_PAD defined to 2. ip_addr not being a packed struct got me the largest performance improvement for all of the performance-improving changes that I made.
|
Bill Auerbach <billauerbach> |
Fri 29 Jan 2010 03:57:35 PM UTC, comment #10:
|
Simon Goldschmidt <goldsimon> |
Fri 29 Jan 2010 02:32:48 PM UTC, comment #9: Which references? References within a struct (ip_hdr) are correct because that struct is packed and alignment unconstrained by its definition. By adding packing on it outside of a containing struct it forces the compiler to treat it unaligned when it would never be (all variables of type ip_addr will be aligned just like all char variables are aligned).
|
Bill Auerbach <billauerbach> |
Fri 29 Jan 2010 02:12:30 PM UTC, comment #8:
|
Simon Goldschmidt <goldsimon> |
Fri 29 Jan 2010 01:54:41 PM UTC, comment #7: When I looked into this, I noticed that all references to an ip_addr within a payload are within a packet and so it works out to be 32-bit aligned. I am running on an alignment required processor with the following:
|
Bill Auerbach <billauerbach> |
Fri 29 Jan 2010 08:44:27 AM UTC, comment #6: There is one problem here: the address pointer as passed to the recv callback functions might be unaligned since it currently points into the pbuf payload. If we wanted to remove packing from struct ip_addr or use an u32_t (and an u32_t pointer in the callbacks?), we either would
|
Simon Goldschmidt <goldsimon> |
Thu 03 Sep 2009 12:37:15 PM UTC, comment #5: The suggestion in comment #4 would be OK. The only downside is that I find such things much harder to reason about when looking at the code than a simple type or structure, but I think code readability should take second place if the performance change is as significant as suggested. |
Kieran Mansley <kieranm> |
Mon 31 Aug 2009 01:25:39 PM UTC, comment #4: IPv4:
|
Bill Auerbach <billauerbach> |
Mon 31 Aug 2009 09:04:40 AM UTC, comment #3: OK, that's definitively a fact I haven't been thinking about. That would also be the reason the struct ip_addr is used as a pointer everywhere, not directly as value.
|
Simon Goldschmidt <goldsimon> |
Mon 31 Aug 2009 03:40:09 AM UTC, comment #2: There could be a significant forward compatibility issue with this proposed change: IPv6.
|
David Empson <dempson> |
Sun 30 Aug 2009 09:14:40 PM UTC, comment #1: Forgot to add: the downsize is of course that every bit of code using lwIP has to be changed. However, I'm not at all OK with using packing where we absolutely don't need it!
|
Simon Goldschmidt <goldsimon> |
Sun 30 Aug 2009 09:11:40 PM UTC, original submission:
This was brought up on lwip-users by Bill Auerbach.
|
Simon Goldschmidt <goldsimon> |
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 10 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2010-05-22 | goldsimon | Category | None | IPv4 | |
Item Group | None | Change Request | |||
Status | In Progress | Fixed | |||
Assigned to | None | goldsimon | |||
Open/Closed | Open | Closed | |||
2010-04-21 | goldsimon | Planned Release | 1.4.0 | ||
2010-02-15 | slesage | Attached File | - | Added ip_addr_not_packed_netbuf.patch, #19702 | |
2010-02-13 | slesage | Attached File | - | Added ip_addr_not_packed.patch, #19698 | |
2010-02-04 | goldsimon | Status | Need Info | In Progress | |
2010-01-29 | goldsimon | Status | None | Need Info |
Powered by Savane 3.13-02a9.
Corresponding source code
Finally, Ifound the time to check in something that is a bit related to Stephane's patch but a little different:
- I introduced a packed- and unpacked version of ip_addr_t (of which the packed version is only used in packet headers)
- IP-src/dest address are copied to aligned storage in ip_input and used from there in all protocols (while in an input function that is called from ip_input)
This results in 2 addresses copied per incoming packet, which is good enough, I guess.
It works for me, I hope I haven't broken anything I couldn't test here, so please report back any errors.