From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43603) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bYlW6-0004Ih-Tg for qemu-devel@nongnu.org; Sat, 13 Aug 2016 22:51:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bYlW2-0002GC-NO for qemu-devel@nongnu.org; Sat, 13 Aug 2016 22:51:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40712) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bYlW2-0002G8-Ew for qemu-devel@nongnu.org; Sat, 13 Aug 2016 22:51:10 -0400 Date: Sun, 14 Aug 2016 05:51:06 +0300 From: "Michael S. Tsirkin" Message-ID: <20160814054808-mutt-send-email-mst@kernel.org> References: <1470842980-32481-1-git-send-email-mst@redhat.com> <1470842980-32481-4-git-send-email-mst@redhat.com> <20160812063828.GG2759@al.usersys.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 3/3] vhost-user: Attempt to fix a race with set_mem_table. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Prerna Saxena Cc: Fam Zheng , "qemu-devel@nongnu.org" , Peter Maydell , "marcandre.lureau@redhat.com" On Fri, Aug 12, 2016 at 07:16:34AM +0000, Prerna Saxena wrote: >=20 > On 12/08/16 12:08 pm, "Fam Zheng" wrote: >=20 >=20 >=20 >=20 >=20 > >On Wed, 08/10 18:30, Michael S. Tsirkin wrote: > >> From: Prerna Saxena > >>=20 > >> The set_mem_table command currently does not seek a reply. Hence, th= ere is > >> no easy way for a remote application to notify to QEMU when it finis= hed > >> setting up memory, or if there were errors doing so. > >>=20 > >> As an example: > >> (1) Qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net > >> application). SET_MEM_TABLE does not require a reply according to th= e spec. > >> (2) Qemu commits the memory to the guest. > >> (3) Guest issues an I/O operation over a new memory region which was= configured on (1). > >> (4) The application has not yet remapped the memory, but it sees the= I/O request. > >> (5) The application cannot satisfy the request because it does not k= now about those GPAs. > >>=20 > >> While a guaranteed fix would require a protocol extension (committed= separately), > >> a best-effort workaround for existing applications is to send a GET_= FEATURES > >> message before completing the vhost_user_set_mem_table() call. > >> Since GET_FEATURES requires a reply, an application that processes v= host-user > >> messages synchronously would probably have completed the SET_MEM_TAB= LE before replying. > >>=20 > >> Signed-off-by: Prerna Saxena > >> Reviewed-by: Michael S. Tsirkin > >> Signed-off-by: Michael S. Tsirkin > > > >Sporadic hangs are seen with test-vhost-user after this patch: > > > >https://travis-ci.org/qemu/qemu/builds > > > >Reverting seems to fix it for me. > > > >Is this a known problem? > > > >Fam >=20 > Hi Fam, > Thanks for reporting the sporadic hangs. I had seen =E2=80=98make check= =E2=80=99 pass on my Centos 6 environment, so missed this. > I am setting up the docker test env to repro this, but I think I can gu= ess the problem : >=20 > In tests/vhost-user-test.c:=20 >=20 > static void chr_read(void *opaque, const uint8_t *buf, int size) > { > ..[snip].. >=20 > case VHOST_USER_SET_MEM_TABLE: > /* received the mem table */ > memcpy(&s->memory, &msg.payload.memory, sizeof(msg.payload.memor= y)); > s->fds_num =3D qemu_chr_fe_get_msgfds(chr, s->fds, G_N_ELEMENTS(= s->fds)); >=20 >=20 > /* signal the test that it can continue */ > g_cond_signal(&s->data_cond); > break; > ..[snip].. > } >=20 >=20 > The test seems to be marked complete as soon as mem_table is copied.=20 > However, this patch 3/3 changes the behaviour of the SET_MEM_TABLE vhos= t command implementation with qemu. SET_MEM_TABLE now sends out a new mes= sage GET_FEATURES, and the call is only completed once it receives featur= es from the remote application. (or the test framework, as is the case he= re.) Hmm but why does it matter that data_cond is woken up? > While the test itself can be modified (Do not signal completion until w= e=E2=80=99ve sent a follow-up response to GET_FEATURES), I am now wonderi= ng if this patch may break existing vhost applications too ? If so, rever= ting it possibly better. What bothers me is that the new feature might cause the same issue once we enable it in the test. How about a patch to tests/vhost-user-test.c adding the new protocol feature? I would be quite interested to see what is going on with it. > What confuses me is why it doesn=E2=80=99t fail all the time, but only = about 20% to 30% time as Fam reports.=20 And succeeds every time on my systems :( >=20 > Thoughts : Michael, Fam, MarcAndre ? >=20 > Regards, > Prerna=20