+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@xxxxxxxxxx> wrote:
>
> On Sat, 10 Apr 2021 03:43:13 +0100
> Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx/
>
> 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) ?
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
>
