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 <amit.s...@redhat.com> escreveu:
> De: Amit Shah <amit.s...@redhat.com>
> Assunto: Re: [Qemu-devel] [Bug 1066055] Re: Network performance regression
> with vde_switch
> Para: "Stefan Hajnoczi" <stefa...@gmail.com>
> Cc: "Bug 1066055" <1066...@bugs.launchpad.net>, qemu-devel@nongnu.org,
> edivaldoapere...@yahoo.com.br
> 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 <amit.s...@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.s...@redhat.com>
> > > Signed-off-by: Michael S.
> Tsirkin <m...@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
>
