Bug 1422534

It looks like we should remove the check for now. And we may want to teach virtio.c to get the last_avail_idx from vhost during migration. Or to disable the check on vhost (I wonder if this is reproducible with vhost-kernel?). Stefan, any other idea?

Can you try reproducing it with the PVP test setup by Maxime?
http://dpdk.org/doc/guides/howto/pvp_reference_benchmark.html?highlight=pvp

This can help since it uses DPDK only (no need for switch like VPP).

(In reply to Marc-Andre Lureau from comment #3)
> It looks like we should remove the check for now. And we may want to teach
> virtio.c to get the last_avail_idx from vhost during migration. Or to
> disable the check on vhost (I wonder if this is reproducible with
> vhost-kernel?). Stefan, any other idea?

Could it be not a bug of qemu? We should have synced last_avail_idx (GET_VRING_BASE) during device stop on src.

(In reply to Amnon Ilan from comment #4)
> Can you try reproducing it with the PVP test setup by Maxime?
> http://dpdk.org/doc/guides/howto/pvp_reference_benchmark.html?highlight=pvp
> 
> This can help since it uses DPDK only (no need for switch like VPP).

Hello Peter ,

Can you confirm the test results using the guide provided by engineering ?

Regards,
Jaison R

Here is the response which we got from Cu. on support case.

This issue exists with PVP too, Ideally we need this fixed with FDIO VPP.

(In reply to jason wang from comment #5)
> (In reply to Marc-Andre Lureau from comment #3)
> > It looks like we should remove the check for now. And we may want to teach
> > virtio.c to get the last_avail_idx from vhost during migration. Or to
> > disable the check on vhost (I wonder if this is reproducible with
> > vhost-kernel?). Stefan, any other idea?
> 
> Could it be not a bug of qemu? We should have synced last_avail_idx
> (GET_VRING_BASE) during device stop on src.

Yes, the vring state needs to be synced.  This is how vhost kernel and dataplane work, too.

Pei, can you please check with "--live" option are you able to reproduce the issue or not?

~~~
Please note, the problem is migrating live OS values,
Which means you need to perform live migration:
virsh migrate --live
~~~

(In reply to VIKRANT from comment #15)
> Pei, can you please check with "--live" option are you able to reproduce the
> issue or not?
> 
> ~~~
> Please note, the problem is migrating live OS values,
> Which means you need to perform live migration:
> virsh migrate --live
> ~~~

Hi Vikrant, still works well, can not reproduce.

[root@dell-per430-10 ~]# virsh migrate --verbose --persistent --live rhel7.3 qemu+ssh://10.73.73.67/system 
Migration: [100 %]

[root@dell-per430-11 ~]# virsh migrate --verbose --persistent --live rhel7.3 qemu+ssh://10.73.73.65/system
Migration: [100 %]

[root@dell-per430-10 ~]# virsh migrate --verbose --undefinesource --live rhel7.3 qemu+ssh://10.73.73.67/system 
Migration: [100 %]

[root@dell-per430-11 ~]# virsh migrate --verbose --undefinesource --live rhel7.3 qemu+ssh://10.73.73.65/system
Migration: [100 %]

Thanks,
Pei

Hi Pei,

Can you please share the output of  following command.

# virsh domiflist rhel7.3

(In reply to VIKRANT from comment #17)
> Hi Pei,
> 
> Can you please share the output of  following command.
> 
> # virsh domiflist rhel7.3

Vikrant, please see below:

# virsh domiflist rhel7.3
Interface  Type       Source     Model       MAC
-------------------------------------------------------
vnet0      bridge     switch     virtio      fa:5e:07:b4:08:91
-          vhostuser  -          virtio      fa:5e:07:b4:08:92
-          vhostuser  -          virtio      fa:5e:07:b4:08:93

XML info:
# cat rhel7.3.xml 
<domain type='kvm'>
  <name>rhel7.3</name>
  <uuid>c52b047f-68a3-4a0d-b34f-e8596977064e</uuid>
  <memory unit='KiB'>8388608</memory>
  <currentMemory unit='KiB'>8388608</currentMemory>
  <memoryBacking>
    <hugepages>
      <page size='1048576' unit='KiB' nodeset='0'/>
    </hugepages>
    <locked/>
  </memoryBacking>
  <vcpu placement='static'>4</vcpu>
  <cputune>
    <vcpupin vcpu='0' cpuset='12'/>
    <vcpupin vcpu='1' cpuset='14'/>
    <vcpupin vcpu='2' cpuset='16'/>
    <vcpupin vcpu='3' cpuset='18'/>
    <emulatorpin cpuset='1,3,5,7,9'/>
  </cputune>
  <numatune>
    <memory mode='strict' nodeset='0'/>
  </numatune>
  <os>
    <type arch='x86_64' machine='pc-i440fx-rhel7.0.0'>hvm</type>
    <boot dev='hd'/>
  </os>
  <cpu mode='host-passthrough'>
    <feature policy='require' name='tsc-deadline'/>
    <numa>
      <cell id='0' cpus='0-3' memory='8388608' unit='KiB' memAccess='shared'/>
    </numa>
  </cpu>
  <devices>
    <emulator>/usr/libexec/qemu-kvm</emulator>
<disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none' io='threads'/>
      <source file='/mnt/nfv/rhel7.3.qcow2'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
    </disk>
    <controller type='usb' index='0' model='none'/>
    <controller type='pci' index='0' model='pci-root'/>

    <interface type='bridge'>
      <mac address='fa:5e:07:b4:08:91'/>
      <source bridge='switch'/>
      <model type='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
    </interface>
    <interface type='vhostuser'>
      <mac address='fa:5e:07:b4:08:92'/>
      <source type='unix' path='/tmp/vhost-user1' mode='client'/>
      <model type='virtio'/>
      <driver name='vhost' rx_queue_size='256' />
      <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/>
    </interface>
    <interface type='vhostuser'>
      <mac address='fa:5e:07:b4:08:93'/>
      <source type='unix' path='/tmp/vhost-user2' mode='client'/>
      <model type='virtio'/>
      <driver name='vhost' rx_queue_size='256' />
      <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/>
    </interface>
    <memballoon model='virtio'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </memballoon>
    <input type='mouse' bus='ps2'/>
    <input type='keyboard' bus='ps2'/>
  </devices>
</domain>



Best Regards,
Pei

It seems we have two things that we need to move forward on.

Now, you could add the 2 host VPP setup to do your own repro or jump on our TB to see the problem themselves. As discussed in the very first comment on this bug, it's the way that FD.io's vhostuser driver works that triggers the ringbuffer handling problem in QEMU, so if you haven't been using the FD.io vhostuser driver you probably haven't triggered the exact issue.

Your engineers immediately seem to have seen the problem (with ringbuffer indexes) and suggest that it is an obvious problem with QEMU and a straightforward fix.  Can you please share your fix for that, and we can run it in our testbed and report back?

Can you suggest how to move forward on these two points?  As you can see, we can do either of them independently.  We would like an urgent resolution to this issue so let's progress both.

As the problem is not reproducible with DPDK or OVS-DPDK on Pei setup, I had a look at VPP's vhost-user implementation.

I found an issue in VPP code, that would explain faced behaviour.
As mentioned by Jason in Comment 5, Qemu requests last_avail_idx to vhost-user backend using GET_VRING_BASE.

In VPP, this is implemented as follow:

static clib_error_t *
vhost_user_socket_read (unix_file_t * uf)
{
...
  switch (msg.request)
    {
...
    case VHOST_USER_GET_VRING_BASE:
      DBG_SOCK ("if %d msg VHOST_USER_GET_VRING_BASE idx %d num %d",
		vui->hw_if_index, msg.state.index, msg.state.num);

      if (msg.state.index >= VHOST_VRING_MAX_N)
	{
	  DBG_SOCK ("invalid vring index VHOST_USER_GET_VRING_BASE:"
		    " %d >= %d", msg.state.index, VHOST_VRING_MAX_N);
	  goto close_socket;
	}

      /* Spec says: Client must [...] stop ring upon receiving VHOST_USER_GET_VRING_BASE. */
      vhost_user_vring_close (vui, msg.state.index);

      msg.state.num = vui->vrings[msg.state.index].last_avail_idx;
      msg.flags |= 4;
      msg.size = sizeof (msg.state);
      break;
...
}

As the comment says, the vq must be stopped before getting the index,
so VPP calls vhost_user_vring_close()

The problem is that this vhost_user_vring_close() function calls vhost_user_vring_init(),
which resets the vhost_user_vring_t struct to zeros.

Since vhost_user_vring_t stores the index, the index gets cleared before being read.

Solution I think is to close the device without re-initializing it.

Maxime,
Could you add include the commit or version of VPP?

It looks like you have access to the source above. Try typing from the top vpp directory
build-root/scripts/version

That worked back to the 16.06 release.

Tom,

I had a look at mainline VPP:
$ build-root/scripts/version
17.04-rc0~382-ga0b34a7

Looking at 16.06 release, it seems to be a regression, as the vring struct is not reset before getting last_avail_idx, only the enable bit is cleared:

    case VHOST_USER_GET_VRING_BASE:
      DBG_SOCK("if %d msg VHOST_USER_GET_VRING_BASE idx %d num %d",
        vui->hw_if_index, msg.state.index, msg.state.num);

      /* Spec says: Client must [...] stop ring upon receiving VHOST_USER_GET_VRING_BASE. */
      vui->vrings[msg.state.index].enabled = 0;

      msg.state.num = vui->vrings[msg.state.index].last_avail_idx;
      msg.flags |= 4;
      msg.size = sizeof(msg.state);
      break;

The regression seems to have been introduced by below commit, which landed into v17.01 release:

$  git blame src/vnet/devices/virtio/vhost-user.c -L 1031
e21c5286 vnet/vnet/devices/virtio/vhost-user.c (Pierre Pfister     2016-09-21 08:04:59 +0100 1031)       vhost_user_vring_close (vui, msg.state.index);

$ git show e21c5286
commit e21c52861d7c503bef7fc464f23bbc7891e150d7
Author: Pierre Pfister <ppfister>
Date:   Wed Sep 21 08:04:59 2016 +0100

    vhost-user: multiqueue support
    
    This patch adds multi-queue support to non-dpdk's vhost-user
    driver.
    Waiting for a unified way to manage threads, this patch
    defines a way to assign threads to interfaces that is
    specific to vhost.
    
    Change-Id: I86298788b1a4e886c5431f187dc17175d12c7a8b
    Signed-off-by: Pierre Pfister <ppfister>

$ git tag --contains e21c5286
v17.01
v17.01-rc1
v17.01-rc2
v17.01.1
v17.04-rc0
We should invite Pierre Pfister from Cisco in the discussion,
as he might have some ideas on the fix to do.

I guess that reverting to just clearing the enabled bit is not the right fix.
Maybe the vring struct shouldn't be re-initialized in the close function,
but should be called directly when needed.

I'm attaching a debug patch to confirm the root cause if someone want to have a try.
It is not intended to be the fix for this issue, and has not neither been compiled nor tested.

Regards,
Maxime

Created attachment 1261140 [details]
VPP: Debug patch to avoid vring re-init before saving its last_avail_idx value

I've checked the suggested patch and it fixes the migration problem

Thanks for testing Peter.
I invited Pierre Pfister to comment on this ticket,
he's the best placed to provide the final fix.

Regards,
Maxime

As per confirmation from Peter. We are good to close this bug.

Once again many thanks guys for quick inputs on this bug.

VPP fix was done in:
http://gerrit.fd.io/r/#/c/5700

Peter, Steven,

Not sure about the fix.
Shouldn't the vring be stopped before saving the last_avail_idx value?

Regards,
Maxime

Hi Maxime,

The whole logic of 'vhost_user_socket_read' function is kept between 'vlib_worker_thread_barrier_sync' and 'vlib_worker_thread_barrier_release', which implements holding all worker threads, so no packets will be processed at that time.
So this simpler fix is fine.

Cheers,
Peter

Ok Peter, thanks for the explanation. You can forget my comment.

I don't know much about VPP internals, but wanted to be sure it was taken into account.

Regards,
Maxime

And just one general comment:
Moving forward, we should see how to convert VPP to use the 
vhost-user from DPDK library.

Yes, that was our intention in the VPP community before 1611 release. Its time for re-syncing on the issues and the LOE for use of dpdk vhost-user moving forward.