Re: [PATCH v4 07/11] migration: add support for fault thread to load pages from disk

A subtle concurrency bug exists in the current patch; this small patch
should fix it.

Current patch uses bitmap_test_and_clear_atomic assuming it performs
an atomic operation over a range, which as pointer out by Peter is not
the case. It performs atomic word by word operations making the
overall operation unsafe.
Therefore, keeping the same bitmap would require protection by a mutex
lock which might not be as efficient. Hence this small patch resizes
the pending_bmap to have exactly one bit per load operation(loading
the larger of host page and guest page).

Thank you,
Aadeshveer Singh

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index e01ac628f5..0bf0f837ad 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1031,9 +1031,13 @@ static bool
postcopy_mapped_ram_load_page(MigrationIncomingState *mis,
     host_page = rb_offset / qemu_ram_pagesize(rb);
     page = rb_offset >> qemu_target_page_bits();

-    if (bitmap_test_and_clear_atomic(
-            rb->pending_bmap, host_page,
-            MAX(1, qemu_target_page_size() / qemu_ram_pagesize(rb)))) {
+    /*
+     * pending_bmap needs the index of host or guest page based on which is
+     * larger. As page index is inversely proportional to page size we use the
+     * minimum of both.
+     */
+    if (bitmap_test_and_clear_atomic(rb->pending_bmap, MIN(host_page, page),
+                                     1)) {
         if (find_next_bit(rb->file_bmap, page + guest_pages_to_load, page) ==
             page + guest_pages_to_load) {
             /* It is efficient to use UFFDIO_ZERO if all pages are zero */
diff --git a/migration/ram.c b/migration/ram.c
index 734fd90e12..52a3af9245 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -269,7 +269,14 @@ static void ramblock_pending_bmap_init(void)

     RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
         assert(!rb->pending_bmap);
-        size_t size = rb->max_length / qemu_ram_pagesize(rb);
+        /*
+         * The pending_bmap granularity must match the maximum of
host and guest
+         * page sizes. This ensures that every load operation checks for one
+         * bit, allowing lockless thread coordination via a single-bit atomic
+         * test-and-clear.
+         */
+        size_t size = rb->max_length /
+                      MAX(qemu_ram_pagesize(rb), qemu_target_page_size());
         rb->pending_bmap = bitmap_new(size);
         bitmap_set(rb->pending_bmap, 0, size);
     }

On Sat, Aug 1, 2026 at 8:07 AM Aadeshveer Singh <[email protected]> wrote:
>
> In fast snapshot load, we would like to serve faults as soon as possible
> hence loading pages directly instead of requesting a source
>
> Add postcopy_mapped_ram_load_page() function which serves single page
> fault. It uses bitmap_test_and_clear_atomic() on pending_bmap to prevent
> multiple threads from loading same page. It loads a page or pages
> depending on size of guest pages and host pages, loading exactly the
> larger of two. If the entire page is zero postcopy_place_page_zero() is
> used for efficiency and in case some part is non zero it is part by part
> loaded on loop using new function postcopy_mapped_ram_load_guest_page()
> which loads a single guest page in a buffer which is then placed using
> postcopy_place_page(). This covers all possible cases for various page
> sizes of host and guest.
>
> Update postcopy_ram_fault_thread to call postcopy_mapped_ram_load_page
> instead of requesting source in case of fast snapshot load. to_src_file
> check is bypassed in fast snapshot load case as there is no source.
>
> Call try_mark_postcopy_blocktime_begin on every page fault to support
> postcopy-blocktime.
>
> Allocate another channel in postcopy_temp_pages_setup(like the preempt
> case), for both the fault thread and eager thread to load pages
> independently.
>
> Signed-off-by: Aadeshveer Singh <[email protected]>
> ---
>  migration/postcopy-ram.c | 174 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 157 insertions(+), 17 deletions(-)
>
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 2e2c9fae10..1da9c4309b 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -949,6 +949,121 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd,
>                         pagesize);
>  }
>
> +/*
> + * Load a single guest page from source file into the buffer.
> + * NOTE: This is not an atomic operation and should not be used to directly load
> + * pages on page faults in postcopy. It is meant to fill in buffer that can then
> + * be copied into the faulting location using UFFDIO_COPY.
> + */
> +static bool postcopy_mapped_ram_load_guest_page(MigrationIncomingState *mis,
> +                                                RAMBlock *rb,
> +                                                ram_addr_t rb_offset, void *buf,
> +                                                Error **errp)
> +{
> +    ERRP_GUARD();
> +    size_t page = rb_offset / qemu_target_page_size();
> +    size_t read;
> +
> +    if (test_bit(page, rb->file_bmap)) {
> +        /*
> +         * This can happen concurrently, but it's thread-safe because
> +         * qemu_get_buffer_at() is thread-safe, and the caller will be using
> +         * different temporary buffers.
> +         */
> +        read =
> +            qemu_get_buffer_at(mis->from_src_file, buf, qemu_target_page_size(),
> +                               rb->pages_offset + rb_offset, errp);
> +
> +        if (read != qemu_target_page_size()) {
> +            error_prepend(errp,
> +                          "Could not read page %zu from RAM Block %s: ", page,
> +                          rb->idstr);
> +            return false;
> +        }
> +    } else {
> +        memset(buf, '\0', qemu_target_page_size());
> +    }
> +    return true;
> +}
> +
> +/**
> + * postcopy_mapped_ram_load_page() - Load pages required to access host address.
> + * @mis: Migration Incoming State.
> + * @rb: RAMBlock from where page is loaded.
> + * @rb_offset: Offset of target page in RAMBlock.
> + * @haddr: Base of target page where to load in page.
> + * @channel: Used to identify between threads and use corresponding temp.
> + * @errp: Set error in case of failure
> + *
> + * Load page(s) from RAMBlock covering the faulting address. We might need to
> + * load multiple pages in the case when host page size is greater than guest
> + * page size. As userfaultfd works on granularity of host pages, we might need
> + * to load guest pages in single operation.
> + *
> + * Return: True on success.
> + */
> +static bool postcopy_mapped_ram_load_page(MigrationIncomingState *mis,
> +                                          RAMBlock *rb, ram_addr_t rb_offset,
> +                                          uint64_t haddr, int channel,
> +                                          Error **errp)
> +{
> +    void *place_source = mis->postcopy_tmp_pages[channel].tmp_huge_page;
> +    char *buffer_ptr = (char *)place_source;
> +    size_t guest_pages_to_load =
> +        MAX(1, qemu_ram_pagesize(rb) / qemu_target_page_size());
> +    size_t host_page;
> +    size_t page;
> +
> +    /*
> +     * If guest page size is greater than host page size uffd needs to load one
> +     * guest page and multiple host pages, hence the offsets need to aligned
> +     * with guest pages (which is automatically aligned with host pages). In the
> +     * same case we need to check range of bits on pending_bmap(bit per host
> +     * page) to decide whether all the page have been loaded
> +     */
> +    rb_offset = ROUND_DOWN(rb_offset, qemu_target_page_size());
> +    haddr = ROUND_DOWN(haddr, qemu_target_page_size());
> +    host_page = rb_offset / qemu_ram_pagesize(rb);
> +    page = rb_offset >> qemu_target_page_bits();
> +
> +    if (bitmap_test_and_clear_atomic(
> +            rb->pending_bmap, host_page,
> +            MAX(1, qemu_target_page_size() / qemu_ram_pagesize(rb)))) {
> +        if (find_next_bit(rb->file_bmap, page + guest_pages_to_load, page) ==
> +            page + guest_pages_to_load) {
> +            /* It is efficient to use UFFDIO_ZERO if all pages are zero */
> +            if (postcopy_place_page_zero(mis, (void *)haddr, rb)) {
> +                error_setg(errp,
> +                           "Failed to place zero page %zu from RAM Block %s at "
> +                           "address %" PRIu64,
> +                           page, rb->idstr, haddr);
> +                return false;
> +            }
> +        } else {
> +            size_t load_size = guest_pages_to_load * qemu_target_page_size();
> +            size_t offset;
> +
> +            for (offset = 0; offset < load_size;
> +                 offset += qemu_target_page_size()) {
> +                if (!postcopy_mapped_ram_load_guest_page(
> +                        mis, rb, rb_offset + offset, buffer_ptr + offset,
> +                        errp)) {
> +                    return false;
> +                }
> +            }
> +
> +            if (postcopy_place_page(mis, (void *)haddr, place_source, rb)) {
> +                error_setg(errp,
> +                           "Failed to place page %zu from RAM Block %s at "
> +                           "address %" PRIu64,
> +                           page, rb->idstr, haddr);
> +                return false;
> +            }
> +        }
> +    }
> +    return true;
> +}
> +
>  /*
>   * NOTE: @tid is only used when postcopy-blocktime feature is enabled, and
>   * also optional: when zero is provided, the fault accounting will be ignored.
> @@ -1310,6 +1425,7 @@ static void *postcopy_ram_fault_thread(void *opaque)
>      int ret;
>      size_t index;
>      RAMBlock *rb = NULL;
> +    Error *local_err = NULL;
>
>      trace_postcopy_ram_fault_thread_entry();
>      rcu_register_thread();
> @@ -1351,11 +1467,13 @@ static void *postcopy_ram_fault_thread(void *opaque)
>              break;
>          }
>
> -        if (!mis->to_src_file) {
> +        if (!migrate_mapped_ram() && !mis->to_src_file) {
>              /*
> -             * Possibly someone tells us that the return path is
> -             * broken already using the event. We should hold until
> -             * the channel is rebuilt.
> +             * Possibly someone tells us that the return path is broken already
> +             * using the event. We should hold until the channel is rebuilt.
> +             * Fast snapshot load doesn't support pause and recover, because
> +             * it's not necessary: we can fail right away when QEMU just booted
> +             * with nothing to lose.
>               */
>              postcopy_pause_fault_thread(mis);
>          }
> @@ -1418,18 +1536,37 @@ static void *postcopy_ram_fault_thread(void *opaque)
>                                                  qemu_ram_get_idstr(rb),
>                                                  rb_offset,
>                                                  msg.arg.pagefault.feat.ptid);
> +
> +            if (migrate_mapped_ram()) {
> +                /* Load page directly in case of fast snapshot load */
> +
> +                uintptr_t aligned = (uintptr_t)ROUND_DOWN(
> +                    msg.arg.pagefault.address, qemu_ram_pagesize(rb));
> +
> +                if (try_mark_postcopy_blocktime_begin(
> +                        mis, rb, rb_offset, (uintptr_t)aligned,
> +                        msg.arg.pagefault.feat.ptid)) {
> +                    if (!postcopy_mapped_ram_load_page(
> +                            mis, rb, rb_offset, aligned, RAM_CHANNEL_POSTCOPY,
> +                            &local_err)) {
> +                        error_report_err(local_err);
> +                        break;
> +                    }
> +                }
> +            } else {
>  retry:
> -            /*
> -             * Send the request to the source - we want to request one
> -             * of our host page sizes (which is >= TPS)
> -             */
> -            ret = postcopy_request_page(mis, rb, rb_offset,
> -                                        msg.arg.pagefault.address,
> -                                        msg.arg.pagefault.feat.ptid);
> -            if (ret) {
> -                /* May be network failure, try to wait for recovery */
> -                postcopy_pause_fault_thread(mis);
> -                goto retry;
> +                /*
> +                 * Send the request to the source - we want to request one
> +                 * of our host page sizes (which is >= TPS)
> +                 */
> +                ret = postcopy_request_page(mis, rb, rb_offset,
> +                                            msg.arg.pagefault.address,
> +                                            msg.arg.pagefault.feat.ptid);
> +                if (ret) {
> +                    /* May be network failure, try to wait for recovery */
> +                    postcopy_pause_fault_thread(mis);
> +                    goto retry;
> +                }
>              }
>          }
>
> @@ -1501,8 +1638,11 @@ static int postcopy_temp_pages_setup(MigrationIncomingState *mis, Error **errp)
>      unsigned i, channels;
>      void *temp_page;
>
> -    if (migrate_postcopy_preempt()) {
> -        /* If preemption enabled, need extra channel for urgent requests */
> +    if (migrate_postcopy_preempt() || migrate_mapped_ram()) {
> +        /*
> +         * If preemption enabled or it is fast snapshot load, need extra channel
> +         * for urgent requests/faults
> +         */
>          mis->postcopy_channels = RAM_CHANNEL_MAX;
>      } else {
>          /* Both precopy/postcopy on the same channel */
> --
> 2.55.0
>