|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH v4] plat/*: Make timer interrupt frequency selectable
Hi Yuri,
Thanks for your review. Please see my comments inline.
On 11/5/18 7:47 PM, Yuri Volchkov wrote:
> Hi Florian,
>
> generally looks good to me, just a couple of comments inline.
>
> - Yuri.
>
> Florian Schmidt <florian.schmidt@xxxxxxxxx> writes:
>
>> Add new configuration options for choosing a timer interrupt frequency.
>> The configured frequency is converted to the timer tick length which can
>> be of use for other modules (e.g., preemptive schedulers).
>>
>> Previously, the tick was 100 Hz on KVM and 1000 Hz on Xen. The default
>> value is now 100 Hz across both platforms.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
>> ---
>> include/uk/arch/time.h | 2 ++
>> include/uk/plat/time.h | 4 ++++
>> plat/Config.uk | 7 +++++++
>> plat/kvm/x86/tscclock.c | 10 ++++------
>> plat/linuxu/include/linuxu/time.h | 3 ++-
>> plat/linuxu/time.c | 2 --
>> plat/xen/x86/arch_time.c | 2 +-
>> 7 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/uk/arch/time.h b/include/uk/arch/time.h
>> index 262fd3b..5a5aa22 100644
>> --- a/include/uk/arch/time.h
>> +++ b/include/uk/arch/time.h
>> @@ -57,6 +57,8 @@ typedef __s64 __snsec;
>> #define __SNSEC_MAX (__S64_MAX)
>> #define __SNSEC_MIN (__S64_MIN)
>>
>> +#define NSEC_PER_SEC 1000000000ULL
> Now, since it is in the public header, should we add a prefix to the
> define to avoid possible conflicts?
>
I'd rather revert this change and keep it for a next patch because:
- It's also defined in plat/common/arm/time.c
- In deed it needs a prefix, but it shouldn't have anything to do with
arch since it's just an utility definition, just like the
ukarch_time_nsec_to_* macros. I'd rather move all these time related
definitions into a time library. And since we have uktimeconv, I would
even rename that to uktime and use it as the library for all time
functions in Unikraft.
@Simon, what do you think?
>> +
>> #define ukarch_time_nsec_to_sec(ns) ((ns) / 1000000000ULL)
>> #define ukarch_time_nsec_to_msec(ns) ((ns) / 1000000ULL)
>> #define ukarch_time_nsec_to_usec(ns) ((ns) / 1000UL)
>> diff --git a/include/uk/plat/time.h b/include/uk/plat/time.h
>> index 202e0f9..d38e3c6 100644
>> --- a/include/uk/plat/time.h
>> +++ b/include/uk/plat/time.h
>> @@ -47,6 +47,10 @@ void ukplat_time_fini(void);
>>
>> __nsec ukplat_monotonic_clock(void);
>>
>> +/* Time tick length */
>> +#define UKPLAT_TIME_TICK_NSEC (NSEC_PER_SEC / CONFIG_HZ)
>> +#define UKPLAT_TIME_TICK_MSEC
>> ukarch_time_nsec_to_msec(UKPLAT_TIME_TICK_NSEC)
>> +
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/plat/Config.uk b/plat/Config.uk
>> index b776c45..ae96487 100644
>> --- a/plat/Config.uk
>> +++ b/plat/Config.uk
>> @@ -17,3 +17,10 @@ config EARLY_PRINT_PL011_UART_ADDR
>> Pl011 serial address used by early debug console.
>>
>> endmenu
>> +
>> +config HZ
>> + int
>> + prompt "Timer frequency (Hz)"
>> + default 100
>> + help
>> + Configure the timer interrupt frequency.
> I would to the help message "change only if you are sure you know what
> you are doing"
>
I totally agree.
>> diff --git a/plat/kvm/x86/tscclock.c b/plat/kvm/x86/tscclock.c
>> index f3fa55a..d3cdb16 100644
>> --- a/plat/kvm/x86/tscclock.c
>> +++ b/plat/kvm/x86/tscclock.c
>> @@ -60,8 +60,6 @@
>> #include <uk/assert.h>
>> #include <uk/bitops.h>
>>
>> -#define NSEC_PER_SEC 1000000000ULL
>> -
>> #define TIMER_CNTR 0x40
>> #define TIMER_MODE 0x43
>> #define TIMER_SEL0 0x00
>> @@ -131,7 +129,7 @@ static void i8254_delay(unsigned int n)
>> {
>> unsigned int cur_tick, initial_tick;
>> int remaining;
>> - const unsigned long timer_rval = TIMER_HZ / 100;
>> + const unsigned long timer_rval = TIMER_HZ / CONFIG_HZ;
> I guess at least we need to check if TIMER_HZ / CONFIG_HZ >= 1 at
> compile time.
>
This makes sense.
>>
>> initial_tick = i8254_gettick();
>>
>> @@ -211,10 +209,10 @@ int tscclock_init(void)
>> {
>> __u64 tsc_freq, rtc_boot;
>>
>> - /* Initialise i8254 timer channel 0 to mode 2 at 100 Hz */
>> + /* Initialise i8254 timer channel 0 to mode 2 at CONFIG_HZ frequency */
>> outb(TIMER_MODE, TIMER_SEL0 | TIMER_RATEGEN | TIMER_16BIT);
>> - outb(TIMER_CNTR, (TIMER_HZ / 100) & 0xff);
>> - outb(TIMER_CNTR, (TIMER_HZ / 100) >> 8);
>> + outb(TIMER_CNTR, (TIMER_HZ / CONFIG_HZ) & 0xff);
>> + outb(TIMER_CNTR, (TIMER_HZ / CONFIG_HZ) >> 8);
>>
>> /*
>> * Read RTC "time at boot". This must be done just before tsc_base is
>> diff --git a/plat/linuxu/include/linuxu/time.h
>> b/plat/linuxu/include/linuxu/time.h
>> index 2df881e..c1a875a 100644
>> --- a/plat/linuxu/include/linuxu/time.h
>> +++ b/plat/linuxu/include/linuxu/time.h
>> @@ -35,9 +35,10 @@
>> #ifndef __LINUXU_TIME_H__
>> #define __LINUXU_TIME_H__
>>
>> +#include <uk/plat/time.h>
>> #include <linuxu/signal.h>
>>
>> -#define TIMER_INTVAL_MSEC 10
>> +#define TIMER_INTVAL_NSEC UKPLAT_TIME_TICK_NSEC
>> #define TIMER_SIGNUM SIGALRM
>>
>>
>> diff --git a/plat/linuxu/time.c b/plat/linuxu/time.c
>> index ead07f5..13439ad 100644
>> --- a/plat/linuxu/time.c
>> +++ b/plat/linuxu/time.c
>> @@ -40,8 +40,6 @@
>> #include <linuxu/syscall.h>
>> #include <linuxu/time.h>
>>
>> -#define TIMER_INTVAL_NSEC ukarch_time_msec_to_nsec(TIMER_INTVAL_MSEC)
>> -
>> static k_timer_t timerid;
>>
>>
>> diff --git a/plat/xen/x86/arch_time.c b/plat/xen/x86/arch_time.c
>> index 95d7b10..a4b77b9 100644
>> --- a/plat/xen/x86/arch_time.c
>> +++ b/plat/xen/x86/arch_time.c
>> @@ -233,7 +233,7 @@ void time_block_until(__snsec until)
>> static void timer_handler(evtchn_port_t ev __unused,
>> struct __regs *regs __unused, void *ign __unused)
>> {
>> - __nsec until = ukplat_monotonic_clock() + ukarch_time_msec_to_nsec(1);
>> + __nsec until = ukplat_monotonic_clock() + UKPLAT_TIME_TICK_NSEC;
>>
>> HYPERVISOR_set_timer_op(until);
>> }
>> --
>> 2.19.1
>>
>>
>> _______________________________________________
>> Minios-devel mailing list
>> Minios-devel@xxxxxxxxxxxxxxxxxxxx
>> https://lists.xenproject.org/mailman/listinfo/minios-devel
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |