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: <20210528175403.4506-1-shy828301@gmail.com> <ffc5612f-9403-9bc9-316f-fa50199769b2@nvidia.com>
In-Reply-To: <ffc5612f-9403-9bc9-316f-fa50199769b2@nvidia.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Fri, 28 May 2021 12:03:53 -0700
Message-ID: <CAHbLzkpMFq4955wOq9bANxtonm3cM+yFPh_gfr8nJmnXu9_fgg@mail.gmail.com>
Subject: Re: [RFC PATCH] mm: dump_page: print total mapcount for compound page
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     Hugh Dickins <hughd@google.com>,
        Matthew Wilcox <willy@infradead.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        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 Fri, May 28, 2021 at 11:26 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 5/28/21 10:54 AM, Yang Shi wrote:
> > The total mapcount is a useful information for debugging, but we can't
> > call total_mapcount() directly since it calls some assertions which may
> > be triggered as commit 6dc5ea16c86f ("mm,
> > dump_page: do not crash with bad compound_mapcount()") met.
> >
> > We could implement yet another implementation for dump_page() but
> > it has the limitation when individual mapcount of subpages is corrupted.
> >
> > Actually the total mapcount could be decoded from refcount, pincount and
> > compound mapcount although it may be not very precise due to some
> > transient references.
>
> If the mapcount calculation were in a separate routine, *and* if something
> else in addition to dump_page() used it, then I'd be interested in
> calling it from dump_page().

There is. The total_mapcount() is used by mm code. But as I mentioned
in the commit log and that discussion email, it is not safe to call it
directly in dump_page() path.

>
> But, just adding a calculation glob like this is not a good idea. If
> the reader really needs the calculation, then that person can, as you
> say, work it out from the other information.
>
> Debug and dump routines are actually supposed to remain fairly simple,
> so that they themselves do not end up with bugs, or stale assumptions
> (which this calculation is very much susceptible to). This goes in the
> wrong direction.
>
> So best to just not do this, IMHO.
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> >
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > ---
> > I think we are on the same page that the total mapcount is useful
> > information and it would be ideal to print this information when dumpping
> > page if possible.  But how to implement it safely seems controversial.
> > Some ideas and potential problems have been discussed by
> > https://lore.kernel.org/linux-mm/alpine.LSU.2.11.2105261733110.16920@eggly.anvils/.
> >
> > So I prepared this patch to show a possible approach to get some
> > feedback.  The same thing could be decoded by the reader of page dump
> > as well by using the same formula used by this patch.  However it sounds
> > more convenient to have kernel do the math.
> >
> >   mm/debug.c | 35 +++++++++++++++++++++++++++++------
> >   1 file changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/debug.c b/mm/debug.c
> > index e73fe0a8ec3d..129efcfcaf79 100644
> > --- a/mm/debug.c
> > +++ b/mm/debug.c
> > @@ -54,8 +54,13 @@ static void __dump_page(struct page *page)
> >        * inaccuracy here due to racing.
> >        */
> >       bool page_cma = is_migrate_cma_page(page);
> > -     int mapcount;
> > +     int mapcount, total_mapcount;
> > +     int nr;
> > +     int refcount;
> > +     int pincount = 0;
> > +     int comp_mapcnt;
> >       char *type = "";
> > +     bool is_slab = PageSlab(head);
> >
> >       if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
> >               /*
> > @@ -82,22 +87,40 @@ static void __dump_page(struct page *page)
> >        * page->_mapcount space in struct page is used by sl[aou]b pages to
> >        * encode own info.
> >        */
> > -     mapcount = PageSlab(head) ? 0 : page_mapcount(page);
> > +     mapcount = is_slab ? 0 : page_mapcount(page);
> > +
> > +     refcount = page_ref_count(head);
> >
> >       pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
> > -                     page, page_ref_count(head), mapcount, mapping,
> > +                     page, refcount, mapcount, mapping,
> >                       page_to_pgoff(page), page_to_pfn(page));
> >       if (compound) {
> > +             comp_mapcnt = head_compound_mapcount(head);
> >               if (hpage_pincount_available(page)) {
> > +                     pincount = head_compound_pincount(head);
> >                       pr_warn("head:%p order:%u compound_mapcount:%d compound_pincount:%d\n",
> >                                       head, compound_order(head),
> > -                                     head_compound_mapcount(head),
> > -                                     head_compound_pincount(head));
> > +                                     comp_mapcnt, pincount);
> >               } else {
> >                       pr_warn("head:%p order:%u compound_mapcount:%d\n",
> >                                       head, compound_order(head),
> > -                                     head_compound_mapcount(head));
> > +                                     comp_mapcnt);
> > +             }
> > +
> > +             nr = compound_nr(head);
> > +             if (is_slab)
> > +                     total_mapcount = 0;
> > +             else if (PageHuge(head))
> > +                     total_mapcount = comp_mapcnt;
> > +             else {
> > +                     if (mapping) {
> > +                             if (!PageAnon(head))
> > +                                     nr = nr * (comp_mapcnt + 1) - comp_mapcnt;
> > +                     } else
> > +                             nr = 0;
> > +                     total_mapcount = refcount - pincount - nr;
> >               }
> > +             pr_warn("total_mapcount(estimated):%d\n", total_mapcount);
> >       }
> >
> >   #ifdef CONFIG_MEMCG
> >
>