[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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?


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

Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.