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
|
Home