[PATCH v1 0/5] migration/ram: background snapshot fixes and optimiations

[PATCH v1 0/5] migration/ram: background snapshot fixes and optimiations

[David Hildenbrand]

Playing with background snapshots in combination with hugetlb and
virtio-mem, I found two issues and some reasonable optimizations (skip
unprotecting when unregistering).

With virtio-mem (RamDiscardManager), we now won't be allocating unnecessary
page tables for unplugged ranges when using uffd-wp with shmem/hugetlb.

Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration)
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Migration)
Cc: Peter Xu <peterx@redhat.com>
Cc: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>

David Hildenbrand (5):
  migration/ram: Fix populate_read_range()
  migration/ram: Fix error handling in ram_write_tracking_start()
  migration/ram: Don't explicitly unprotect when unregistering uffd-wp
  migration/ram: Rely on used_length for uffd_change_protection()
  migration/ram: Optimize ram_write_tracking_start() for
    RamDiscardManager

 migration/ram.c | 54 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 14 deletions(-)

-- 
2.39.0

[PATCH v1 1/5] migration/ram: Fix populate_read_range()

[David Hildenbrand]

Unfortunately, commit f7b9dcfbcf44 broke populate_read_range(): the loop
end condition is very wrong, resulting in that function not populating the
full range. Lets' fix that.

Fixes: f7b9dcfbcf44 ("migration/ram: Factor out populating pages readable in ram_block_populate_pages()")
Cc: qemu-stable@nongnu.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 334309f1c6..b8f58d2a40 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1774,13 +1774,15 @@ out:
 static inline void populate_read_range(RAMBlock *block, ram_addr_t offset,
                                        ram_addr_t size)
 {
+    const ram_addr_t end = offset + size;
+
     /*
      * We read one byte of each page; this will preallocate page tables if
      * required and populate the shared zeropage on MAP_PRIVATE anonymous memory
      * where no page was populated yet. This might require adaption when
      * supporting other mappings, like shmem.
      */
-    for (; offset < size; offset += block->page_size) {
+    for (; offset < end; offset += block->page_size) {
         char tmp = *((char *)block->host + offset);
 
         /* Don't optimize the read out */
-- 
2.39.0

Re: [PATCH v1 1/5] migration/ram: Fix populate_read_range()

[Juan Quintela]

David Hildenbrand <david@redhat.com> wrote:
> Unfortunately, commit f7b9dcfbcf44 broke populate_read_range(): the loop
> end condition is very wrong, resulting in that function not populating the
> full range. Lets' fix that.
>
> Fixes: f7b9dcfbcf44 ("migration/ram: Factor out populating pages readable in ram_block_populate_pages()")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v1 2/5] migration/ram: Fix error handling in ram_write_tracking_start()

[David Hildenbrand]

If something goes wrong during uffd_change_protection(), we would miss
to unregister uffd-wp and not release our reference. Fix it by
performing the uffd_change_protection(true) last.

Note that a uffd_change_protection(false) on the recovery path without a
prior uffd_change_protection(false) is fine.

Fixes: 278e2f551a09 ("migration: support UFFD write fault processing in ram_save_iterate()")
Cc: qemu-stable@nongnu.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index b8f58d2a40..6e4e41952a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1896,13 +1896,14 @@ int ram_write_tracking_start(void)
                 block->max_length, UFFDIO_REGISTER_MODE_WP, NULL)) {
             goto fail;
         }
+        block->flags |= RAM_UF_WRITEPROTECT;
+        memory_region_ref(block->mr);
+
         /* Apply UFFD write protection to the block memory range */
         if (uffd_change_protection(rs->uffdio_fd, block->host,
                 block->max_length, true, false)) {
             goto fail;
         }
-        block->flags |= RAM_UF_WRITEPROTECT;
-        memory_region_ref(block->mr);
 
         trace_ram_write_tracking_ramblock_start(block->idstr, block->page_size,
                 block->host, block->max_length);
-- 
2.39.0

Re: [PATCH v1 2/5] migration/ram: Fix error handling in ram_write_tracking_start()

