Hi,
On 24/11/14 08:49, Eric Auger wrote:
> On 11/21/2014 02:00 PM, Marc Zyngier wrote:
>> On 21/11/14 12:53, Christoffer Dall wrote:
>>> On Mon, Nov 17, 2014 at 10:36:13AM +0000, Marc Zyngier wrote:
>>>> On 17/11/14 09:27, wanghaibin wrote:
>>>>> When vgic_update_irq_pending with level-sensitive false, it is need to
>>>>> deactivates an interrupt, and, it can go to out directly.
>>>>> Here return a false value, because it will be not need to kick.
>>>>>
>>>>> Signed-off-by: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
>>>>> ---
>>>>> virt/kvm/arm/vgic.c | 3 +++
>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>>> index 52f3bfa..b663140 100644
>>>>> --- a/virt/kvm/arm/vgic.c
>>>>> +++ b/virt/kvm/arm/vgic.c
>>>>> @@ -1665,6 +1665,9 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>>>>> } else {
>>>>> vgic_dist_irq_clear_pending(vcpu, irq_num);
>>>>> }
> Hi all,
>
> As we are going to touch this function, I think the above else section
> is dead code. Also the test of level_triggered in !level is useless.
> In case we have a edge sensitive IRQ, level cannot be 0 here since
> vgic_validate_injection would have caused a return before.
I completely agree on this, I came to the same conclusions (and did
actually prove the dead code with tracing).
I also did test runs with some tracing in place to see how much this
patch actually gives us. While I could spot quite a lot of early exits
with this patch, I found that in practice we wouldn't have kicked the
other VCPUs because the PL011 IRQs (the only level triggered device we
usually have, right?) returned false most of the time anyway later in
that function. So it boiled down to about 1% of "return true"s changed
to "return false" with this patch.
Also this is in the userland code path, so we don't save any VMEXITs.
Faking virtio to use level triggered interrupts avoided some more VCPU
kicks, fwiw.
Cheers,
Andre.
>>>>> +
>>>>> + ret = false;
>>>>> + goto out;
>>>>> }
>>>>>
>>>>> enabled = vgic_irq_is_enabled(vcpu, irq_num);
>>>>>
>>>>
>>>> Have you actually tested this on real hardware? This looks like an
>>>> interesting optimization, but I want to see some actual data.
>>>>
>>> Do you see any harm in merging this? I think it looks reasonable and
>>> non-disruptive?
>>
>> Agreed. I'll take it.
>>
>> M.
>>
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
