Re: [Xen-devel] [RFC 5/6] arm64: call enter_hypervisor_head only when it is needed

[Julien Grall <julien.grall@arm.com>]

Hi Andrii,

On 01/08/2019 08:33, Andrii Anisov wrote:
> 
> 
> On 31.07.19 14:02, Julien Grall wrote:
>> Everything is in the hot path here, yet there are a lot of other branches. So 
>> why this branch in particular?
> 
> This branch and function call is particular because I find this piece of code 
> quite confusing:

All the commit message is based on "performance improvement".... Now you are 
selling it as this is confusing. What are the real reasons for this patch?

> 
>>> I'm not seeing any benefits in calling `enter_hypervisor_head()` from 
>>> functions named `do_trap_hyp_*`. That code is confusing for me.
>>> IMHO, dividing `do_trap_irq/fiq()` into guest and hyp specific functions is 
>>> not a big deal. Even for ARM32. Moreover, it will make more obvious the fact 
>>> that nothing from `enter_hypervisor_head()` is done for IRQ traps taken from 
>>> hyp.
> 
> And I believe this patch balances patch "xen/arm: Re-enable interrupt later in 
> the trap path" what you NAKed because of increased IRQ latency.

I never NAKed the patch as you keep claiming it. You are sending a patch without 
any justification three time in a row, so it is normal to request more details 
and to be slightly annoyed.

If you expect me to ack a patch without understanding the implications, then I 
am afraid this is not going to happen. Additionally, it is important to keep 
track of the reasoning of we can come back in 2 years time and find out quickly 
why interrupts are masked for a long period of time.

As I pointed out Xen supports multiple use-cases. I am concerned you are trying 
to optimize for your use-case and disregard the rest. I have actually requested 
more details on your use case to understand a bit more where you are coming 
from. See my e-mail [1].

I know I wrote the patch but it was from debugging other than real improvement. 
 From my understanding, you are using to optimize the case where all LRs are 
full. Is it something you have seen during your testing?

If so, how many LRs does the platform provide? Can you provide more details on 
your use case? I don't need the full details, but roughly the number of 
interrupts used and often they trigger.

Additionally, it would be good to know the usage over the time.  You could 
modify Xen to record the number of LRs used to each entry.

> While them together make the code more straight forward and clear, because:
>   - you start all C-coded common trap handlers with IRQs locked, and free from 
> asm code misunderstanding

Well, there are only one (two if you count the FIQ) case where interrupts are 
kept disabled, this is when receiving an interrupt. I don't see it as a good 
enough justification to have to impose that to all the handlers.

>   - all common trap handlers are distinct in their naming, purpose and action. 
> In terms of calling `enter_hypervisor_head()` only from the traps taken from guest.

There are nearly no difference between receiving an interrupt while running the 
guest mode and while running in hypervisor mode. So what do you really gain with 
the duplication?

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02335.html

-- 
Julien Grall

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