On Mon, 11 Jan 2021 16:35:30 +0100 Ahmad Fatoum wrote:

Hello Jakub,

On 08.01.21 17:32, Jakub Kicinski wrote:

quoted

On Fri, 8 Jan 2021 11:07:26 +0100 Ahmad Fatoum wrote:  

quoted

quoted

quoted

quoted

+struct __packed tcan4x5x_map_buf { 
+	struct tcan4x5x_buf_cmd cmd; 
+	u8 data[256 * sizeof(u32)]; 
+} ____cacheline_aligned;       

Due to the packing of the struct tcan4x5x_buf_cmd it should have a length of 4
bytes. Without __packed, will the "u8 data" come directly after the cmd?    

Yup, u8 with no alignment attribute will follow the previous
field with no holes.    

__packed has a documentation benefit though. It documents that the author
considers the current layout to be the only correct one. (and thus extra
care should be taken when modifying it).  

____cacheline_aligned adds a big architecture dependent padding at the
end of this struct, so the size of this structure is architecture
dependent. Besides using packed forced the compiler to use byte by byte
loads on architectures without unaligned access, so __packed is not
free.  

https://godbolt.org/z/j68x8n

seems to indicate that explicit alignment "overrules" packed's implicit
alignment of 1 as
 there isn't any byte-by-byte access generated for a struct
that is both packed and cacheline aligned. packed only structs are accessed
byte-by-byte however.

Did I get something wrong in my testcase?

I compiled with ARM gcc 8.2  -mno-unaligned-access -fno-strict-aliasing -O2

I see, that's why I said combining ____cacheline_aligned with __packed
looks very confusing. Good to know which one takes precedence.
That doesn't change my recommendation to remove __packed, though, let's
not leave readers of this code scratching their heads.