Re: [Qemu-devel] [PATCH v4 44/47] Postcopy; Handle userfault requests

[David Gibson]

On Fri, Oct 03, 2014 at 06:47:50PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> userfaultfd is a Linux syscall that gives an fd that receives a stream
> of notifications of accesses to pages marked as MADV_USERFAULT, and
> allows the program to acknowledge those stalls and tell the accessing
> thread to carry on.
> 
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>

[snip]
>  /*
> + * Tell the kernel that we've now got some memory it previously asked for.
> + * Note: We're not allowed to ack a page which wasn't requested.
> + */
> +static int ack_userfault(MigrationIncomingState *mis, void *start, size_t 
> len)
> +{
> +    uint64_t tmp[2];
> +
> +    /*
> +     * Kernel wants the range that's now safe to access
> +     * Note it always takes 64bit values, even on a 32bit host.
> +     */
> +    tmp[0] = (uint64_t)(uintptr_t)start;
> +    tmp[1] = (uint64_t)(uintptr_t)start + (uint64_t)len;
> +
> +    if (write(mis->userfault_fd, tmp, 16) != 16) {
> +        int e = errno;

Is an EOF (i.e. write() returns 0) ever possible here?  If so errno
may not have a meaningful value.

> +        if (e == ENOENT) {
> +            /* Kernel said it wasn't waiting - one case where this can
> +             * happen is where two threads triggered the userfault
> +             * and we receive the page and ack it just after we received
> +             * the 2nd request and that ends up deciding it should ack it
> +             * We could optimise it out, but it's rare.
> +             */
> +            /*fprintf(stderr, "ack_userfault: %p/%zx ENOENT\n", start, len); 
> */
> +            return 0;
> +        }
> +        error_report("postcopy_ram: Failed to notify kernel for %p/%zx (%d)",
> +                     start, len, e);
> +        return -errno;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
>   * Handle faults detected by the USERFAULT markings
>   */
>  static void *postcopy_ram_fault_thread(void *opaque)
>  {
>      MigrationIncomingState *mis = (MigrationIncomingState *)opaque;
> +    void *hostaddr;
> +    int ret;
> +    size_t hostpagesize = getpagesize();
> +    RAMBlock *rb = NULL;
> +    RAMBlock *last_rb = NULL; /* last RAMBlock we sent part of */
>  
> -    fprintf(stderr, "postcopy_ram_fault_thread\n");
> -    /* TODO: In later patch */
> +    DPRINTF("%s", __func__);
>      qemu_sem_post(&mis->fault_thread_sem);
> -    while (1) {
> -        /* TODO: In later patch */
> -    }
> +    while (true) {
> +        PostcopyPMIState old_state, tmp_state;
> +        ram_addr_t rb_offset;
> +        ram_addr_t in_raspace;
> +        unsigned long bitmap_index;
> +        struct pollfd pfd[2];
> +
> +        /*
> +         * We're mainly waiting for the kernel to give us a faulting HVA,
> +         * however we can be told to quit via userfault_quit_fd which is
> +         * an eventfd
> +         */
> +        pfd[0].fd = mis->userfault_fd;
> +        pfd[0].events = POLLIN;
> +        pfd[0].revents = 0;
> +        pfd[1].fd = mis->userfault_quit_fd;
> +        pfd[1].events = POLLIN; /* Waiting for eventfd to go positive */
> +        pfd[1].revents = 0;
> +
> +        if (poll(pfd, 2, -1 /* Wait forever */) == -1) {
> +            perror("userfault poll");
> +            break;
> +        }
>  
> +        if (pfd[1].revents) {
> +            DPRINTF("%s got quit event", __func__);
> +            break;

I don't see any cleanup path in the userfault thread.  So wouldn't it
be simpler to just pthread_cancel() it rather than using an extra fd
for quit notifications.

> +        }
> +
> +        ret = read(mis->userfault_fd, &hostaddr, sizeof(hostaddr));
> +        if (ret != sizeof(hostaddr)) {
> +            if (ret < 0) {
> +                perror("Failed to read full userfault hostaddr");
> +                break;
> +            } else {
> +                error_report("%s: Read %d bytes from userfaultfd expected 
> %zd",
> +                             __func__, ret, sizeof(hostaddr));
> +                break; /* Lost alignment, don't know what we'd read next */
> +            }
> +        }
> +
> +        rb = qemu_ram_block_from_host(hostaddr, true, &in_raspace, 
> &rb_offset,
> +                                      &bitmap_index);
> +        if (!rb) {
> +            error_report("postcopy_ram_fault_thread: Fault outside guest: 
> %p",
> +                         hostaddr);
> +            break;
> +        }
> +
> +        DPRINTF("%s: Request for HVA=%p index=%lx rb=%s offset=%zx",
> +                __func__, hostaddr, bitmap_index, qemu_ram_get_idstr(rb),
> +                rb_offset);
> +
> +        tmp_state = postcopy_pmi_get_state(mis, bitmap_index);
> +        do {
> +            old_state = tmp_state;
> +
> +            switch (old_state) {
> +            case POSTCOPY_PMI_REQUESTED:
> +                /* Do nothing - it's already requested */
> +                break;
> +
> +            case POSTCOPY_PMI_RECEIVED:
> +                /* Already arrived - no state change, just kick the kernel */
> +                DPRINTF("postcopy_ram_fault_thread: notify pre of %p",
> +                        hostaddr);
> +                if (ack_userfault(mis,
> +                                  (void *)((uintptr_t)hostaddr
> +                                           & ~(hostpagesize - 1)),
> +                                  hostpagesize)) {
> +                    assert(0);
> +                }
> +                break;
> +
> +            case POSTCOPY_PMI_MISSING:
> +
> +                tmp_state = postcopy_pmi_change_state(mis, bitmap_index,
> +                                           old_state, 
> POSTCOPY_PMI_REQUESTED);
> +                if (tmp_state == POSTCOPY_PMI_MISSING) {
> +                    /*
> +                     * Send the request to the source - we want to request 
> one
> +                     * of our host page sizes (which is >= TPS)
> +                     */
> +                    if (rb != last_rb) {
> +                        last_rb = rb;
> +                        migrate_send_rp_reqpages(mis, qemu_ram_get_idstr(rb),
> +                                                 rb_offset, hostpagesize);
> +                    } else {
> +                        /* Save some space */
> +                        migrate_send_rp_reqpages(mis, NULL,
> +                                                 rb_offset, hostpagesize);
> +                    }
> +                }
> +                break;
> +           }
> +        } while (tmp_state != old_state);
> +    }
> +    DPRINTF("%s: exit", __func__);
>      return NULL;
>  }
>  
>  int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>  {
> -    /* Create the fault handler thread and wait for it to be ready */
> +    uint64_t tmp64;
> +
> +    /* Open the fd for the kernel to give us userfaults */
> +    mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC);
> +    if (mis->userfault_fd == -1) {
> +        perror("Failed to open userfault fd");
> +        return -1;
> +    }
> +
> +    /*
> +     * Version handshake, we send it the version we want and expect to get 
> the
> +     * same back.
> +     */
> +    tmp64 = USERFAULTFD_PROTOCOL;
> +    if (write(mis->userfault_fd, &tmp64, sizeof(tmp64)) != sizeof(tmp64)) {
> +        perror("Writing userfaultfd version");
> +        close(mis->userfault_fd);
> +        return -1;
> +    }
> +    if (read(mis->userfault_fd, &tmp64, sizeof(tmp64)) != sizeof(tmp64)) {
> +        perror("Reading userfaultfd version");
> +        close(mis->userfault_fd);
> +        return -1;
> +    }
> +    if (tmp64 != USERFAULTFD_PROTOCOL) {
> +        error_report("Mismatched userfaultfd version, expected %zx, got %zx",
> +                     (size_t)USERFAULTFD_PROTOCOL, (size_t)tmp64);
> +        close(mis->userfault_fd);
> +        return -1;
> +    }
> +
> +    /* Now an eventfd we use to tell the fault-thread to quit */
> +    mis->userfault_quit_fd = eventfd(0, EFD_CLOEXEC);
> +    if (mis->userfault_quit_fd == -1) {
> +        perror("Opening userfault_quit_fd");
> +        close(mis->userfault_fd);
> +        return -1;
> +    }
> +
>      qemu_sem_init(&mis->fault_thread_sem, 0);
>      qemu_thread_create(&mis->fault_thread, "postcopy/fault",
>                         postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE);
>      qemu_sem_wait(&mis->fault_thread_sem);
> +    mis->have_fault_thread = true;
>  
>      /* Mark so that we get notified of accesses to unwritten areas */
>      if (qemu_ram_foreach_block(postcopy_ram_sensitise_area, mis)) {
>          return -1;
>      }
>  
> +    DPRINTF("postcopy_ram_enable_notify: Sensitised");
> +
>      return 0;
>  }
>  
> @@ -612,11 +814,12 @@ int postcopy_place_page(MigrationIncomingState *mis, 
> void *host, void *from,
>  
>      if (syscall(__NR_remap_anon_pages, host, from, hps, 0) !=
>              getpagesize()) {
> +        int e = errno;
>          perror("remap_anon_pages in postcopy_place_page");
>          fprintf(stderr, "host: %p from: %p pmi=%d\n", host, from,
>                  postcopy_pmi_get_state(mis, bitmap_offset));
>  
> -        return -errno;
> +        return -e;

Unrelated change, should probably be folded into the patch which added
this code.

>      }
>  
>      tmp_state = postcopy_pmi_get_state(mis, bitmap_offset);
> @@ -629,7 +832,10 @@ int postcopy_place_page(MigrationIncomingState *mis, 
> void *host, void *from,
>  
>  
>      if (old_state == POSTCOPY_PMI_REQUESTED) {
> -        /* TODO: Notify kernel */
> +        /* Send the kernel the host address that should now be accessible */
> +        DPRINTF("%s: Notifying kernel bitmap_offset=0x%lx host=%p",
> +                __func__, bitmap_offset, host);
> +        return ack_userfault(mis, host, hps);
>      }
>  
>      return 0;

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgp6pGgW1l07M.pgp
Description: PGP signature