From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [PATCH 1/3] kvm tools: Make virtio-net kvm__irq_line thread safe Date: Fri, 29 Apr 2011 09:45:57 +0200 Message-ID: <20110429074557.GA17418@elte.hu> References: <1304058985-13833-1-git-send-email-asias.hejun@gmail.com> <4DBA6519.5070509@gmail.com> <4DBA6AE5.8090703@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Pekka Enberg , Cyrill Gorcunov , Sasha Levin , Prasad Joshi , kvm@vger.kernel.org To: Asias He Return-path: Received: from mx2.mail.elte.hu ([157.181.151.9]:58840 "EHLO mx2.mail.elte.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756063Ab1D2HqH (ORCPT ); Fri, 29 Apr 2011 03:46:07 -0400 Content-Disposition: inline In-Reply-To: <4DBA6AE5.8090703@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: * Asias He wrote: > On 04/29/2011 03:15 PM, Pekka Enberg wrote: > > On Fri, Apr 29, 2011 at 10:13 AM, Asias He wrote: > >> No. We only need it in RX path. Sasha's threadpool patch breaks this. > >> I'm just moving it back. > > > > OK, cool! Can you send the fix as a separate patch? > > > > Here you go. > > -- > Best Regards, > Asias He > >From 9c188853435be23ad2a73b5062690eb9f2ff1d25 Mon Sep 17 00:00:00 2001 > From: Asias He > Date: Fri, 29 Apr 2011 10:43:35 +0800 > Subject: [PATCH] kvm tools: Fix virtio-net RX path IRQ latency > > The TX path does not need this ;-) no explanation ... > Signed-off-by: Asias He > --- > tools/kvm/virtio-net.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tools/kvm/virtio-net.c b/tools/kvm/virtio-net.c > index 58b3de4..310c061 100644 > --- a/tools/kvm/virtio-net.c > +++ b/tools/kvm/virtio-net.c > @@ -77,9 +77,10 @@ static void virtio_net_rx_callback(struct kvm *self, void *param) > head = virt_queue__get_iov(vq, iov, &out, &in, self); > len = readv(net_device.tap_fd, iov, in); > virt_queue__set_used_elem(vq, head, len); > - } > > - kvm__irq_line(self, VIRTIO_NET_IRQ, 1); > + /* We should interrupt guest right now, otherwise latency is huge. */ > + kvm__irq_line(self, VIRTIO_NET_IRQ, 1); > + } I'm wondering, why is this needed? The change seems to *reduce* the number of irq events on the guest side. How can this reduce stalls? I.e. it would be nice to see an explanation about what it did before, how that caused problems and how this fix is the best solution to that. Otherwise it gives a "tinker the state machine until the stall goes away" kind of hack, which risks leaving some real bug ununderstood, unfixed or possibly reintroduced in the future. The explanation is often more important than the fix itself! Thanks, Ingo