Message-ID: <0eb79bb3-7384-11c6-a380-c027f09305f2@collabora.com>
Date:   Mon, 23 Jan 2023 18:15:00 +0500
From:   Muhammad Usama Anjum <usama.anjum@...labora.com>
To:     Peter Xu <peterx@...hat.com>, Andrei Vagin <avagin@...il.com>,
        Danylo Mocherniuk <mdanylo@...gle.com>
Cc:     Muhammad Usama Anjum <usama.anjum@...labora.com>,
        David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michał Mirosław 
        <emmir@...gle.com>, Paul Gofman <pgofman@...eweavers.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Shuah Khan <shuah@...nel.org>,
        Christian Brauner <brauner@...nel.org>,
        Yang Shi <shy828301@...il.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Yun Zhou <yun.zhou@...driver.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Alex Sierra <alex.sierra@....com>,
        Matthew Wilcox <willy@...radead.org>,
        Pasha Tatashin <pasha.tatashin@...een.com>,
        Mike Rapoport <rppt@...nel.org>, Nadav Amit <namit@...are.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kselftest@...r.kernel.org,
        Greg KH <gregkh@...uxfoundation.org>, kernel@...labora.com
Subject: Re: [PATCH v7 0/4] Implement IOCTL to get and/or the clear info about
 PTEs

On 1/19/23 3:12 AM, Peter Xu wrote:
> On Mon, Jan 09, 2023 at 11:45:15AM +0500, Muhammad Usama Anjum wrote:
>> *Changes in v7:*
>> - Add uffd wp async
>> - Update the IOCTL to use uffd under the hood instead of soft-dirty
>>   flags
>>
>> Stop using the soft-dirty flags for finding which pages have been
>> written to. It is too delicate and wrong as it shows more soft-dirty
>> pages than the actual soft-dirty pages. There is no interest in
>> correcting it [A][B] as this is how the feature was written years ago.
>> It shouldn't be updated to changed behaviour. Peter Xu has suggested
>> using the async version of the UFFD WP [C] as it is based inherently
>> on the PTEs.
>>
>> So in this patch series, I've added a new mode to the UFFD which is
>> asynchronous version of the write protect. When this variant of the
>> UFFD WP is used, the page faults are resolved automatically by the
>> kernel. The pages which have been written-to can be found by reading
>> pagemap file (!PM_UFFD_WP). This feature can be used successfully to
>> find which pages have been written to from the time the pages were
>> write protected. This works just like the soft-dirty flag without
>> showing any extra pages which aren't soft-dirty in reality.
>>
>> [A] https://lore.kernel.org/all/20221220162606.1595355-1-usama.anjum@collabora.com
>> [B] https://lore.kernel.org/all/20221122115007.2787017-1-usama.anjum@collabora.com
>> [C] https://lore.kernel.org/all/Y6Hc2d+7eTKs7AiH@x1n
>>
>> *Changes in v6:*
>> - Updated the interface and made cosmetic changes
>>
>> *Cover Letter in v5:*
>> Hello,
> 
> Please consider either drop the cover letter below this point or rephrase,
> otherwise many of them are not true anymore and it can confuse the
> reviewers.
I'll remove.

> 
> I have a few high level comments/questions here, please bare with me if any
> of them are already discussed by others in the old versions; I'd be happy
> to read them when there's a pointer to the relevant answers.
> 
> Firstly, doc update is more than welcomed to explain the new interface
> first (before throwing the code..).  That can be done in pagemap.rst on
> pagemap changes, or userfaultfd.rst on userfaultfd.
Okay. I'll add the documentation in next version or after the series has
been accepted. Initially I'd added the documentation. But the code kept on
changing so much that I had to spend considerable time on updating the
documentation. I know it is better to add documentation with the patches.
I'll try to add it.

> 
> Besides, can you provide more justification on the new pagemap-side
> interface design?
> 
> It seems it came from the Windows API GetWriteWatch(), but it's definitely
> not exactly that.  Let me spell some points out..
Initially, we just wanted a way to emulate Windows API GetWriteWatch(). So
we had added `max_pages` in the IOCTL arguments which is optional and can
be used to specify how many pages we want to find of our interest. There
was only one set of flags to be matched with the pages.

> 
> There're four kinds of masks (required/anyof/excluded/return).  Are they
> all needed?  Why this is a good interface design?
Then, CRIU developers Andrea [1] and Danylo [2], asked to include all these
different kinds of masks. I'd thought of these masks as fancy filter inside
the kernel. But there wasn't anyone else to review. So I'd included them to
move forward. Please let me know your thoughts after reading emails from [1].

> 
> I saw you used page_region structure to keep the information.  I think you
> wanted to have a densed output, especially if counting in the "return mask"
> above it starts to make more sense. If with a very limited return mask it
> means many of the (continuous) page information can be merged into a single
> page_region struct when the kernel is scanning.
Correct.

> 
> However, at the meantime the other three masks (required/anyof/excluded)
> made me quite confused - it means you wanted to somehow filter the pages
> and only some of them will get collected.  The thing is for a continuous
> page range if any of the page got skipped due to the masks (e.g. not in
> "required" or in "excluded") it also means it can never be merged into
> previous page_region either.  That seems to be against the principle of
> having densed output.
The filtering is being done. But the output can still be condensed
regardless. There isn't that randomness in the page flags of the
consecutive pages.

> 
> I hope you can help clarify what's the major use case here.
> 
> There's also the new interface to do atomic "fetch + update" on wrprotected
> pages.  Is that just for efficiency or is the accuracy required in some of
> the applications?
"Atomic fetch and update/clear" or "Atomic fetch Written-to status and
clear it" is needed to support GetWriteWatch() and there is no already
present way to perform this operation atomically. We want efficiency and
accuracy both to get good performance/speed. So this IOCTL is needed to
achieve:
1) New functionality which isn't already present
2) Most efficient and accurate method to perform the operation (it isn't
possible through soft-dirty feature)

> 
> Thanks,
> 

[1] https://lore.kernel.org/all/YyiDg79flhWoMDZB@gmail.com
[2] https://lore.kernel.org/all/20221014134802.1361436-1-mdanylo@google.com

-- 
BR,
Muhammad Usama Anjum

Powered by blists - more mailing lists