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;
Message-ID: <2a1c5bd2-cb8c-b93b-68af-de620438d19a@redhat.com>
Date:   Wed, 26 Jan 2022 19:54:53 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Content-Language: en-US
To:     Yang Shi <shy828301@gmail.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>
References: <20220120202805.3369-1-shy828301@gmail.com>
 <af603cbe-4a38-9947-5e6d-9a9328b473fb@redhat.com>
 <CAG48ez1xuZdELb=5ed1i0ruoFu5kAaWsf0LgRXEGhrDAcHz8fw@mail.gmail.com>
 <f7f82234-7599-9e39-1108-f8fbe2c1efc9@redhat.com>
 <CAG48ez17d3p53tSfuDTNCaANyes8RNNU-2i+eFMqkMwuAbRT4Q@mail.gmail.com>
 <5b4e2c29-8f1a-5a68-d243-a30467cc02d4@redhat.com>
 <CAHbLzkqLTkVJk+z8wpa03ponf7k30=Sx6qULwsGsvr5cq5d1aw@mail.gmail.com>
 <5a565d5a-0540-4041-ce63-a8fd5d1bb340@redhat.com>
 <CAHbLzkqXy-W9sD5HFOK_rm_TR8uSP29b+RjKjA5zOZ+0dkqMbQ@mail.gmail.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [v2 PATCH] fs/proc: task_mmu.c: don't read mapcount for migration
 entry
In-Reply-To: <CAHbLzkqXy-W9sD5HFOK_rm_TR8uSP29b+RjKjA5zOZ+0dkqMbQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

>>> 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.

> 
> 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.

-- 
Thanks,

David / dhildenb