Date:   Thu, 16 Feb 2023 11:29:47 -0500
From:   Peter Xu <peterx@redhat.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Axel Rasmussen <axelrasmussen@google.com>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Muhammad Usama Anjum <usama.anjum@collabora.com>
Subject: Re: [PATCH] mm/uffd: UFFD_FEATURE_WP_ZEROPAGE
Message-ID: <Y+5Z+88Z3T2TyxUI@x1n>
References: <20230215210257.224243-1-peterx@redhat.com>
 <7eb2bce9-d0b1-a0e3-8be3-f28d858a61a0@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <7eb2bce9-d0b1-a0e3-8be3-f28d858a61a0@redhat.com>
Precedence: bulk

On Thu, Feb 16, 2023 at 11:47:23AM +0100, David Hildenbrand wrote:
> On 15.02.23 22:02, Peter Xu wrote:
> > This is a new feature that controls how uffd-wp handles zero pages (aka,
> > empty ptes), majorly for anonymous pages only.
> > 
> > Note, here we used "zeropage" as a replacement of "empty pte" just to avoid
> > introducing the pte idea into uapi, since "zero page" is more well known to
> > an user app developer.
> > 
> > File memories handles none ptes consistently by allowing wr-protecting of
> > none ptes because of the unawareness of page cache being exist or not.  For
> > anonymous it was not as persistent because we used to assume that we don't
> > need protections on none ptes or known zero pages.
> > 
> > But it's actually not true.
> > 
> > One use case was VM live snapshot, where if without wr-protecting empty
> > ptes the snapshot can contain random rubbish in the holes of the anonymous
> > memory, which can cause misbehave of the guest when the guest assumes the
> > pages should (and were) all zeros.
> > 
> > QEMU worked it around by pre-populate the section with reads to fill in
> > zero page entries before starting the whole snapshot process [1].
> > 
> > Recently there's another need that raised on using userfaultfd wr-protect
> > for detecting dirty pages (to replace soft-dirty) [2].  In that case if
> > without being able to wr-protect zero pages by default, the dirty info can
> > get lost as long as a zero page is written, even after the tracking was
> > started.
> > 
> > In general, we want to be able to wr-protect empty ptes too even for
> > anonymous.
> > 
> > This patch implements UFFD_FEATURE_WP_ZEROPAGE so that it'll make uffd-wp
> > handling on zeropage being consistent no matter what the memory type is
> > underneath.  It doesn't have any impact on file memories so far because we
> > already have pte markers taking care of that.  So it only affects
> > anonymous.
> > 
> > One way to implement this is to also install pte markers for anonymous
> > memories.  However here we can actually do better (than i.e. shmem) because
> > we know there's no page that is backing the pte, so the better solution is
> > to directly install a zeropage read-only pte, so that if there'll be a
> > upcoming read it'll not trigger a fault at all.  It will also reduce the
> > changeset to implement this feature too.
> > 
> 
> There are various reasons why I think a UFFD_FEATURE_WP_UNPOPULATED, using
> PTE markers, would be more benficial:
> 
> 1) It would be applicable to anon hugetlb

Anon hugetlb should already work with non ptes with the markers?

> 2) It would be applicable even when the zeropage is disallowed
>    (mm_forbids_zeropage())

Do you mean s390 can disable zeropage with mm_uses_skeys()?  So far uffd-wp
doesn't support s390 yet, I'm not sure whether we over worried on this
effect.

Or is there any other projects / ideas that potentially can enlarge forbid
zero pages to more contexts?

> 3) It would be possible to optimize even without the huge zeropage, by
>    using a PMD marker.

This patch doesn't need huge zeropage being exist.

> 4) It would be possible to optimize even on the PUD level using a PMD
>    marker.

I think 3+4 is in general an interesting idea on using pte markers on
higher than pte levels, but that needs more changes.

Firstly, keep using pte markers is somehow preallocating the pgtables, so a
side effect of it could be speeding up future faults because they'll all
split into pmd locks and read doesn't need to fault at all, only writes.

Imagine when you hit a page fault on a pmd marker, it means you'll need to
spread that "marker" information to child ptes and you must - it moves the
slow operation of WP into future page faults in some way.  In some cases
(I'd say, most cases..) that's not wanted.  The same to PUDs.

> 
> Especially when uffd-wp'ing large ranges that are possibly all unpopulated
> (thinking about the existing VM background snapshot use case either with
> untouched memory or with things like free page reporting), we might neither
> be reading or writing that memory any time soon.

Right, I think that's a trade-off. But I still think large portion of
totally unpopulated memory should be rare case rather than majority, or am
I wrong?  Not to mention that requires a more involved changeset to the
kernel.

So what I proposed here is the (AFAIU) simplest solution towards providing
such a feature in a complete form.  I think we have chance to implement it
in other ways like pte markers, but that's something we can work upon, and
so far I'm not sure how much benefit we can get out of it yet.

Thanks,

-- 
Peter Xu