On Tue, Jun 27, 2017 at 12:03:10PM +0800, Peter Xu wrote:
> 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 <a.pereva...@samsung.com>
>
> 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, ©_struct);
> > + ret = ioctl(userfault_fd, UFFDIO_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.
yes, here could be a problem in stats, so it worth to mention about
it in stats series.
>
> [...]
>
> > +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.
no problem )
>
> [...]
>
> > +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)
I agree with Dave, looks like I invented bitmap_clear ;)
>
> [...]
>
> > @@ -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,
yes, this is common place for all copying operations here.
>
> > - 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
>
--
BR
Alexey
