Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Michael S. Tsirkin]

On Wed, Nov 11, 2015 at 09:24:36PM +0800, Yuanhan Liu wrote:
> 
> Patch 1 rename RESET_DEVICE back to RESET_OWNER
> 
> Patch 2 introduced a new function: vhost_net_reset(), which is invoked
>         when reset happens, say, driver is unloaded (unbinded).
> 
>         Michael suggested to do that only when MQ is not negotiated.
>         However, reset happens no matter MQ is enabled or negotiated
>         or not, and we should give a sign to backend to reset some
>         device to a proper state after it happens.

I don't think it's needed: we set all state at start anyway.

>         Note that the message sent is still RESET_OWNER. It might not
>         be a good idea, but we could not simply rename it to RESET_DEVICE,
>         and maybe adding another RESET_DEVICE might be better.

So modern clients don't need this at all.  Old clients need something to
stop device, but singling out reset is not a good idea: even if driver
is unloaded, you need to do that.  snabbswitch (ab)uses RESET_OWNER for
this, so maybe RESET_OWNER should be renamed DISABLE_ALL, to just make
it stop all queues.

Does any dpdk version that was released respond to RESET_OWNER in some
way? How exactly?


> Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
>         SET_FEATURES.
> 
> Patch 5 send SET_VRING_ENABLE at start/stop
> 
>         Michael, I intended to send it when MQ is negotiated as you suggested,
>         however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in 
> vhost-user.c,
>         which is not accessible to vhost.c.
> 
>         Exporting it to vhost.h will resolve that, however, it's not a good
>         idea to move vhost user specific stuff to there. We could also 
> introduce
>         another vhost callback to test whether MQ is negotiated: I just don't
>         think it's worthy.
> 
>         Hence, here I just used a simple test: invoke set_vring_enable() just
>         when it is defined. Judging the callback self has another MQ check,
>         I guess it's okay.
> 
> And sorry that it took so long to send this version.

Hmm, you are saying everyone needs SET_VRING_ENABLE?
Maybe we should make SET_VRING_ENABLE depend on protocol features then,
and not MQ?

I applied patches 1 and 5 for now.

Thanks!

> ---
> Yuanhan Liu (5):
>   vhost: rename RESET_DEVICE backto RESET_OWNER
>   vhost: reset vhost net when virtio_net_reset happens
>   vhost: introduce vhost_set/get_protocol_features callbacks
>   vhost: send SET_PROTOCOL_FEATURES at start
>   vhost: send SET_VRING_ENABLE at start/stop
> 
>  docs/specs/vhost-user.txt         |  4 ++--
>  hw/net/vhost_net.c                | 20 ++++++++++++++------
>  hw/net/virtio-net.c               | 14 ++++++++++++++
>  hw/virtio/vhost-backend.c         |  2 +-
>  hw/virtio/vhost-user.c            | 13 ++++++++++---
>  hw/virtio/vhost.c                 | 17 +++++++++++++++++
>  include/hw/virtio/vhost-backend.h |  6 ++++++
>  include/net/vhost_net.h           |  1 +
>  linux-headers/linux/vhost.h       |  2 +-
>  tests/vhost-user-bridge.c         |  6 +++---
>  tests/vhost-user-test.c           |  4 ++--
>  11 files changed, 71 insertions(+), 18 deletions(-)
> 
> -- 
> 1.9.0