buglwIP - 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: 

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.

Simon Goldschmidt <goldsimon>
Group administrator
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.

Nevertheless, I still think packing ip_addr_t should be avoided in case a platform can't use ETH_PAD_SIZE==2 (e.g. because of limitations of DMA-enabled MACs).

Simon Goldschmidt <goldsimon>
Group administrator
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.

Well this is good to know and better still to clarify.  So, processors that require 32-bit alignment don't need packing at all.  Is this what this means?

My tests were all about not packing ip_addr_t and I didn't widen the test I guess thinking that a 32-bit aligned CPU needed some packing.

Bill Auerbach <billauerbach>
Wed 19 May 2010 08:15:00 PM UTC, comment #59: 


> I needed pack(1) as things break without it.


You mean wit no packing at all everything breaks? I did think that (having correctly aligned ethernet packets), even this would work...

Simon Goldschmidt <goldsimon>
Group administrator
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.

I'm using the NOIS II too which as you know needs alignment and I have no packing specified for ip_addr_t.  I DO have struct packing of 2 otherwise.  Benchmark no packing and packing on the NIOS II - the speed change was substantial.

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: 


> 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.


So this is rather an issue of documentation to tell the people creating ports that packing means "pack(2)", not "pack(1)"?

Anyway, I did review that, but not to the extent that I'd bet my life for it. That's why I wrote this on the wiki:

"In any case, make sure the system runs stable when chosing a structure packing setting different to 1!"

From looking at the code, I do think that it's OK, but I haven't tested it yet nor have I put too much time in reviewing the code thoroughly. I just preferred the method of trying to avoid packing on ip_addr_t.

However, since I'm in the process of designing (and testing) a NIOS-II-based system, I'll give it a try and see if it runs stable (and faster). Still, I think this is just an issue of updating the documentation for porters, not of changing the lwIP code.

Simon Goldschmidt <goldsimon>
Group administrator
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.

This works fine except for the case we discussed before: the place where the IP source address is passed to the various recv callback functions (TCP, UDP, RAW). And with that the netbufs. Currently, the IP address just points into the pbuf.

With my patch, that would be changed to pointing to a variable on the stack or a global variable. I still think that this is the best way to go as the current pointer has the downside of both being unaligned and being unclear about the lifetime.

If we did that change, we would have to clearly document it in the 1.4.0 release note and change the netbuf implementation to hold a copy (not a reference) of an IP address - this may be regarded as a disadvantage regarding IPv6 addresses (there's no big change for IPv4), but I don't see any other way to solve it neatly.

Simon Goldschmidt <goldsimon>
Group administrator
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??

If it wasn't, we would have to change all structure packing macros to take a number...

Simon Goldschmidt <goldsimon>
Group administrator
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.

Perhaps a cc.h macro ALIGNMENT_REQUIRED can be added which allows lifting alignment enforcing code and packing constructs when 0 and leaves the code in to enforce it if 1.  IMO it's come up enough times and you've reviewed it and put in sufficient time to warrant making it right for both conditions, don't you?

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).

I hope to find the time to review the code in order to copy all addresses to the stack instead of passing on (unaligned) pointers. However, since that is quite unsafe (since there is no warning!), I think I'll add an option for that for now, so that those who want to take the "risk" can try to improve speed/code size by not packing ip_addr_t.

Simon Goldschmidt <goldsimon>
Group administrator
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:

http://savannah.gnu.org/cookbook/?func=detailitem&item_id=125

Kieran Mansley <kieranm>
Group Member
Mon 17 May 2010 05:47:07 AM UTC, comment #49: 

Re comment #47:

I understand why the compiler warns and actually we are using the double cast including void-pointers to silence the compiler's warnings where we know our own alignment macros ensure correctness. However, I just don't understand why there is no warning in the packed struct case.

Re comment #48:
My bad, I keep forgetting that. I shouldn't write code that way here, it's kind of hard to read... ;-)

Simon Goldschmidt <goldsimon>
Group administrator
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:

u8_t *a = &some_u8t;
u32_t b = (u32_t)a; <<-- compiler warns

why would the compiler warn you about converting a pointer to an integer?

Did you actually mean this:

u32_t b = (u32_t )a;

I'd expect a warning there, because some_u8t could be at any address, which may not be aligned to a 32-bit boundary. The compiler doesn't know whether it can access *b because it might not be aligned.

If you typecast via a void pointer, as in:

u32_t c = (u32_t )(void *)a;

