Re: [PATCH 1/3] kvm tools: Make virtio-net kvm__irq_line thread safe

[Ingo Molnar <mingo@elte.hu>]

* Asias He <asias.hejun@gmail.com> wrote:

> On 04/29/2011 03:15 PM, Pekka Enberg wrote:
> > On Fri, Apr 29, 2011 at 10:13 AM, Asias He <asias.hejun@gmail.com> 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 <asias.hejun@gmail.com>
> 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 <asias.hejun@gmail.com>
> ---
>  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