Re: linux-next: build failure after merge of the net-next tree

Jakub Kicinski
2024-09-13T20:24:39Z

On Fri, 13 Sep 2024 13:05:32 -0700 Mina Almasry wrote:
> Change, got NAKed:
> https://lore.kernel.org/netdev/ZuSQ9BT9Vg7O2kXv@casper.infradead.org/

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?