the warning is defeated but the same problem applies - c may be pointing to an unaligned object. You've just hidden a potential bug. This doesn't produce a warning because the compiler expects you to know what you are doing when you use (void *) to hide the details of a pointer type.

The lack of warning when creating a pointer to a 32-bit unaligned member of a packed structure is probably an oversight on the part of the compiler. The resulting pointer has the same issue.

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:

u8_t *a = &some_u8t;
u32_t b = (u32_t)a; <<-- compiler warns
u32_t c = (u32_t)(void*)a; <<-- compiler doesn't warn

If we only used the 1st cast, what could go wrong? How could it give an unaligned type? An of course, the next lines give warnings, too:

typedef u32_t some_type_t;
some_type_t *d = (some_type_t)a;

So far so good. However, I would have expected to be warned when casting from a (possibly) unaligned u32_t member of a packed struct to an u32_t pointer (that requires alignment), but it seems I have been wrong there.

Being like that, we need some other guards here to prevent creating unaligned pointers.

Simon Goldschmidt <goldsimon>
Group administrator
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:

Unfortunately, gcc (nios2-elf-gcc, v3.4.1) does not warn about casting the (possibly unaligned) IP address 'src' in struct ip_hdr to an ip_addr_t pointer on the stack (which requires alignment).

Does anybody have an idea how to get such a warning? Without that, I think it's too dangerous to use that typedef as we risk unaligned pointers which would result in bad IP addresses...

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group administrator
Mon 15 Feb 2010 02:02:26 AM UTC, comment #42: 


new patch updated:
- works with ipaddr_copy from Simon
- netbuf stores address instead of pointer
- inet_chksum_pseudo must be given htons(proto) which is a constant

verified with sockets, so everything is working (netconn and raw api callbacks)



