| Message ID | 20240913213351.3537411-1-almasrymina@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [net-next,v2] page_pool: fix build on powerpc with GCC 14 | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
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.
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?
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.
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.
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.
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
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.
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
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) \
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!
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) \ >
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
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.
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))
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(-)