Re: [Qemu-devel] [PATCH v8 00/27] vhost-user: add migration support

[Thibaut Collet]

On Mon, Oct 19, 2015 at 5:39 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Mon, Oct 19, 2015 at 03:22:12PM +0200, Thibaut Collet wrote:
>> On Sun, Oct 18, 2015 at 10:21 AM, Michael S. Tsirkin <address@hidden> wrote:
>> >
>> > On Tue, Oct 13, 2015 at 02:19:41PM +0200, Thibaut Collet wrote:
>> > > Hi,
>> > >
>> > > I have still a comment on this serie. During rebase operation with 
>> > > multiqueue a
>> > > modification has been lost.
>> > > This lost impact only guest without GUEST_ANNOUNCE capabilities: the 
>> > > backend is
>> > > not notified to send a fake RARP to reduce VM outage.
>> > >
>> > > Sorry for the late notice but I have tested only recent guest to give my 
>> > > ack
>> > > yesterday.
>> > >
>> > > Marc Andre and Michael could you apply the attached fixup to the patch 
>> > > "vhost
>> > > user: add support of live migration" on the pull request ?
>> > >
>> > > Thanks
>> > >
>> > > Best regards.
>> >
>> > The way to post fixups is just like a regular patch, but prepend
>> > fixup! on the subject line.
>> > Comments can come after the --- line.
>> >
>> >
>> >
>> > > On Mon, Oct 12, 2015 at 5:56 PM, Thibaut Collet <address@hidden>
>> > > wrote:
>> > >
>> > >
>> > >
>> > >     On Fri, Oct 9, 2015 at 5:17 PM, <address@hidden> wrote:
>> > >
>> > >         From: Marc-André Lureau <address@hidden>
>> > >
>> > >         Hi,
>> > >
>> > >         The following series implement shareable log for vhost-user to 
>> > > support
>> > >         memory tracking during live migration. On qemu-side, the 
>> > > solution is
>> > >         fairly straightfoward since vhost already supports the dirty 
>> > > log, only
>> > >         vhost-user couldn't access the log memory until then.
>> > >
>> > >         The series includes "vhost user: Add live migration" patches from
>> > >         Thibaut Collet.
>> > >
>> > >         v7->v8:
>> > >         - rebased
>> > >         - fix build on osx & aarch64
>> > >         - add seccomp patch from Eduardo
>> > >         - fix enum usage and MQ (squashed Thibaut fix)
>> > >         - fixed vhost_net_notify_migration_done() fallback
>> > >         - split util-obj- on multi-lines in seperate patch
>> > >
>> > >         v6->v7:
>> > >         - add migration blocker if memfd failed
>> > >         - add doc about the qemu_memfd_alloc() helper
>> > >         - (rebase on top of Michael pci branch)
>> > >
>> > >         v5->v6:
>> > >         - rebased
>> > >         - fix protocol feature mask update
>> > >         - add a patch from Thibault Collet using enum for features, and
>> > >           compute mask
>> > >         - add unistd.h linux headers to help building memfd if missing on
>> > >           build host. Hopefully will be useful for other syscalls some 
>> > > day
>> > >         - reorder/merge patches to share-allocate the log only if needed
>> > >         - split the memfd helper to allow to drop the fallback code
>> > >         - allow to call qemu_memfd_free() even if alloc failed
>> > >         - add some missing spaces
>> > >
>> > >         v4->v5:
>> > >         - rebase on top of last Michael S. Tsirkin PULL request
>> > >         - block live migration if !PROTOCOL_F_LOG_SHMFD
>> > >         - wait for a reply after SET_LOG_BASE
>> > >         - split vhost_set_log_base from the rest of vhost_call 
>> > > refactoring
>> > >         - use a seperate global vhost_log_shm
>> > >
>> > >         v3->v4:
>> > >         - add the proto negotiation & the migration series
>> > >         - replace the varargs vhost_call() approach for callbacks
>> > >         - only share-allocate when the backend needs it
>> > >
>> > >         v2->v3:
>> > >         - changed some patch summary
>> > >         - added migration tests
>> > >         - added a patch to replace error message with a trace
>> > >
>> > >         The development branch I used is:
>> > >         https://github.com/elmarco/qemu branch "vhost-user"
>> > >
>> > >         Eduardo Otubo (1):
>> > >           seccomp: add memfd_create to whitelist
>> > >
>> > >         Marc-André Lureau (22):
>> > >           configure: probe for memfd
>> > >           linux-headers: add unistd.h
>> > >           build-sys: split util-obj- on multi-lines
>> > >           util: add linux-only memfd fallback
>> > >           util: add memfd helpers
>> > >           util: add fallback for qemu_memfd_alloc()
>> > >           vhost: document log resizing
>> > >           vhost: add vhost_set_log_base op
>> > >           vhost-user: add vhost_user_requires_shm_log()
>> > >           vhost: alloc shareable log
>> > >           vhost-user: send log shm fd along with log_base
>> > >           vhost-user: add a migration blocker
>> > >           vhost: use a function for each call
>> > >           vhost-user: document migration log
>> > >           net: add trace_vhost_user_event
>> > >           vhost: add migration block if memfd failed
>> > >           vhost-user-test: move wait_for_fds() out
>> > >           vhost-user-test: remove useless static check
>> > >           vhost-user-test: wrap server in TestServer struct
>> > >           vhost-user-test: learn to tweak various qemu arguments
>> > >           vhost-user-test: add live-migration test
>> > >           vhost-user-test: check ownership during migration
>> > >
>> > >         Michael S. Tsirkin (1):
>> > >           exec: factor out duplicate mmap code
>> > >
>> > >         Thibaut Collet (3):
>> > >           vhost user: add support of live migration
>> > >           vhost user: add rarp sending after live migration for legacy 
>> > > guest
>> > >           vhost-user: use an enum helper for features mask
>> > >
>> > >          configure                          |   19 +
>> > >          docs/specs/vhost-user.txt          |   63 ++-
>> > >          exec.c                             |   47 +-
>> > >          hw/net/vhost_net.c                 |   35 +-
>> > >          hw/scsi/vhost-scsi.c               |    7 +-
>> > >          hw/virtio/vhost-backend.c          |  121 +++-
>> > >          hw/virtio/vhost-user.c             |  576 ++++++++++++-------
>> > >          hw/virtio/vhost.c                  |  121 ++--
>> > >          include/hw/virtio/vhost-backend.h  |   77 ++-
>> > >          include/hw/virtio/vhost.h          |   15 +-
>> > >          include/net/vhost_net.h            |    1 +
>> > >          include/qemu/memfd.h               |   26 +
>> > >          include/qemu/mmap-alloc.h          |   10 +
>> > >          linux-headers/asm-arm/unistd.h     |  448 +++++++++++++++
>> > >          linux-headers/asm-arm64/kvm.h      |   37 +-
>> > >          linux-headers/asm-arm64/unistd.h   |   16 +
>> > >          linux-headers/asm-mips/unistd.h    | 1063
>> > >         ++++++++++++++++++++++++++++++++++++
>> > >          linux-headers/asm-powerpc/unistd.h |  392 +++++++++++++
>> > >          linux-headers/asm-s390/unistd.h    |  404 ++++++++++++++
>> > >          linux-headers/asm-x86/unistd.h     |   15 +
>> > >          linux-headers/asm-x86/unistd_32.h  |  377 +++++++++++++
>> > >          linux-headers/asm-x86/unistd_64.h  |  330 +++++++++++
>> > >          linux-headers/asm-x86/unistd_x32.h |  319 +++++++++++
>> > >          net/vhost-user.c                   |   34 +-
>> > >          qemu-seccomp.c                     |    3 +-
>> > >          scripts/update-linux-headers.sh    |    7 +-
>> > >          tests/vhost-user-test.c            |  372 +++++++++++--
>> > >          trace-events                       |    3 +
>> > >          util/Makefile.objs                 |   13 +-
>> > >          util/memfd.c                       |  162 ++++++
>> > >          util/mmap-alloc.c                  |   71 +++
>> > >          util/oslib-posix.c                 |   28 +-
>> > >          32 files changed, 4814 insertions(+), 398 deletions(-)
>> > >          create mode 100644 include/qemu/memfd.h
>> > >          create mode 100644 include/qemu/mmap-alloc.h
>> > >          create mode 100644 linux-headers/asm-arm/unistd.h
>> > >          create mode 100644 linux-headers/asm-arm64/unistd.h
>> > >          create mode 100644 linux-headers/asm-mips/unistd.h
>> > >          create mode 100644 linux-headers/asm-powerpc/unistd.h
>> > >          create mode 100644 linux-headers/asm-s390/unistd.h
>> > >          create mode 100644 linux-headers/asm-x86/unistd.h
>> > >          create mode 100644 linux-headers/asm-x86/unistd_32.h
>> > >          create mode 100644 linux-headers/asm-x86/unistd_64.h
>> > >          create mode 100644 linux-headers/asm-x86/unistd_x32.h
>> > >          create mode 100644 util/memfd.c
>> > >          create mode 100644 util/mmap-alloc.c
>> > >
>> > >         --
>> > >         2.4.3
>> > >
>> > >
>> > >
>> > >     Acked-by: Thibaut Collet <address@hidden>
>> > >
>> > >
>> > >
>> > >
>> >
>> > > From bd39e9efb7034077f5516debd8b504ffcc10c780 Mon Sep 17 00:00:00 2001
>> > > From: Thibaut Collet <address@hidden>
>> > > Date: Tue, 13 Oct 2015 14:01:44 +0200
>> > > Subject: [PATCH] FIXUP: vhost user: add support of live migration
>> > >
>> > > Removal of receive disable has been lost during rebase with multiqueue 
>> > > feature.
>> > > Without this fixup migration of old guest (without GUEST_ANNOUNCE) is not
>> > > notified as the RARP request is not sent to the vhost user backend.
>> > >
>> > > Signed-off-by: Thibaut Collet <address@hidden>
>> >
>> > I picked this up, but I had to do a lot of tweaking to even
>> > get the patch series to apply.
>> >
>> > If you want all this to go upstream, please test it here:
>> > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git refs/tags/for_thibaut
>> >
>> > And send a tested-by tag on list.
>> >
>>
>> Thanks for your work.
>> I have just finished to test migration with your branch with the
>> following conditions:
>>  - Ubuntu14, multiqueue not set, VM with GUEST_ANNOUNCE feature
>>  - Ubuntu14, multiqueue not set, VM without GUEST_ANNOUNCE feature
>>  - Ubuntu14, multiqueue set (8 queue pairs), VM with GUEST_ANNOUNCE feature
>>  - Ubuntu14, multiqueue set (8 queue pairs), VM without GUEST_ANNOUNCE 
>> feature
>>
>> The VM is just pinged during the migration to have small traffic.
>>
>> After my test I have two remarks:
>> 1. The branch does not compile due to a rebase issue (for commit
>> ed2ae0bee5). Please could you apply the attached fixup on your branch
>> to solve it (I hope the fixup is correctly written) ?
>> 2. Migration test with multiqueue set has an incorrect behaviour. I
>> have just send a patch on the qemu mailing list to solve it ([PATCH
>> 0/1] vhost-user: support of live migration with multiqueue and [PATCH
>> 1/1] vhost: set the correct queue index in case of migration with
>> multiqueue)
>>
>> With the attached fixup and "vhost: set the correct queue index in
>> case of migration with multiqueue" patch the migration works in the 4
>> described cases.
>> If you agree and integrate the fixup and the patch on your branch I
>> can give you my tested-by
>>
>> Best regards.
>>
>> Thibaut.
>
> I don't see any fixup - is this the part below?
> Please post patches in the regular way.
>