(file #19702)

Stephane Lesage <slesage>
Sat 13 Feb 2010 08:20:43 PM UTC, comment #41: 

Hi all,

I've prepared a patch that removes the packing constraint on struct _ip_addr. Given that Simon already abstracted the ip_addr_t type, it will be very easy to typedef it to u32_t.

What it does:
- include my previous patch for ip_addr_copy/ip_addr_set_safe
- copy before calling raw api callbacks
- copy before calling tcp_rst (maybe we could use the pcb addresses?)
- copy before calling a few other functions
- changed inet_chksum_pseudo to use u32_t as it's v4 specific, and it would be silly to copy before, pass pointer and copy inside.

What it does not:
- solve the netbuf issue which stores a pointer inside the pbuf which may be misaligned



(file #19698)

Stephane Lesage <slesage>
Mon 08 Feb 2010 11:02:20 AM UTC, comment #40: 


>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...


we all use ip_addr for netif for example, and some useful macros of ip_addr don't exist for in_addr, which force to cast to use them.
It would be nice to unify this.

>After all, in_addr is a socket-only define


ok we must have it for sockets, but it doesn't mean we can't use it internally...

>and in my opinion, the code gets rather ugly when using it...


really? what's the difference between ->addr and ->s_addr ?
anyway, with your the new macros we don't have to worry about that.


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>
Group Member
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...

And I really don't understand what a typedef to u32_t has to do with byte order?? Oh, and when the structs are packed, I don't think we can copy u16_t because the port may be free to use unaligned pbuf payloads when setting MEM_ALIGN to 1!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Feb 2010 06:41:24 AM UTC, comment #37: 


> BTW, ip_addr_set should be use only for safe copy, the latest
> modifications use it everywhere, even if we don't have to check
> the pointer.


I might have to check that again, but I think the only places where I used that unchecked is the SNMP code. And there, the problem lies in the poorly written code: it relies on void-pointers being aligned because the are allocated using mem(p)_malloc. That assumption has been in there before my changes and I didn't want to change the whole code right now.

If there are other places where it is used unchecked, please tell me.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 07 Feb 2010 11:34:14 PM UTC, comment #36: 

Hi again,
well maybe my last messages should go to a new thread about general optimization, but a part of it is actually linked to the ip_addr_t type definition.

Summary:
- we want to stick to passing addresses for the necessary evolution to dual v4/v6 stack.
- we want to remove the packing from the ip_addr_t when it's stored in local variables or PCB, and we can still use packing when using it as a field of protocol headers. So a copy would be necessary sometimes.

My POV:
- I use sockets only and ip_addr.h contains useful macros/functions operating only on ip_addr_t. As a new user, this was confusing and I still have to cast pointers.
- in macros, we want to access individual octets.
- sometime we could copy the two u16_t instead of calling SMEMCPY for 4 bytes...
- I think removing packing is necessary but now I don't think changing ip_addr_t to u32_t is good, because storing addresses in u32_t is often associated to host byte order (IP_XXX / IN_xxx macros).

What I propose:
Define ip_addr_t to / use only:

struct in_addr {
  union {
    u32_t s_addr;
    struct { u16_t s_w1, s_w2; };
    struct { u8_t s_b1, s_b2, s_b3, s_b4; };
  };
};

BTW, ip_addr_set should be use only for safe copy, the latest modifications use it everywhere, even if we don't have to check the pointer.

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.

By the way, I think we should separate cc.h and sys_arch documentations, as they obviously are independent (both .txt and wiki pages)

Also, MEM_ALIGNMENT, MEMCPY and SMEMCPY should be moved to cc.h.




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.

mix that with misaligned access because of packing and you get really UGLY code, which is both neither fast nor small.

In my case, when I changed to pack(2) and intrinsics, I gained 8 KiB of code...

Most of the time, just reading one u32_t value is very inefficient with pack(1) and the standard ntohl() function for little endian.

This was my case, so imagine the code I get with
#define IPH_TTL(hdr) (ntohs((hdr)->_ttl_proto) >> 8)
#define IPH_PROTO_SET(hdr, proto) (hdr)->_ttl_proto = (htons((proto) | (IPH_TTL(hdr) << 8))) !!

misaligned read, call inefficient ntohs, shifts R+L (optimized as and ?), or, call inefficient htons, misaligned write !!!

why isn't _ttl_proto separated as 2 u8_t ??
and this would result in a simple byte write, as it should be.

or #define IPH_V(hdr)  (ntohs((hdr)->_v_hl_tos) >> 12)

why read 16 bits, convert endianness, to finally get only 4 bits that could be read directly from the containing byte ?

Maybe ntohX as macros can help.

But I'd better optimize these types of macros and use my optimized byteswap4() intrinsic for the cases where it's really needed.

I'd also better use macros to read and convert.
#define read_u32(p) for example could be defined as:

1. big endian, no packing
#define read_u32(p) (*(p))

2. little endian, no packing
#define read_u32(p) ntohl(*(p))

3. big endian, pack(2)
#define read_u32(p) ( (((u16_t*)(p)[0])<<16) | ((u16_t*)(p)[1]) )

4. little endian pack(2) or pack(1)
#define read_u32(p) ( (((u8_t*)(p)[0])<<24) | (((u8_t*)(p)[1])<<16) | (((u8_t*)(p)[2])<<8) | ((u8_t*)(p)[3]) )

We could offer the choice to the user, explaining the details, packing needed with(out) ETH_PAD_SIZE, and providing advice for the different architectures the developpers use here.


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.

By the way, if I understood right, Simon mentioned macros with constants "should be improved by the compiler".  I have yet to see a compiler, even with no optimizer at all, that doesn't do constant folding.  This makes IP4_ADDR always evaluate to a single constant with no runtime code.

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?).

For 16-bit platforms, you might be right.

I get your idea about the macros now. I have already been thinking about having preprocessor versions of the htonX functions (that would prevent all the #if LITTLE_ENDIAN blocks), but I haven't yet checked whether compilers can optimize shifting of constants away or not.

Until then, I think your idea about the macros is good.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 Feb 2010 02:00:12 PM UTC, comment #31: 


>At least struct etharp_hdr contains an unaligned IP address (sipaddr).
>This is currently the reason we have the struct ip_addr2. >However, if that's the only problem we can just add an u16_t array there
>since we only access this field using SMEMCPY, anyway.


By "word" aligned I mean "2-bytes" = even addresses.
etharp_hdr->sipaddr is indeed word aligned.

>Only some structs would still require packing (netbios and ip_reass_helper mainly).


no I think netbios_name_hdr fields are also word aligned.
Of course, as other structures, IP payload can be aligned at 2 or 4, when using padding.
Compiling with #pragma pack(2) works for me.

>> - define IP4_ADDR() and ip4_addrX() macros directly according to endianness
>On big endian platforms, the ntohX/htonX functions should be defined to nothing,
>so that should improved away by the compiler.


the problem is little endian...

what about this ?

#if BYTE_ORDER == BIG_ENDIAN
#define IP4_ADDR(ipaddr, a,b,c,d) \
        (ipaddr)->addr = (((u32_t)((a) & 0xff) << 24) | \
                          ((u32_t)((b) & 0xff) << 16) | \
                          ((u32_t)((c) & 0xff) << 8) | \
                           (u32_t)((d) & 0xff))

#else
#define IP4_ADDR(ipaddr, a,b,c,d) \
        (ipaddr)->addr = (((u32_t)((d) & 0xff) << 24) | \
                          ((u32_t)((c) & 0xff) << 16) | \
                          ((u32_t)((b) & 0xff) << 8) | \
                           (u32_t)((a) & 0xff))
#endif

#if BYTE_ORDER == BIG_ENDIAN
#define ip4_addr1(ipaddr) (((u8_t*)&((ipaddr)->addr))[0])
#define ip4_addr2(ipaddr) (((u8_t*)&((ipaddr)->addr))[1])
#define ip4_addr3(ipaddr) (((u8_t*)&((ipaddr)->addr))[2])
#define ip4_addr4(ipaddr) (((u8_t*)&((ipaddr)->addr))[3])
#else
#define ip4_addr1(ipaddr) (((u8_t*)&((ipaddr)->addr))[3])
#define ip4_addr2(ipaddr) (((u8_t*)&((ipaddr)->addr))[2])
#define ip4_addr3(ipaddr) (((u8_t*)&((ipaddr)->addr))[1])
#define ip4_addr4(ipaddr) (((u8_t*)&((ipaddr)->addr))[0])
#endif

Stephane Lesage <slesage>
Fri 05 Feb 2010 10:29:26 AM UTC, comment #30: 


> I get the folowing warning about everywhere:
> "taking the address of a misaligned field is not recommended"


At least that tells us that there are compilers that do warn about this. However, most of the time, I'm developing under MS Visual Studio using the MS cl.exe compiler. Given that the x86 platform supports unaligned pointers, this compiler unfortunately does not warn me about using them. Although using unaligned pointers decreases performance on this platform, as well.

I'll have to try the embedded compiler next week and see at which places we have to copy the address onto the stack.

> - all packed structures seem to be fine with word alignment,


Most of them are, I guess, but not all. At least struct etharp_hdr contains an unaligned IP address (sipaddr). This is currently the reason we have the struct ip_addr2. However, if that's the only problem we can just add an u16_t array there, since we only access this field using SMEMCPY, anyway.

If the incoming packets' IP headers are aligned correctly then indeed one might try to disable packing completely. Only some structs would still require packing (netbios and ip_reass_helper mainly).

> - define IP4_ADDR() and ip4_addrX() macros directly according to endianness


On big endian platforms, the ntohX/htonX functions should be defined to nothing, so that should improved away by the compiler.

> Also why not define the macros to work on values instead of pointers ???


Macros are not a problem here: they are evaluated in place and the compiler should still know about packing. We only might have to avoid casting to an ip_addr_t pointer in a macro.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 Feb 2010 08:05:19 AM UTC, comment #29: 


I looked at the compiled code and it looks ok. The access is compiled accordingly to the packed attribute of the pointer. So this is better now (we still need the copies), but there's room for improvement:

- all packed structures seem to be fine with word alignment, I tested it successfully. some structures like udp_hdr even don't need packing (assuming pointers are even which should always be the case, no ?)...
or we could add the min alignment as a parameter of the PACK_STRUCT macros

- define IP4_ADDR() and ip4_addrX() macros directly according to endianness, not using htonl()

Stephane Lesage <slesage>
Fri 05 Feb 2010 03:41:03 AM UTC, comment #28: 


Hi,

I tried typedef u32_t ip_addr_t;
and redefined the macros of ip_addr.h like
#define ip_addr_set(dest, src) (*(dest) = \
                               ((src) == NULL ? 0: *(src)))

I get the folowing warning about everywhere:
"taking the address of a misaligned field is not recommended"

just because we use the & operator...
and despite an expression like
ip_addr_set(&iphdr->src, &iphdr->dest);
should perfectly work even when mixing mis/aligned data

maybe other compilers will issue an warning/error only when assigning a misaligned pointer to an aligned pointer...

anyway we can track the places where we need to copy.

Also why not define the macros to work on values instead of pointers ???


Stephane Lesage <slesage>
Thu 04 Feb 2010 08:15:07 PM UTC, comment #27: 

I've done some preliminary work on this:
- changed all the code to not know about the actual IP address type layout and use defines instead (no performance darwback but much cleaner code - also regarding IPv6 upgrade);
- changed "struct ip_addr" to "struct _ip_addr" and added a "typedef struct _ip_addr ip_addr_t"
- replaced usage of "struct ip_addr" by "ip_addr_t" throughout the stack

This means we are now relatively free to change the actual typedef of the IP address.

However, I'm still not convinced it all just works when we now just change the typedef from "typedef struct ip_addr ip_addr_t" to "typedef u32_t ip_addr_t": For example we would have to pay attention when passing the ip address from protocol headers to other functions (e.g. checksum calculation), or do compilers warn about that?

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group administrator
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.

I agree that with pointers you do have to worry about lifetimes (and alignment still if the pointer is the result of a cast).

Maybe the IP address for a packet can be stored more statically somewhere else (e.g. pointer to the arp table)? I'm just throwing out an idea, not saying this can work.  The pointer to it can be used which is more efficient and is aligned because it can be made to be within the table.

It's all worth discussing if IPv6 is truly going to be fully supported.

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.

This might be too constructed, but my intention here is to change the API now and prevent changes in the future if we can.

Sure enough, if we don't want to consider IPv6 migration now, then passing by value is the easiest solution (especially since it helps understanding the limited lifetime of the address - passing as a pointer might be confusing here).

Simon Goldschmidt <goldsimon>
Group administrator
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.

This means that ip_addr becomes a typedef and for IPv4 it's u32_t and for IPv6 it's some structure.  The point being is that the C code is compatible with either typedef.

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!

And after that single copy (meaning netconn/socket api), the number of copies will be the same, no matter if we pass a copy or an u32_t to the raw api callback.

Simon Goldschmidt <goldsimon>
Group administrator
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:

struct ip_hdr *pHdr;
struct ip_addr ipAddr;

pHdr = (struct ip_hdr *) p->payload;
ipAddr = pHdr->src.addr;
someCallback(&ipAddr);

And I'm thinking:

pHdr = (struct ip_hdr *) p->payload;
someCallback(pHdr->src.addr);

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?

And it's the cleanest thing to do anyway since we had that bug where someone tried to dereference the address pointer in a recv callback after freeing the pbuf it was located in (since it's currently a pointer into the pbuf). We solved it by adding documentation that the address is only valid until the pbuf is freed, but I think comment #6 a) is the cleaner solution.

