On Wed, Jun 21, 2017 at 09:22:07PM +0200, Juan Quintela wrote:
> Alexey Perevalov <a.pereva...@samsung.com> wrote:
> > This patch adds ability to track down already received
> > pages, it's necessary for calculation vCPU block time in
> > postcopy migration feature, maybe for restore after
> > postcopy migration failure.
> > Also it's necessary to solve shared memory issue in
> > postcopy livemigration. Information about received pages
> > will be transferred to the software virtual bridge
> > (e.g. OVS-VSWITCHD), to avoid fallocate (unmap) for
> > already received pages. fallocate syscall is required for
> > remmaped shared memory, due to remmaping itself blocks
> > ioctl(UFFDIO_COPY, ioctl in this case will end with EEXIT
> > error (struct page is exists after remmap).
> >
> > Bitmap is placed into RAMBlock as another postcopy/precopy
> > related bitmaps.
> >
> > Signed-off-by: Alexey Perevalov <a.pereva...@samsung.com>
>
> Hi
>
> A few commets.
>
> Happy with all the changes that you did.
>
> > @@ -60,6 +62,7 @@ static inline void *ramblock_ptr(RAMBlock *block,
> > ram_addr_t offset)
> > return (char *)block->host + offset;
> > }
> >
> > +unsigned long int ramblock_recv_bitmap_offset(void *host_addr, RAMBlock
> > *rb);
> > long qemu_getrampagesize(void);
> > unsigned long last_ram_page(void);
> > RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>
> I think we shouldn't export it at all. But if we do, we should export
> it with the other functions.
>
>
> > +unsigned long int ramblock_recv_bitmap_offset(void *host_addr, RAMBlock
> > *rb)
>
> I think this function can be made static, as nothing else should use it, no?
Peter asked me to export it in "Re: [Qemu-devel] [PATCH v2 3/3]
migration: add bitmap for received page",
looks like he needs it, but he suggested static inline,
ok, it will be static inline
>
> > +{
> > + uint64_t host_addr_offset = (uint64_t)(uintptr_t)(host_addr
> > + - (void *)rb->host);
>
> Intdentation is weird:
>
> uint64_t host_addr_offset =
> (uint64_t)(uintptr_t)(host_addr - (void *)rb->host);
>
>
> > + return host_addr_offset >> TARGET_PAGE_BITS;
> > +}
> > +
> > +int ramblock_recv_bitmap_test(void *host_addr, RAMBlock *rb)
> > +{
> > + return test_bit(ramblock_recv_bitmap_offset(host_addr, rb),
> > + rb->receivedmap);
> > +}
> > +
> > +void ramblock_recv_bitmap_set(void *host_addr, RAMBlock *rb)
> > +{
> > + set_bit_atomic(ramblock_recv_bitmap_offset(host_addr, rb),
> > rb->receivedmap);
>
> Line is too long?
It shouldn't, I checked patches with checkscript, that line is 80
characters long.
>
> > +static void ramblock_recv_bitmap_clear_range(uint64_t start, size_t length,
> > + RAMBlock *rb)
> > +{
> > + int i, range_count;
> > + range_count = length >> TARGET_PAGE_BITS;
> > + for (i = 0; i < range_count; i++) {
> > + ramblock_recv_bitmap_clear((void *)((uint64_t)(intptr_t)rb->host +
> > + start), rb);
> > + start += TARGET_PAGE_SIZE;
>
> bitmap_clear(dst, pos, nbits) Clear specified bit area
>
> Looks like the right operation here, no?
yes, we already have an offset here.
>
> > +
> > +void ramblock_recv_map_init(void);
> > +int ramblock_recv_bitmap_test(void *host_addr, RAMBlock *rb);
> > +void ramblock_recv_bitmap_set(void *host_addr, RAMBlock *rb);
> > +void ramblock_recv_bitmap_clear(void *host_addr, RAMBlock *rb);
> > +
> > #endif
>
> Later, Juan.
>
