On Fri, 13 Sep 2024 13:05:32 -0700 Mina Almasry wrote: > Change, got NAKed: > https://lore.kernel.org/netdev/ZuSQ9BT9Vg7O2kXv@xxxxxxxxxxxxxxxxxxxx/ Humpf. > But AFAICT we don't really need to do this inside of mm, affecting > things like compound_head. This equivalent change also makes the build > pass. Does this look good? > > diff --git a/include/net/netmem.h b/include/net/netmem.h > index 8a6e20be4b9d..58f2120cd392 100644 > --- a/include/net/netmem.h > +++ b/include/net/netmem.h > @@ -100,7 +100,15 @@ static inline netmem_ref net_iov_to_netmem(struct > net_iov *niov) > > static inline netmem_ref page_to_netmem(struct page *page) > { > - return (__force netmem_ref)page; > + /* page* exported from the mm stack would not have the LSB set, but the > + * GCC 14 powerpc compiler will optimize reads into this pointer into > + * unaligned reads as it sees address arthemetic in _compound_head(). > + * > + * Explicitly clear the LSB until what looks like a GCC compiler issue > + * is resolved. > + */ > + DEBUG_NET_WARN_ON_ONCE((unsigned long)page & 1UL); > + return (__force netmem_ref)page & ~1UL; > } Hmm. Not really, the math this is doing is a bit of a cargo cult, AFAIU the operation itself is meaningless. It works because it achieves breaking the optimization/register chain in the compiler. But the exact ALU op doesn't matter. So pretending LSB is meaningful could be confusing to the reader. I think this will achieve the same effect without the spurious ALU operations (apologies for broken whitespace): diff --git a/net/core/page_pool.c b/net/core/page_pool.c index a813d30d2135..b7e0acaed933 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -864,7 +864,11 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data, allow_direct = page_pool_napi_local(pool); for (i = 0; i < count; i++) { - netmem_ref netmem = page_to_netmem(virt_to_head_page(data[i])); + struct page *page = virt_to_head_page(data[i]); + netmem_ref netmem; + + /* $explanation */ + netmem = page_to_netmem(READ_ONCE(page)); /* It is not the last user for the page frag case */ if (!page_pool_is_last_ref(netmem)) If it makes sense could you polish it up and submit?