Message-ID: <Y+v2HJ8+3i/KzDBu@x1n>
Date:   Tue, 14 Feb 2023 15:59:08 -0500
From:   Peter Xu <peterx@...hat.com>
To:     Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc:     David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michał Mirosław <emmir@...gle.com>,
        Andrei Vagin <avagin@...il.com>,
        Danylo Mocherniuk <mdanylo@...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 v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or
 the clear info about PTEs

On Tue, Feb 14, 2023 at 12:57:21PM +0500, Muhammad Usama Anjum wrote:
> On 2/14/23 2:42 AM, Peter Xu wrote:
> > 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@...labora.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?
> 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.

I'll see whether I can quickly play with UFFD_FEATURE_WP_ZEROPAGE with some
patch at the meantime.  That's something we wanted before too, when the app
cares about zero pages on anon.  We used to populate the pages before doing
ioctl(UFFDIO_WP) to make sure zero pages will be repoted too, but that flag
should be more efficient.

> 
> > 
> >>
> >> 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.
> Yeah, this would have worked as you have described. But in
> pagemap_scan_output(), we are flushing prev to p->vec. But later in
> export_prev_to_out() we need to flush prev to user_memory directly.

I think there's a loop to copy_to_user().  Could you use the new helper so
the copy_to_user() loop will work without export_prev_to_out()?

I really hope we can get rid of export_prev_to_out().  Thanks,

-- 
Peter Xu

Powered by blists - more mailing lists