[Xen-ia64-devel] Re: PATCH: live migration

[Alex Williamson <alex.williamson@xxxxxx>]

Hi Tristan,

   Sorry for the delay, I didn't have as much spare time last week at
OLS as I was hoping.  A few comments on this patch below.  Thanks,

        Alex

On Tue, 2006-07-18 at 11:03 +0200, Tristan Gingold wrote:
> +
> +#define BITS_PER_LONG (sizeof(unsigned long) * 8)
> +#define BITMAP_SIZE   ((max_pfn + BITS_PER_LONG - 1) / 8)

   Looks like this is just borrowed from the x86 flavors, but I'm not
sure it makes sense.  It appears we're rounding BITMAP_SIZE up, but why
not round it up to an even multiple of longs?  Would something like this
work better:

(((max_pfn + (BITS_PER_LONG - 1)) & ~(BITS_PER_LONG - 1)) / 8)

> +        /* Dirtied pages won't be saved.
> +           slightly wasteful to peek the whole array evey time,
> +           but this is fast enough for the moment. */
> +        if (!last_iter) {
> +            /* FIXME!! */
> +            for (i = 0; i < BITMAP_SIZE; i += PAGE_SIZE)
> +                to_send[i] = 0;

   This zero'ing loop is repeated in several places, but it doesn't make
sense to me.  BITMAP_SIZE is in bytes, to_send is an unsigned long
pointer, and the PAGE_SIZE increment seems rather random.  Looks like it
should segfault and only very sparsely zero the bitmap array.  Am I
missing the point?

> +
>      free (page_array);
> -
> +    free (to_send);
> +    free (to_skip);


   Shouldn't we check for NULL before free'ing?

if (to_send)
    free(to_send);
etc...


> +
> +               atomic64_set (&d->arch.shadow_fault_count, 0);
> +               atomic64_set (&d->arch.shadow_dirty_count, 0);
> +
> +               d->arch.shadow_bitmap_size = (d->max_pages + 63) &
> ~63;

   63 may be an obvious value, but I prefer the (BITS_PER_LONG - 1)
usage in the userspace tools.  Magic numbers are bad.

> +
> +               if ( sc->pages > d->arch.shadow_bitmap_size )
> +                       sc->pages = d->arch.shadow_bitmap_size; 
> +
> +#define chunk (8*1024) /* Transfer and clear in 1kB chunks for L1
> cache. */

  Please move this #define out of the function and rename it to
something in all caps so it's easy to recognize as a macro.

> +               for ( i = 0; i < sc->pages; i += chunk )
> +               {
> +                       int bytes = ((((sc->pages - i) > chunk) ?
> +                                     chunk : (sc->pages - i)) + 7) /
> 8;
> +     
> +                       if ( copy_to_guest_offset(
> +                                    sc->dirty_bitmap,
> +                                    i/(8*sizeof(unsigned long)),
> +                                    d->arch.shadow_bitmap
> +(i/(8*sizeof(unsigned long))),

   BITS_PER_LONG would seem to be a useful simplification here.

> + 
> +               if ( sc->pages > d->arch.shadow_bitmap_size )
> +                       sc->pages = d->arch.shadow_bitmap_size; 
> +
> +               if ( copy_to_guest(sc->dirty_bitmap, 
> +                                  d->arch.shadow_bitmap,
> +                                  (((sc->pages+7)/8)+sizeof(unsigned
> long)-1) /
> +                                  sizeof(unsigned long)) )

   A comment might be in order for the calculations going on in this
last parameter.

>  
> +    /* Bitmap of shadow dirty bits.
> +       Set iff shadow mode is enabled.  */
> +    u64 *shadow_bitmap;
> +    /* Length (in byte) of shadow bitmap.  */
> +    unsigned long shadow_bitmap_size;

   The usage of shadow_bitmap_size seems to be in bits.  Is this comment
correct?

-- 
Alex Williamson                             HP Open Source & Linux Org.


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel