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

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

To: Costin Lupu <costin.lup@xxxxxxxxx>, Yuri Volchkov <yuri.volchkov@xxxxxxxxx>, <minios-devel@xxxxxxxxxxxxx>
From: Florian Schmidt <Florian.Schmidt@xxxxxxxxx>
Date: Wed, 2 Jan 2019 15:28:28 +0100
Cc: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>, Costin Lupu <costin.lupu@xxxxxxxxx>
Delivery-date: Wed, 02 Jan 2019 14:28:39 +0000
List-id: Mini-os development list <minios-devel.lists.xenproject.org>



On 11/7/18 11:11 AM, Costin Lupu wrote:

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.

The idea here was that it's not really a unikraft-specific definition,in that it's a fundamental physical variable. A second will always haveexactly 1000000000 nanoseconds, no more, no less, nowhere. (Yes, there'sadjtime and clock slewing, but that's just silly. ;-) and not my point.)

Honestly, any definition of NSEC_PER_SEC that is NOT that value shouldraise all alarms and transform your computer into a flowerpot. And anequivalent definition in another piece of code would be silentlyignored, because it's not really a redefinition, as far as thepreprocessor is concerned.

Then again, I guess all of those arguments also go for the followingmacros of ukarch_time_nsec_to_sec etc., so... I'll conform to that andmake it UKARCH_TIME_NSEC_TO_SEC if you prefer.

The reason I don't want to keep this change outside is that this patchrelies on having this definition in several places, so I'd rather leavethe definition in include/uk/arch/time.h and make a unified uktimelater. Especially considering this patch has been sitting around forwhat's frankly an embarrassingly long time (not the least due to meletting it fall off my desk), and the treading patch series waits forthis patch to go forward.


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


I'll add that.

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.

OK, gonna add a preprocessor check. I agree it doesn't hurt and works asa sanity check, though we'll be in deep deep troubles WAY before ourtick frequency goes beyond 1193182 Hz. ;-)

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


--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

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

Prev by Date: Re: [Minios-devel] [UNIKRAFT PATCH v3 5/8] plat: Add global struct to keep x86 CPU information
Next by Date: Re: [Minios-devel] [UNIKRAFT PATCH 4/8] lib/uksched: Introduce thread info
Previous by thread: Re: [Minios-devel] [UNIKRAFT PATCH 2/8] lib/uksched: Add function for allocating scheduler common info
Next by thread: Re: [Minios-devel] [UNIKRAFT PATCH 4/8] lib/uksched: Introduce thread info
Index(es):
- Date
- Thread

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