Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210525162145.3510-1-shy828301@gmail.com> <20210525162145.3510-2-shy828301@gmail.com>
 <alpine.LSU.2.11.2105251412140.2003@eggly.anvils> <CAHbLzkqs32HkRiAoQeSv2ik9fBYmdhwUFq5vWkcs7PY0rUu2fA@mail.gmail.com>
 <alpine.LSU.2.11.2105251643320.2804@eggly.anvils> <CAHbLzkrmtw-GFEW2zmik6y0R8JjpB4T1hnfYf9O8kz5Zqrr4RQ@mail.gmail.com>
 <alpine.LSU.2.11.2105261523250.16407@eggly.anvils>
In-Reply-To: <alpine.LSU.2.11.2105261523250.16407@eggly.anvils>
From:   Yang Shi <shy828301@gmail.com>
Date:   Wed, 26 May 2021 16:04:35 -0700
Message-ID: <CAHbLzkoDW+f0PKprtyY=ipoi9F-1C0z5Bt80k2h7ppPvNhCc5A@mail.gmail.com>
Subject: Re: [v3 PATCH 2/2] mm: thp: check page_mapped instead of
 page_mapcount for split
To:     Hugh Dickins <hughd@google.com>
Cc:     Zi Yan <ziy@nvidia.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        =?UTF-8?B?SE9SSUdVQ0hJIE5BT1lBKOWggOWPoyDnm7TkuZ8p?= 
        <naoya.horiguchi@nec.com>, Wang Yugui <wangyugui@e16-tech.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, May 26, 2021 at 3:48 PM Hugh Dickins <hughd@google.com> wrote:
>
> On Wed, 26 May 2021, Yang Shi wrote:
> > On Tue, May 25, 2021 at 4:58 PM Hugh Dickins <hughd@google.com> wrote:
> > > On Tue, 25 May 2021, Yang Shi wrote:
> > > >
> > > > We should be able to make dump_page() print total mapcount, right? The
> > > > dump_page() should be just called in some error paths so taking some
> > > > extra overhead to dump more information seems harmless, or am I
> > > > missing something? Of course, this can be done in a separate patch.
> > >
> > > I didn't want to ask that of you, but yes, if you're willing to add
> > > total_mapcount() into dump_page(), I think that would be ideal; and
> > > could be helpful for other cases too.
> > >
> > > Looking through total_mapcount(), I think it's safe to call from
> > > dump_page() - I always worry about extending crash info with
> > > something that depends on a maybe-corrupted pointer which would
> > > generate a further crash and either recurse or truncate the output -
> > > but please check that carefully.
> >
> > Yes, it is possible. If the THP is being split, some VM_BUG_* might be
> > triggered if total_mapcount() is called. But it is still feasible to
> > print total mapcount as long as we implement a more robust version for
> > dump_page().
>
> Oh dear. I think the very last thing the kernel needs is yet another
> subtly different variant of *mapcount*().
>
> Do you have a specific VM_BUG_* in mind there?  Of course there's
> the VM_BUG_ON_PAGE(PageTail) at the start of it, and you'd want to
> print total_mapcount(head) to avoid that one.

There are two more places in total_mapcount() other than the tail page
assertion.

#1. compound_mapcount() has !PageCompound assertion. The similar
problem has been met before, please refer to commit 6dc5ea16c86f ("mm,
dump_page: do not crash with bad compound_mapcount()").
#2. PageDoubleMap has !PageHead assertion.

>
> Looks like __dump_page() is already careful about "head", checking
> whether "page" is within the expected bounds.  Of course, once we're
> in serious VM_WARN territory, there might be races which could flip
> fields midway: PageTail set by the time it reaches total_mapcount()?

It seems possible, at least theoretically.

> Narrow the race (rather like it does with PageSlab) by testing
> PageTail immediately before calling total_mapcount(head)?

TBH I don't think of a simple testing to narrow all the races. We have
to add multiple testing in total_mapcount(), it seems too hacky.
Another variant like below might be neater?

+static inline int __total_mapcount(struct page *head)
+{
+       int i, compound, nr, ret;
+
+       compound = head_compound_mapcount(head);
+       nr = compound_nr(head);
+       if (PageHuge(head))
+               return compound;
+       ret = compound;
+       for (i = 0; i < nr; i++)
+               ret += atomic_read(&head[i]._mapcount) + 1;
+       /* File pages has compound_mapcount included in _mapcount */
+       if (!PageAnon(head))
+               return ret - compound * nr;
+       if (head[1].flags & PG_double_map)
+               ret -= nr;
+       return ret;
+}

>
> Hugh