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@xxxxxxxxxxxxx>
>>>> ---
>>>> 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.
>
>>
>> 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.
>
--
BR,
Muhammad Usama Anjum
