Re: [Xen-devel] [RFC 01/16] xen/arm: Re-enable interrupt later in the trap path

From: Julien Grall <julien.grall@arm.com>
To: Andrii Anisov <andrii.anisov@gmail.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	nd <nd@arm.com>, Andrii Anisov <andrii_anisov@epam.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Andre Przywara <andre.przywara@arm.com>
Subject: Re: [Xen-devel] [RFC 01/16] xen/arm: Re-enable interrupt later in the trap path
Date: Tue, 30 Jul 2019 21:46:38 +0100	[thread overview]
Message-ID: <e1ee3ca4-edff-1a62-1f0a-0b832b4d5924@arm.com> (raw)
In-Reply-To: <b7b24214-f7d0-41d0-b2c7-286d45f6da1a@gmail.com>

(+ Andre)

On 7/30/19 6:34 PM, Andrii Anisov wrote:
> Hello Julien,
> 
> It looks I missed answering this email. So do this now:
> 
> On 29.11.18 00:00, Julien Grall wrote:
>> On 28/11/2018 21:31, Andrii Anisov wrote:
>>> From: Julien Grall <julien.grall@arm.com>
>>>
>>> This makes function enter_hypervisor_head() being executed with
>>> irqs locked. This also give a fine side effect - it assures that
>>> LRs are cleared prior to any IRQs processing, which leads to a
>>> better (faster) IRQs processing.
>>
>> Again, this need some rationale why is it faster. And how this is going
>> to impact the other vGIC...
> 
> With the current code, there is a chance that hypervisor performs 
> do_IRQ() before fetching LRs with the information about IRQs being 
> processed by a guest. This could happen in guest trap paths where 
> interrupts are enabled before `enter_hypervisor_head()` call.
> 
> Performing `do_IRQ()` prior to `vgic_sync_from_lrs()` is assumed as less 
> efficient than doing it vice versa for high IRQ rate use-cases with the 
> number of different interrupts (specific for multimedia scenarios).

What you suggest here is really specific to your use case. However, 
think about the Real-Time case where you may want to receive your 
interrupt as soon as possible.

In general, keeping the interrupt disabled for a long time is a pretty 
bad idea because you increase latency. Interrupts should only be 
disabled when it is strictly necessary.

At the moment I don't view this change as necessary. But I am happy to 
be proven otherwise.

>   - For the old vgic implementation, `vgic_sync_from_lrs()` release LRs 
> from processed interrupts also shortens `inflight_irqs` sorted list. So 
> `do_IRQ()` has better chances to write IRQ directly to LR instead of 
> saving it into `lr_pending` list and possibly faster insert the new IRQ 
> into `inflight_irqs` list.
Saving to lr_pending means that you have all the LRs full.

I vaguely recall we spoke about it before but I can't find anything in 
my inbox. So can you explain again your use-case, the number of LRs...?

Note this is were writing down everything in the commit message (or 
after ---) helps people to understand where you are coming from.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel