page_pool: fix build on powerpc with GCC 14

[PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Mina Almasry 1 year, 4 months ago

Building net-next with powerpc with GCC 14 compiler results in this
build error:

/home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
/home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
not a multiple of 4)
make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
net/core/page_pool.o] Error 1

Root caused in this thread:
https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/

We try to access offset 40 in the pointer returned by this function:

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

The GCC 14 (but not 11) compiler optimizes this by doing:

ld page + 39

Rather than:

ld (page - 1) + 40

And causing an unaligned load. Get around this by issuing a READ_ONCE as
we convert the page to netmem.  That disables the compiler optimizing the
load in this way.

Cc: Simon Horman <horms@kernel.org>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: David Miller <davem@davemloft.net>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Networking <netdev@vger.kernel.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Cc: Linux Next Mailing List <linux-next@vger.kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Cc: Matthew Wilcox <willy@infradead.org>

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mina Almasry <almasrymina@google.com>

---

v2: https://lore.kernel.org/netdev/20240913192036.3289003-1-almasrymina@google.com/

- Work around this issue as we convert the page to netmem, instead of
  a generic change that affects compound_head().
---
 net/core/page_pool.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a813d30d2135..74ea491d0ab2 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -859,12 +859,25 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
 {
 	int i, bulk_len = 0;
 	bool allow_direct;
+	netmem_ref netmem;
+	struct page *page;
 	bool in_softirq;
 
 	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]));
+		page = virt_to_head_page(data[i]);
+
+		/* GCC 14 powerpc compiler will optimize reads into the
+		 * resulting netmem_ref into unaligned reads as it sees address
+		 * arithmetic in _compound_head() call that the page has come
+		 * from.
+		 *
+		 * The READ_ONCE here gets around that by breaking the
+		 * optimization chain between the address arithmetic and later
+		 * indexing.
+		 */
+		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))
-- 
2.46.0.662.g92d0881bb0-goog

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Michael Ellerman 1 year, 4 months ago

Mina Almasry <almasrymina@google.com> writes:
> Building net-next with powerpc with GCC 14 compiler results in this
> build error:
>
> /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> not a multiple of 4)
> make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> net/core/page_pool.o] Error 1
>
> Root caused in this thread:
> https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/

Sorry I'm late to this, the original report wasn't Cc'ed to linuxppc-dev :D

I think this is a bug in the arch/powerpc inline asm constraints.

Can you try the patch below, it fixes the build error for me.

I'll run it through some boot tests and turn it into a proper patch over
the weekend.

cheers


diff --git a/arch/powerpc/include/asm/atomic.h b/arch/powerpc/include/asm/atomic.h
index 5bf6a4d49268..0e41c1da82dd 100644
--- a/arch/powerpc/include/asm/atomic.h
+++ b/arch/powerpc/include/asm/atomic.h
@@ -23,6 +23,12 @@
 #define __atomic_release_fence()					\
 	__asm__ __volatile__(PPC_RELEASE_BARRIER "" : : : "memory")
 
+#ifdef CONFIG_CC_IS_CLANG
+#define DS_FORM_CONSTRAINT "Z<>"
+#else
+#define DS_FORM_CONSTRAINT "YZ<>"
+#endif
+
 static __inline__ int arch_atomic_read(const atomic_t *v)
 {
 	int t;
@@ -197,7 +203,7 @@ static __inline__ s64 arch_atomic64_read(const atomic64_t *v)
 	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));
+		__asm__ __volatile__("ld%U1%X1 %0,%1" : "=r"(t) : DS_FORM_CONSTRAINT (v->counter));
 
 	return t;
 }
@@ -208,7 +214,7 @@ static __inline__ void arch_atomic64_set(atomic64_t *v, s64 i)
 	if (IS_ENABLED(CONFIG_PPC_KERNEL_PREFIXED))
 		__asm__ __volatile__("std %1,0(%2)" : "=m"(v->counter) : "r"(i), "b"(&v->counter));
 	else
-		__asm__ __volatile__("std%U0%X0 %1,%0" : "=m<>"(v->counter) : "r"(i));
+		__asm__ __volatile__("std%U0%X0 %1,%0" : "=" DS_FORM_CONSTRAINT (v->counter) : "r"(i));
 }
 
 #define ATOMIC64_OP(op, asm_op)						\

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Stephen Rothwell 1 year, 4 months ago

Hi all,

On Sat, 14 Sep 2024 12:02:09 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Mina Almasry <almasrymina@google.com> writes:
> > Building net-next with powerpc with GCC 14 compiler results in this
> > build error:
> >
> > /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> > /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> > not a multiple of 4)
> > make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> > net/core/page_pool.o] Error 1
> >
> > Root caused in this thread:
> > https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/  
> 
> Sorry I'm late to this, the original report wasn't Cc'ed to linuxppc-dev :D

Yeah, sorry about that.

> I think this is a bug in the arch/powerpc inline asm constraints.
> 
> Can you try the patch below, it fixes the build error for me.
> 
> I'll run it through some boot tests and turn it into a proper patch over
> the weekend.
> 
> cheers
> 
> 
> diff --git a/arch/powerpc/include/asm/atomic.h b/arch/powerpc/include/asm/atomic.h
> index 5bf6a4d49268..0e41c1da82dd 100644
> --- a/arch/powerpc/include/asm/atomic.h
> +++ b/arch/powerpc/include/asm/atomic.h
> @@ -23,6 +23,12 @@
>  #define __atomic_release_fence()					\
>  	__asm__ __volatile__(PPC_RELEASE_BARRIER "" : : : "memory")
>  
> +#ifdef CONFIG_CC_IS_CLANG
> +#define DS_FORM_CONSTRAINT "Z<>"
> +#else
> +#define DS_FORM_CONSTRAINT "YZ<>"
> +#endif
> +
>  static __inline__ int arch_atomic_read(const atomic_t *v)
>  {
>  	int t;
> @@ -197,7 +203,7 @@ static __inline__ s64 arch_atomic64_read(const atomic64_t *v)
>  	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));
> +		__asm__ __volatile__("ld%U1%X1 %0,%1" : "=r"(t) : DS_FORM_CONSTRAINT (v->counter));
>  
>  	return t;
>  }
> @@ -208,7 +214,7 @@ static __inline__ void arch_atomic64_set(atomic64_t *v, s64 i)
>  	if (IS_ENABLED(CONFIG_PPC_KERNEL_PREFIXED))
>  		__asm__ __volatile__("std %1,0(%2)" : "=m"(v->counter) : "r"(i), "b"(&v->counter));
>  	else
> -		__asm__ __volatile__("std%U0%X0 %1,%0" : "=m<>"(v->counter) : "r"(i));
> +		__asm__ __volatile__("std%U0%X0 %1,%0" : "=" DS_FORM_CONSTRAINT (v->counter) : "r"(i));
>  }
>  
>  #define ATOMIC64_OP(op, asm_op)						\

I have applied this by hand to my fixes branch for today and will
remove it when it (or something better) is applied somewhere appropriate.

-- 
Cheers,
Stephen Rothwell

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Christophe Leroy 1 year, 4 months ago


Le 14/09/2024 à 04:02, Michael Ellerman a écrit :
> Mina Almasry <almasrymina@google.com> writes:
>> Building net-next with powerpc with GCC 14 compiler results in this
>> build error:
>>
>> /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
>> /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
>> not a multiple of 4)
>> make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
>> net/core/page_pool.o] Error 1
>>
>> Root caused in this thread:
>> https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/
> 
> Sorry I'm late to this, the original report wasn't Cc'ed to linuxppc-dev :D
> 
> I think this is a bug in the arch/powerpc inline asm constraints.
> 
> Can you try the patch below, it fixes the build error for me.
> 
> I'll run it through some boot tests and turn it into a proper patch over
> the weekend.
> 
> cheers
> 
> 
> diff --git a/arch/powerpc/include/asm/atomic.h b/arch/powerpc/include/asm/atomic.h
> index 5bf6a4d49268..0e41c1da82dd 100644
> --- a/arch/powerpc/include/asm/atomic.h
> +++ b/arch/powerpc/include/asm/atomic.h
> @@ -23,6 +23,12 @@
>   #define __atomic_release_fence()					\
>   	__asm__ __volatile__(PPC_RELEASE_BARRIER "" : : : "memory")
>   
> +#ifdef CONFIG_CC_IS_CLANG
> +#define DS_FORM_CONSTRAINT "Z<>"
> +#else
> +#define DS_FORM_CONSTRAINT "YZ<>"
> +#endif

I see we have the same in uaccess.h, added by commit 2d43cc701b96 
("powerpc/uaccess: Fix build errors seen with GCC 13/14")

Should that go in a common header, maybe ppc_asm.h ?

> +
>   static __inline__ int arch_atomic_read(const atomic_t *v)
>   {
>   	int t;
> @@ -197,7 +203,7 @@ static __inline__ s64 arch_atomic64_read(const atomic64_t *v)
>   	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));
> +		__asm__ __volatile__("ld%U1%X1 %0,%1" : "=r"(t) : DS_FORM_CONSTRAINT (v->counter));
>   
>   	return t;
>   }
> @@ -208,7 +214,7 @@ static __inline__ void arch_atomic64_set(atomic64_t *v, s64 i)
>   	if (IS_ENABLED(CONFIG_PPC_KERNEL_PREFIXED))
>   		__asm__ __volatile__("std %1,0(%2)" : "=m"(v->counter) : "r"(i), "b"(&v->counter));
>   	else
> -		__asm__ __volatile__("std%U0%X0 %1,%0" : "=m<>"(v->counter) : "r"(i));
> +		__asm__ __volatile__("std%U0%X0 %1,%0" : "=" DS_FORM_CONSTRAINT (v->counter) : "r"(i));
>   }
>   
>   #define ATOMIC64_OP(op, asm_op)						\
>

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Michael Ellerman 1 year, 4 months ago

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 14/09/2024 à 04:02, Michael Ellerman a écrit :
...
>> 
>> diff --git a/arch/powerpc/include/asm/atomic.h b/arch/powerpc/include/asm/atomic.h
>> index 5bf6a4d49268..0e41c1da82dd 100644
>> --- a/arch/powerpc/include/asm/atomic.h
>> +++ b/arch/powerpc/include/asm/atomic.h
>> @@ -23,6 +23,12 @@
>>   #define __atomic_release_fence()					\
>>   	__asm__ __volatile__(PPC_RELEASE_BARRIER "" : : : "memory")
>>   
>> +#ifdef CONFIG_CC_IS_CLANG
>> +#define DS_FORM_CONSTRAINT "Z<>"
>> +#else
>> +#define DS_FORM_CONSTRAINT "YZ<>"
>> +#endif
>
> I see we have the same in uaccess.h, added by commit 2d43cc701b96 
> ("powerpc/uaccess: Fix build errors seen with GCC 13/14")

Yep.

> Should that go in a common header, maybe ppc_asm.h ?

That would be the obvious place, but unfortunately including ppc_asm.h
in atomic.h breaks the build due to header spaghetti.

For now I've put the defines in asm-compat.h, which is not ideal but
seems to work.

cheers

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Jakub Kicinski 1 year, 4 months ago

On Sat, 14 Sep 2024 12:02:09 +1000 Michael Ellerman wrote:
> Can you try the patch below, it fixes the build error for me.

Excellent, fixes it for me too!
-- 
pw-bot: nap

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Matthew Wilcox 1 year, 4 months ago