Only to make sure we all agree on the same thing: My proposed solution copies the address onto the stack (e.g. in udp_input()) before calling the application's recv function and passing a pointer to that stack-variable (OK, that's one extra copy) and the netconn API must copy that address into the netbuf (instead of copying its pointer) - except for TCP.

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group administrator
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?

Option c) might be "Replace &(var_iphdr) with var_iphdr"

Bill Auerbach <billauerbach>
Sun 31 Jan 2010 12:21:20 PM UTC, comment #15: 


> I vote for solution (a)


Me too, especially since that solution is compatible to IPv6, too.

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group Member
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>
Group Member
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>
Group administrator
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.

I guess the summary is, there are many processors/platforms where alignment isn't a concern or where ETH_PAD is 2.  These systems probably incur a significant performance hit for ip_addr packing. I'd sure like to see others in this camp do a quick benchmark without ip_addr packing and see what they get.

Bill Auerbach <billauerbach>
Fri 29 Jan 2010 03:57:35 PM UTC, comment #10: 


> Or I'm missing your point?


I guess so: when receiving UDP or RAW packets, we are passing the pointer to a struct ip_addr and this pointer points into the pbuf. Now if the address is unaligned in that packet, the information to access it packed is gone once we use a normal pointer:

See this code from udp_input():
pcb->recv(pcb->recv_arg, pcb, p, &iphdr->src, src);

