Re: [PATCH] vchost: Notify application of ownership change

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Mon, Oct 26, 2015 at 02:54:07PM +0900, Tetsuya Mukawa wrote:
> On 2015/10/25 2:16, Thomas Monjalon wrote:
> > 2015-08-12 03:34, Xie, Huawei:
> >> On 8/8/2015 1:21 AM, Jan Kiszka wrote:
> >>> On VHOST_*_RESET_OWNER, we reinitialize the device but without telling
> >>> the application. That will cause crashes when it continues to invoke
> >>> vhost services on the device. Fix it by calling the destruction hook if
> >>> the device is still in use.
> > [...]
> >>> --- a/lib/librte_vhost/virtio-net.c
> >>> +++ b/lib/librte_vhost/virtio-net.c
> >>> @@ -402,6 +402,9 @@ reset_owner(struct vhost_device_ctx ctx)
> >>>
> >>>  	ll_dev = get_config_ll_entry(ctx);
> >>>
> >>> +	if ((ll_dev->dev.flags & VIRTIO_DEV_RUNNING))
> >>> +		notify_ops->destroy_device(&ll_dev->dev);
> >> To me this patch makes sense here.
> >> Whether RESET_OWNER is really needed is another question. Whenever the
> >> vhost itself needs to process the vhost device, we need to notify the
> >> switch application to remove it from data plane.
> > Huawei,
> > some patches have been accepted for RESET_OWNER management.
> > Is this patch obsolete?

I think it's still appliable, at least so far.

> 
> Hi Yuanhan and Huawei,
> 
> I also have the same question. Do we have a patch for this issue?
> 
> Today, I've download Yuanhan's multiple queues patches and applied it on
> latest dpdk tree.
> Then, tried to apply my vhost PMD patch on it.
> 
> When I check the patch, it seems I've faced this issue.
> Here are steps to reproduce.

Above patch should fix your issue, right? If so, we need it.

> 
> 1. Start vhost-user backend application.
>      (In my case, testpmd using vhost PMD is the application)
> 2. Start a VM with vhost-user.
>      You can see below message from the backend application.
>       VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
>       VHOST_CONFIG: set queue enable: 1 to qp idx: 0
>       (snip)
>       VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> 3. After booting Linux on guest, bind the virtio-net device to igb_uio.
>     Then below messages are shown.
>     VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
>     VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
>     VHOST_CONFIG: read message VHOST_USER_GET_VRING_BASE
> 
> The point is we will have VHOST_USER_RESET_OWNER before
> VHOST_USER_GET_VRING_BASE.

Note that there is an ongoing work at QEMU community (from me) to
handle RESET_OWNER correctly: it will be moved to somewhere else
instead of before VHOST_USER_GET_VRING_BASE.

	--yliu

> Currently, in RESET_OWNER function, all virtio-net data is initialized.
> As a result, we also initialize virtio-net flags.
> When we get GET_VRING_BASE, we cannot call destroy callback handler
> because RUNNING flag has been initialized already.
> 
>  I guess when we get RESET_OWNER message, I don't need to do anything.
> And all finalizations should be done in GET_VRING_BASE.
> (Or some finalizations might be done when next SET_MEM_TABLE is called.)
> 
> Thanks,
> Tetsuya