Re: [Qemu-devel] [PATCH v8 06/11] migration: add postcopy blocktime ctx into MigrationIncomingState

[Alexey Perevalov]

On 06/07/2017 03:43 PM, Juan Quintela wrote:
> Alexey Perevalov <address@hidden> wrote:
> > This patch adds request to kernel space for UFFD_FEATURE_THREAD_ID,
> > in case when this feature is provided by kernel.
> I think this function is wrong

migration_exit_cb will be called at QEMU exit, but see later

> > +static void migration_exit_cb(Notifier *n, void *data)
> > +{
> > +    PostcopyBlocktimeContext *ctx = container_of(n, PostcopyBlocktimeContext,
> > +                                                 exit_notifier);
> > +    destroy_blocktime_context(ctx);
> > +}
> > +
> > +static struct PostcopyBlocktimeContext *blocktime_context_new(void)
> > +{
> > +    PostcopyBlocktimeContext *ctx = g_new0(PostcopyBlocktimeContext, 1);
> > +    ctx->page_fault_vcpu_time = g_new0(int64_t, smp_cpus);
> > +    ctx->vcpu_addr = g_new0(uint64_t, smp_cpus);
> > +    ctx->vcpu_blocktime = g_new0(int64_t, smp_cpus);
> > +
> > +    ctx->exit_notifier.notify = migration_exit_cb;
> > +    qemu_add_exit_notifier(&ctx->exit_notifier);
> > +    add_migration_state_change_notifier(&ctx->postcopy_notifier);
> Or you don't want to call it this awy.
>
> This will destroy the context at every migration state change.
>
> Or I am missing something here?  Look at ui/spice-core.c to see how to
> use it only for some states (I guess you will need to do it for
> error/cleanup/completion changes only).
I forgot, to remove

add_migration_state_change_notifier(&ctx->postcopy_notifier);

in previous version, here was callback with migration state check.

> Later, Juan.
>
> > +    return ctx;
> > +}
> >   
> >   /**
> >    * receive_ufd_features: check userfault fd features, to request only 
> > supported
> > @@ -155,6 +207,19 @@ static bool ufd_check_and_apply(int ufd, 
> > MigrationIncomingState *mis)
> >           }
> >       }
> >   
> > +#ifdef UFFD_FEATURE_THREAD_ID
> > +    if (migrate_postcopy_blocktime() && mis &&
> > +        UFFD_FEATURE_THREAD_ID & supported_features) {
> > +        /* kernel supports that feature */
> > +        /* don't create blocktime_context if it exists */
> > +        if (!mis->blocktime_ctx) {
> > +            mis->blocktime_ctx = blocktime_context_new();
> > +        }
> > +
> > +        asked_features |= UFFD_FEATURE_THREAD_ID;
> > +    }
> > +#endif
> > +
> >       /*
> >        * request features, even if asked_features is 0, due to
> >        * kernel expects UFFD_API before UFFDIO_REGISTER, per

--
Best regards,
Alexey Perevalov