[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly |
Date: |
Thu, 12 Nov 2015 15:33:41 +0200 |
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
- [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly, Yuanhan Liu, 2015/11/11
- [Qemu-devel] [PATCH v4 3/5] vhost: introduce vhost_set/get_protocol_features callbacks, Yuanhan Liu, 2015/11/11
- [Qemu-devel] [PATCH v4 2/5] vhost: reset vhost net when virtio_net_reset happens, Yuanhan Liu, 2015/11/11
- [Qemu-devel] [PATCH v4 1/5] vhost: rename RESET_DEVICE backto RESET_OWNER, Yuanhan Liu, 2015/11/11
- [Qemu-devel] [PATCH v4 4/5] vhost: send SET_PROTOCOL_FEATURES at start, Yuanhan Liu, 2015/11/11
- [Qemu-devel] [PATCH v4 5/5] vhost: send SET_VRING_ENABLE at start/stop, Yuanhan Liu, 2015/11/11
- Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly,
Michael S. Tsirkin <=