Dear Amit,

On a suggestion of Stefan, I've already tested the modification in you patch, and it didn't work; but for confirmation I tested it once again, on the latest snapshot; same result, that is, it didn't work; the problem is still there.

I didn't take enough time to uderstand the code, so unfortunately I fear there is not much I could do to solve the problem, apart from trying your suggestions. But I'll try to spend a little more time on it, until we find a solution.

Thank you very much.

Edivaldo

--- Em seg, 22/10/12, Amit Shah <email address hidden> escreveu:

> De: Amit Shah <email address hidden>
> Assunto: Re: [Qemu-devel] [Bug 1066055] Re: Network performance regression with vde_switch
> Para: "Stefan Hajnoczi" <email address hidden>
> Cc: "Bug 1066055" <email address hidden>, <email address hidden>, <email address hidden>
> Data: Segunda-feira, 22 de Outubro de 2012, 4:18
> 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 <email address hidden>
> > > 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
> <email address hidden>
> > >     Signed-off-by: Michael S.
> Tsirkin <email address hidden>
> > > 
> > > :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
>