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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

> > *policy_stats  is just a convenience pointer, reducing the amount of
> > indirection on
> > every access.  I though this made it easier to read.
> > 
> > > > +    policy_stats = &ctx->save.stats;
> > > > +
> > > > +    if ( precopy_policy == NULL )
> > > > +         precopy_policy = simple_precopy_policy;
> > > > +
> > > > +    bitmap_set(dirty_bitmap, ctx->save.p2m_size);
> > > > +
> > > > +    do {
> > > > +        policy_decision = precopy_policy(*policy_stats, data);
> > > The comment at the top says:
> > > 
> > > "Called after every batch of page data sent during the precopy phase"
> > > 
> > > Yet here the hook seems to be called before any processing has been
> > > done for the first iteration of the loop.
> > 
> > I'll change to "Called before and after every batch ...."
> > 
> > > > +        x++;
> > > Also updating x here seems weird, we completely ignore iteration 0.
> > 
> > The line above the 'x++' checks the policy using 'iteration 0'.  In
> > patch v1 I used the x variable in initialising the stats, to try and
> > suggest this, but as its zero, and the default value for a struct is
> > zero, it was concluded that was unnecessary.  In any case,
> > logically, this is where it moves from one 'iteration' to another.
> > Previously there was no iteration zero, as it started on zero.
> > Now iteration zero is to indicate the starting state.
> > 
> > Combining this comment with Paul's, it could use:
> >     for (x = 1; ; ++x)
> > If this is thought to be more readable - although Andrew cooper
> > described a loop looking like this as "suspicious" on Joshua's version
> > of this patch.
> > 
> > I have no strong feelings on the matter.... let me know.
> 
> I don't really have a strong opinion, I tend to use 'for ( ; ; )' for
> unbounded loops, but it's mostly a question of taste.
> 

I don't care either. Please pick the style you like. ;-)

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