On Wed, 26 May 2021, Yang Shi wrote:

On Wed, May 26, 2021 at 3:48 PM Hugh Dickins [off-list ref] wrote:

quoted

On Wed, 26 May 2021, Yang Shi wrote:

quoted

On Tue, May 25, 2021 at 4:58 PM Hugh Dickins [off-list ref] wrote:

quoted

On Tue, 25 May 2021, Yang Shi wrote:

quoted

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()").

Thanks for the useful reference.

#2. PageDoubleMap has !PageHead assertion.

quoted

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.

quoted

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;
+}

I still don't want any more of those lovely functions.

My current preference is just to drop the idea of trying
to show total_mapcount from __dump_page().

I might end up compromising on printing the result of
	for (i = 0; i < nr; i++)
		ret += atomic_read(&head[i]._mapcount) + 1;
(if we can safely decide nr), and leave all the convoluted
flags logic to the poor reader of the page dump.

But that in itself shows the limitation of printing total_mapcount(),
when individual corrupt _mapcounts might be -1, +2, -3, +4, ...

To some extent (modulo racing references to the THP), a good
total_mapcount can be inferred from the page reference count.
(But that probably describes better the situation when everything
is going correctly: maybe problems tend to come precisely when there
are multiple racing references.)

Dunno. My mind is not on it at the moment. I'm more concerned that
Wang Yugui's crash turns out not to be solved by any of the fixes
I have lined up: we certainly did not promise that it would be,
and it shouldn't stop advancing the fixes we already know, but I do
want to give it a little more thought before resuming on my patches.

Hugh