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

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

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.

> *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.

Thanks, Roger.

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