Re: [Minios-devel] [UNIKRAFT PATCH v4] plat/*: Make timer interrupt frequency selectable

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

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?

> +
>  #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"

> 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.

>  
>       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

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel