From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id BCD359E3 for ; Fri, 28 Apr 2017 09:39:40 +0200 (CEST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Apr 2017 00:39:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,387,1488873600"; d="scan'208";a="94863413" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga005.fm.intel.com with ESMTP; 28 Apr 2017 00:39:32 -0700 Date: Fri, 28 Apr 2017 15:35:53 +0800 From: Yuanhan Liu To: Maxime Coquelin Cc: Zhiyong Yang , dev@dpdk.org, ciara.loftus@intel.com, =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , "Michael S. Tsirkin" Message-ID: <20170428073553.GV11512@yliu-dev.sh.intel.com> References: <1493274893-40764-1-git-send-email-zhiyong.yang@intel.com> <57212715-6192-dba2-418a-2418a5d14953@redhat.com> <20170427082047.GL11512@yliu-dev.sh.intel.com> <0bb0c833-1178-c587-8085-12137ebd91d5@redhat.com> <20170428022558.GM11512@yliu-dev.sh.intel.com> <9ecbbd0b-a2a0-7674-e84d-c8beeeeff0e6@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <9ecbbd0b-a2a0-7674-e84d-c8beeeeff0e6@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] vhost: fix MQ fails to startup X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Apr 2017 07:39:41 -0000 On Fri, Apr 28, 2017 at 09:23:54AM +0200, Maxime Coquelin wrote: > > > On 04/28/2017 04:25 AM, Yuanhan Liu wrote: > >On Thu, Apr 27, 2017 at 10:52:20AM +0200, Maxime Coquelin wrote: > >> > >> > >>On 04/27/2017 10:20 AM, Yuanhan Liu wrote: > >>>On Thu, Apr 27, 2017 at 09:56:47AM +0200, Maxime Coquelin wrote: > >>>>Hi Zhiyong, > >>>> > >>>>+Marc-André > >>>> > >>>>On 04/27/2017 08:34 AM, Zhiyong Yang wrote: > >>>>>vhost since dpdk17.02 + qemu2.7 and above will cause failures of > >>>>>new connection when negotiating to set MQ. (one queue pair works > >>>>>well).Because there exist some bugs in qemu code when introducing > >>>>>VHOST_USER_PROTOCOL_F_REPLY_ACK to qemu. when dealing with the vhost > >>>>>message VHOST_USER_SET_MEM_TABLE for the second time, qemu indeed > >>>>>doesn't send the messge (The message needs to be sent only once)but > >>>>>still will be waiting for dpdk's reply ack, then, qemu is always > >>>>>freezing. DPDK code works in the right way. > >>>> > >>>>I'm looking at Qemu's vhost_user_set_mem_table() function, but fail to > >>>>see how it could wait for the reply-ack if it didn't send the > >>>>VHOST_USER_SET_MEM_TABLE request before. > >>>> > >>>>>But the feature > >>>>>VHOST_USER_PROTOCOL_F_REPLY_ACK has to be disabled by default at the > >>>>>dpdk side in order to avoid the feature support of DPDK + qemu at > >>>>>the same time. if doing like that, MQ can works well. Once Qemu bugs > >>>>>have been fixed and upstreamed, we can enable it. > >>>> > >>>>The problem is for DPDK to detect whether bug is fixed in Qemu. > >>>>Maybe only way would be to have a new protocol feature flag, which is > >>>>not really its role. > >>> > >>>Wouldn't that be an overkill, judging that REPLY_ACK is not a must > >>>feature? > >> > >>Yes, maybe. But it was introduced to fix (possible) race conditions: > >>https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06173.html > > > >But AFAIK, that commit has been reverted: > > > > commit 94c9cb31c04737f86be29afefbff401cd23bc24d > > Author: Michael S. Tsirkin > > Date: Mon Aug 15 16:35:24 2016 +0300 > > Revert "vhost-user: Attempt to fix a race with set_mem_table." > > This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23. > > I still think it's the right thing to do, but > > tests have been failing sporadically. > > Revert for now, and hope to fix it before the release. > > No, what has been reverted is a workaround when REPLY_ACK protocol > feature has not been negotiated. Good to know. > > Instead of waiting for the backend to send the ack, the workaround > consisted in sending a GET_FEATURES request after having sent the > SET_MEM_TABLE request, in order to ensure SET_MEM_TABLE request handling > was done before. > > The problem is that it sometimes created a deadlock when when running > QEMU's vhost-user-test in TCG mode. > > >> > >>Note that I planned to use this feature for the device IOTLB > >>implementation to let the backend decide whether it wants the IOTLB > >>misses synchronous or asynchronous. But I can still change the protocol > >>spec to make this behavior specific to this request. > > > >Maybe we could introduce a version message? With that, we could tell > >whether the frontend has fixed the known bug or not. > > That's a possibility, but this is not really the role of a protocol > version. As in this case, the protocol does not change, just an > implementation. Maybe. Well, you might could think this way: we do increase the version when we make a new release (with bugs being fixed). Or, we could also make the version two parts: major and minor. We increase major for major updates (say, new features, etc). We increase minor for bug fixes. The only thing that doesn't make too much sense is the bug is actually from the QEMU implementation but not from the vhost-user spec. Talking about that, it may make more sense to introduce a new message to carry the frontend version, something like a string "QEMU v2.8". --yliu > > >Note that we already has the "version" info in current vhost-user spec. > >It's just 2 bits in the message "flag" field though, which is not quite > >enough. > > Indeed, it does not let room for lots of bugs :) > > Thanks, > Maxime > > --yliu > >