Re: [Qemu-devel] [Bug 1066055] Re: Network performance regression with vde_switch

[Amit Shah <amit.shah@redhat.com>]

On (Tue) 16 Oct 2012 [09:48:09], Stefan Hajnoczi wrote:
> On Mon, Oct 15, 2012 at 09:46:06PM -0000, Edivaldo de Araujo Pereira wrote:
> > Hi Stefan,
> > 
> > Thank you, very much for taking the time to help me, and excuse me for
> > not seeing your answer early...
> > 
> > I've run the procedure you pointed me out, and the result is:
> > 
> > 0d8d7690850eb0cf2b2b60933cf47669a6b6f18f is the first bad commit
> > commit 0d8d7690850eb0cf2b2b60933cf47669a6b6f18f
> > Author: Amit Shah <amit.shah@redhat.com>
> > Date:   Tue Sep 25 00:05:15 2012 +0530
> > 
> >     virtio: Introduce virtqueue_get_avail_bytes()
> > 
> >     The current virtqueue_avail_bytes() is oddly named, and checks if a
> >     particular number of bytes are available in a vq.  A better API is to
> >     fetch the number of bytes available in the vq, and let the caller do
> >     what's interesting with the numbers.
> > 
> >     Introduce virtqueue_get_avail_bytes(), which returns the number of bytes
> >     for buffers marked for both, in as well as out.  virtqueue_avail_bytes()
> >     is made a wrapper over this new function.
> > 
> >     Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > :040000 040000 1a58b06a228651cf844621d9ee2f49b525e36c93
> > e09ea66ce7f6874921670b6aeab5bea921a5227d M      hw
> > 
> > I tried to revert that patch in the latest version, but it obviously
> > didnt work; I'm trying to figure out the problem, but I don't know very
> > well the souce code, so I think it's going to take some time. For now,
> > it's all I could do.
> 
> After git-bisect(1) completes it is good to sanity-check the result by
> manually testing 0d8d7690850eb0cf2b2b60933cf47669a6b6f18f^ (the commit
> just before the bad commit) and 0d8d7690850eb0cf2b2b60933cf47669a6b6f18f
> (the bad commit).
> 
> This will verify that the commit indeed introduces the regression.  I
> suggest doing this just to be sure that you've found the bad commit.
> 
> Regarding this commit, I notice two things:
> 
> 1. We will now loop over all vring descriptors because we calculate the
>    total in/out length instead of returning early as soon as we see
>    there is enough space.  Maybe this makes a difference, although I'm a
>    little surprised you see such a huge regression.
> 
> 2. The comparision semantics have changed from:
> 
>      (in_total += vring_desc_len(desc_pa, i)) >= in_bytes
> 
>    to:
> 
>      (in_bytes && in_bytes < in_total)
> 
>    Notice that virtqueue_avail_bytes() now returns 0 when in_bytes ==
>    in_total.  Previously, it would return 1.  Perhaps we are starving or
>    delaying I/O due to this comparison change.  You can easily change
>    '<' to '<=' to see if it fixes the issue.

Hi Edivaldo,

Can you try the following patch, that will confirm if it's the
descriptor walk or the botched compare that's causing the regression.

Thanks,

diff --git a/hw/virtio.c b/hw/virtio.c
index 6821092..bb08ed8 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -406,8 +406,8 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
     unsigned int in_total, out_total;
 
     virtqueue_get_avail_bytes(vq, &in_total, &out_total);
-    if ((in_bytes && in_bytes < in_total)
-        || (out_bytes && out_bytes < out_total)) {
+    if ((in_bytes && in_bytes <= in_total)
+        || (out_bytes && out_bytes <= out_total)) {
         return 1;
     }
     return 0;


		Amit