Date:   Tue, 28 Feb 2023 14:42:45 -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/5ZNTESKfntBSF3@x1n>
References: <Y+5uIS5E9sTLi41T@x1n>
 <456f8e2e-9554-73a3-4fdb-be21f9cc54b6@redhat.com>
 <Y+6NKPuty9V3nycI@x1n>
 <4dbc9913-3483-d22d-bbd2-e4f510fff56d@redhat.com>
 <Y/AIEvHJoMUnh4b6@x1n>
 <c463c421-2f33-9ae3-7d41-b394d1737d42@redhat.com>
 <Y/VQMyssclyIGa5i@x1n>
 <91d7c512-ee57-7d71-34b7-90e45f5c109b@redhat.com>
 <Y/Z9ACW8l2E3kOVk@x1n>
 <4b3c2f37-3b84-3147-7513-4293e5408fdd@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4b3c2f37-3b84-3147-7513-4293e5408fdd@redhat.com>
Precedence: bulk

On Thu, Feb 23, 2023 at 03:35:13PM +0100, David Hildenbrand wrote:
> I had some idea of using two markers: PTE_UFFD_WP and PT_UFFD_NO_WP, and
> being pte_none() being something fuzzy in between that the application knows
> how to deal with ("not touched since we registered uffd-wp").
> 
> The goal would be to not populate page tables just to insert PTE
> markers/zeropages, but to only special case on the "there is a page table
> with a present PTE and we're unmapping something with uffd-wp set or uffd-wp
> not set". Because when we're unmapping we already have a page table entry we
> can just set instead of allocating a page table.
> 
> Sorry for throwing semi-finished ideas at you, but the basic idea would be
> to only special case when we're unmapping something: there, we already do
> have a page mapped and can remember uffd-wp-set (clean) vs. !uffd-wp-set
> (dirty).
> 
> 
> uffd-wp protecting a range:
> * !pte_none() -> set uffd-wp bit and wrprotect
> * pte_none() -> nothing to do
> * PTE_UFFD_WP -> nothing to do
> * PTE_UFFD_NO_WP -> set PTE_UFFD_WP
> 
> unmapping a page (old way: !pte_none() -> pte_none()):
> * uffd-wp bit set: set PTE_UFFD_WP
> * uffd-wp bit not set: set PTE_UFFD_NO_WP
> 
> (re)mapping a page (old: pte_none() -> !pte_none()):
> * PTE_UFFD_WP -> set pte bit for new PTE
> * PTE_UFFD_NO_WP -> don't set pte bit for new PTE
> * pte_none() -> set pte bit for new PTE
> 
> Zapping an anon page using MADV_DONTNEED is a bit confusing. It's actually
> similar to a memory write (-> write zeroes), but we don't notify uffd-wp for
> that (I think that's something you comment on below). Theoretically, we'd
> want to set PTE_UFFD_NO_WP ("dirty") in the async mode. But that might need
> more thought of what the expected semantics actually are.
> 
> When we walk over the page tables we would get the following information
> after protecting the range:
> 
> * PTE_UFFD_WP -> clean, not modified since last protection round
> * PTE_UFFD_NO_WP -> dirty, modified since last protection round
> * pte_none() -> not mapped and therefore not modified since beginning of
>                 protection.
> * !pte_none() -> uffd-wp bit decides

I can't say I thought a lot but I feel like it may work. I'd probably avoid
calling it PTE_UFFD_NO_WP or it'll be confusing.. maybe WP_WRITTEN or
WP_RESOLVED instead.

But that interface looks weird in that the protection happens right after
VM_UFFD_WP applied to VMA and that keeps true until unregister.  One needs
to reprotect using ioctl(UFFDIO_WRITEPROTECT) OTOH after the 1st round of
tracking.  It just looks a little bit over-complicated, not to mention we
will need two markers only for userfault-wp.  I had a feeling this
complexity can cause us some trouble elsewhere.

IIUC this can be something done on top even if it'll work (I think the
userspace API doesn't need to change at all), so I'd suggest giving it some
more thoughts and we start with simple and working.

In general, I'll be happy with anything simpler if Muhammad is happy with
its current performance..  For myself, WP_UNPOPULATED is definitely much
better than the old workaround in QEMU live snapshots, so I never worry that.

> Yes, I focused on anon. Let's see if any of the above I said makes sense. :)
> 
> Anyhow, what we're discussing here is yet another uffd-wp addition, if ever,
> so don't feel blocked by my comments.

Thanks.  I've just posted a new version.

-- 
Peter Xu