Sorry the fixup was not attached (and it is different from the first one)
I have sent the fixup by this way as the issue appears only on your
tree on the branch for_thibaut. This fixup is mainly just for you to
update the for_thibaut branch.
With the rebase of a patch from live migration series the function
vhost_kernel_get_vq_index appears twice in the file
hw/virtio/vhost-backend.c

> I'm not sure which commit were you testing.
> My tree does not have the lines you removed and it does build.

I am testing the head of the for_thibaut branch

>
> Can you pls check refs/heads/for_thibaut?
> It should have your patch as the latest commit.

I do not see yet my patch on the for_thibaut branch

>
>
>> >
>> > > ---
>> > >  net/vhost-user.c | 2 --
>> > >  1 file changed, 2 deletions(-)
>> > >
>> > > diff --git a/net/vhost-user.c b/net/vhost-user.c
>> > > index cfe11b8..17b5c2a 100644
>> > > --- a/net/vhost-user.c
>> > > +++ b/net/vhost-user.c
>> > > @@ -212,8 +212,6 @@ static int net_vhost_user_init(NetClientState *peer, 
>> > > const char *device,
>> > >          snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to 
>> > > %s",
>> > >                   i, chr->label);
>> > >
>> > > -        /* We don't provide a receive callback */
>> > > -        nc->receive_disabled = 1;
>> > >          nc->queue_index = i;
>> > >
>> > >          s = DO_UPCAST(VhostUserState, nc, nc);
>> > > --
>> > > 2.1.4
>> > >
>> >

0001-fixup-vhost-use-a-function-for-each-call.patch
Description: Text Data