[Juan Quintela]

David Hildenbrand <david@redhat.com> wrote:
> If something goes wrong during uffd_change_protection(), we would miss
> to unregister uffd-wp and not release our reference. Fix it by
> performing the uffd_change_protection(true) last.
>
> Note that a uffd_change_protection(false) on the recovery path without a
> prior uffd_change_protection(false) is fine.
>
> Fixes: 278e2f551a09 ("migration: support UFFD write fault processing in ram_save_iterate()")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v1 3/5] migration/ram: Don't explicitly unprotect when unregistering uffd-wp

[David Hildenbrand]

When unregistering uffd-wp, older kernels before commit f369b07c86143
("mm/uffd:reset write protection when unregister with wp-mode") won't
clear the uffd-wp PTE bit. When re-registering uffd-wp, the previous
uffd-wp PTE bits would trigger again. With above commit, the kernel will
clear the uffd-wp PTE bits when unregistering itself.

Consequently, we'll clear the uffd-wp PTE bits now twice -- whereby we
don't care about clearing them at all: a new background snapshot will
re-register uffd-wp and re-protect all memory either way.

So let's skip the manual clearing of uffd-wp. If ever relevant, we
could clear conditionally in uffd_unregister_memory() -- we just need a
way to figure out more recent kernels.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 6e4e41952a..6a3dbee2c3 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1918,12 +1918,6 @@ fail:
         if ((block->flags & RAM_UF_WRITEPROTECT) == 0) {
             continue;
         }
-        /*
-         * In case some memory block failed to be write-protected
-         * remove protection and unregister all succeeded RAM blocks
-         */
-        uffd_change_protection(rs->uffdio_fd, block->host, block->max_length,
-                false, false);
         uffd_unregister_memory(rs->uffdio_fd, block->host, block->max_length);
         /* Cleanup flags and remove reference */
         block->flags &= ~RAM_UF_WRITEPROTECT;
@@ -1949,9 +1943,6 @@ void ram_write_tracking_stop(void)
         if ((block->flags & RAM_UF_WRITEPROTECT) == 0) {
             continue;
         }
-        /* Remove protection and unregister all affected RAM blocks */
-        uffd_change_protection(rs->uffdio_fd, block->host, block->max_length,
-                false, false);
         uffd_unregister_memory(rs->uffdio_fd, block->host, block->max_length);
 
         trace_ram_write_tracking_ramblock_stop(block->idstr, block->page_size,
-- 
2.39.0

Re: [PATCH v1 3/5] migration/ram: Don't explicitly unprotect when unregistering uffd-wp

[Juan Quintela]

David Hildenbrand <david@redhat.com> wrote:
> When unregistering uffd-wp, older kernels before commit f369b07c86143
> ("mm/uffd:reset write protection when unregister with wp-mode") won't
> clear the uffd-wp PTE bit. When re-registering uffd-wp, the previous
> uffd-wp PTE bits would trigger again. With above commit, the kernel will
> clear the uffd-wp PTE bits when unregistering itself.
>
> Consequently, we'll clear the uffd-wp PTE bits now twice -- whereby we
> don't care about clearing them at all: a new background snapshot will
> re-register uffd-wp and re-protect all memory either way.
>
> So let's skip the manual clearing of uffd-wp. If ever relevant, we
> could clear conditionally in uffd_unregister_memory() -- we just need a
> way to figure out more recent kernels.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Fixing a bug by removing code O:-)

[PATCH v1 4/5] migration/ram: Rely on used_length for uffd_change_protection()

[David Hildenbrand]

ram_mig_ram_block_resized() will abort migration (including background
snapshots) when resizing a RAMBlock. ram_block_populate_read() will only
populate RAM up to used_length, so at least for anonymous memory
protecting everything between used_length and max_length won't
actually be protected and is just a NOP.

So let's only protect everything up to used_length.

