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

[Peter Xu]

On Wed, Jul 26, 2017 at 06:24:11PM +0300, Alexey Perevalov wrote:
> On 07/26/2017 11:43 AM, Peter Xu wrote:
> >On Wed, Jul 26, 2017 at 11:07:17AM +0300, Alexey Perevalov wrote:
> >>On 07/26/2017 04:49 AM, Peter Xu wrote:
> >>>On Thu, Jul 20, 2017 at 09:52:34AM +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.
> >>>>
> >>>>Reviewed-by: Peter Xu <address@hidden>
> >>>>Signed-off-by: Alexey Perevalov <address@hidden>
> >>>>---
> >>>[...]
> >>>
> >>>>  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);
> >>>>+    }
> >>>>+    if (!ret) {
> >>>>+        ramblock_recv_bitmap_set(host_addr, rb);
> >>>Wait...
> >>>
> >>>Now we are using 4k-page/bit bitmap, do we need to take care of the
> >>>huge pages here?  Looks like we are only setting the first bit of it
> >>>if it is a huge page?
> >>First version was per ramblock page size, IOW bitmap was smaller in
> >>case of hugepages.
> >Yes, but this is not the first version any more. :)
> >
> >This patch is using:
> >
> >   bitmap_new(rb->max_length >> TARGET_PAGE_BITS);
> >
> >to allocate bitmap, so it is using small pages always for bitmap,
> >right? (I should not really say "4k" pages, here I think the size is
> >host page size, which is the thing returned from getpagesize()).
> >
> >>
> >>You mentioned that TARGET_PAGE_SIZE is reasonable for precopy case,
> >>in "Re: [Qemu-devel] [PATCH v1 2/2] migration: add bitmap for copied page"
> >>I though TARGET_PAGE_SIZE as transmition unit, is using in precopy even
> >>hugepage case.
> >>But it's not so logically, page being marked as dirty, should be sent as a
> >>whole page.
> >Sorry if I misunderstood, but I didn't see anything wrong - we are
> >sending pages in small pages, but when postcopy is there, we do
> >UFFDIO_COPY in huge page, so everything is fine?
> I think yes, we chose TARGET_PAGE_SIZE because of wider
> use case ranges.

So... are you going to post another version? IIUC we just need to use
a bitmap_set() to replace the ramblock_recv_bitmap_set(), while set
the size with "pagesize / TARGET_PAGE_SIZE"?

(I think I was wrong when saying getpagesize() above: the small page
 should be target page size, while the huge page should be the host's)

-- 
Peter Xu