Re: [PATCH v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs

[Peter Xu <peterx@redhat.com>]

On Mon, Feb 13, 2023 at 05:55:19PM +0500, Muhammad Usama Anjum wrote:
> On 2/9/23 3:15 AM, Peter Xu wrote:
> > On Thu, Feb 02, 2023 at 04:29:12PM +0500, Muhammad Usama Anjum wrote:
> >> This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear
> >> the info about page table entries. The following operations are supported
> >> in this ioctl:
> >> - Get the information if the pages have been written-to (PAGE_IS_WRITTEN),
> >>   file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped
> >>   (PAGE_IS_SWAPPED).
> >> - Write-protect the pages (PAGEMAP_WP_ENGAGE) to start finding which
> >>   pages have been written-to.
> >> - Find pages which have been written-to and write protect the pages
> >>   (atomic PAGE_IS_WRITTEN + PAGEMAP_WP_ENGAGE)
> >>
> >> To get information about which pages have been written-to and/or write
> >> protect the pages, following must be performed first in order:
> >> - The userfaultfd file descriptor is created with userfaultfd syscall.
> >> - The UFFD_FEATURE_WP_ASYNC feature is set by UFFDIO_API IOCTL.
> >> - The memory range is registered with UFFDIO_REGISTER_MODE_WP mode
> >>   through UFFDIO_REGISTER IOCTL.
> >> Then the any part of the registered memory or the whole memory region
> >> can be write protected using the UFFDIO_WRITEPROTECT IOCTL or
> >> PAGEMAP_SCAN IOCTL.
> >>
> >> struct pagemap_scan_args is used as the argument of the IOCTL. In this
> >> struct:
> >> - The range is specified through start and len.
> >> - The output buffer of struct page_region array and size is specified as
> >>   vec and vec_len.
> >> - The optional maximum requested pages are specified in the max_pages.
> >> - The flags can be specified in the flags field. The PAGEMAP_WP_ENGAGE
> >>   is the only added flag at this time.
> >> - The masks are specified in required_mask, anyof_mask, excluded_ mask
> >>   and return_mask.
> >>
> >> This IOCTL can be extended to get information about more PTE bits. This
> >> IOCTL doesn't support hugetlbs at the moment. No information about
> >> hugetlb can be obtained. This patch has evolved from a basic patch from
> >> Gabriel Krisman Bertazi.
> >>
> >> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> >> ---
> >> Changes in v10:
> >> - move changes in tools/include/uapi/linux/fs.h to separate patch
> >> - update commit message
> >>
> >> Change in v8:
> >> - Correct is_pte_uffd_wp()
> >> - Improve readability and error checks
> >> - Remove some un-needed code
> >>
> >> Changes in v7:
> >> - Rebase on top of latest next
> >> - Fix some corner cases
> >> - Base soft-dirty on the uffd wp async
> >> - Update the terminologies
> >> - Optimize the memory usage inside the ioctl
> >>
> >> Changes in v6:
> >> - Rename variables and update comments
> >> - Make IOCTL independent of soft_dirty config
> >> - Change masks and bitmap type to _u64
> >> - Improve code quality
> >>
> >> Changes in v5:
> >> - Remove tlb flushing even for clear operation
> >>
> >> Changes in v4:
> >> - Update the interface and implementation
> >>
> >> Changes in v3:
> >> - Tighten the user-kernel interface by using explicit types and add more
> >>   error checking
> >>
> >> Changes in v2:
> >> - Convert the interface from syscall to ioctl
> >> - Remove pidfd support as it doesn't make sense in ioctl
> >> ---
> >>  fs/proc/task_mmu.c      | 290 ++++++++++++++++++++++++++++++++++++++++
> >>  include/uapi/linux/fs.h |  50 +++++++
> >>  2 files changed, 340 insertions(+)
> >>
> >> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> >> index e35a0398db63..c6bde19d63d9 100644
> >> --- a/fs/proc/task_mmu.c
> >> +++ b/fs/proc/task_mmu.c
> >> @@ -19,6 +19,7 @@
> >>  #include <linux/shmem_fs.h>
> >>  #include <linux/uaccess.h>
> >>  #include <linux/pkeys.h>
> >> +#include <linux/minmax.h>
> >>  
> >>  #include <asm/elf.h>
> >>  #include <asm/tlb.h>
> >> @@ -1135,6 +1136,22 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
> >>  }
> >>  #endif
> >>  
> >> +static inline bool is_pte_uffd_wp(pte_t pte)
> >> +{
> >> +	if ((pte_present(pte) && pte_uffd_wp(pte)) ||
> >> +	    (pte_swp_uffd_wp_any(pte)))
> >> +		return true;
> >> +	return false;
> > 
> > Sorry I should have mentioned this earlier: you can directly return here.
> No problem at all. I'm replacing these two helper functions with following
> in next version so that !present pages don't show as dirty:
> 
> 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?

> 
> static inline bool is_pmd_written(pmd_t pmd)
> {
> 	if ((pmd_present(pmd) && pmd_uffd_wp(pmd)) ||
> 	    (is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd)))
> 		return false;
> 	return (pmd_present(pmd) || is_swap_pmd(pmd));
> }

