Re: [PATCH 1/2] kvm tools: Respect ISR status in virtio header

[Ingo Molnar <mingo@elte.hu>]

* Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/07/2011 09:02 AM, Ingo Molnar wrote:
> >Well the optimization also avoids unnecessary VM exits (due to the injection,
> >which interrupts a guest context immediately, even if it's running on another
> >CPU), not just system calls - so it could be more expensive than a system call,
> >right?
> 
> If there's IRQ is already pending, the syscall should be a nop.  If
> the IRQ is extraneous, then there's a pretty short window when the
> IRQ wouldn't already be pending.
> 
> I took a look at the current virtio code and noticed a few things
> that are sub-optimal:
> 
> 1) No MSI.  MSI makes interrupts edge triggered and avoids the ISR
> read entirely.  The ISR read is actually pretty expensive.
> 
> 2) The access pattern is basically:
> 
> while pending_requests:
>    a = get_next_request();
>    process_next_request(a);
> 
> But this is not the optimal pattern with virtio.  The optimal pattern is:
> 
> if pending_requests:
>   disable_notifications();
> 
> again:
>   while pending_requests:
>     a = get_next_request();
>     process_next_request();
> 
>   enable_notifications();
>   if pending_requests:
>      goto again;
> 
> You're currently taking exits way more than you should, particularly
> since you're using threads for processing the ring queues.
> 
> 3) You aren't advertising VIRTIO_F_NOTIFY_ON_EMPTY.  Particularly
> with TX, it's advantageous to inject an IRQ even if they're disabled
> when there are no longer packets in the ring.
> 
> (2) and (1) are definitely the most important for performance.  (2)
> should be a pretty simple change and should have a significant
> impact.

Can you anything in the virtio protocol implementation that would explain 
networking lags, which seem to be caused by guest notifications either not be 
sent or being missed?

In particular this sequence:

> while pending_requests:
>    a = get_next_request();
>    process_next_request(a);

is apparently not what Qemu uses - so maybe there's some latent bug or some 
silly oversight somewhere.

It is suboptimal and i agree with you that the better sequence should be 
implemented, but the above *should* work, yet it does not.

Thanks,

	Ingo