On Fri, Sep 13, 2024 at 09:33:51PM +0000, Mina Almasry wrote:
> Building net-next with powerpc with GCC 14 compiler results in this
> build error:
> 
> /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> not a multiple of 4)
> make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> net/core/page_pool.o] Error 1
> 
> Root caused in this thread:
> https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/

It would be better to include a direct link to the GCC bugzilla.

> We try to access offset 40 in the pointer returned by this function:
> 
> 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);
> }
> 
> The GCC 14 (but not 11) compiler optimizes this by doing:
> 
> ld page + 39
> 
> Rather than:
> 
> ld (page - 1) + 40
> 
> And causing an unaligned load. Get around this by issuing a READ_ONCE as
> we convert the page to netmem.  That disables the compiler optimizing the
> load in this way.
> 
> Cc: Simon Horman <horms@kernel.org>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: David Miller <davem@davemloft.net>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Networking <netdev@vger.kernel.org>
> Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Cc: Linux Next Mailing List <linux-next@vger.kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> 
> ---
> 
> v2: https://lore.kernel.org/netdev/20240913192036.3289003-1-almasrymina@google.com/
> 
> - Work around this issue as we convert the page to netmem, instead of
>   a generic change that affects compound_head().
> ---
>  net/core/page_pool.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index a813d30d2135..74ea491d0ab2 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -859,12 +859,25 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
>  {
>  	int i, bulk_len = 0;
>  	bool allow_direct;
> +	netmem_ref netmem;
> +	struct page *page;
>  	bool in_softirq;
>  
>  	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]));
> +		page = virt_to_head_page(data[i]);
> +
> +		/* GCC 14 powerpc compiler will optimize reads into the
> +		 * resulting netmem_ref into unaligned reads as it sees address
> +		 * arithmetic in _compound_head() call that the page has come
> +		 * from.
> +		 *
> +		 * The READ_ONCE here gets around that by breaking the
> +		 * optimization chain between the address arithmetic and later
> +		 * indexing.
> +		 */
> +		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))
> -- 
> 2.46.0.662.g92d0881bb0-goog
>

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Mina Almasry 1 year, 4 months ago

On Fri, Sep 13, 2024 at 2:55 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Sep 13, 2024 at 09:33:51PM +0000, Mina Almasry wrote:
> > Building net-next with powerpc with GCC 14 compiler results in this
> > build error:
> >
> > /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> > /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> > not a multiple of 4)
> > make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> > net/core/page_pool.o] Error 1
> >
> > Root caused in this thread:
> > https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/
>
> It would be better to include a direct link to the GCC bugzilla.
>

I have not reported the issue to GCC yet. From the build break thread
it seemed a fix was urgent, so I posted the fix and was planning to
report the issue after. If not, no problem, I'll report the issue and
repost the fix with a GCC bugzilla link, waiting 24hr before reposts
this time. I just need to go through the steps in
https://gcc.gnu.org/bugs/, shouldn't be an issue.

-- 
Thanks,
Mina

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Jakub Kicinski 1 year, 4 months ago

On Fri, 13 Sep 2024 15:20:13 -0700 Mina Almasry wrote:
> I have not reported the issue to GCC yet. From the build break thread
> it seemed a fix was urgent, so I posted the fix and was planning to
> report the issue after. If not, no problem, I'll report the issue and
> repost the fix with a GCC bugzilla link, waiting 24hr before reposts
> this time.

I should have clarified, the "please post ASAP" applies
to all devmem build fixes, ignore the cool down period :)

> I just need to go through the steps in https://gcc.gnu.org/bugs/,
> shouldn't be an issue.

Just post the link here, I'll add it to the commit msg when applying.

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Mina Almasry 1 year, 4 months ago

