Re: [Qemu-devel] [PATCH v3 3/3] migration: add bitmap for received page

[Perevalov Alexey]

On Wed, Jun 21, 2017 at 09:22:07PM +0200, Juan Quintela wrote:
> Alexey Perevalov <address@hidden> 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 <address@hidden>
> 
> 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.
>