buglwIP - A Lightweight TCP/IP stack - Bugs: bug #50097, Disabling struct packing should be...

 
 

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

bug #50097: Disabling struct packing should be official

Submitter:  Daniel Elstner <danielk>
Submitted:  Thu 19 Jan 2017 07:33:20 PM UTC
   
 
Category:  Documentation Severity:  3 - Normal
Item Group:  Feature Request Status:  Wont Fix
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Tue 02 Jan 2018 06:59:26 AM UTC, comment #9: 

I think this has too many options to fail in the future to make it officially supported. We have enough work without this already ;-)

This doesn't mean I wouldn't take patches to keep this working, but I'm not willing to add work for this myself.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 26 Jan 2017 02:39:04 PM UTC, comment #8: 

I agree this does not work for PPP RX!

Simon Goldschmidt <goldsimon>
Group administrator
Thu 26 Jan 2017 01:49:43 PM UTC, comment #7: 


> The check of the link header length should catch that.


How ?  When generating packets for output, we can create as many pbuf as necessary and chaining them for alignment requirements for each layer we need to append, but for input we can't determine if IP, UDP, TCP payloads are always going to be on alignment boundary.

PPP have multiple compression options which remove one or two bytes of PPP header like pcomp or accomp. For L2TP we have IP over IP, meaning we have a IP header in the payload of a UDP packet (UDP header + IP header + PPP fields + PPPoE/oS if PPPoL2TP over PPPoE or PPPoS is used). PPPoS is using RAW pbuf which bypass ETH_PAD_SIZE.

Sylvain Rochet <gradator>
Group Member
Thu 26 Jan 2017 01:08:35 PM UTC, comment #6: 

The check of the link header length should catch that.

Daniel Elstner <danielk>
Thu 26 Jan 2017 12:47:12 PM UTC, comment #5: 

I'm not against that, but at least pleeeeeease add a #error statement if struct packing is disabled and PPP or SLIP or anything else that isn't Ethernet is enabled :-)

Sylvain Rochet <gradator>
Group Member
Thu 26 Jan 2017 12:35:17 PM UTC, comment #4: 

Yes, I know that it is about alignment. The struct size check was intended to verify that the compiler did not insert any alignment padding. If you say that's guaranteed up-front, then the problem becomes smaller.

The pbuf code actually verifies alignment already from what I have seen. Specifically, memory is already allocated in such a way that the payload is aligned to MEM_ALIGNMENT.

As far as I can tell, the code assumes that protocol headers are completely contained within the first pbuf of a pbuf chain. Given that the packed structs are all protocol headers, it seems to me that verifying the link header padding is all that is needed to ensure that what follows is properly aligned.

Daniel Elstner <danielk>
Wed 25 Jan 2017 07:37:26 PM UTC, comment #3: 

Struct size is not really the problem: most of the structs are OK without packing. We use "packed" as "unaligned" rather. And in fact, the code bloat you saw comes from "unaligned", too. I'm not so concerned about struct layout but about getting aligned pointers.

Being like that, we wouldn't have to check PBUF_LINK_HLEN is correct but we'd rather need to check every pointer of such a now-not-packed struct for correct alignment (to catch errors of wrong alignment in the future, as often a processor will not halt on wrong alignment but just read/write garbage).

If someone presents a decent patch checking these pointers (and there might be quite a few), I'm OK with making this officially supported. Otherwise, it might just work for the version you tested but might not for any future version (as noone continuously tests it).

And again, I do think it would be great if this was officially supported!

Simon Goldschmidt <goldsimon>
Group administrator
Wed 25 Jan 2017 11:30:43 AM UTC, comment #2: 

Hm, one way would be to use static assertions to verify the size of structs in the proto headers. E.g. have something like this:


#define STATIC_ASSERT(cond, name) typedef char assert_##name[(cond) ? 1 : -1]
struct foo_header { u16 a; u8 b; u8 c; u32 d; };
STATIC_ASSERT(sizeof(struct foo_header) == 8, foo_header_size);


Additionally, if struct packing is disabled there should be a check that PBUF_LINK_HLEN (which includes ETH_PAD_SIZE) is a multiple of MEM_ALIGNMENT.

Daniel Elstner <danielk>
Wed 25 Jan 2017 11:14:20 AM UTC, comment #1: 

I do think it would be good if lwIP works without struct packing. However, if this was "official", we would kind of guarantee it works. Now you have proven it works for you, but personally, I cannot test this to work.

Given that, how would we ensure this keeps working in the future? I don't want to add support for something I cannot test...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 19 Jan 2017 07:33:20 PM UTC, original submission:  

My project is based on the AVR32 UC3C microcontroller, which does not support unaligned access. For packed structs, the compiler has to assume byte alignment and thus generates code that does single byte reads/writes and combines them to 32-bit words using logic operations.

To avoid having to pay that penalty, I define ETH_PAD_SIZE to 2 in my config and have this in my cc.h header:

#define LWIP_SKIP_PACKING_CHECK 1
#define PACK_STRUCT_FIELD(x) x
#define PACK_STRUCT_STRUCT
#define PACK_STRUCT_BEGIN
#define PACK_STRUCT_END


This alone accounts for about 6 kiB code size reduction of my flash image. I haven't made detailed measurements of the runtime speed-up, but the saving should be to the tune of 6 to 7 cycles per 32-bit word access.

The wiki hints at the possibility to disable structure packing, while mentioning that one should "make sure the system runs stable" when doing so.

Well I did (for several months), and I'm happy to report that it works like a charm. I also went through all the proto header files and reviewed them to make sure that none of the proto structs change layout when structure packing is disabled. All of them appear fine to me.

Thus I think disabling structure packing should perhaps be made an official configuration option, or at least be mentioned in the reference documentation.

Daniel Elstner <danielk>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by danielk (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-02 goldsimon StatusNone Wont Fix
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code