From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TO1sw-0002YM-BW for qemu-devel@nongnu.org; Tue, 16 Oct 2012 03:48:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TO1ss-0007EG-7K for qemu-devel@nongnu.org; Tue, 16 Oct 2012 03:48:18 -0400 Received: from mail-bk0-f45.google.com ([209.85.214.45]:65227) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TO1sr-0007Ca-VA for qemu-devel@nongnu.org; Tue, 16 Oct 2012 03:48:14 -0400 Received: by mail-bk0-f45.google.com with SMTP id jf3so2391989bkc.4 for ; Tue, 16 Oct 2012 00:48:12 -0700 (PDT) Date: Tue, 16 Oct 2012 09:48:09 +0200 From: Stefan Hajnoczi Message-ID: <20121016074809.GA23066@stefanha-thinkpad.redhat.com> References: <20121012173423.27389.27726.malonedeb@chaenomeles.canonical.com> <20121015214606.18996.44940.malone@gac.canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20121015214606.18996.44940.malone@gac.canonical.com> Subject: Re: [Qemu-devel] [Bug 1066055] Re: Network performance regression with vde_switch List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bug 1066055 <1066055@bugs.launchpad.net> Cc: qemu-devel@nongnu.org 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 > 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 > Signed-off-by: Michael S. Tsirkin > > :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. Stefan