From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40062) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPqAz-0000Kn-Fo for qemu-devel@nongnu.org; Thu, 13 Aug 2015 06:56:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZPqAu-0002JV-Na for qemu-devel@nongnu.org; Thu, 13 Aug 2015 06:56:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56284) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPqAu-0002JE-30 for qemu-devel@nongnu.org; Thu, 13 Aug 2015 06:55:56 -0400 Date: Thu, 13 Aug 2015 13:55:51 +0300 From: "Michael S. Tsirkin" Message-ID: <20150813135500-mutt-send-email-mst@redhat.com> References: <1432776186-24515-1-git-send-email-changchun.ouyang@intel.com> <1439360742-2186-1-git-send-email-changchun.ouyang@intel.com> <1439360742-2186-2-git-send-email-changchun.ouyang@intel.com> <20150813121013-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v6 1/2] vhost-user: add multi queue support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Maxime Leroy Cc: snabb-devel , Thibaut Collet , qemu-devel , Nikolay Nikolaev , Luke Gorrie , "thomas.long" , Ouyang Changchun 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 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 > >> Signed-off-by: Changchun Ouyang > >> --- > >> 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