On Fri, Sep 13, 2024 at 5:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 13 Sep 2024 15:20:13 -0700 Mina Almasry wrote:
> > I have not reported the issue to GCC yet. From the build break thread
> > it seemed a fix was urgent, so I posted the fix and was planning to
> > report the issue after. If not, no problem, I'll report the issue and
> > repost the fix with a GCC bugzilla link, waiting 24hr before reposts
> > this time.
>
> I should have clarified, the "please post ASAP" applies
> to all devmem build fixes, ignore the cool down period :)
>
> > I just need to go through the steps in https://gcc.gnu.org/bugs/,
> > shouldn't be an issue.
>
> Just post the link here, I'll add it to the commit msg when applying.

Ah, I need a GCC bugzilla account before I can file bugs there. I
don't currently have one and creating an account involves emailing
them and waiting 24hr. I've done that and am waiting for an account.
I'll file the issue as soon as I get access and post the link here.
I'm also poking to see if anyone around already has an account and can
file the issue on my behalf.

--
Thanks,
Mina

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Stanislav Fomichev 1 year, 4 months ago

On 09/13, Mina Almasry wrote:
> Building net-next with powerpc with GCC 14 compiler results in this
> build error:
> 
> /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> not a multiple of 4)
> make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> net/core/page_pool.o] Error 1
> 
> Root caused in this thread:
> https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/
> 
> We try to access offset 40 in the pointer returned by this function:
> 
> 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);
> }
> 
> The GCC 14 (but not 11) compiler optimizes this by doing:
> 
> ld page + 39
> 
> Rather than:
> 
> ld (page - 1) + 40
> 
> And causing an unaligned load. Get around this by issuing a READ_ONCE as
> we convert the page to netmem.  That disables the compiler optimizing the
> load in this way.
> 
> Cc: Simon Horman <horms@kernel.org>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: David Miller <davem@davemloft.net>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Networking <netdev@vger.kernel.org>
> Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Cc: Linux Next Mailing List <linux-next@vger.kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> 
> ---
> 
> v2: https://lore.kernel.org/netdev/20240913192036.3289003-1-almasrymina@google.com/
> 
> - Work around this issue as we convert the page to netmem, instead of
>   a generic change that affects compound_head().
> ---
>  net/core/page_pool.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index a813d30d2135..74ea491d0ab2 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -859,12 +859,25 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
>  {
>  	int i, bulk_len = 0;
>  	bool allow_direct;
> +	netmem_ref netmem;
> +	struct page *page;
>  	bool in_softirq;
>  
>  	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]));
> +		page = virt_to_head_page(data[i]);
> +
> +		/* GCC 14 powerpc compiler will optimize reads into the
> +		 * resulting netmem_ref into unaligned reads as it sees address
> +		 * arithmetic in _compound_head() call that the page has come
> +		 * from.
> +		 *
> +		 * The READ_ONCE here gets around that by breaking the
> +		 * optimization chain between the address arithmetic and later
> +		 * indexing.
> +		 */
> +		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))

Are we sure this is the only place where we can hit by this?
Any reason not to hide this inside page_to_netmem?

diff --git a/include/net/netmem.h b/include/net/netmem.h
index 8a6e20be4b9d..46bc362acec4 100644
--- a/include/net/netmem.h
+++ b/include/net/netmem.h
@@ -100,7 +100,7 @@ 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;
+       return (__force netmem_ref)READ_ONCE(page);
 }

 static inline int netmem_ref_count(netmem_ref netmem)

Is it gonna generate slower code elsewhere?

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Mina Almasry 1 year, 4 months ago

