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

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

[Jakub Kicinski]

On Fri, 13 Sep 2024 20:41:38 +1000 Stephen Rothwell wrote:
> I have bisected it (just using the net-next tree) to commit
> 
> 8ab79ed50cf10f338465c296012500de1081646f is the first bad commit
> commit 8ab79ed50cf10f338465c296012500de1081646f
> Author: Mina Almasry <almasrymina@google.com>
> Date:   Tue Sep 10 17:14:49 2024 +0000
> 
>     page_pool: devmem support
>     
> 
> And it may be pointing at arch/powerpc/include/asm/atomic.h line 200
> which is this:
> 
> static __inline__ s64 arch_atomic64_read(const atomic64_t *v)
> {
>         s64 t;
> 
>         /* -mprefixed can generate offsets beyond range, fall back hack */
>         if (IS_ENABLED(CONFIG_PPC_KERNEL_PREFIXED))
>                 __asm__ __volatile__("ld %0,0(%1)" : "=r"(t) : "b"(&v->counter))
> ;
>         else
>                 __asm__ __volatile__("ld%U1%X1 %0,%1" : "=r"(t) : "m<>"(v->counter));
> 
>         return t;
> }
> 
> The second "asm" above (CONFIG_PPC_KERNEL_PREFIXED is not set).  I am
> guessing by searching for "39" in net/core/page_pool.s
> 
> This is maybe called from page_pool_unref_netmem()

Thanks! The compiler version helped, I can repro with GCC 14.

It's something special about compound page handling on powerpc64,
AFAICT. I'm guessing that the assembler is mad that we're doing
an unaligned read:

   3300         ld 8,39(8)       # MEM[(const struct atomic64_t *)_29].counter, t

which does indeed look unaligned to a naked eye. If I replace
virt_to_head_page() with virt_to_page() on line 867 in net/core/page_pool.c
I get:

   2982         ld 8,40(10)      # MEM[(const struct atomic64_t *)_94].counter, t

and that's what we'd expect. It's reading pp_ref_count which is at
offset 40 in struct net_iov. I'll try to take a closer look at 
the compound page handling, with powerpc assembly book in hand, 
but perhaps this rings a bell for someone?

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

[Jakub Kicinski]

On Fri, 13 Sep 2024 08:34:26 -0700 Jakub Kicinski wrote:
> > The second "asm" above (CONFIG_PPC_KERNEL_PREFIXED is not set).  I am
> > guessing by searching for "39" in net/core/page_pool.s
> > 
> > This is maybe called from page_pool_unref_netmem()  
> 
> Thanks! The compiler version helped, I can repro with GCC 14.
> 
> It's something special about compound page handling on powerpc64,
> AFAICT. I'm guessing that the assembler is mad that we're doing
> an unaligned read:
> 
>    3300         ld 8,39(8)       # MEM[(const struct atomic64_t *)_29].counter, t
> 
> which does indeed look unaligned to a naked eye. If I replace
> virt_to_head_page() with virt_to_page() on line 867 in net/core/page_pool.c
> I get:
> 
>    2982         ld 8,40(10)      # MEM[(const struct atomic64_t *)_94].counter, t
> 
> and that's what we'd expect. It's reading pp_ref_count which is at
> offset 40 in struct net_iov. I'll try to take a closer look at 
> the compound page handling, with powerpc assembly book in hand, 
> but perhaps this rings a bell for someone?

Oh, okay, I think I understand now. My lack of MM knowledge showing.
So if it's a compound head we do:

static inline unsigned long _compound_head(const struct page *page)             
{                                                                               
        unsigned long head = READ_ONCE(page->compound_head);                    
                                                                                
        if (unlikely(head & 1))                                                 
                return head - 1;                                                
        return (unsigned long)page_fixed_fake_head(page);                       
}

Presumably page->compound_head stores the pointer to the head page.
I'm guessing the compiler is "smart" and decides "why should I do
ld (page - 1) + 40, when I can do ld page + 39 :|

I think it's a compiler bug...

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

[LEROY Christophe]

Le 13/09/2024 à 17:49, Jakub Kicinski a écrit :
> On Fri, 13 Sep 2024 08:34:26 -0700 Jakub Kicinski wrote:
>>> The second "asm" above (CONFIG_PPC_KERNEL_PREFIXED is not set).  I am
>>> guessing by searching for "39" in net/core/page_pool.s
>>>
>>> This is maybe called from page_pool_unref_netmem()
>>
>> Thanks! The compiler version helped, I can repro with GCC 14.
>>
>> It's something special about compound page handling on powerpc64,
>> AFAICT. I'm guessing that the assembler is mad that we're doing
>> an unaligned read:
>>
>>     3300         ld 8,39(8)       # MEM[(const struct atomic64_t *)_29].counter, t
>>
>> which does indeed look unaligned to a naked eye. If I replace
>> virt_to_head_page() with virt_to_page() on line 867 in net/core/page_pool.c
>> I get:
>>
>>     2982         ld 8,40(10)      # MEM[(const struct atomic64_t *)_94].counter, t
>>
>> and that's what we'd expect. It's reading pp_ref_count which is at
>> offset 40 in struct net_iov. I'll try to take a closer look at
>> the compound page handling, with powerpc assembly book in hand,
>> but perhaps this rings a bell for someone?
> 
> Oh, okay, I think I understand now. My lack of MM knowledge showing.
> So if it's a compound head we do:
> 
> static inline unsigned long _compound_head(const struct page *page)
> {
>          unsigned long head = READ_ONCE(page->compound_head);
>                                                                                  
>          if (unlikely(head & 1))
>                  return head - 1;
>          return (unsigned long)page_fixed_fake_head(page);
> }
> 
> Presumably page->compound_head stores the pointer to the head page.
> I'm guessing the compiler is "smart" and decides "why should I do
> ld (page - 1) + 40, when I can do ld page + 39 :|
> 
> I think it's a compiler bug...
> 

