From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42415) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi14s-0007Ks-W2 for qemu-devel@nongnu.org; Thu, 08 Sep 2016 11:17:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bi14o-0006DA-FV for qemu-devel@nongnu.org; Thu, 08 Sep 2016 11:17:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53026) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi14o-0006Cp-7C for qemu-devel@nongnu.org; Thu, 08 Sep 2016 11:17:18 -0400 Date: Thu, 8 Sep 2016 18:17:16 +0300 From: "Michael S. Tsirkin" Message-ID: <20160908181543-mutt-send-email-mst@kernel.org> References: <1473323650-13298-1-git-send-email-maxime.coquelin@redhat.com> <1473323650-13298-3-git-send-email-maxime.coquelin@redhat.com> <86AAE458-EC2F-4814-88A9-E3E7F93AD274@nutanix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <86AAE458-EC2F-4814-88A9-E3E7F93AD274@nutanix.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] vhost-user: only seek a reply if needed in set_mem_table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Prerna Saxena Cc: Maxime Coquelin , "qemu-devel@nongnu.org" , "marcandre.lureau@redhat.com" , "peter.maydell@linaro.org" On Thu, Sep 08, 2016 at 11:33:34AM +0000, Prerna Saxena wrote: > Hi Maxime, >=20 >=20 > On 08/09/16 2:04 pm, "Maxime Coquelin" wro= te: >=20 > >The goal of this patch is to only request a sync (reply_ack, > >or get_features) in set_mem_table only when necessary. > > > >It should not be necessary the first time we set the table, > >or when we add a new regions which hadn't been merged with an > >existing ones. >=20 >=20 > I don=E2=80=99t think so.=20 > This patch is not helping us solve the issue. > The hang introduced by original use of get_features() in set_mem_table > was traced down to use of TCG mode for vhost-user test. Right but we don't know for sure there are no backends that do this kind of simplistic thing, and assume that get features does not happen later. And for most setups without memory hotplug, they would be right. > This has now > been fixed via: >=20 > ----- > commit cdafe929615ec5eca71bcd5a3d12bab5678e5886 > Author: Eduardo Habkost > Date: Fri Sep 2 15:59:43 2016 -0300 >=20 >=20 > vhost-user-test: Use libqos instead of pxe-virtio.rom > =20 > vhost-user-test relies on iPXE just to initialize the virtio-net > device, and doesn't do any actual packet tx/rx testing. > =20 > In addition to that, the test relies on TCG, which is > imcompatible with vhost. The test only worked by accident: a bug > the memory backend initialization made memory regions not have > the DIRTY_MEMORY_CODE bit set in dirty_log_mask. > =20 > This changes vhost-user-test to initialize the virtio-net device > using libqos, and not use TCG nor pxe-virtio.rom. > =20 > Signed-off-by: Eduardo Habkost >=20 > ------- >=20 > So I think the original hang seems to have been fixed with Patch 1/2 of= this series alone. >=20 > Regarding Patch 2/2: > This patch seems to filter responses from set_mem_table only for certai= n updates of memory regions. It violates the definition of the REPLY_ACK = feature. This feature expects the client to send a response for every cal= l of set_mem_table. And here, qemu exits the set_mem_table() function in = some cases without even waiting for the reply that is going to come in. >=20 > As for use of this approach with get_features, we have already debated = that on the list before : https://lists.nongnu.org/archive/html/qemu-deve= l/2016-07/msg00689.html > To quote: > "I do not entirely agree with that. The first set_mem_table command is = not much > different from subsequent set_mem_table calls." >=20 > Regards, > Prerna >=20