On Fri, Sep 13, 2024 at 2:55 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 09/13, Mina Almasry wrote:
> > Building net-next with powerpc with GCC 14 compiler results in this
> > build error:
> >
> > /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> > /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> > not a multiple of 4)
> > make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> > net/core/page_pool.o] Error 1
> >
> > Root caused in this thread:
> > https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/
> >
> > We try to access offset 40 in the pointer returned by this function:
> >
> > 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);
> > }
> >
> > The GCC 14 (but not 11) compiler optimizes this by doing:
> >
> > ld page + 39
> >
> > Rather than:
> >
> > ld (page - 1) + 40
> >
> > And causing an unaligned load. Get around this by issuing a READ_ONCE as
> > we convert the page to netmem.  That disables the compiler optimizing the
> > load in this way.
> >
> > Cc: Simon Horman <horms@kernel.org>
> > Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: David Miller <davem@davemloft.net>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: Networking <netdev@vger.kernel.org>
> > Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> > Cc: Linux Next Mailing List <linux-next@vger.kernel.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
> > Cc: Matthew Wilcox <willy@infradead.org>
> >
> > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Mina Almasry <almasrymina@google.com>
> >
> > ---
> >
> > v2: https://lore.kernel.org/netdev/20240913192036.3289003-1-almasrymina@google.com/
> >
> > - Work around this issue as we convert the page to netmem, instead of
> >   a generic change that affects compound_head().
> > ---
> >  net/core/page_pool.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index a813d30d2135..74ea491d0ab2 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -859,12 +859,25 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
> >  {
> >       int i, bulk_len = 0;
> >       bool allow_direct;
> > +     netmem_ref netmem;
> > +     struct page *page;
> >       bool in_softirq;
> >
> >       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]));
> > +             page = virt_to_head_page(data[i]);
> > +
> > +             /* GCC 14 powerpc compiler will optimize reads into the
> > +              * resulting netmem_ref into unaligned reads as it sees address
> > +              * arithmetic in _compound_head() call that the page has come
> > +              * from.
> > +              *
> > +              * The READ_ONCE here gets around that by breaking the
> > +              * optimization chain between the address arithmetic and later
> > +              * indexing.
> > +              */
> > +             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))
>
> Are we sure this is the only place where we can hit by this?
> Any reason not to hide this inside page_to_netmem?
>
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 8a6e20be4b9d..46bc362acec4 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -100,7 +100,7 @@ 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;
> +       return (__force netmem_ref)READ_ONCE(page);
>  }
>
>  static inline int netmem_ref_count(netmem_ref netmem)
>
> Is it gonna generate slower code elsewhere?

Yeah, I think it will likely generate slower code elsewhere, and
avoiding the overhead when this is the only callsite that needs this
really seemed like a plus.

-- 
Thanks,
Mina

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Matthew Wilcox 1 year, 4 months ago

On Fri, Sep 13, 2024 at 02:55:19PM -0700, Stanislav Fomichev wrote:
> On 09/13, Mina Almasry wrote:
> > Building net-next with powerpc with GCC 14 compiler results in this
> > build error:
> > 
> > /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> > /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> > not a multiple of 4)
> > make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> > net/core/page_pool.o] Error 1
> 
> Are we sure this is the only place where we can hit by this?

It's a compilation error, so yes, we're sure.

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Stanislav Fomichev 1 year, 4 months ago

On 09/13, Matthew Wilcox wrote:
> On Fri, Sep 13, 2024 at 02:55:19PM -0700, Stanislav Fomichev wrote:
> > On 09/13, Mina Almasry wrote:
> > > Building net-next with powerpc with GCC 14 compiler results in this
> > > build error:
> > > 
> > > /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> > > /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> > > not a multiple of 4)
> > > make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> > > net/core/page_pool.o] Error 1
> > 
> > Are we sure this is the only place where we can hit by this?
> 
> It's a compilation error, so yes, we're sure.

We also have netmem_compound_head() which does page_to_netmem(compound_head()).
Wondering whether we'll eventually hit a similar issue over there.

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Mina Almasry 1 year, 4 months ago