Would it work if you replace it with following ?

	return head & ~1;

Christophe

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

[Mina Almasry]

On Fri, Sep 13, 2024 at 9:13 AM LEROY Christophe
<christophe.leroy2@cs-soprasteria.com> wrote:
>
>
>
> Le 13/09/2024 à 17:49, Jakub Kicinski a écrit :
> > On Fri, 13 Sep 2024 08:34:26 -0700 Jakub Kicinski wrote:
> >>> The second "asm" above (CONFIG_PPC_KERNEL_PREFIXED is not set).  I am
> >>> guessing by searching for "39" in net/core/page_pool.s
> >>>
> >>> This is maybe called from page_pool_unref_netmem()
> >>
> >> Thanks! The compiler version helped, I can repro with GCC 14.
> >>
> >> It's something special about compound page handling on powerpc64,
> >> AFAICT. I'm guessing that the assembler is mad that we're doing
> >> an unaligned read:
> >>
> >>     3300         ld 8,39(8)       # MEM[(const struct atomic64_t *)_29].counter, t
> >>
> >> which does indeed look unaligned to a naked eye. If I replace
> >> virt_to_head_page() with virt_to_page() on line 867 in net/core/page_pool.c
> >> I get:
> >>
> >>     2982         ld 8,40(10)      # MEM[(const struct atomic64_t *)_94].counter, t
> >>
> >> and that's what we'd expect. It's reading pp_ref_count which is at
> >> offset 40 in struct net_iov. I'll try to take a closer look at
> >> the compound page handling, with powerpc assembly book in hand,
> >> but perhaps this rings a bell for someone?
> >
> > Oh, okay, I think I understand now. My lack of MM knowledge showing.
> > So if it's a compound head we do:
> >
> > static inline unsigned long _compound_head(const struct page *page)
> > {
> >          unsigned long head = READ_ONCE(page->compound_head);
> >
> >          if (unlikely(head & 1))
> >                  return head - 1;
> >          return (unsigned long)page_fixed_fake_head(page);
> > }
> >
> > Presumably page->compound_head stores the pointer to the head page.
> > I'm guessing the compiler is "smart" and decides "why should I do
> > ld (page - 1) + 40, when I can do ld page + 39 :|
> >
> > I think it's a compiler bug...
> >
>
> Would it work if you replace it with following ?
>
>         return head & ~1;
>

I was able to reproduce with the correct compiler version, and yes,
this fixes the build for me. Thanks!

Probably healthy to add UL, yes?

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 5769fe6e4950..ea4005d2d1a9 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -239,8 +239,8 @@ static inline unsigned long _compound_head(const
struct page *page)
 {
        unsigned long head = READ_ONCE(page->compound_head);

-       if (unlikely(head & 1))
-               return head - 1;
+       if (unlikely(head & 1UL))
+               return head & ~1UL;
        return (unsigned long)page_fixed_fake_head(page);
 }

Other than that I think this is a correct fix. Jakub, what to do here.
Do I send this fix to the mm tree or to net-next?

-- 
Thanks,
Mina

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

[Jakub Kicinski]

On Fri, 13 Sep 2024 09:27:17 -0700 Mina Almasry wrote:
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 5769fe6e4950..ea4005d2d1a9 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -239,8 +239,8 @@ static inline unsigned long _compound_head(const
> struct page *page)
>  {
>         unsigned long head = READ_ONCE(page->compound_head);
> 
> -       if (unlikely(head & 1))
> -               return head - 1;
> +       if (unlikely(head & 1UL))
> +               return head & ~1UL;
>         return (unsigned long)page_fixed_fake_head(page);
>  }
> 
> Other than that I think this is a correct fix. Jakub, what to do here.
> Do I send this fix to the mm tree or to net-next?

Yes, please, send this out and CC all the relevant people.
We can decide which tree it will go into once its reviewed.

Stephen, would you be willing to slap this on top of linux-next for now?
I can't think of a better bandaid we could put in net-next,
and it'd be sad to revert a major feature because of a compiler bug(?)

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

[Mina Almasry]

On Fri, Sep 13, 2024 at 11:36 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 13 Sep 2024 09:27:17 -0700 Mina Almasry wrote:
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 5769fe6e4950..ea4005d2d1a9 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -239,8 +239,8 @@ static inline unsigned long _compound_head(const
> > struct page *page)
> >  {
> >         unsigned long head = READ_ONCE(page->compound_head);
> >
> > -       if (unlikely(head & 1))
> > -               return head - 1;
> > +       if (unlikely(head & 1UL))
> > +               return head & ~1UL;
> >         return (unsigned long)page_fixed_fake_head(page);
> >  }
> >
> > Other than that I think this is a correct fix. Jakub, what to do here.
> > Do I send this fix to the mm tree or to net-next?
>
> Yes, please, send this out and CC all the relevant people.
> We can decide which tree it will go into once its reviewed.
>
> Stephen, would you be willing to slap this on top of linux-next for now?
> I can't think of a better bandaid we could put in net-next,
> and it'd be sad to revert a major feature because of a compiler bug(?)

Change, got NAKed:
https://lore.kernel.org/netdev/ZuSQ9BT9Vg7O2kXv@casper.infradead.org/

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;
 }

--
Thanks,
Mina

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@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?