| Message ID | 20240913192036.3289003-1-almasrymina@google.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] mm: fix build on powerpc with GCC 14 | expand |
On Fri, Sep 13, 2024 at 07:20:36PM +0000, Mina Almasry wrote: > +++ 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); NAK, that pessimises compound_head().
Hi, Le 13/09/2024 à 21:22, Matthew Wilcox a écrit : > On Fri, Sep 13, 2024 at 07:20:36PM +0000, Mina Almasry wrote: >> +++ 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); > > NAK, that pessimises compound_head(). > Can you please give more details on what the difference is ? I can't see what it pessimises. In both cases, you test if the value is odd, when it is odd you make it even. Christophe
On Sat, Sep 14, 2024 at 08:50:46AM +0200, Christophe Leroy wrote: > Hi, > > Le 13/09/2024 à 21:22, Matthew Wilcox a écrit : > > On Fri, Sep 13, 2024 at 07:20:36PM +0000, Mina Almasry wrote: > > > +++ 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); > > > > NAK, that pessimises compound_head(). > > > > Can you please give more details on what the difference is ? > > I can't see what it pessimises. In both cases, you test if the value is odd, > when it is odd you make it even. On x86, for example, it is perfectly valid to load a 64-bit value from an offset of 0x2f relative to a pointer. So there's no need to make it even.
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); }
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 Causing an unaligned read error. Fix this by bitwise operand instead of an arthimetic operation to clear the pointer, which probably communicates the intention of the code a bit better anyway. 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> Suggested-by: LEROY Christophe <christophe.leroy2@cs-soprasteria.com> Signed-off-by: Mina Almasry <almasrymina@google.com> --- include/linux/page-flags.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)