[kbuild-all] Re: Bogus struct page layout on 32-bit

+CC Grygorii for the cpsw part as Ivan's email is not valid anymore Thanks for catching this. Interesting indeed... On Sat, 10 Apr 2021 at 09:22, Jesper Dangaard Brouer <brouer(a)redhat.com&gt; wrote: > > On Sat, 10 Apr 2021 03:43:13 +0100 > Matthew Wilcox <willy(a)infradead.org&gt; wrote: > >> On Sat, Apr 10, 2021 at 06:45:35AM +0800, kernel test robot wrote: >>>>> include/linux/mm_types.h:274:1: error: static_assert failed due to requirement '__builtin_offsetof(struct page, lru) == __builtin_offsetof(struct folio, lru)' "offsetof(struct page, lru) == offsetof(struct folio, lru)" >>> FOLIO_MATCH(lru, lru); >>> include/linux/mm_types.h:272:2: note: expanded from macro 'FOLIO_MATCH' >>> static_assert(offsetof(struct page, pg) == offsetof(struct folio, fl)) >> >> Well, this is interesting. pahole reports: >> >> struct page { >> long unsigned int flags; /* 0 4 */ >> /* XXX 4 bytes hole, try to pack */ >> union { >> struct { >> struct list_head lru; /* 8 8 */ >> ... >> struct folio { >> union { >> struct { >> long unsigned int flags; /* 0 4 */ >> struct list_head lru; /* 4 8 */ >> >> so this assert has absolutely done its job. >> >> But why has this assert triggered? Why is struct page layout not what >> we thought it was? Turns out it's the dma_addr added in 2019 by commit >> c25fff7171be ("mm: add dma_addr_t to struct page"). On this particular >> config, it's 64-bit, and ppc32 requires alignment to 64-bit. So >> the whole union gets moved out by 4 bytes. > > Argh, good that you are catching this! > >> Unfortunately, we can't just fix this by putting an 'unsigned long pad' >> in front of it. It still aligns the entire union to 8 bytes, and then >> it skips another 4 bytes after the pad. >> >> We can fix it like this ... >> >> +++ b/include/linux/mm_types.h >> @@ -96,11 +96,12 @@ struct page { >> unsigned long private; >> }; >> struct { /* page_pool used by netstack */ >> + unsigned long _page_pool_pad; > > I'm fine with this pad. Matteo is currently proposing[1] to add a 32-bit > value after @dma_addr, and he could use this area instead. > > [1] https://lore.kernel.org/netdev/20210409223801.104657-3-mcroce@linux.micro... > > When adding/changing this, we need to make sure that it doesn't overlap > member @index, because network stack use/check page_is_pfmemalloc(). > As far as my calculations this is safe to add. I always try to keep an > eye out for this, but I wonder if we could have a build check like yours. > > >> /** >> * @dma_addr: might require a 64-bit value even on >> * 32-bit architectures. >> */ >> - dma_addr_t dma_addr; >> + dma_addr_t dma_addr __packed; >> }; >> struct { /* slab, slob and slub */ >> union { >> >> but I don't know if GCC is smart enough to realise that dma_addr is now >> on an 8 byte boundary and it can use a normal instruction to access it, >> or whether it'll do something daft like use byte loads to access it. >> >> We could also do: >> >> + dma_addr_t dma_addr __packed __aligned(sizeof(void *)); >> >> and I see pahole, at least sees this correctly: >> >> struct { >> long unsigned int _page_pool_pad; /* 4 4 */ >> dma_addr_t dma_addr __attribute__((__aligned__(4))); /* 8 8 */ >> } __attribute__((__packed__)) __attribute__((__aligned__(4))); >> >> This presumably affects any 32-bit architecture with a 64-bit phys_addr_t >> / dma_addr_t. Advice, please? > > I'm not sure that the 32-bit behavior is with 64-bit (dma) addrs. > > I don't have any 32-bit boards with 64-bit DMA. Cc. Ivan, wasn't your > board (572x ?) 32-bit with driver 'cpsw' this case (where Ivan added > XDP+page_pool) ?