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

[Jakub Kicinski]

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?