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

[Peter Xu]

On Mon, Jun 26, 2017 at 11:35:20AM +0300, Alexey Perevalov 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>

Mostly good to me, some minor nits only...

[...]

>  static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
> -        void *from_addr, uint64_t pagesize)
> +                               void *from_addr, uint64_t pagesize, RAMBlock 
> *rb)
>  {
> +    int ret;
>      if (from_addr) {
>          struct uffdio_copy copy_struct;
>          copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
>          copy_struct.src = (uint64_t)(uintptr_t)from_addr;
>          copy_struct.len = pagesize;
>          copy_struct.mode = 0;
> -        return ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
> +        ret = ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
>      } else {
>          struct uffdio_zeropage zero_struct;
>          zero_struct.range.start = (uint64_t)(uintptr_t)host_addr;
>          zero_struct.range.len = pagesize;
>          zero_struct.mode = 0;
> -        return ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
> +        ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
> +    }
> +    /* received page isn't feature of blocktime calculation,
> +     * it's more general entity, so keep it here,
> +     * but gup betwean two following operation could be high,
> +     * and in this case blocktime for such small interval will be lost */

I would drop this comment for this patch. It didn't help me to be
clearer on the code but a bit more messy... Maybe it suites for some
place in the blocktime series? Not sure.

[...]

> +void ramblock_recv_map_init(void)
> +{
> +    RAMBlock *rb;
> +
> +    RAMBLOCK_FOREACH(rb) {
> +        unsigned long pages;
> +        pages = rb->max_length >> TARGET_PAGE_BITS;
> +        assert(!rb->receivedmap);
> +        rb->receivedmap = bitmap_new(pages);

I'll prefer removing pages variable since used only once.

[...]

> +static void ramblock_recv_bitmap_clear_range(uint64_t start, size_t length,
> +                                             RAMBlock *rb)
> +{
> +    int i, range_count;
> +    long nr_bit = start >> TARGET_PAGE_BITS;
> +    range_count = length >> TARGET_PAGE_BITS;
> +    for (i = 0; i < range_count; i++) {
> +        clear_bit(nr_bit, rb->receivedmap);
> +        nr_bit += 1;

(Dave commented this one)

[...]

> @@ -2513,6 +2560,7 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>          ram_addr_t addr, total_ram_bytes;
>          void *host = NULL;
>          uint8_t ch;
> +        RAMBlock *rb = NULL;
>  
>          addr = qemu_get_be64(f);
>          flags = addr & ~TARGET_PAGE_MASK;
> @@ -2520,15 +2568,15 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  
>          if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
> -            RAMBlock *block = ram_block_from_stream(f, flags);
> +            rb = ram_block_from_stream(f, flags);
>  
> -            host = host_from_ram_block_offset(block, addr);
> +            host = host_from_ram_block_offset(rb, addr);
>              if (!host) {
>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>                  ret = -EINVAL;
>                  break;
>              }

IMHO it's ok to set the bit once here.  Thanks,

> -            trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
> +            trace_ram_load_loop(rb->idstr, (uint64_t)addr, flags, host);
>          }
>  
>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> @@ -2582,10 +2630,12 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  
>          case RAM_SAVE_FLAG_ZERO:
>              ch = qemu_get_byte(f);
> +            ramblock_recv_bitmap_set(host, rb);
>              ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
>              break;
>  
>          case RAM_SAVE_FLAG_PAGE:
> +            ramblock_recv_bitmap_set(host, rb);
>              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
>              break;
>  
> @@ -2596,10 +2646,13 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>                  ret = -EINVAL;
>                  break;
>              }
> +
> +            ramblock_recv_bitmap_set(host, rb);
>              decompress_data_with_multi_threads(f, host, len);
>              break;
>  
>          case RAM_SAVE_FLAG_XBZRLE:
> +            ramblock_recv_bitmap_set(host, rb);
>              if (load_xbzrle(f, addr, host) < 0) {
>                  error_report("Failed to decompress XBZRLE page at "
>                               RAM_ADDR_FMT, addr);

-- 
Peter Xu