Le Lundi 24 Juillet 2006 20:47, Alex Williamson a écrit :
> 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)
Makes sense!
> > + /* 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?
You are right about the possible segfault: I should have written
to_send[i/sizeof(unsigned long))].
The purpose of this loop is to setup the translation cache. I will create a
function to do this and the hypercall.
> > +
> > 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...
This is not required according to ansi-c:
The free function causes the space pointed to by ptr to be deallocated, that
is, made available for further allocation. If ptr is a null pointer, no
action occurs.
> > + 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.
Sure.
> > + 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.
Definitly a style point! This was copied from x86.
> > + 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.
Ok.
> > + 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.
Ok.
> > + /* 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?
Oups, you are right :-)
Thank you for this review. I will update the patch.
Tristan.
_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel
