Re: [Qemu-devel] [PATCH v8 33/54] postcopy: Incoming initialisation

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> Reviewed-by: David Gibson <address@hidden>
> Reviewed-by: Amit Shah <address@hidden>

> +/*
> + * At the end of migration, undo the effects of init_range
> + * opaque should be the MIS.
> + */
> +static int cleanup_range(const char *block_name, void *host_addr,
> +                        ram_addr_t offset, ram_addr_t length, void *opaque)
> +{
> +    MigrationIncomingState *mis = opaque;
> +    struct uffdio_range range_struct;
> +    trace_postcopy_cleanup_range(block_name, host_addr, offset, length);
> +
> +    /*
> +     * We turned off hugepage for the precopy stage with postcopy enabled
> +     * we can turn it back on now.
> +     */
> +#ifdef MADV_HUGEPAGE
> +    if (madvise(host_addr, length, MADV_HUGEPAGE)) {
> +        error_report("%s HUGEPAGE: %s", __func__, strerror(errno));
> +        return -1;
> +    }
> +#endif

this should be the same than:

       qemu_madvise(host_addr, lenght, QEMU_MADV_HUGEPAGE);

Only problem I can see, is that there is no way to differentiate that
madvise() has given one error or that MADV_HUGEPAGE is not defined.

If we really want that:

   if (QEMU_MADV_HUGEPAGE != QEM_MADV_INVALID) {
      if (qemu_madvise(host_addr, length, QEMU_MADV_HUGEPAGE)) {
        error_report("%s HUGEPAGE: %s", __func__, strerror(errno));
        return -1;
   }

But I am not sure if we want it.


> +
> +    /*
> +     * We can also turn off userfault now since we should have all the
> +     * pages.   It can be useful to leave it on to debug postcopy
> +     * if you're not sure it's always getting every page.
> +     */
> +    range_struct.start = (uintptr_t)host_addr;
> +    range_struct.len = length;
> +
> +    if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
> +        error_report("%s: userfault unregister %s", __func__, 
> strerror(errno));
> +
> +        return -1;
> +    }
> +
> +    return 0;
> +}


I still think that exposing the userfault API all around is a bad idea,
that it would be easier to just export:

qemu_userfault_register_range(addr, lenght);
qemu_userfault_unregister_range(addr, lenght);

And hide the details on a header file.

Later, Juan.