Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: multipart/mixed; boundary="------------X26NNqKh6qmB3gIBG6DEU1Rl"
Message-ID: <bfcae708-db21-04b4-0bbe-712badd03071@redhat.com>
Date:   Mon, 21 Nov 2022 16:55:56 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH v6 0/3] Implement IOCTL to get and/or the clear info about
 PTEs
Content-Language: en-US
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>,
        =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= <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>
References: <20221109102303.851281-1-usama.anjum@collabora.com>
 <9c167d01-ef09-ec4e-b4a1-2fff62bf01fe@redhat.com>
 <6fdce544-8d4f-8b3c-9208-735769a9e624@collabora.com>
 <a90ee936-67a9-340d-bf2c-2f331617b0da@redhat.com>
 <254130e7-7fb1-6cf1-e8fa-5bc2d4450431@collabora.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <254130e7-7fb1-6cf1-e8fa-5bc2d4450431@collabora.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------X26NNqKh6qmB3gIBG6DEU1Rl
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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

--------------X26NNqKh6qmB3gIBG6DEU1Rl
Content-Type: text/x-csrc; charset=UTF-8; name="softdirty_mprotect.c"
Content-Disposition: attachment; filename="softdirty_mprotect.c"
Content-Transfer-Encoding: base64

I2luY2x1ZGUgPHN0ZGxpYi5oPgojaW5jbHVkZSA8c3RyaW5nLmg+CiNpbmNsdWRlIDxzdGRp
by5oPgojaW5jbHVkZSA8ZmNudGwuaD4KI2luY2x1ZGUgPHVuaXN0ZC5oPgojaW5jbHVkZSA8
c3RkaW50Lmg+CiNpbmNsdWRlIDxzdGRib29sLmg+CiNpbmNsdWRlIDxzeXMvbW1hbi5oPgoK
c3RhdGljIHNpemVfdCBwYWdlc2l6ZTsKc3RhdGljIGludCBwYWdlbWFwX2ZkOwoKc3RhdGlj
IHZvaWQgY2xlYXJfc29mdGRpcnR5KHZvaWQpCnsKCWludCBmZCA9IG9wZW4oIi9wcm9jL3Nl
bGYvY2xlYXJfcmVmcyIsIE9fV1JPTkxZKTsKCWNvbnN0IGNoYXIgKmN0cmwgPSAiNCI7Cglp
bnQgcmV0OwoKCWlmIChmZCA8IDApIHsKCQlmcHJpbnRmKHN0ZGVyciwgIm9wZW4oKSBmYWls
ZWRcbiIpOwoJCWV4aXQoMSk7Cgl9CglyZXQgPSB3cml0ZShmZCwgY3RybCwgc3RybGVuKGN0
cmwpKTsKCWNsb3NlKGZkKTsKCWlmIChyZXQgIT0gc3RybGVuKGN0cmwpKSB7CgkJZnByaW50
ZihzdGRlcnIsICJ3cml0ZSgpIGZhaWxlZFxuIik7CgkJZXhpdCgxKTsKCX0KfQoKc3RhdGlj
IHVpbnQ2NF90IHBhZ2VtYXBfZ2V0X2VudHJ5KGludCBmZCwgY2hhciAqc3RhcnQpCnsKCWNv
bnN0IHVuc2lnbmVkIGxvbmcgcGZuID0gKHVuc2lnbmVkIGxvbmcpc3RhcnQgLyBwYWdlc2l6
ZTsKCXVpbnQ2NF90IGVudHJ5OwoJaW50IHJldDsKCglyZXQgPSBwcmVhZChmZCwgJmVudHJ5
LCBzaXplb2YoZW50cnkpLCBwZm4gKiBzaXplb2YoZW50cnkpKTsKCWlmIChyZXQgIT0gc2l6
ZW9mKGVudHJ5KSkgewoJCWZwcmludGYoc3RkZXJyLCAicHJlYWQoKSBmYWlsZWRcbiIpOwoJ
CWV4aXQoMSk7Cgl9CgoJcmV0dXJuIGVudHJ5Owp9CgpzdGF0aWMgYm9vbCBwYWdlbWFwX2lz
X3NvZnRkaXJ0eShpbnQgZmQsIGNoYXIgKnN0YXJ0KQp7Cgl1aW50NjRfdCBlbnRyeSA9IHBh
Z2VtYXBfZ2V0X2VudHJ5KGZkLCBzdGFydCk7CgoJcmV0dXJuIGVudHJ5ICYgMHgwMDgwMDAw
MDAwMDAwMDAwdWxsOwp9Cgp2b2lkIG1haW4odm9pZCkKewoJY2hhciAqbWVtLCAqbWVtMjsK
CglwYWdlc2l6ZSA9IGdldHBhZ2VzaXplKCk7CglwYWdlbWFwX2ZkID0gb3BlbigiL3Byb2Mv
c2VsZi9wYWdlbWFwIiwgT19SRE9OTFkpOwoJaWYgKHBhZ2VtYXBfZmQgPCAwKSB7CgkJZnBy
aW50ZihzdGRlcnIsICJvcGVuKCkgZmFpbGVkXG4iKTsKCQlleGl0KDEpOwoJfQoKCS8qIE1h
cCAyIHBhZ2VzLiAqLwoJbWVtID0gbW1hcCgwLCAyICogcGFnZXNpemUsIFBST1RfUkVBRHxQ
Uk9UX1dSSVRFLAoJCSAgIE1BUF9QUklWQVRFfE1BUF9BTk9OLCAtMSwgMCk7CglpZiAobWVt
ID09IE1BUF9GQUlMRUQpIHsKCQlmcHJpbnRmKHN0ZGVyciwgIm1tYXAoKSBmYWlsZWRcbiIp
OwoJCWV4aXQoMSk7Cgl9CgoJLyogUG9wdWxhdGUgYm90aCBwYWdlcy4gKi8KCW1lbXNldCht
ZW0sIDEsIDIgKiBwYWdlc2l6ZSk7CgoJaWYgKCFwYWdlbWFwX2lzX3NvZnRkaXJ0eShwYWdl
bWFwX2ZkLCBtZW0pKQoJCWZwcmludGYoc3RkZXJyLCAiUGFnZSAjMSBzaG91bGQgYmUgc29m
dGRpcnR5XG4iKTsKCWlmICghcGFnZW1hcF9pc19zb2Z0ZGlydHkocGFnZW1hcF9mZCwgbWVt
ICsgcGFnZXNpemUpKQoJCWZwcmludGYoc3RkZXJyLCAiUGFnZSAjMiBzaG91bGQgYmUgc29m
dGRpcnR5XG4iKTsKCgkvKgoJICogU3RhcnQgc29mdGRpcnR5IHRyYWNraW5nLiBDbGVhciBW
TV9TT0ZURElSVFkgYW5kIGNsZWFyIHRoZSBzb2Z0ZGlydHkKCSAqIFBURSBiaXQuCgkgKi8K
CWNsZWFyX3NvZnRkaXJ0eSgpOwoKCWlmIChwYWdlbWFwX2lzX3NvZnRkaXJ0eShwYWdlbWFw
X2ZkLCBtZW0pKQoJCWZwcmludGYoc3RkZXJyLCAiUGFnZSAjMSBzaG91bGQgbm90IGJlIHNv
ZnRkaXJ0eVxuIik7CglpZiAocGFnZW1hcF9pc19zb2Z0ZGlydHkocGFnZW1hcF9mZCwgbWVt
ICsgcGFnZXNpemUpKQoJCWZwcmludGYoc3RkZXJyLCAiUGFnZSAjMiBzaG91bGQgbm90IGJl
IHNvZnRkaXJ0eVxuIik7CgoJLyoKCSAqIFJlbWFwIHRoZSBzZWNvbmQgcGFnZS4gVGhlIFZN
QSBnZXRzIFZNX1NPRlRESVJUWSBzZXQuIEJvdGggVk1BcwoJICogZ2V0IG1lcmdlZCBzdWNo
IHRoYXQgdGhlIHJlc3VsdGluZyBWTUEgaGFzIFZNX1NPRlRESVJUWSBzZXQuCgkgKi8KCW1l
bTIgPSBtbWFwKG1lbSArIHBhZ2VzaXplLCBwYWdlc2l6ZSwgUFJPVF9SRUFEfFBST1RfV1JJ
VEUsCgkJICAgIE1BUF9QUklWQVRFfE1BUF9BTk9OfE1BUF9GSVhFRCwgLTEsIDApOwoJaWYg
KG1lbTIgPT0gTUFQX0ZBSUxFRCkgewoJCWZwcmludGYoc3RkZXJyLCAibW1hcCgpIGZhaWxl
ZFxuIik7CgkJZXhpdCgxKTsKCX0KCgkvKiBQcm90ZWN0ICsgdW5wcm90ZWN0LiAqLwoJbXBy
b3RlY3QobWVtLCAyICogcGFnZXNpemUsIFBST1RfUkVBRCk7CgltcHJvdGVjdChtZW0sIDIg
KiBwYWdlc2l6ZSwgUFJPVF9SRUFEfFBST1RfV1JJVEUpOwoKCS8qIE1vZGlmeSBib3RoIHBh
Z2VzLiAqLwoJbWVtc2V0KG1lbSwgMiwgMiAqIHBhZ2VzaXplKTsKCglpZiAoIXBhZ2VtYXBf
aXNfc29mdGRpcnR5KHBhZ2VtYXBfZmQsIG1lbSkpCgkJZnByaW50ZihzdGRlcnIsICJQYWdl
ICMxIHNob3VsZCBiZSBzb2Z0ZGlydHlcbiIpOwoJaWYgKCFwYWdlbWFwX2lzX3NvZnRkaXJ0
eShwYWdlbWFwX2ZkLCBtZW0gKyBwYWdlc2l6ZSkpCgkJZnByaW50ZihzdGRlcnIsICJQYWdl
ICMyIHNob3VsZCBiZSBzb2Z0ZGlydHlcbiIpOwp9Cg==

--------------X26NNqKh6qmB3gIBG6DEU1Rl--