From: David Hildenbrand <david@redhat.com>
To: "Muhammad Usama Anjum" <usama.anjum@collabora.com>,
"Michał Mirosław" <emmir@google.com>,
"Andrei Vagin" <avagin@gmail.com>,
"Danylo Mocherniuk" <mdanylo@google.com>,
"Alexander Viro" <viro@zeniv.linux.org.uk>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Paul Gofman" <pgofman@codeweavers.com>
Cc: Suren Baghdasaryan <surenb@google.com>,
Greg KH <gregkh@linuxfoundation.org>,
Christian Brauner <brauner@kernel.org>,
Peter Xu <peterx@redhat.com>, Yang Shi <shy828301@gmail.com>,
Vlastimil Babka <vbabka@suse.cz>,
Zach O'Keefe <zokeefe@google.com>,
"Matthew Wilcox (Oracle)" <willy@infradead.org>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Dan Williams <dan.j.williams@intel.com>,
kernel@collabora.com,
Gabriel Krisman Bertazi <krisman@collabora.com>,
Peter Enderborg <peter.enderborg@sony.com>,
"open list : KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
open list <linux-kernel@vger.kernel.org>,
"open list : PROC FILESYSTEM" <linux-fsdevel@vger.kernel.org>,
"open list : MEMORY MANAGEMENT" <linux-mm@kvack.org>
Subject: Re: [PATCH v6 0/3] Implement IOCTL to get and/or the clear info about PTEs
Date: Mon, 21 Nov 2022 16:55:56 +0100 [thread overview]
Message-ID: <bfcae708-db21-04b4-0bbe-712badd03071@redhat.com> (raw)
In-Reply-To: <254130e7-7fb1-6cf1-e8fa-5bc2d4450431@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 5526 bytes --]
On 21.11.22 16:00, Muhammad Usama Anjum wrote:
> Hello,
>
> Thank you for replying.
>
> On 11/14/22 8:46 PM, David Hildenbrand wrote:
>>> The soft-dirtiness is stored in the PTE. VMA is marked dirty to store the
>>> dirtiness for reused regions. Clearing the soft-dirty status of whole
>>> process is straight forward. When we want to clear/monitor the
>>> soft-dirtiness of a part of the virtual memory, there is a lot of internal
>>> noise. We don't want the non-dirty pages to become dirty because of how the
>>> soft-dirty feature has been working. Soft-dirty feature wasn't being used
>>> the way we want to use now. While monitoring a part of memory, it is not
>>> acceptable to get non-dirty pages as dirty. Non-dirty pages become dirty
>>> when the two VMAs are merged without considering if they both are dirty or
>>> not (34228d473efe). To monitor changes over the memory, sometimes VMAs are
>>> split to clear the soft-dirty bit in the VMA flags. But sometimes kernel
>>> decide to merge them backup. It is so waste of resources.
>>
>> Maybe you'd want a per-process option to not merge if the VM_SOFTDIRTY
>> property differs. But that might be just one alternative for handling this
>> case.
>>
>>>
>>> To keep things consistent, the default behavior of the IOCTL is to output
>>> even the extra non-dirty pages as dirty from the kernel noise. A optional
>>> PAGEMAP_NO_REUSED_REGIONS flag is added for those use cases which aren't
>>> tolerant of extra non-dirty pages. This flag can be considered as something
>>> which is by-passing the already present buggy implementation in the kernel.
>>> It is not buggy per say as the issue can be solved if we don't allow the
>>> two VMA which have different soft-dirty bits to get merged. But we are
>>> allowing that so that the total number of VMAs doesn't increase. This was
>>> acceptable at the time, but now with the use case of monitoring a part of
>>> memory for soft-dirty doesn't want this merging. So either we need to
>>> revert 34228d473efe and PAGEMAP_NO_REUSED_REGIONS flag will not be needed
>>> or we should allow PAGEMAP_NO_REUSED_REGIONS or similar mechanism to ignore
>>> the extra dirty pages which aren't dirty in reality.
>>>
>>> When PAGEMAP_NO_REUSED_REGIONS flag is used, only the PTEs are checked to
>>> find if the pages are dirty. So re-used regions cannot be detected. This
>>> has the only side-effect of not checking the VMAs. So this is limitation of
>>> using this flag which should be acceptable in the current state of code.
>>> This limitation is okay for the users as they can clear the soft-dirty bit
>>> of the VMA before starting to monitor a range of memory for soft-dirtiness.
>>>
>>>
>>>> Please separate that part out from the other changes; I am still not
>>>> convinced that we want this and what the semantical implications are.
>>>>
>>>> Let's take a look at an example: can_change_pte_writable()
>>>>
>>>> /* Do we need write faults for softdirty tracking? */
>>>> if (vma_soft_dirty_enabled(vma) && !pte_soft_dirty(pte))
>>>> return false;
>>>>
>>>> We care about PTE softdirty tracking, if it is enabled for the VMA.
>>>> Tracking is enabled if: vma_soft_dirty_enabled()
>>>>
>>>> /*
>>>> * Soft-dirty is kind of special: its tracking is enabled when
>>>> * the vma flags not set.
>>>> */
>>>> return !(vma->vm_flags & VM_SOFTDIRTY);
>>>>
>>>> Consequently, if VM_SOFTDIRTY is set, we are not considering the soft_dirty
>>>> PTE bits accordingly.
>>> Sorry, I'm unable to completely grasp the meaning of the example. We have
>>> followed clear_refs_write() to write the soft-dirty bit clearing code in
>>> the current patch. Dirtiness of the VMA and the PTE may be set
>>> independently. Newer allocated memory has dirty bit set in the VMA. When
>>> something is written the memory, the soft dirty bit is set in the PTEs as
>>> well regardless if the soft dirty bit is set in the VMA or not.
>>>
>>
>> Let me try to find a simple explanation:
>>
>> After clearing a SOFTDIRTY PTE flag inside an area with VM_SOFTDIRTY set,
>> there are ways that PTE could get written to and it could become dirty,
>> without the PTE becoming softdirty.
>>
>> Essentially, inside a VMA with VM_SOFTDIRTY set, the PTE softdirty values
>> might be stale: there might be entries that are softdirty even though the
>> PTE is *not* marked softdirty.
> Can someone please share the example to reproduce this? In all of my
> testing, even if I ignore VM_SOFTDIRTY and only base my decision of
> soft-dirtiness on individual pages, it always passes.
Quick reproducer (the first and easiest one that triggered :) )
attached.
With no kernel changes, it works as expected.
# ./softdirty_mprotect
With the following kernel change to simulate what you propose it fails:
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index d22687d2e81e..f2c682bf7f64 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1457,8 +1457,8 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
flags |= PM_FILE;
if (page && !migration && page_mapcount(page) == 1)
flags |= PM_MMAP_EXCLUSIVE;
- if (vma->vm_flags & VM_SOFTDIRTY)
- flags |= PM_SOFT_DIRTY;
+ //if (vma->vm_flags & VM_SOFTDIRTY)
+ // flags |= PM_SOFT_DIRTY;
return make_pme(frame, flags);
}
# ./softdirty_mprotect
Page #1 should be softdirty
--
Thanks,
David / dhildenb
[-- Attachment #2: softdirty_mprotect.c --]
[-- Type: text/x-csrc, Size: 2677 bytes --]
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdint.h>
#include <stdbool.h>
#include <sys/mman.h>
static size_t pagesize;
static int pagemap_fd;
static void clear_softdirty(void)
{
int fd = open("/proc/self/clear_refs", O_WRONLY);
const char *ctrl = "4";
int ret;
if (fd < 0) {
fprintf(stderr, "open() failed\n");
exit(1);
}
ret = write(fd, ctrl, strlen(ctrl));
close(fd);
if (ret != strlen(ctrl)) {
fprintf(stderr, "write() failed\n");
exit(1);
}
}
static uint64_t pagemap_get_entry(int fd, char *start)
{
const unsigned long pfn = (unsigned long)start / pagesize;
uint64_t entry;
int ret;
ret = pread(fd, &entry, sizeof(entry), pfn * sizeof(entry));
if (ret != sizeof(entry)) {
fprintf(stderr, "pread() failed\n");
exit(1);
}
return entry;
}
static bool pagemap_is_softdirty(int fd, char *start)
{
uint64_t entry = pagemap_get_entry(fd, start);
return entry & 0x0080000000000000ull;
}
void main(void)
{
char *mem, *mem2;
pagesize = getpagesize();
pagemap_fd = open("/proc/self/pagemap", O_RDONLY);
if (pagemap_fd < 0) {
fprintf(stderr, "open() failed\n");
exit(1);
}
/* Map 2 pages. */
mem = mmap(0, 2 * pagesize, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANON, -1, 0);
if (mem == MAP_FAILED) {
fprintf(stderr, "mmap() failed\n");
exit(1);
}
/* Populate both pages. */
memset(mem, 1, 2 * pagesize);
if (!pagemap_is_softdirty(pagemap_fd, mem))
fprintf(stderr, "Page #1 should be softdirty\n");
if (!pagemap_is_softdirty(pagemap_fd, mem + pagesize))
fprintf(stderr, "Page #2 should be softdirty\n");
/*
* Start softdirty tracking. Clear VM_SOFTDIRTY and clear the softdirty
* PTE bit.
*/
clear_softdirty();
if (pagemap_is_softdirty(pagemap_fd, mem))
fprintf(stderr, "Page #1 should not be softdirty\n");
if (pagemap_is_softdirty(pagemap_fd, mem + pagesize))
fprintf(stderr, "Page #2 should not be softdirty\n");
/*
* Remap the second page. The VMA gets VM_SOFTDIRTY set. Both VMAs
* get merged such that the resulting VMA has VM_SOFTDIRTY set.
*/
mem2 = mmap(mem + pagesize, pagesize, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANON|MAP_FIXED, -1, 0);
if (mem2 == MAP_FAILED) {
fprintf(stderr, "mmap() failed\n");
exit(1);
}
/* Protect + unprotect. */
mprotect(mem, 2 * pagesize, PROT_READ);
mprotect(mem, 2 * pagesize, PROT_READ|PROT_WRITE);
/* Modify both pages. */
memset(mem, 2, 2 * pagesize);
if (!pagemap_is_softdirty(pagemap_fd, mem))
fprintf(stderr, "Page #1 should be softdirty\n");
if (!pagemap_is_softdirty(pagemap_fd, mem + pagesize))
fprintf(stderr, "Page #2 should be softdirty\n");
}
next prev parent reply other threads:[~2022-11-21 15:57 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-09 10:23 [PATCH v6 0/3] Implement IOCTL to get and/or the clear info about PTEs Muhammad Usama Anjum
2022-11-09 10:23 ` [PATCH v6 1/3] fs/proc/task_mmu: update functions to clear the soft-dirty PTE bit Muhammad Usama Anjum
2022-11-09 10:23 ` [PATCH v6 2/3] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs Muhammad Usama Anjum
2022-11-09 23:54 ` Andrei Vagin
2022-11-11 10:10 ` Muhammad Usama Anjum
[not found] ` <202211120107.cYLiq2cH-lkp@intel.com>
2022-11-11 17:53 ` Muhammad Usama Anjum
2022-12-12 20:42 ` Cyrill Gorcunov
2022-12-13 13:04 ` Muhammad Usama Anjum
2022-12-13 22:22 ` Cyrill Gorcunov
2022-11-09 10:23 ` [PATCH v6 3/3] selftests: vm: add pagemap ioctl tests Muhammad Usama Anjum
2022-11-09 10:34 ` [PATCH v6 0/3] Implement IOCTL to get and/or the clear info about PTEs David Hildenbrand
2022-11-11 7:08 ` Muhammad Usama Anjum
2022-11-14 15:46 ` David Hildenbrand
2022-11-21 15:00 ` Muhammad Usama Anjum
2022-11-21 15:55 ` David Hildenbrand [this message]
2022-11-30 11:42 ` Muhammad Usama Anjum
2022-11-30 12:10 ` David Hildenbrand
2022-12-05 15:29 ` Muhammad Usama Anjum
2022-12-05 15:39 ` David Hildenbrand
2022-11-23 14:11 ` Peter Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bfcae708-db21-04b4-0bbe-712badd03071@redhat.com \
--to=david@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=avagin@gmail.com \
--cc=brauner@kernel.org \
--cc=dan.j.williams@intel.com \
--cc=emmir@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=gustavoars@kernel.org \
--cc=kernel@collabora.com \
--cc=krisman@collabora.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mdanylo@google.com \
--cc=peter.enderborg@sony.com \
--cc=peterx@redhat.com \
--cc=pgofman@codeweavers.com \
--cc=shuah@kernel.org \
--cc=shy828301@gmail.com \
--cc=surenb@google.com \
--cc=usama.anjum@collabora.com \
--cc=vbabka@suse.cz \
--cc=viro@zeniv.linux.org.uk \
--cc=willy@infradead.org \
--cc=zokeefe@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).