On Fri, Sep 13, 2024 at 3:27 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 09/13, Matthew Wilcox wrote:
> > On Fri, Sep 13, 2024 at 02:55:19PM -0700, Stanislav Fomichev wrote:
> > > On 09/13, Mina Almasry wrote:
> > > > Building net-next with powerpc with GCC 14 compiler results in this
> > > > build error:
> > > >
> > > > /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> > > > /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> > > > not a multiple of 4)
> > > > make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> > > > net/core/page_pool.o] Error 1
> > >
> > > Are we sure this is the only place where we can hit by this?
> >
> > It's a compilation error, so yes, we're sure.
>
> We also have netmem_compound_head() which does page_to_netmem(compound_head()).
> Wondering whether we'll eventually hit a similar issue over there.

A bit of a head scratcher why the compiler isn't running into the same
issue for netmem_compound_head.

The callsites of netmem_compound_head are in net/core/skbuff.c, in
skb_pp_frag_ref & napi_pp_put_page. Looking at the assembly generated,
looks like somehow the compiler completely optimized out the call in
napi_pp_put_page, and the call in skb_pp_frag_ref morphs into:

 # net/core/skbuff.c:1047:      return
napi_pp_put_page(page_to_netmem(virt_to_page(data)));
        addis 9,2,.LC63@toc@ha   # tmp158,,
        ld 10,.LC63@toc@l(9)     #, tmp140
 # ./arch/powerpc/include/asm/page.h:230:       return __pa(kaddr) >>
PAGE_SHIFT;
        rldicl 9,31,48,20        #, _17, head,
 # net/core/skbuff.c:1047:      return
napi_pp_put_page(page_to_netmem(virt_to_page(data)));
        sldi 9,9,6       #, _18, _17
 # net/core/skbuff.c:1047:      return
napi_pp_put_page(page_to_netmem(virt_to_page(data)));
        ld 3,0(10)       # vmemmap, vmemmap
 # net/core/skbuff.c:1047:      return
napi_pp_put_page(page_to_netmem(virt_to_page(data)));
        add 3,3,9        #, vmemmap, _18

Since it's page_to_netmem(virt_to_page(data)) (not virt_to_head_page),
the we don't hit there right now. It's certainly possible to trigger
this in the future.

I think we could also READ_ONCE in netmem_compound_head for some
future proofness.

--
Thanks,
Mina

Re: [PATCH net-next v2] page_pool: fix build on powerpc with GCC 14

Posted by Mina Almasry 1 year, 4 months ago

On Fri, Sep 13, 2024 at 2:33 PM Mina Almasry <almasrymina@google.com> wrote:
>
> Building net-next with powerpc with GCC 14 compiler results in this
> build error:
>
> /home/sfr/next/tmp/ccuSzwiR.s: Assembler messages:
> /home/sfr/next/tmp/ccuSzwiR.s:2579: Error: operand out of domain (39 is
> not a multiple of 4)
> make[5]: *** [/home/sfr/next/next/scripts/Makefile.build:229:
> net/core/page_pool.o] Error 1
>
> Root caused in this thread:
> https://lore.kernel.org/netdev/913e2fbd-d318-4c9b-aed2-4d333a1d5cf0@cs-soprasteria.com/
>
> We try to access offset 40 in the pointer returned by this function:
>
> 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);
> }
>
> The GCC 14 (but not 11) compiler optimizes this by doing:
>
> ld page + 39
>
> Rather than:
>
> ld (page - 1) + 40
>
> And causing an unaligned load. Get around this by issuing a READ_ONCE as
> we convert the page to netmem.  That disables the compiler optimizing the
> load in this way.
>
> Cc: Simon Horman <horms@kernel.org>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: David Miller <davem@davemloft.net>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Networking <netdev@vger.kernel.org>
> Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Cc: Linux Next Mailing List <linux-next@vger.kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
> Cc: Matthew Wilcox <willy@infradead.org>
>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
>

Gah, right after I hit send I realized I missed the 24hr rule.
Although I'm unsure about the urgency of build fixes. Sorry about
that.

-- 
Thanks,
Mina