Note: it still makes sense to register uffd-wp for max_length, such
that RAM_UF_WRITEPROTECT is independent of a changing used_length.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 6a3dbee2c3..73a443f683 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1901,7 +1901,7 @@ int ram_write_tracking_start(void)
 
         /* Apply UFFD write protection to the block memory range */
         if (uffd_change_protection(rs->uffdio_fd, block->host,
-                block->max_length, true, false)) {
+                                   block->used_length, true, false)) {
             goto fail;
         }
 
-- 
2.39.0

Re: [PATCH v1 4/5] migration/ram: Rely on used_length for uffd_change_protection()

[Juan Quintela]

David Hildenbrand <david@redhat.com> wrote:
> ram_mig_ram_block_resized() will abort migration (including background
> snapshots) when resizing a RAMBlock. ram_block_populate_read() will only
> populate RAM up to used_length, so at least for anonymous memory
> protecting everything between used_length and max_length won't
> actually be protected and is just a NOP.
>
> So let's only protect everything up to used_length.
>
> Note: it still makes sense to register uffd-wp for max_length, such
> that RAM_UF_WRITEPROTECT is independent of a changing used_length.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v1 5/5] migration/ram: Optimize ram_write_tracking_start() for RamDiscardManager

[David Hildenbrand]

ram_block_populate_read() already optimizes for RamDiscardManager.
However, ram_write_tracking_start() will still try protecting discarded
memory ranges.

Let's optimize, because discarded ranges don't map any pages and

(1) For anonymous memory, trying to protect using uffd-wp without a mapped
    page is ignored by the kernel and consequently a NOP.

(2) For shared/file-backed memory, we will fill present page tables in the
    range with PTE markers. However, we will even allocate page tables
    just to fill them with unnecessary PTE markers and effectively
    waste memory.

So let's exclude these ranges, just like ram_block_populate_read()
already does.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 73a443f683..50ee1fa147 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1865,6 +1865,39 @@ void ram_write_tracking_prepare(void)
     }
 }
 
+static inline int uffd_protect_section(MemoryRegionSection *section,
+                                       void *opaque)
+{
+    const hwaddr size = int128_get64(section->size);
+    const hwaddr offset = section->offset_within_region;
+    RAMBlock *rb = section->mr->ram_block;
+    int uffd_fd = (uintptr_t)opaque;
+
+    return uffd_change_protection(uffd_fd, rb->host + offset, size, true,
+                                  false);
+}
+
+static int ram_block_uffd_protect(RAMBlock *rb, int uffd_fd)
+{
+    assert(rb->flags & RAM_UF_WRITEPROTECT);
+
+    /* See ram_block_populate_read() */
+    if (rb->mr && memory_region_has_ram_discard_manager(rb->mr)) {
+        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(rb->mr);
+        MemoryRegionSection section = {
+            .mr = rb->mr,
+            .offset_within_region = 0,
+            .size = rb->mr->size,
+        };
+
+        return ram_discard_manager_replay_populated(rdm, &section,
+                                                    uffd_protect_section,
+                                                    (void *)(uintptr_t)uffd_fd);
+    }
+    return uffd_change_protection(uffd_fd, rb->host,
+                                  rb->used_length, true, false);
+}
+
 /*
  * ram_write_tracking_start: start UFFD-WP memory tracking
  *
@@ -1900,8 +1933,7 @@ int ram_write_tracking_start(void)
         memory_region_ref(block->mr);
 
         /* Apply UFFD write protection to the block memory range */
-        if (uffd_change_protection(rs->uffdio_fd, block->host,
-                                   block->used_length, true, false)) {
+        if (ram_block_uffd_protect(block, uffd_fd)) {
             goto fail;
         }
 
-- 
2.39.0

Re: [PATCH v1 5/5] migration/ram: Optimize ram_write_tracking_start() for RamDiscardManager

[Juan Quintela]

David Hildenbrand <david@redhat.com> wrote:
> ram_block_populate_read() already optimizes for RamDiscardManager.
> However, ram_write_tracking_start() will still try protecting discarded
> memory ranges.
>
> Let's optimize, because discarded ranges don't map any pages and
>
> (1) For anonymous memory, trying to protect using uffd-wp without a mapped
>     page is ignored by the kernel and consequently a NOP.
>
> (2) For shared/file-backed memory, we will fill present page tables in the
>     range with PTE markers. However, we will even allocate page tables
>     just to fill them with unnecessary PTE markers and effectively
>     waste memory.
>
> So let's exclude these ranges, just like ram_block_populate_read()
> already does.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v1 0/5] migration/ram: background snapshot fixes and optimiations

