|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH v2] plat/*: Configure timer interrupt frequency
Hi Florian,
Thanks for the review. Please see me answers inline.
On 10/2/18 9:59 AM, Florian Schmidt wrote:
> Hi Costin,
>
> thanks for the patch. I have a few comments inline.
>
> On 09/18/2018 05:19 PM, Costin Lupu wrote:
>> Add new configuration options for choosing a timer interrupt frequency.
>> This patch adds only 100 Hz and 1000 Hz as currently supported values.
>
> Why choose only those two? Wouldn't it make more sense to just make this
> an integer field and let whoever builds unikraft choose their preferred
> frequency? While I like the patch idea to allow the developer freedom in
> setting the tick, I'm not sure I like stopping halfway and only giving
> two choices.
>
I find allowing users the freedom to set any frequency they like to have
2 disadvantages:
1. The tick frequency should be chosen based on a prior analysis
depending on the application. An integer field would allow users to set
any frequency they want whether they know what they're doing or not. I
find this level of freedom dangerous here.
2. A given frequency value may not be supported by the underlying timers
and therefore it would either have no effect (e.g. being ignored) or
generate an incorrect behavior.
Now regarding the number of options, two should be enough for making the
case for how we can configure the timer. Please feel free to amend this
patch or to submit another one for extending the number of options.
>> The configured frequency is converted to the timer tick length which can
>> be of use for other modules (e.g. preemptive schedulers).
>>
>> Since the frequency configuration is common to all platforms, we
>> introduce the plat/common/time.c for keeping time related functionality
>> which is common to all platforms.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>> include/uk/plat/time.h | 4 ++++
>> plat/Config.uk | 17 +++++++++++++++
>> plat/common/time.c | 45
>> +++++++++++++++++++++++++++++++++++++++
>> plat/kvm/Makefile.uk | 1 +
>> plat/kvm/x86/time.c | 2 ++
>> plat/kvm/x86/tscclock.c | 8 +++----
>> plat/linuxu/Makefile.uk | 1 +
>> plat/linuxu/include/linuxu/time.h | 3 ++-
>> plat/linuxu/time.c | 1 +
>> plat/xen/Makefile.uk | 1 +
>> plat/xen/x86/arch_time.c | 3 ++-
>> 11 files changed, 80 insertions(+), 6 deletions(-)
>> create mode 100644 plat/common/time.c
>>
>> diff --git a/include/uk/plat/time.h b/include/uk/plat/time.h
>> index 202e0f9..0fde1c6 100644
>> --- a/include/uk/plat/time.h
>> +++ b/include/uk/plat/time.h
>> @@ -44,9 +44,13 @@ extern "C" {
>> void ukplat_time_init(void);
>> void ukplat_time_fini(void);
>> +void ukplat_time_frequency_init(void);
>> __nsec ukplat_monotonic_clock(void);
>> +/* Time tick length */
>> +extern __nsec ukplat_time_tick_nsec;
>> +
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/plat/Config.uk b/plat/Config.uk
>> index b776c45..849d7c2 100644
>> --- a/plat/Config.uk
>> +++ b/plat/Config.uk
>> @@ -17,3 +17,20 @@ config EARLY_PRINT_PL011_UART_ADDR
>> Pl011 serial address used by early debug console.
>> endmenu
>> +
>> +choice
>> + prompt "Timer frequency"
>> + default HZ_100
>> + help
>> + Configure the timer interrupt frequency.
>> +
>> + config HZ_100
>> + bool "100 HZ"
>> + config HZ_1000
>> + bool "1000 HZ"
>> +endchoice
>> +
>> +config HZ
>> + int
>> + default 100 if HZ_100
>> + default 1000 if HZ_1000
>> diff --git a/plat/common/time.c b/plat/common/time.c
>> new file mode 100644
>> index 0000000..0c492a0
>> --- /dev/null
>> +++ b/plat/common/time.c
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in
>> the
>> + * documentation and/or other materials provided with the
>> distribution.
>> + * 3. Neither the name of the copyright holder nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> from
>> + * this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
>> CONTRIBUTORS "AS IS"
>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>> TO, THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>> PURPOSE
>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
>> CONTRIBUTORS BE
>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> BUSINESS
>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
>> WHETHER IN
>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>> OTHERWISE)
>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
>> ADVISED OF THE
>> + * POSSIBILITY OF SUCH DAMAGE.
>> + *
>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>> + */
>> +
>> +#include <uk/config.h>
>> +#include <uk/print.h>
>> +#include <uk/plat/time.h>
>> +
>> +__nsec ukplat_time_tick_nsec;
>> +
>> +void ukplat_time_frequency_init(void)
>> +{
>> + uk_printd(DLVL_INFO, "Timer interrupt frequency is %u Hz\n",
>> CONFIG_HZ);
>> + ukplat_time_tick_nsec = ukarch_time_msec_to_nsec(1000 / CONFIG_HZ);
>> +}
>> diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
>
> I'm not sure I see the point in this file at this point in time. All it
> does is have a function (that is only called from once, from one place
> in the code) that sets a global variable to a value known at compile
> time, which is also only used once. I'd scrap all this and use (1000 /
> CONFIG_HZ) in plat/xen/x86/arch_time.c directly.
>
Alright. I will introduce a macro definition instead for the time tick
in nanoseconds in v2.
>> index b493bba..c57a419 100644
>> --- a/plat/kvm/Makefile.uk
>> +++ b/plat/kvm/Makefile.uk
>> @@ -73,6 +73,7 @@ LIBKVMPLAT_SRCS-y +=
>> $(LIBKVMPLAT_BASE)/irq.c
>> LIBKVMPLAT_SRCS-y += $(LIBKVMPLAT_BASE)/io.c
>> LIBKVMPLAT_SRCS-y += $(UK_PLAT_COMMON_BASE)/lcpu.c|common
>> LIBKVMPLAT_SRCS-y +=
>> $(UK_PLAT_COMMON_BASE)/memory.c|common
>> +LIBKVMPLAT_SRCS-y += $(UK_PLAT_COMMON_BASE)/time.c|common
>> ##
>> ## PCI library definitions
>> diff --git a/plat/kvm/x86/time.c b/plat/kvm/x86/time.c
>> index 3d8a842..50ce470 100644
>> --- a/plat/kvm/x86/time.c
>> +++ b/plat/kvm/x86/time.c
>> @@ -69,6 +69,8 @@ void ukplat_time_init(void)
>> {
>> int rc;
>> + ukplat_time_frequency_init();
>> +
>> rc = ukplat_irq_register(0, timer_handler, NULL);
>> if (rc < 0)
>> UK_CRASH("Failed to register timer interrupt handler\n");
>> diff --git a/plat/kvm/x86/tscclock.c b/plat/kvm/x86/tscclock.c
>> index 8961659..32de9d6 100644
>> --- a/plat/kvm/x86/tscclock.c
>> +++ b/plat/kvm/x86/tscclock.c
>> @@ -131,7 +131,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;
>> initial_tick = i8254_gettick();
>> @@ -211,10 +211,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/Makefile.uk b/plat/linuxu/Makefile.uk
>> index b00540c..d37a960 100644
>> --- a/plat/linuxu/Makefile.uk
>> +++ b/plat/linuxu/Makefile.uk
>> @@ -34,3 +34,4 @@ LIBLINUXUPLAT_SRCS-y +=
>> $(LIBLINUXUPLAT_BASE)/irq.c
>> LIBLINUXUPLAT_SRCS-y += $(LIBLINUXUPLAT_BASE)/time.c
>> LIBLINUXUPLAT_SRCS-y +=
>> $(UK_PLAT_COMMON_BASE)/lcpu.c|common
>> LIBLINUXUPLAT_SRCS-y +=
>> $(UK_PLAT_COMMON_BASE)/memory.c|common
>> +LIBLINUXUPLAT_SRCS-y +=
>> $(UK_PLAT_COMMON_BASE)/time.c|common
>> diff --git a/plat/linuxu/include/linuxu/time.h
>> b/plat/linuxu/include/linuxu/time.h
>> index 2df881e..c5e804e 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/config.h>
>> #include <linuxu/signal.h>
>> -#define TIMER_INTVAL_MSEC 10
>> +#define TIMER_INTVAL_MSEC (1000 / CONFIG_HZ)
>> #define TIMER_SIGNUM SIGALRM
>> diff --git a/plat/linuxu/time.c b/plat/linuxu/time.c
>> index ead07f5..b5b1295 100644
>> --- a/plat/linuxu/time.c
>> +++ b/plat/linuxu/time.c
>> @@ -76,6 +76,7 @@ void ukplat_time_init(void)
>> struct k_itimerspec its;
>> int rc;
>> + ukplat_time_frequency_init();
>> ukplat_irq_register(TIMER_SIGNUM, timer_handler, NULL);
>> memset(&sigev, 0, sizeof(sigev));
>> diff --git a/plat/xen/Makefile.uk b/plat/xen/Makefile.uk
>> index 76a3264..e001133 100644
>> --- a/plat/xen/Makefile.uk
>> +++ b/plat/xen/Makefile.uk
>> @@ -29,6 +29,7 @@ LIBXENPLAT_SRCS-y +=
>> $(LIBXENPLAT_BASE)/hypervisor.c
>> LIBXENPLAT_SRCS-y += $(LIBXENPLAT_BASE)/memory.c
>> LIBXENPLAT_SRCS-y += $(UK_PLAT_COMMON_BASE)/lcpu.c|common
>> LIBXENPLAT_SRCS-y +=
>> $(UK_PLAT_COMMON_BASE)/memory.c|common
>> +LIBXENPLAT_SRCS-y += $(UK_PLAT_COMMON_BASE)/time.c|common
>> ifneq (,$(filter x86_32 x86_64,$(CONFIG_UK_ARCH)))
>> LIBXENPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
>> $(UK_PLAT_COMMON_BASE)/x86/trace.c|common
>> diff --git a/plat/xen/x86/arch_time.c b/plat/xen/x86/arch_time.c
>> index f96426f..89b5045 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;
>
> This changes the current behavior, right? at the moment, this sets a
> time one msec into the future, but afterwards, it's one tick into the
> future, which can happen to be 1msec, but likely (and by default with
> CONFIG_HZ = 100) isn't. Is there are an underlying reason for this
> change? And if so, I'd feel more comfortable with this as a separate
> patch (because it changes functionality) or at least a mention in the
> commit description.
You're right we should outline this change. I'll mention it in the
commit message for v2.
>> HYPERVISOR_set_timer_op(until);
>> }
>> @@ -244,6 +244,7 @@ static evtchn_port_t port;
>> void ukplat_time_init(void)
>> {
>> uk_printd(DLVL_EXTRA, "Initializing timer interface\n");
>> + ukplat_time_frequency_init();
>> port = bind_virq(VIRQ_TIMER, &timer_handler, NULL);
>> unmask_evtchn(port);
>> }
>>
>
> Cheers,
> Florian
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |