[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y2WQ7I4LXh8iUIRd@intel.com>
Date: Sat, 5 Nov 2022 00:23:40 +0200
From: Ville Syrjälä <ville.syrjala@...ux.intel.com>
To: Naoya Horiguchi <naoya.horiguchi@...ux.dev>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>,
Mike Kravetz <mike.kravetz@...cle.com>,
Miaohe Lin <linmiaohe@...wei.com>,
Liu Shixin <liushixin2@...wei.com>,
Yang Shi <shy828301@...il.com>,
Oscar Salvador <osalvador@...e.de>,
Muchun Song <songmuchun@...edance.com>,
linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
regressions@...ts.linux.dev, naoya.horiguchi@....com
Subject: Re: [mm-unstable PATCH v7 2/8] mm/hugetlb: make pud_huge() and
follow_huge_pud() aware of non-present pud entry
On Sat, Nov 05, 2022 at 12:59:30AM +0900, Naoya Horiguchi wrote:
> On Wed, Nov 02, 2022 at 10:51:40PM +0200, Ville Syrjälä wrote:
> > On Thu, Jul 14, 2022 at 01:24:14PM +0900, Naoya Horiguchi wrote:
> > > +/*
> > > + * pud_huge() returns 1 if @pud is hugetlb related entry, that is normal
> > > + * hugetlb entry or non-present (migration or hwpoisoned) hugetlb entry.
> > > + * Otherwise, returns 0.
> > > + */
> > > int pud_huge(pud_t pud)
> > > {
> > > - return !!(pud_val(pud) & _PAGE_PSE);
> > > + return !pud_none(pud) &&
> > > + (pud_val(pud) & (_PAGE_PRESENT|_PAGE_PSE)) != _PAGE_PRESENT;
> > > }
> >
> > Hi,
> >
> > This causes i915 to trip a BUG_ON() on x86-32 when I start X.
>
> Hello,
>
> Thank you for finding and reporting the issue.
>
> x86-32 does not enable CONFIG_ARCH_HAS_GIGANTIC_PAGE, so pud_huge() is
> supposed to be false on x86-32. Doing like below looks to me a fix
> (reverting to the original behavior for x86-32):
> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index 6b3033845c6d..bf73f25aaa32 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -37,8 +37,12 @@ int pmd_huge(pmd_t pmd)
> */
> int pud_huge(pud_t pud)
> {
> +#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> return !pud_none(pud) &&
> (pud_val(pud) & (_PAGE_PRESENT|_PAGE_PSE)) != _PAGE_PRESENT;
> +#else
> + return !!(pud_val(pud) & _PAGE_PSE); // or "return 0;" ?
> +#endif
> }
>
> #ifdef CONFIG_HUGETLB_PAGE
>
>
> Let me guess what the PUD entry was there when triggering the issue.
> Assuming that the original code (before 3a194f3f8ad0) was correct, the PSE
> bit in pud_val(pud) should be always cleared. So, when pud_huge() returns
> true since 3a194f3f8ad0, the PRESENT bit should be clear and some other
> bits (rather than PRESENT and PSE) are set so that pud_none() is false.
> I'm not sure what such a non-present PUD entry does mean.
pud_val()==0 when it blows up, and pud_none() is false because
pgtable-nopmd.h says so with 2 level paging.
And given that I just tested with PAE / 3 level paging,
and sure enough it no longer blows up.
So looks to me like maybe this new code just doesn't understand
how the levels get folded.
I might also be missing something obvious, but why is it even
necessary to treat PRESENT==0+PSE==0 as a huge entry?
--
Ville Syrjälä
Intel
Powered by blists - more mailing lists