The application recv callback gets a 'struct ip_addr*' and wouldn't know that this pointer has to be accessed packed!

> Doesn't everyone using 32-bit aligned processors use ETH_PAD 2
> to align everything?


That doesn't work for me as the DMA of the MAC would have to handle copying from 32-bit aligned (internal buffer of the MAC) to 16-bit aligned (pbuf, because of the 2-byte ETH_PAD).

Simon Goldschmidt <goldsimon>
Group administrator
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).

The point was, if the packet is aligned, so is ip_addr in the packet.  If it's not aligned, it's still handled correctly because of the packing requirement of ip_hdr.  ipHdr.src will work on any address because of packing on ip_hdr.  I don't see the reason to pack something within a packed struct - it already is treated packed this way, and when ip_addr is used outside of ip_hdr, the packing causes unnecessary poor code generation?  Or I'm missing your point?

Doesn't everyone using 32-bit aligned processors use ETH_PAD 2 to align everything?

Bill Auerbach <billauerbach>
Fri 29 Jan 2010 02:12:30 PM UTC, comment #8: 


> When I looked into this, I noticed that all references to an
> ip_addr within a payload are within a packet


Well, that's the point: without special optimization, all these addresses are 16-bit aligned because of the 14-byte ethernet header. And on some platforms where the DMA of the MAC can only transfer to/from 32-bit aligned memory, this means to get these addresses aligned we would have to issue a memcpy/memmove.