[...]

> >> +	bitmap = cur & p->return_mask;
> >> +	if (cpy && bitmap) {
> >> +		if ((prev->len) && (prev->bitmap == bitmap) &&
> >> +		    (prev->start + prev->len * PAGE_SIZE == addr)) {
> >> +			prev->len += len;
> >> +			p->found_pages += len;
> >> +		} else if (p->vec_index < p->vec_len) {
> >> +			if (prev->len) {
> >> +				memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region));
> >> +				p->vec_index++;
> >> +			}
> > 
> > IIUC you can have:
> > 
> >   int pagemap_scan_deposit(p)
> >   {
> >         if (p->vec_index >= p->vec_len)
> >                 return -ENOSPC;
> > 
> >         if (p->prev->len) {
> >                 memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region));
> >                 p->vec_index++;
> >         }
> > 
> >         return 0;
> >   }
> > 
> > Then call it here.  I think it can also be called below to replace
> > export_prev_to_out().
> No this isn't possible. We fill up prev until the next range doesn't merge
> with it. At that point, we put prev into the output buffer and new range is
> put into prev. Now that we have shifted to smaller page walks of <= 512
> entries. We want to visit all ranges before finally putting the prev to
> output. Sorry to have this some what complex method. The problem is that we
> want to merge the consective matching regions into one entry in the output.
> So to achieve this among multiple different page walks, the prev is being used.
> 
> Lets suppose we want to visit memory from 0x7FFF00000000 to 7FFF00400000
> having length of 1024 pages and all of the memory has been written.
> walk_page_range() will be called 2 times. In the first call, prev will be
> set having length of 512. In second call, prev will be updated to 1024 as
> the previous range stored in prev could be extended. After this, the prev
> will be stored to the user output buffer consuming only 1 struct of page_range.
> 
> If we store prev back to output memory in every walk_page_range() call, we
> wouldn't get 1 struct of page_range with length 1024. Instead we would get
> 2 elements of page_range structs with half the length.

I didn't mean to merge PREV for each pgtable walk.  What I meant is I think
with such a pagemap_scan_deposit() you can rewrite it as:

if (cpy && bitmap) {
        if ((prev->len) && (prev->bitmap == bitmap) &&
            (prev->start + prev->len * PAGE_SIZE == addr)) {
                prev->len += len;
                p->found_pages += len;
        } else {
                if (pagemap_scan_deposit(p))
                        return -ENOSPC;
                prev->start = addr;
                prev->len = len;
                prev->bitmap = bitmap;
                p->found_pages += len;
        }
}

Then you can reuse pagemap_scan_deposit() when before returning to
userspace, just to flush PREV to p->vec properly in a single helper.
It also makes the code slightly easier to read.

-- 
Peter Xu