On Fri, May 28, 2021 at 11:26 AM John Hubbard <jhubbard@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
> >
>
