From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52441) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zwt6w-0002YG-57 for qemu-devel@nongnu.org; Thu, 12 Nov 2015 09:44:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zwt6s-0001br-0S for qemu-devel@nongnu.org; Thu, 12 Nov 2015 09:44:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37579) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zwt6r-0001bj-PQ for qemu-devel@nongnu.org; Thu, 12 Nov 2015 09:44:21 -0500 Date: Thu, 12 Nov 2015 16:44:19 +0200 From: "Michael S. Tsirkin" Message-ID: <20151112164232-mutt-send-email-mst@redhat.com> References: <1447248281-15227-1-git-send-email-yuanhan.liu@linux.intel.com> <20151112152332-mutt-send-email-mst@redhat.com> <20151112140815.GK2326@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151112140815.GK2326@yliu-dev.sh.intel.com> Subject: Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yuanhan Liu Cc: qemu-devel@nongnu.org On Thu, Nov 12, 2015 at 10:08:15PM +0800, Yuanhan Liu wrote: > On Thu, Nov 12, 2015 at 03:33:41PM +0200, Michael S. Tsirkin wrote: > > 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. > > Agree with that. > > > > > > 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? > > It just resets some states, such as closing call fd and kick fd, > unmapping buf from hugetlbfs from set_mem_table. And is this in some dpdk version that has been released? > And, apparently we could do that on stop, too. So, from this pov, we > don't need RESET_OWNER. > > > > > > 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'm thinking something same. Otherwise, there is still no way to inform > the backend (or client) when a vhost dev is stopped when MQ is disabled > (which is the default state). > > So, let's assume all clients have protocol features enabled, and send > SET_VRING_ENABLE at start/stop? And if it does not, it's just like we > are back to QEMU v2.3, where no RESET_OWNER nor SET_VRING_ENABLE > messages are sent on stop: it worked before, and it should also work > now. So maybe we should drop RESET_OWNER from stop then? > > > > I applied patches 1 and 5 for now. > > Do you have comment about patch 3 and 4? Should we set protocol features > just like we set features at start? > > Thanks. > > --yliu We don't set the at start - we set them on connect.