Message ID | 20190503213249.30008-3-johannes@sipsolutions.net |
---|---|
State | Superseded |
Headers | show |
Series | [1/3] arch: um: fix os_timer_one_shot() | expand |
On Fri, 2019-05-03 at 21:32 +0000, Johannes Berg wrote: > > @@ -65,12 +75,17 @@ int os_timer_set_interval(void) > > if (timer_settime(event_high_res_timer, 0, &its, NULL) == -1) > return -errno; > +#else > + timer_enabled = true; > + timer_expiry = virtual_time + UM_NSEC_PER_SEC / UM_HZ; > +#endif This is broken in two ways - first of all, I missed the interval completely, and secondly if you end up with a process that does something like x = gettime(); while (x == gettime()) { /* nothing */ } then it gets stuck forever ... I have a new version almost ready. johannes
On 04/05/2019 20:05, Johannes Berg wrote: > On Fri, 2019-05-03 at 21:32 +0000, Johannes Berg wrote: >> >> @@ -65,12 +75,17 @@ int os_timer_set_interval(void) >> >> if (timer_settime(event_high_res_timer, 0, &its, NULL) == -1) >> return -errno; >> +#else >> + timer_enabled = true; >> + timer_expiry = virtual_time + UM_NSEC_PER_SEC / UM_HZ; >> +#endif > > This is broken in two ways - first of all, I missed the interval > completely, and secondly if you end up with a process that does > something like > > x = gettime(); > while (x == gettime()) { /* nothing */ } > > then it gets stuck forever ... > > I have a new version almost ready. > > johannes > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um > I had a quick look at them. 1 and 2 are clear and I will most likely ack them once you have a final version. I can understand your use case for patch 3. What I am not clear is where should that config option live. It should be somewhere under debug/kernel hacking with a sufficient number of big fat warnings so people do not enable it accidentally by mistake. Ideas?
On Sat, 2019-05-04 at 21:42 +0100, Anton Ivanov wrote: > > I had a quick look at them. Thanks. > 1 and 2 are clear and I will most likely ack them once you have a final > version. I think those two are OK now really. I think I'll put another small bit into patch 2: int os_timer_one_shot(unsigned long ticks) { - struct itimerspec its; - unsigned long long nsec; - unsigned long sec; + unsigned long long nsec = ticks + 1; + struct itimerspec its = { + .it_value.tv_sec = nsec / UM_NSEC_PER_SEC, + .it_value.tv_nsec = nsec % UM_NSEC_PER_SEC, - nsec = (ticks + 1); - sec = nsec / UM_NSEC_PER_SEC; - nsec = nsec % UM_NSEC_PER_SEC; - - its.it_value.tv_sec = nsec / UM_NSEC_PER_SEC; - its.it_value.tv_nsec = nsec; - - its.it_interval.tv_sec = 0; - its.it_interval.tv_nsec = 0; // we cheat here + .it_interval.tv_sec = 0, + .it_interval.tv_nsec = 0, // we cheat here + }; which almost seems like a bugfix too - note how the computed value "sec" is never used in the original code? FYI, I also sent a patch to netdev for a fix, just in case you run into that: https://patchwork.ozlabs.org/patch/1095009/ (compilation of this is broken right now) > I can understand your use case for patch 3. What I am not clear is where > should that config option live. It should be somewhere under > debug/kernel hacking with a sufficient number of big fat warnings so > people do not enable it accidentally by mistake. Yeah, good question. No real objection to moving it there. There are some quirks with UML still, so I'm trying to figure out if I can do the same in KVM, but it's kinda cool I can do it at all :-) johannes
On Sat, May 4, 2019 at 10:43 PM Anton Ivanov <anton.ivanov@cambridgegreys.com> wrote: > On 04/05/2019 20:05, Johannes Berg wrote: > > On Fri, 2019-05-03 at 21:32 +0000, Johannes Berg wrote: > I can understand your use case for patch 3. What I am not clear is where > should that config option live. It should be somewhere under > debug/kernel hacking with a sufficient number of big fat warnings so > people do not enable it accidentally by mistake. I think enabling the config option should not also enable the feature, but just add code that allows to enable the feature by a kernel command line option, and/or by a special file in debugfs. Gr{oetje,eeting}s, Geert
On Mon, 2019-05-06 at 09:45 +0200, Geert Uytterhoeven wrote: > On Sat, May 4, 2019 at 10:43 PM Anton Ivanov > <anton.ivanov@cambridgegreys.com> wrote: > > On 04/05/2019 20:05, Johannes Berg wrote: > > > On Fri, 2019-05-03 at 21:32 +0000, Johannes Berg wrote: > > > > I can understand your use case for patch 3. What I am not clear is where > > should that config option live. It should be somewhere under > > debug/kernel hacking with a sufficient number of big fat warnings so > > people do not enable it accidentally by mistake. > > I think enabling the config option should not also enable the feature, but > just add code that allows to enable the feature by a kernel command line > option, and/or by a special file in debugfs. Hmm. Might be doable with a command line argument, I don't think having the kernel boot in "real time" and then continue running at some point from "virtual time" when enabled via debugfs makes sense, and I also don't see how you could really switch back and forth without a lot of effort in the code. johannes
diff --git a/arch/um/Kconfig b/arch/um/Kconfig index ec9711d068b7..181ffddc03fc 100644 --- a/arch/um/Kconfig +++ b/arch/um/Kconfig @@ -180,6 +180,23 @@ config SECCOMP If unsure, say Y. +config UML_VIRTUAL_TIME + bool + prompt "Use virtual time (e.g. for test execution)" + help + Use this option to use virtual time inside the UML instance, + which means that whenever there's nothing to do it just skips + time forward rather than waiting for any real time to elapse. + + Note that this changes behaviour a bit - used CPU time doesn't + cause the virtual time to increase. + + When in doubt, say N. + + In particular, do NOT set this option if you plan on using the + system interactively as that will just cause it to busy-loop + while waiting for you to enter input. + endmenu source "arch/um/drivers/Kconfig" diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c index 3e31d7ba84c6..d27e6cf43904 100644 --- a/arch/um/kernel/time.c +++ b/arch/um/kernel/time.c @@ -134,3 +134,10 @@ void __init time_init(void) timer_set_signal_handler(); late_time_init = um_timer_setup; } + +#ifdef CONFIG_UML_VIRTUAL_TIME +unsigned long calibrate_delay_is_known(void) +{ + return 1; +} +#endif diff --git a/arch/um/os-Linux/time.c b/arch/um/os-Linux/time.c index 6a60e7506f8d..8d5c0260c627 100644 --- a/arch/um/os-Linux/time.c +++ b/arch/um/os-Linux/time.c @@ -15,8 +15,15 @@ #include <os.h> #include <string.h> #include <timer-internal.h> +#include <generated/autoconf.h> +#ifdef CONFIG_UML_VIRTUAL_TIME +static unsigned long long virtual_time; +static unsigned long long timer_expiry; +static bool timer_enabled; +#else static timer_t event_high_res_timer = 0; +#endif static inline long long timeval_to_ns(const struct timeval *tv) { @@ -42,16 +49,19 @@ long long os_persistent_clock_emulation(void) */ int os_timer_create(void) { +#ifndef CONFIG_UML_VIRTUAL_TIME timer_t *t = &event_high_res_timer; if (timer_create(CLOCK_MONOTONIC, NULL, t) == -1) return -1; +#endif return 0; } int os_timer_set_interval(void) { +#ifndef CONFIG_UML_VIRTUAL_TIME struct itimerspec its; unsigned long long nsec; @@ -65,12 +75,17 @@ int os_timer_set_interval(void) if (timer_settime(event_high_res_timer, 0, &its, NULL) == -1) return -errno; +#else + timer_enabled = true; + timer_expiry = virtual_time + UM_NSEC_PER_SEC / UM_HZ; +#endif return 0; } int os_timer_one_shot(unsigned long ticks) { +#ifndef CONFIG_UML_VIRTUAL_TIME struct itimerspec its; unsigned long long nsec; unsigned long sec; @@ -86,6 +101,11 @@ int os_timer_one_shot(unsigned long ticks) its.it_interval.tv_nsec = 0; // we cheat here timer_settime(event_high_res_timer, 0, &its, NULL); +#else + timer_enabled = true; + timer_expiry = virtual_time + ticks + 1; +#endif + return 0; } @@ -94,18 +114,26 @@ int os_timer_one_shot(unsigned long ticks) */ void os_timer_disable(void) { +#ifndef CONFIG_UML_VIRTUAL_TIME struct itimerspec its; memset(&its, 0, sizeof(struct itimerspec)); timer_settime(event_high_res_timer, 0, &its, NULL); +#else + timer_enabled = false; +#endif } long long os_nsecs(void) { +#ifndef CONFIG_UML_VIRTUAL_TIME struct timespec ts; clock_gettime(CLOCK_MONOTONIC,&ts); return timespec_to_ns(&ts); +#else + return virtual_time; +#endif } /** @@ -114,6 +142,7 @@ long long os_nsecs(void) */ void os_idle_sleep(unsigned long long nsecs) { +#ifndef CONFIG_UML_VIRTUAL_TIME struct timespec ts; ts = ((struct timespec) { @@ -126,4 +155,16 @@ void os_idle_sleep(unsigned long long nsecs) */ if (clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, NULL)) deliver_alarm(); +#else + unsigned long long next = virtual_time + nsecs; + + if (timer_enabled && timer_expiry <= next) { + virtual_time = timer_expiry; + timer_enabled = false; + deliver_alarm(); + return; + } + + virtual_time = next; +#endif }