Re: [Xen-devel] [PATCH v2 2/3] Introduce migration precopy policy

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 21, 2017 at 12:13:55PM +0100, Wei Liu wrote:
> On Thu, Sep 21, 2017 at 12:08:04PM +0100, Roger Pau Monné wrote:
> > On Wed, Sep 20, 2017 at 05:18:16PM +0100, Jennifer Herbert wrote:
> > > On 20/09/17 11:20, Roger Pau Monné wrote:
> > > > On Tue, Sep 19, 2017 at 07:06:26PM +0100, Jennifer Herbert wrote:
> > > > > +        ? XGS_POLICY_STOP_AND_COPY
> > > > > +        : XGS_POLICY_CONTINUE_PRECOPY;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > >    * Send memory while guest is running.
> > > > >    */
> > > > >   static int send_memory_live(struct xc_sr_context *ctx)
> > > > > @@ -474,21 +491,58 @@ static int send_memory_live(struct 
> > > > > xc_sr_context *ctx)
> > > > >       xc_interface *xch = ctx->xch;
> > > > >       xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size };
> > > > >       char *progress_str = NULL;
> > > > > -    unsigned x;
> > > > > +    unsigned int x = 0;
> > > > >       int rc;
> > > > > +    int policy_decision;
> > > > > +
> > > > > +    DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
> > > > > +                                    &ctx->save.dirty_bitmap_hbuf);
> > > > > +
> > > > > +    precopy_policy_t precopy_policy = 
> > > > > ctx->save.callbacks->precopy_policy;
> > > > > +    void *data = ctx->save.callbacks->data;
> > > > > +
> > > > > +    struct precopy_stats *policy_stats;
> > > > >       rc = update_progress_string(ctx, &progress_str, 0);
> > > > >       if ( rc )
> > > > >           goto out;
> > > > > -    rc = send_all_pages(ctx);
> > > > > -    if ( rc )
> > > > > -        goto out;
> > > > > +    ctx->save.stats = (struct precopy_stats)
> > > > > +        { .dirty_count   = ctx->save.p2m_size };
> > > > This is exactly the same as 'stats' at this point. I'm slightly
> > > > confused about why you need 2 different stats variable, plus a pointer
> > > > to a stats variable (stats, ctx->save.stats and *policy_stats).
> > > 
> > > They do start off similar, and are certainly closely related.
> > > xc_shadow_op_stats_t stats has different fields in it then precopy_stats
> > > policy_stats.
> > > The former has a fault and dirty count, per iteration, while the latter 
> > > has
> > > iteration number, total_written (over all iterations) and dirty count.
> > 
> > OK. I'm not that familiar with this code, so maybe this doesn't make
> > sense, but wouldn't it be clearer to expand the xc_shadow_op_stats_t
> > type so that a single variable can contain all this information?
> > 
> > I find it slightly confusing to use two variables of the same type
> > that track different things.
> > 
> 
> The xc_shadow_op_stats_t is in fact xen_domctl_shadow_op_stats, which
> gets passed directly to the hypervisor. So I think having two separate
> structs here is okay. They are describing different things after all.

You could have one structure nested inside of the other, but I don't
have such a strong opinion, ie: this is fine.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel