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
