From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35397) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TdOQY-0003rH-2q for qemu-devel@nongnu.org; Tue, 27 Nov 2012 11:54:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TdOQW-0006jb-GF for qemu-devel@nongnu.org; Tue, 27 Nov 2012 11:54:30 -0500 Received: from nm10-vm4.bullet.mail.gq1.yahoo.com ([98.136.218.95]:28564) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TdOQW-0006jM-49 for qemu-devel@nongnu.org; Tue, 27 Nov 2012 11:54:28 -0500 Message-ID: <1354035266.55841.YahooMailClassic@web163902.mail.gq1.yahoo.com> Date: Tue, 27 Nov 2012 08:54:26 -0800 (PST) From: Edivaldo de Araujo Pereira In-Reply-To: <20121127162504.GA6210@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] virtio: limit avail bytes lookahead List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah , "Michael S. Tsirkin" Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, Anthony Liguori , Bug 1066055 <1066055@bugs.launchpad.net> Dear friends, =0A=0APlease excuse-me for not reporting earlier... I confirm= that the patch by Michael really fixes the problem I've reported. The regr= ession has gone away when I used it, so I think it is good to be applied.= =0A=0AThanks,=0A=0AEdivaldo de Ara=FAjo Pereira=0A=0A=0A--- Em ter, 27/11/1= 2, Michael S. Tsirkin escreveu:=0A=0A> De: Michael S. Tsir= kin =0A> Assunto: Re: [PATCH] virtio: limit avail bytes loo= kahead=0A> Para: "Amit Shah" =0A> Cc: "Stefan Hajnocz= i" , "Edivaldo de Araujo Pereira" , qemu-devel@nongnu.org, "Anthony Liguori" , "Bug 1066055" <1066055@bugs.launchpad.net>=0A> Data: Ter=E7a-feira, 27 d= e Novembro de 2012, 8:25=0A> On Thu, Nov 01, 2012 at 06:07:21PM=0A> +0200, = Michael S. Tsirkin wrote:=0A> > Commit 0d8d7690850eb0cf2b2b60933cf47669a6b6= f18f=0A> introduced=0A> > a regression in virtio-net performance because it= =0A> looks=0A> > into the ring aggressively while we really only care=0A> >= about a single packet worth of buffers.=0A> > To fix, add parameters limit= ing lookahead, and=0A> > use in virtqueue_avail_bytes.=0A> > =0A> > Signed-= off-by: Michael S. Tsirkin =0A> > Reported-by: Edivaldo de = Araujo Pereira =0A> =0A> Ping.=0A> Anthony -= going to apply this?=0A> =0A> =0A> > ---=0A> > =0A> > Edivaldo could you p= lease confirm this fixes=0A> regression?=0A> > =0A> > diff --git a/hw/virti= o-serial-bus.c=0A> b/hw/virtio-serial-bus.c=0A> > index d20bd8b..a761cdc 10= 0644=0A> > --- a/hw/virtio-serial-bus.c=0A> > +++ b/hw/virtio-serial-bus.c= =0A> > @@ -297,7 +297,7 @@ size_t=0A> virtio_serial_guest_ready(VirtIOSeria= lPort *port)=0A> >=A0 =A0 =A0 if (use_multiport(port->vser)=0A> && !port->g= uest_connected) {=0A> >=A0 =A0 =A0 =A0 =A0 return 0;=0A> >=A0 =A0 =A0 }=0A>= > -=A0 =A0 virtqueue_get_avail_bytes(vq,=0A> &bytes, NULL);=0A> > +=A0 =A0= virtqueue_get_avail_bytes(vq,=0A> &bytes, NULL, UINT_MAX, 0);=0A> >=A0 =A0= =A0 return bytes;=0A> >=A0 }=0A> >=A0 =0A> > diff --git a/hw/virtio.c b/hw= /virtio.c=0A> > index ec8b7d8..f40a8c5 100644=0A> > --- a/hw/virtio.c=0A> >= +++ b/hw/virtio.c=0A> > @@ -336,7 +336,8 @@ static unsigned=0A> virtqueue_= next_desc(hwaddr desc_pa,=0A> >=A0 }=0A> >=A0 =0A> >=A0 void virtqueue_get_= avail_bytes(VirtQueue *vq,=0A> unsigned int *in_bytes,=0A> > -=A0 =A0 =A0 = =A0 =A0 =A0 =A0=0A> =A0 =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0=A0=A0unsigned int *= out_bytes)=0A> > +=A0 =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0 =A0 =A0 =A0 =A0 =A0 = =A0=0A> =A0=A0=A0unsigned int *out_bytes,=0A> > +=A0 =A0 =A0 =A0 =A0 =A0 = =A0=0A> =A0 =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0=A0=A0unsigned max_in_bytes, uns= igned=0A> max_out_bytes)=0A> >=A0 {=0A> >=A0 =A0 =A0 unsigned int idx;=0A> = >=A0 =A0 =A0 unsigned int total_bufs, in_total,=0A> out_total;=0A> > @@ -38= 5,6 +386,9 @@ void=0A> virtqueue_get_avail_bytes(VirtQueue *vq, unsigned in= t=0A> *in_bytes,=0A> >=A0 =A0 =A0 =A0 =A0 =A0 =A0 } else=0A> {=0A> >=A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0 out_total +=3D vring_desc_len(desc_pa, i);= =0A> >=A0 =A0 =A0 =A0 =A0 =A0 =A0 }=0A> > +=A0 =A0 =A0 =A0 =A0 =A0 if (in_t= otal=0A> >=3D max_in_bytes && out_total >=3D max_out_bytes)=0A> {=0A> > += =A0 =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0 goto done;=0A> > +=A0 =A0 =A0 =A0 =A0 = =A0 }=0A> >=A0 =A0 =A0 =A0 =A0 } while ((i =3D=0A> virtqueue_next_desc(desc= _pa, i, max)) !=3D max);=0A> >=A0 =0A> >=A0 =A0 =A0 =A0 =A0 if (!indirect)= =0A> > @@ -392,6 +396,7 @@ void=0A> virtqueue_get_avail_bytes(VirtQueue *vq= , unsigned int=0A> *in_bytes,=0A> >=A0 =A0 =A0 =A0 =A0 else=0A> >=A0 =A0 = =A0 =A0 =A0 =A0 =A0=0A> total_bufs++;=0A> >=A0 =A0 =A0 }=0A> > +done:=0A> >= =A0 =A0 =A0 if (in_bytes) {=0A> >=A0 =A0 =A0 =A0 =A0 *in_bytes =3D=0A> in_t= otal;=0A> >=A0 =A0 =A0 }=0A> > @@ -405,12 +410,8 @@ int=0A> virtqueue_avail= _bytes(VirtQueue *vq, unsigned int in_bytes,=0A> >=A0 {=0A> >=A0 =A0 =A0 un= signed int in_total, out_total;=0A> >=A0 =0A> > -=A0 =A0 virtqueue_get_avai= l_bytes(vq,=0A> &in_total, &out_total);=0A> > -=A0 =A0 if ((in_bytes && in_= bytes <=0A> in_total)=0A> > -=A0 =A0 =A0 =A0 || (out_bytes &&=0A> out_bytes= < out_total)) {=0A> > -=A0 =A0 =A0 =A0 return 1;=0A> > -=A0 =A0 }=0A> > -= =A0 =A0 return 0;=0A> > +=A0 =A0 virtqueue_get_avail_bytes(vq,=0A> &in_tota= l, &out_total, in_bytes, out_bytes);=0A> > +=A0 =A0 return in_bytes <=3D in= _total=0A> && out_bytes <=3D out_total;=0A> >=A0 }=0A> >=A0 =0A> >=A0 void = virtqueue_map_sg(struct iovec *sg, hwaddr=0A> *addr,=0A> > diff --git a/hw/= virtio.h b/hw/virtio.h=0A> > index ac482be..1278328 100644=0A> > --- a/hw/v= irtio.h=0A> > +++ b/hw/virtio.h=0A> > @@ -150,7 +150,8 @@ int virtqueue_pop= (VirtQueue *vq,=0A> VirtQueueElement *elem);=0A> >=A0 int virtqueue_avail_b= ytes(VirtQueue *vq, unsigned=0A> int in_bytes,=0A> >=A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0=0A> =A0 =A0 =A0 =A0 =A0 =A0 unsigned int=0A> out_bytes);=0A> >=A0 = void virtqueue_get_avail_bytes(VirtQueue *vq,=0A> unsigned int *in_bytes,= =0A> > -=A0 =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0 =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0= =A0=A0unsigned int *out_bytes);=0A> > +=A0 =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0 = =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0=A0=A0unsigned int *out_bytes,=0A> > +=A0 = =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0 =A0 =A0 =A0 =A0 =A0 =A0=0A> =A0=A0=A0unsign= ed max_in_bytes, unsigned=0A> max_out_bytes);=0A> >=A0 =0A> >=A0 void virti= o_notify(VirtIODevice *vdev, VirtQueue=0A> *vq);=0A> >=A0 =0A>