[Peter Xu]

On Thu, Jan 05, 2023 at 01:45:23PM +0100, David Hildenbrand wrote:
> Playing with background snapshots in combination with hugetlb and
> virtio-mem, I found two issues and some reasonable optimizations (skip
> unprotecting when unregistering).
> 
> With virtio-mem (RamDiscardManager), we now won't be allocating unnecessary
> page tables for unplugged ranges when using uffd-wp with shmem/hugetlb.
> 
> Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration)
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Migration)
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
> 
> David Hildenbrand (5):
>   migration/ram: Fix populate_read_range()
>   migration/ram: Fix error handling in ram_write_tracking_start()
>   migration/ram: Don't explicitly unprotect when unregistering uffd-wp
>   migration/ram: Rely on used_length for uffd_change_protection()
>   migration/ram: Optimize ram_write_tracking_start() for
>     RamDiscardManager

For the series:

Acked-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [PATCH v1 0/5] migration/ram: background snapshot fixes and optimiations

[David Hildenbrand]

On 05.01.23 22:11, Peter Xu wrote:
> On Thu, Jan 05, 2023 at 01:45:23PM +0100, David Hildenbrand wrote:
>> Playing with background snapshots in combination with hugetlb and
>> virtio-mem, I found two issues and some reasonable optimizations (skip
>> unprotecting when unregistering).
>>
>> With virtio-mem (RamDiscardManager), we now won't be allocating unnecessary
>> page tables for unplugged ranges when using uffd-wp with shmem/hugetlb.
>>
>> Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration)
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Migration)
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
>>
>> David Hildenbrand (5):
>>    migration/ram: Fix populate_read_range()
>>    migration/ram: Fix error handling in ram_write_tracking_start()
>>    migration/ram: Don't explicitly unprotect when unregistering uffd-wp
>>    migration/ram: Rely on used_length for uffd_change_protection()
>>    migration/ram: Optimize ram_write_tracking_start() for
>>      RamDiscardManager
> 
> For the series:
> 
> Acked-by: Peter Xu <peterx@redhat.com>

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 0/5] migration/ram: background snapshot fixes and optimiations

[David Hildenbrand]

On 05.01.23 13:45, David Hildenbrand wrote:
> Playing with background snapshots in combination with hugetlb and
> virtio-mem, I found two issues and some reasonable optimizations (skip
> unprotecting when unregistering).
> 
> With virtio-mem (RamDiscardManager), we now won't be allocating unnecessary
> page tables for unplugged ranges when using uffd-wp with shmem/hugetlb.
> 
> Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration)
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Migration)
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>

@Juan, David: I can take this via my tree (especially the last patch 
only optimized virtio-mem for now). Just let me know.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 0/5] migration/ram: background snapshot fixes and optimiations

[Juan Quintela]

David Hildenbrand <david@redhat.com> wrote:
> On 05.01.23 13:45, David Hildenbrand wrote:
>> Playing with background snapshots in combination with hugetlb and
>> virtio-mem, I found two issues and some reasonable optimizations (skip
>> unprotecting when unregistering).
>> With virtio-mem (RamDiscardManager), we now won't be allocating
>> unnecessary
>> page tables for unplugged ranges when using uffd-wp with shmem/hugetlb.
>> Cc: Juan Quintela <quintela@redhat.com> (maintainer:Migration)
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Migration)
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
>
> @Juan, David: I can take this via my tree (especially the last patch
> only optimized virtio-mem for now). Just let me know.

I reviewed everything now.

Queued on my tree.

Later, Juan.