Date:   Wed, 15 Feb 2023 16:12:17 -0500
From:   Peter Xu <peterx@redhat.com>
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= <emmir@google.com>,
        Andrei Vagin <avagin@gmail.com>,
        Danylo Mocherniuk <mdanylo@google.com>,
        Paul Gofman <pgofman@codeweavers.com>,
        Cyrill Gorcunov <gorcunov@gmail.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Shuah Khan <shuah@kernel.org>,
        Christian Brauner <brauner@kernel.org>,
        Yang Shi <shy828301@gmail.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Liam R . Howlett" <Liam.Howlett@oracle.com>,
        Yun Zhou <yun.zhou@windriver.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Alex Sierra <alex.sierra@amd.com>,
        Matthew Wilcox <willy@infradead.org>,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Mike Rapoport <rppt@kernel.org>, Nadav Amit <namit@vmware.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        "Gustavo A . R . Silva" <gustavoars@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
        Greg KH <gregkh@linuxfoundation.org>, kernel@collabora.com
Subject: Re: [PATCH v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or
 the clear info about PTEs
Message-ID: <Y+1KsQSYPYO/egvt@x1n>
References: <20230202112915.867409-1-usama.anjum@collabora.com>
 <20230202112915.867409-4-usama.anjum@collabora.com>
 <Y+QfDN4Y5Q10x8GQ@x1n>
 <8b2959fb-2a74-0a1f-8833-0b18eab142dc@collabora.com>
 <Y+qur8iIUQTLyE8f@x1n>
 <39217d9a-ed7e-f1ff-59b9-4cbffa464999@collabora.com>
 <Y+v2HJ8+3i/KzDBu@x1n>
 <884f5aa6-5d12-eecc-ed71-7d653828ca20@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <884f5aa6-5d12-eecc-ed71-7d653828ca20@collabora.com>
Precedence: bulk

On Wed, Feb 15, 2023 at 03:03:09PM +0500, Muhammad Usama Anjum wrote:
> On 2/15/23 1:59 AM, Peter Xu wrote:
> [..]
> >>>> static inline bool is_pte_written(pte_t pte)
> >>>> {
> >>>> 	if ((pte_present(pte) && pte_uffd_wp(pte)) ||
> >>>> 	    (pte_swp_uffd_wp_any(pte)))
> >>>> 		return false;
> >>>> 	return (pte_present(pte) || is_swap_pte(pte));
> >>>> }
> >>>
> >>> Could you explain why you don't want to return dirty for !present?  A page
> >>> can be written then swapped out.  Don't you want to know that happened
> >>> (from dirty tracking POV)?
> >>>
> >>> The code looks weird to me too..  We only have three types of ptes: (1)
> >>> present, (2) swap, (3) none.
> >>>
> >>> Then, "(pte_present() || is_swap_pte())" is the same as !pte_none().  Is
> >>> that what you're really looking for?
> >> Yes, this is what I've been trying to do. I'll use !pte_none() to make it
> >> simpler.
> > 
> > Ah I think I see what you wanted to do now.. But I'm afraid it won't work
> > for all cases.
> > 
> > So IIUC the problem is anon pte can be empty, but since uffd-wp bit doesn't
> > persist on anon (but none) ptes, then we got it lost and we cannot identify
> > it from pages being written.  Your solution will solve problem for
> > anonymous, but I think it'll break file memories.
> > 
> > Example:
> > 
> > Consider one shmem page that got mapped, write protected (using UFFDIO_WP
> > ioctl), written again (removing uffd-wp bit automatically), then zapped.
> > The pte will be pte_none() but it's actually written, afaiu.
> > 
> > Maybe it's time we should introduce UFFD_FEATURE_WP_ZEROPAGE, so we'll need
> > to install pte markers for anonymous too (then it will work similarly like
> > shmem/hugetlbfs, that we'll report writting to zero pages), then you'll
> > need to have the new UFFD_FEATURE_WP_ASYNC depend on it.  With that I think
> > you can keep using the old check and it should start to work.
> > 
> > Please let me know if my understanding is correct above.
> Thank you for identifying it. Your understanding seems on point. I'll have
> research things up about PTE Markers. I'm looking at your patches about it
> [1]. Can you refer me to "mm alignment sessions" discussion in form of
> presentation or if any transcript is available?

No worry now, after a second thought I think zero page is better than pte
markers, and I've got a patch that works for it here by injecting zero
pages for anonymous:

https://lore.kernel.org/all/20230215210257.224243-1-peterx@redhat.com/

I think we'd also better to enforce your new WP_ASYNC feature bit to depend
on this one, so fail the UFFDIO_API if WP_ASYNC && !WP_ZEROPAGE.

Could you please try by rebasing your work upon this one?  Hope it'll work
for you already.  Note again that you'll need to go back to the old
is_pte|pmd_written() to make things work always, I think.

[...]

> I truly understand how you feel about export_prev_to_out(). It is really
> difficult to understand. Even I had to made a hard try to come up with the
> current code to avoid consuming a lot of kernel's memory while giving user
> the compact output. I can surely map both of these with a dirty looking
> macro. But I'm unable to find a decent macro to replace these. I think I'll
> put a comment some where to explain whats going-on.

So maybe I still missed something? I'll read the new version when it comes.

Thanks,

-- 
Peter Xu