All of lore.kernel.org
 help / color / mirror / Atom feed
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, 14 Nov 2022 16:46:18 +0100	[thread overview]
Message-ID: <a90ee936-67a9-340d-bf2c-2f331617b0da@redhat.com> (raw)
In-Reply-To: <6fdce544-8d4f-8b3c-9208-735769a9e624@collabora.com>

> 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.

These are, AFAIU, the current semantics, and I am not sure if we want 
user space to explicitly work around that.

>>
>>
>> I'd suggest moving forward without this controversial
>> PAGEMAP_NO_REUSED_REGIONS functionality for now, and preparing it as a
>> clear add-on we can discuss separately.Like I've described above, I've only added this flag to not get the
> non-dirty pages as dirty. Can there be some alternative to adding this
> flag? Please suggest.

Please split it out into a separate patch for now. We can discuss 
further what the semantics are and if there are better alternatives for 
that. In the meantime, you could move forward without 
PAGEMAP_NO_REUSED_REGIONS while we are discussing them further.

-- 
Thanks,

David / dhildenb


  reply	other threads:[~2022-11-14 15:47 UTC|newest]

Thread overview: 23+ 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
2022-11-10 17:58   ` kernel test robot
2022-11-11 17:13   ` kernel test robot
2022-11-11 17:53     ` Muhammad Usama Anjum
2022-11-18  1:32   ` kernel test robot
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 [this message]
2022-11-21 15:00       ` Muhammad Usama Anjum
2022-11-21 15:55         ` David Hildenbrand
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=a90ee936-67a9-340d-bf2c-2f331617b0da@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.