[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHS8izPf29T51QB4u46NJRc=C77vVDbR1nXekJ5-ysJJg8fK8g@mail.gmail.com>
Date: Fri, 13 Sep 2024 09:27:17 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: christophe.leroy2@...soprasteria.com
Cc: Jakub Kicinski <kuba@...nel.org>, Stephen Rothwell <sfr@...b.auug.org.au>,
David Miller <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>,
Networking <netdev@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Linux Next Mailing List <linux-next@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: linux-next: build failure after merge of the net-next tree
On Fri, Sep 13, 2024 at 9:13 AM LEROY Christophe
<christophe.leroy2@...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
Powered by blists - more mailing lists