Re: [v2 PATCH] fs/proc: task_mmu.c: don't read mapcount for migration entry

From: Yang Shi <shy828301@gmail.com>
To: David Hildenbrand <david@redhat.com>
Cc: Jann Horn <jannh@google.com>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Matthew Wilcox <willy@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Linux MM <linux-mm@kvack.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	stable <stable@vger.kernel.org>
Subject: Re: [v2 PATCH] fs/proc: task_mmu.c: don't read mapcount for migration entry
Date: Thu, 27 Jan 2022 11:42:47 -0800	[thread overview]
Message-ID: <CAHbLzkongysm41LbMc1FMT8Xeg33==1rn3nUu6MTAAwKSbfv_Q@mail.gmail.com> (raw)
In-Reply-To: <2a1c5bd2-cb8c-b93b-68af-de620438d19a@redhat.com>

On Wed, Jan 26, 2022 at 10:54 AM David Hildenbrand <david@redhat.com> wrote:
>
> >>> Just page lock or elevated page refcount could serialize against THP
> >>> split AFAIK.
> >>>
> >>>>
> >>>> But yeah, using the mapcount of a page that is not even mapped
> >>>> (migration entry) is clearly wrong.
> >>>>
> >>>> To summarize: reading the mapcount on an unlocked page will easily
> >>>> return a wrong result and the result should not be relied upon. reading
> >>>> the mapcount of a migration entry is dangerous and certainly wrong.
> >>>
> >>> Depends on your usecase. Some just want to get a snapshot, just like
> >>> smaps, they don't care.
> >>
> >> Right, but as discussed, even the snapshot might be slightly wrong. That
> >> might be just fine for smaps (and I would have enjoyed a comment in the
> >> code stating that :) ).
> >
> > I think that is documented already, see Documentation/filesystems/proc.rst:
> >
> > Note: reading /proc/PID/maps or /proc/PID/smaps is inherently racy (consistent
> > output can be achieved only in the single read call).
>
> Right, but I think there is a difference between
>
> * Atomic values that change immediately afterwards ("this value used to
>   be true at one point in time")
> * Values that are unstable because we cannot read them atomically ("this
>   value never used to be true")
>
> I'd assume with the documented race we actually talk about the first
> point, but I might be just wrong.

I think so too.

>
> >
> > Of course, if the extra note is preferred in the code, I could try to
> > add some in a separate patch.
>
> When staring at the (original) code I would have hoped to find something
> like:
>
> /*
>  * We use page_mapcount() to get a snapshot of the mapcount. Without
>  * holding the page lock this snapshot can be slightly wrong as we
>  * cannot always read the mapcount atomically. As long we hold the PT
>  * lock, the page cannot get unmapped and it's at safe to call
>  * page_mapcount().
>  */
>
> With the addition of
>
> "... For unmapped pages (e.g., migration entries) we cannot guarantee
> that, so treat the mapcount as being 1."
>
> But this is just my personal preference ... :) I do think the patch does
> the right thing in regard to migration entries.

I will prepare a patch.

>
> --
> Thanks,
>
> David / dhildenb
>