On Thu, Aug 13, 2015 at 12:24:16PM +0200, Maxime Leroy wrote: > On Thu, Aug 13, 2015 at 11:18 AM, Michael S. Tsirkin <m...@redhat.com> wrote: > > On Wed, Aug 12, 2015 at 02:25:41PM +0800, Ouyang Changchun wrote: > >> Based on patch by Nikolay Nikolaev: > >> Vhost-user will implement the multi queue support in a similar way > >> to what vhost already has - a separate thread for each queue. > >> To enable the multi queue functionality - a new command line parameter > >> "queues" is introduced for the vhost-user netdev. > >> > >> The RESET_OWNER change is based on commit: > >> 294ce717e0f212ed0763307f3eab72b4a1bdf4d0 > >> If it is reverted, the patch need update for it accordingly. > >> > >> Signed-off-by: Nikolay Nikolaev <n.nikol...@virtualopensystems.com> > >> Signed-off-by: Changchun Ouyang <changchun.ouy...@intel.com> > >> --- > >> Changes since v5: > >> - fix the message descption for VHOST_RESET_OWNER in vhost-user txt > >> > >> Changes since v4: > >> - remove the unnecessary trailing '\n' > >> > >> Changes since v3: > >> - fix one typo and wrap one long line > >> > >> Changes since v2: > >> - fix vq index issue for set_vring_call > >> When it is the case of VHOST_SET_VRING_CALL, The vq_index is not > >> initialized before it is used, > >> thus it could be a random value. The random value leads to crash in > >> vhost after passing down > >> to vhost, as vhost use this random value to index an array index. > >> - fix the typo in the doc and description > >> - address vq index for reset_owner > >> > >> Changes since v1: > >> - use s->nc.info_str when bringing up/down the backend > >> > >> docs/specs/vhost-user.txt | 7 ++++++- > >> hw/net/vhost_net.c | 3 ++- > >> hw/virtio/vhost-user.c | 11 ++++++++++- > >> net/vhost-user.c | 37 ++++++++++++++++++++++++------------- > >> qapi-schema.json | 6 +++++- > >> qemu-options.hx | 5 +++-- > >> 6 files changed, 50 insertions(+), 19 deletions(-) > >> > >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt > >> index 70da3b1..9390f89 100644 > >> --- a/docs/specs/vhost-user.txt > >> +++ b/docs/specs/vhost-user.txt > >> @@ -135,6 +135,11 @@ As older slaves don't support negotiating protocol > >> features, > >> a feature bit was dedicated for this purpose: > >> #define VHOST_USER_F_PROTOCOL_FEATURES 30 > >> > >> +Multi queue support > >> +------------------- > >> +The protocol supports multiple queues by setting all index fields in the > >> sent > >> +messages to a properly calculated value. > >> + > >> Message types > >> ------------- > >> > >> @@ -198,7 +203,7 @@ Message types > >> > >> Id: 4 > >> Equivalent ioctl: VHOST_RESET_OWNER > >> - Master payload: N/A > >> + Master payload: vring state description > >> > >> Issued when a new connection is about to be closed. The Master will > >> no > >> longer own this connection (and will usually close it). > > > > This is an interface change, isn't it? > > We can't make it unconditionally, need to make it dependent > > on a protocol flag. > > Agree. It can potential break vhost-user driver implementation > checking the size of the message. We should not change the vhost-user > protocol without a new protocol flag. > > I think the first issue here that VHOST_RESET_OWNER should happen on > vhost_dev_cleanup and not in vhost_net_stop_one. > > VHOST_RESET_OWNER should be the counter part of VHOST_SET_OWNER. So it > don't need to have a payload like VHOST_SET_OWNER. > > Thus I agree with this email > (http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg05971.html) > > Maybe should we use an other message to tell to the backend that the > vring is not anymore available in vhost_net_stop_one ? > > Maxime
I think the cleanest fix is to rename this message to e.g. VHOST_RESET_DEVICE. This way we won't break existing users. -- MST
