From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52865) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9Zku-0002mN-Nh for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:42:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9Zkq-0008Av-IN for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:42:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60193) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9Zkq-0008Ao-Ae for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:42:04 -0500 Date: Thu, 17 Dec 2015 16:41:59 +0200 From: Victor Kaplansky Message-ID: <20151217163721-mutt-send-email-victork@redhat.com> References: <1449136399-4158-1-git-send-email-didier.pallard@6wind.com> <1449136399-4158-2-git-send-email-didier.pallard@6wind.com> <20151209173600-mutt-send-email-victork@redhat.com> <56685F7E.6020807@6wind.com> <20151210143219-mutt-send-email-victork@redhat.com> <566995A3.1040603@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <566995A3.1040603@6wind.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Didier Pallard Cc: "Michael S. Tsirkin" , Thibaut Collet , Jean-Mickael Guerin , QEMU , =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , pbonzini@redhat.com On Thu, Dec 10, 2015 at 04:09:23PM +0100, Didier Pallard wrote: > On 12/10/2015 01:56 PM, Victor Kaplansky wrote: > >On Wed, Dec 09, 2015 at 06:06:06PM +0100, Didier Pallard wrote: > >>On 12/09/2015 04:59 PM, Victor Kaplansky wrote: > >>>On Mon, Dec 07, 2015 at 02:31:36PM +0100, Marc-Andr=E9 Lureau wrote: > >>>>Hi > >>>> > >>>>On Thu, Dec 3, 2015 at 10:53 AM, Didier Pallard > >>>> wrote: > >>>>>unix_send_msgfds is used by vhost-user control socket. qemu_chr_fe= _write_all > >>>>>is used to send a message and retries as long as EAGAIN errno is s= et, > >>>>>but write_msgfds buffer is freed after first EAGAIN failure, causi= ng > >>>>>message to be sent without proper fds attachment. > >>>>> > >>>>>In case unix_send_msgfds is called through qemu_chr_fe_write, it w= ill be > >>>>>user responsability to resend message as is or to free write_msgfd= s > >>>>>using set_msgfds(0) > >>>>> > >>>>>Signed-off-by: Didier Pallard > >>>>>Reviewed-by: Thibaut Collet > >>>>>--- > >>>>> qemu-char.c | 10 ++++++++++ > >>>>> 1 file changed, 10 insertions(+) > >>>>> > >>>>>diff --git a/qemu-char.c b/qemu-char.c > >>>>>index 5448b0f..26d5f2e 100644 > >>>>>--- a/qemu-char.c > >>>>>+++ b/qemu-char.c > >>>>>@@ -2614,6 +2614,16 @@ static int unix_send_msgfds(CharDriverState= *chr, const uint8_t *buf, int len) > >>>>> r =3D sendmsg(s->fd, &msgh, 0); > >>>>> } while (r < 0 && errno =3D=3D EINTR); > >>>>> > >>>>>+ /* Ancillary data are not sent if no byte is written > >>>>>+ * so don't free msgfds buffer if return value is EAGAIN > >>>>>+ * If called from qemu_chr_fe_write_all retry will come soon > >>>>>+ * If called from qemu_chr_fe_write, it is the user responsib= ility > >>>>>+ * to resend message or free fds using set_msgfds(0) > >>>>>+ */ > >>>>>+ if (r < 0 && errno =3D=3D EAGAIN) { > >>>>>+ return r; > >>>>>+ } > >>>>>+ > >>>> > >>>>This looks reasonable to me. However, I don't know what happens wit= h > >>>>partial write of ancillary data. Hopefully it's all or nothing. > >>>>Apparently, reading unix_stream_sendmsg() in kernel shows that as l= ong > >>>>as a few bytes have been sent, the ancillary data is sent. So it lo= oks > >>>>like it still does the right thing in case of a partial write. > >>> > >>>If I may put my two cents in, it looks to me very similar to an > >>>fd leakage on back-end side. When a new set_call_fd request > >>>arrives, it is very easy to forget closing the previous fil > >>>descriptor. As result, if interrupts are actively maksed/unmasked > >>>by the guest, the back-end can easily reach maximum fds, which > >>>will cause receiving side silently drop new fds in aux data. > >>>--Victor > >>> > >> > >>Hi victor, > >>This is not a problem of fd exausted. This was my first axe of > >>investigation, but fd management is correct in our vhost-user backend= , there > >>is no fd leakage. > > > >That's good. > > > >>And i guess you are refering to the problem fixed by patches 2 and 3,= since > >>the problem corrected by this patch is a message arriving from qemu w= ithout > >>ancillary data, whatever the state of the fds in the vhost-user backe= nd. > > > >I'm talking about the problem that supposed to be fixed by the > >first patch. It is not clear to me how the patch fixes the > >partial send. sendmsg() is called in qemu-char.c:unix_send_msgfds > >with zero flags, which means a blocking operation, so I'm > >surprised that sendmsg can return with errno =3D=3D EAGAIN. > > >=20 > Well, vhost-user socket is started with following chardev: > -chardev socket,id=3Dvhostuserchr0,path=3D/tmp/vhost_sock0,server > and according to code in tcp_chr_add_client: > static int tcp_chr_add_client(CharDriverState *chr, int fd) > { > ... > qemu_set_nonblock(fd); >=20 > So fd is set in non blocking mode. This is enough to have an > EAGAIN returned value on socket buffer full, whatever flags used in sen= dmsg, > i think. > Perhaps changing the blocking mode here may also correct the first prob= lem, > but I am not able to measure the impact that may have such a modificati= on... Thanks for the clarification. I was able to reproduce both issues - with send_msgfds() partial send and lost interrupts using code based on vhost-user-bridge test application.=20 I'm working on a fix for the lost interrupts. Will send an RFC patch by the Sunday. -- Victor >=20 >=20 > >> > >>thanks > >>didier > >> > >>>> > >>>>Reviewed-by: Marc-Andr=E9 Lureau > >>>> > >>>>> /* free the written msgfds, no matter what */ > >>>>> if (s->write_msgfds_num) { > >>>>> g_free(s->write_msgfds); > >>>>>-- > >>>>>2.1.4 > >>>>> > >>>>> > >>>> > >>>> > >>>> > >>>>-- > >>>>Marc-Andr=E9 Lureau > >>>> > >> > >> > >> >=20 >=20