For the tx side, this is no problem though, since we only copy the address (into a packed struct). But we cannot just pass around a received address.

Simon Goldschmidt <goldsimon>
Group administrator
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:

struct ip_addr {
  u32_t addr;
};

There is no alignment or packing specified in this struct which makes it equivalent to replacing it with u32_t. I'm using TCP and UDP.  One compiler for sure (GCC) handles this much more efficiently - it will load a 32-bit word - otherwise with packing specified it loads 4 bytes ANDing shifting and ORing into the address.

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
a) have to copy the address or
b) directly use an u32_t for the callbacks instead of a pointer

The problem with a) is the addresses scope when it is passed on to netconn/socket api. The problem with b) is that it is uneffective with IPv6.

Given that, I'd chose a) and copy the address when using netconn/socket api because that way, we prevent having to change the api again once IPv6 gets usable again (which I'd like to see).

Any other opinions?

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group Member
Mon 31 Aug 2009 01:25:39 PM UTC, comment #4: 

IPv4:

typedef u32_t ip_addr[1];

IPv6:

typedef u32_t ip_addr[4];

And this way ip_addr is still a compile time address.

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.

However, we could still convert the IPv6 struct to

PACK_STRUCT_BEGIN
struct _ip_addr {
PACK_STRUCT_FIELD(u32_t addr[4]);
} ip_addr PACK_STRUCT_STRUCT;
PACK_STRUCT_END

That way it is used without 'struct', much the same way as 'typedef u32_t ip_addr;'.

The IPv6 implementation must be changed anyway, so that v4 and v6 can be used at the same time (e.g. renaming the struct/type to ip6_addr).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 31 Aug 2009 03:40:09 AM UTC, comment #2: 

There could be a significant forward compatibility issue with this proposed change: IPv6.

If the IPv4 version of struct ip_addr is changed to a simple typedef u32_t, and API changes are made accordingly, it will break the current IPv6 implementation and require reversing these changes in future if IPv6 is fully implemented.

At present, the partial IPv6 implementation in LWIP has a similar struct ip_addr definition, which includes four 32-bit integers:

PACK_STRUCT_BEGIN
 struct ip_addr {
  PACK_STRUCT_FIELD(u32_t addr[4]);
} PACK_STRUCT_STRUCT;
PACK_STRUCT_END

This cannot be declared as a simple integer type in any existing compiler (unless there is a platform which directly supports 128-bit integers).

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!

Since this is rather a big API change, I'd love to hear comments from active developers, please.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 30 Aug 2009 09:11:40 PM UTC, original submission:  

This was brought up on lwip-users by Bill Auerbach.
It would both speed up and decrease memory footprint by not using a packed struct where we don't need it:

typedef u32_t ip_addr;

instead of

PACK_STRUCT_BEGIN
struct ip_addr {
  PACK_STRUCT_FIELD(u32_t addr);
} PACK_STRUCT_STRUCT;
PACK_STRUCT_END

Simon Goldschmidt <goldsimon>
Group administrator

 

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

Attached Files
file #19702:  ip_addr_not_packed_netbuf.patch added by slesage (30KiB - application/octet-stream)
file #19698:  ip_addr_not_packed.patch added by slesage (36KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by slesage (Posted a comment)
  • -email is unavailable- added by kleshov (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by billauerbach (Posted a comment)
  • -email is unavailable- added by dempson (Posted a comment)
  • -email is unavailable- added by goldsimon (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-05-22 goldsimon CategoryNone IPv4
        Item GroupNone Change Request
        StatusIn Progress Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen 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 StatusNeed Info In Progress
    2010-01-29 goldsimon StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code