[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] Fw: [UNIKRAFT PATCH 7/7] plat/common: Implement generic_timer_cpu_block
On 18/01/2019 13:12, Jia He wrote:
Hi Julien
Hi,
On 2019/1/18 20:57, Justin He (Arm Technology China) wrote:
From: Julien Grall <julien.grall@xxxxxxxxxx>
Sent: Friday, December 14, 2018 7:00 PM
To: Wei Chen (Arm Technology China); minios-devel@xxxxxxxxxxxxxxxxxxxx;
simon.kuenzer@xxxxxxxxx; florian.schmidt@xxxxxxxxx; yuri.volchkov@xxxxxxxxx;
Sharan.Santhanam@xxxxxxxxx; Felipe.Huici@xxxxxxxxx
Cc: Kaly Xin (Arm Technology China); nd; Jianyong Wu (Arm Technology China);
Justin He (Arm Technology China)
Subject: Re: [Minios-devel] [UNIKRAFT PATCH 7/7] plat/common: Implement
generic_timer_cpu_block
Hi Wei,
On 13/12/2018 09:19, Wei Chen wrote:
This function will be used when system enter sleep and need wakeup
in a specific time. For ns_to_ticks precision, we limited the max
sleep time to 3600 seconds.
Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
---
plat/common/arm/time.c | 78 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 73 insertions(+), 5 deletions(-)
diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c
index ae539a0..46f5f93 100644
--- a/plat/common/arm/time.c
+++ b/plat/common/arm/time.c
@@ -76,6 +76,11 @@ static uint32_t tick_per_ns;
*/
#define __MAX_CONVERT_SECS 3600UL
+/*
+ * Minimum delta to sleep using generic timer.
+ */
+static uint32_t counter_mini_delta;
+
/* How many nanoseconds per second */
#define NSEC_PER_SEC ukarch_time_sec_to_nsec(1)
@@ -219,6 +224,67 @@ static uint64_t generic_timer_epochoffset(void)
return 0;
}
+/*
+ * Returns early if any interrupts are serviced, or if the requested delay is
+ * too short. Must be called with interrupts disabled, will enable interrupts
+ * "atomically" during idle loop.
+ *
+ * This function must be called only from the scheduler. It will screw
+ * your system if you do otherwise. And, there is no reason you
+ * actually want to use it anywhere else. THIS IS NOT A YIELD or any
+ * kind of mutex_lock. It will simply halt the cpu, not allowing any
+ * other thread to execute.
+ */
+static void generic_timer_cpu_block(uint64_t until_ns)
+{
+ uint64_t now_ns, delta_ns;
+ uint64_t now_ticks, delta_ticks;
+
+ UK_ASSERT(ukplat_lcpu_irqs_disabled());
+
+ /* Record current ticks */
+ now_ticks = generic_timer_get_ticks();
+ now_ns = ticks_to_ns(now_ticks - boot_ticks);
+
+ /*
+ * Compute delta in counter ticks. Return if it is less than minimum
+ * safe amount of ticks. Essentially this will cause us to spin until
+ * the timeout.
+ */
+ delta_ns = until_ns - now_ns;
+ delta_ticks = ns_to_ticks(delta_ns);
+ if (delta_ticks < counter_mini_delta) {
+ /*
+ * Since we are "spinning", quickly enable interrupts in
+ * the hopes that we might get new work and can do something
+ * else than spin.
+ */
+ ukplat_lcpu_enable_irq();
+ nop();
Why do you need a nop() here? Should not just be sufficient to do enable ->
disable?
This is to keep pace with unikraft X86 timer codes.
And the X86 codes are derived from solo5 implementation.
https://github.com/Solo5/solo5/commit/73d7cb8e#diff-0d26d6b707a6a7d2dcac185bc0821ef5R428
While I understand we want to keep the behavior of a function similar between
x86 and arm, we should not do a 1 to 1 instruction match.
If the only reason for the NOP is to follow x86, then this should be removed. If
not, then please explain the rationale for it.
[...]
static int generic_timer_init(int fdt_timer)
{
/* Get counter frequency from DTB or register */
@@ -244,6 +310,12 @@ static int generic_timer_init(int fdt_timer)
/* We disallow zero ns_per_tick */
UK_BUGON(!tick_per_ns);
+ /*
+ * Set minimal counter delta, programming seems to have an overhead
+ * of 3-4us, but play it safe here.
Overhead on which platform? Bear in mind that the Arm Arm does not tell you how
long it will take to write to the timer. So you at least need to explain how you
found out that.
I tested in qemu guest (-machine virt -cpu a53) in a thunder X2
the minimal overhead of generic_timer_cpu_block is about 800 ticks (12us).
So I would set the min value to 20us in the next version.
What's your opnion?
You are basing your number in a KVM environment under a specific machine. In the
case of KVM, a WFI will imply a trap and potential a re-schedule. So I think
this is not correct because different virtualization or baremetal solution may
have a really small overhead.
From my understanding of the x86 code, they are looking at the overhead to
write into the timer comparison register and not the cost of the function.
But I don't think you can even have a correct number here. Now the question is
do you really have place in unikraft with such small deadline?
Cheers,
--
Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
