Re: [PATCH v7 0/4] Implement IOCTL to get and/or the clear info about PTEs

[Peter Xu <peterx@redhat.com>]

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

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

There're four kinds of masks (required/anyof/excluded/return).  Are they
all needed?  Why this is a good interface design?

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.

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.

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?

Thanks,

-- 
Peter Xu