Support domain schedule units (us & ticks)#95
Conversation
|
Generally, this would be good addition, but we do need to retain the ability to specify the duration in ticks, so we can't just change the unit. We'll need a mechanism to specify the unit. The whole point of making the interface be ticks is that the FIXME for AArch32 that is still in there is not nicely solvable in general (the method the kernel currently has is flawed in multiple ways). It does not necessarily need to be fully solved in general, because on the platforms where it is problematic you likely to want to compute in ticks anyway. I.e. if we use the current (flawed) kernel method for conversion on AArch32 but also have a capDL ticks interface available, the flaws are avoidable for the systems where that matters. |
| * timer frequency to us at build time, in Hertz */ | ||
| uin64_t timer_frequency = CONFIG_TIMER_FREQUENCY; | ||
|
|
||
| /* FIXME: On ARM32 we need to pull in a 64-bit division helper; |
There was a problem hiding this comment.
Those should be part of the compiler built-in already: There is an __aeabi_uldivmod which will be used automatically for 64-bit divisions on 32-bit Arm.
This is one of the reasons why doing this in user space is easier than in the kernel.
There was a problem hiding this comment.
Well, that's part of libgcc (GCC) or compiler-rt (LLVM), which may or may not be linked in the builds here; however, broadly yes.
There was a problem hiding this comment.
I suppose it depends on how verified you want your user space to be, that's the only reason I can think of for not linking to the compiler library in user space. But I don't see anyone putting that amount of effort into this for 32-bit platforms. (And if they do, they can always use the ticks interface if this is a problem for verification.)
There was a problem hiding this comment.
OK, it looks like maybe because the frequency is a constant this is elided entirely. That or it is already linked, so this just does work for arm32.
There was a problem hiding this comment.
Easy to check with a disassemble. But if the value is constant the division is probably replaced with shifts and multiplies. If frequency is a whole MHz, the calculation can be transformed back to duration_us multiplied by a constant. The maaxboard has frequency 8333333, that's probably a better test.
| Note: there are some values of us which would cause unrepresentable | ||
| values in ticks; we should add a static check to error out. |
There was a problem hiding this comment.
Yes, that's the main question: How do you deal with non-whole MHz clock frequencies. If there is no exact conversion you at least want to warn the user about this. Ideally the user should be able to specify whether a time needs to be exact or not.
Hrm. I don't think I know if there's any cases like this, I suppose... Might need to hand-roll something specific for units here. ACK, this is just an implementation thing mostly then. |
Yes, no problem with us from the conceptual side, as long as we can also do ticks. |
|
I haven't tested this at runtime yet (working on it), but in theory the capDL spec now supports units, parses them, and then sends them to the C loader which either computes ticks at runtime or uses the us value. Here's what that looks like: The canonicalised becomes: |
| typedef struct { | ||
| /* kind of the schedentry: a ticks entry, us entry, or an end marker */ | ||
| CDL_DomainSchedEntryKind_t kind; | ||
| /* 8-bit domain number */ | ||
| uint8_t domain; | ||
| /* 56-bit duration */ | ||
| uint64_t duration; | ||
| } CDL_DomainSchedEntry; |
There was a problem hiding this comment.
This structure has padding, I thought it was preferred to avoid that for verification?
There was a problem hiding this comment.
Since we're not currently verifying the C version of the loader, we can be leave this for now. When we do at a later point, it'll not be too hard to add a manual padding field, which should not be a breaking change. So I don't think it's a problem for now.
f8d9ef5 to
e28d1f4
Compare
|
I will note that if I specify a very large number in the camkes domain file it will silently truncate this to 64 bits which is weird, but I can now test invalid input cases. |
e28d1f4 to
b9fce32
Compare
Style checker was complaining. Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
In the work on [RFC-20] it was proposed that it would make sense to remove the (micro)seconds-to-ticks conversions from seL4 and migrate all of (MCS) userspace APIs to be in ticks, to make dealing with these calculations and their trickiness to instead be done at user-level - and the tradeoffs one has to make to be no longer kernel policy. This PR demonstrates implements this for the capDL initialiser. For other cases supported by the capDL specification, such (MCS) budgets and periods currently specified in microseconds, the capDL initialiser should be able to paper over the change in kernel APIs without the users needing to deal with this distinction. These changes are motivated by the [Domain schedules] PR to the seL4 Microkit, which creates a capDL specification for the (Rust version) of the capDL initialiser to use. At the moment, this PR is unable to be consistent about the values exposed to the user for the units of the domain schedule: whilst we can statically determine the us->ticks conversion for ARM/RISC-V platforms, on x86 this is a runtime-known value only. Since the spec is packed at system-build-time, we cannot therefore specify x86 domain schedules in a consistent set of units. The concrete specification changes from this PR is the addition of a 'unit' suffix inside the domain schedule syntax. The initialiser can then support either ticks (the default without any units) or microseconds, and it performs a runtime conversion of this value to ticks for the domain scheduler API. Canonically, the end marker is left without units as (0, 0), but we permit inputs of (0, 0 ticks) or (0, 0 us) for consistency. Note: the meaning of a kernel tick is different between MCS and non-MCS configurations. One consequence of taking input in 'us' is that there are cases where the input is unrepresentable in 'ticks'; and so this introduces the possiblity of failure in the initialiser that is unchecked at build-time. For non-MCS, as kernel ticks are an integer multiple of 1 ms, the conversion from us to ticks will fail if the conversion is not exact. This is easy to avoid, one has to specify a 'us' value that is a multiple of (CONFIG_TIMER_TICK_MS * 1000), something which does not differ per platform. For MCS, the kernel tick reflects the underlying timer frequency (for the domain scheduler). Thus, there exists many frequencies for which it is impossible to be exact in the conversion from us to ticks. (those frequencies above 1 MHz, which is most of them on modern 64-bit platforms). We guarantee a conversion that is accurate to the nearest tick value, or failure otherwise. This can be ±1 timer period. If you want to specify durations of length close to the period between timer ticks, please use ticks exactly. However, depending on kernel WCET domains of this duration may not be schedulable. [RFC-20]: seL4/rfcs#33 [Domain schedules]: seL4/microkit#445 Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
b9fce32 to
b1df4dc
Compare
|
OK, this should be ready for re-review now. |
| uint64_t ticks = duration_s * timer_frequency | ||
| + ((duration_us_frac * timer_frequency + US_IN_SECOND / 2) / (US_IN_SECOND)); |
There was a problem hiding this comment.
It only doesn't fit because you chose unnecessarily long names for all the variables. :-)
Worth mentioning that you do rounding here.
There was a problem hiding this comment.
Yes, I suppose I should mention the same notes I left in the documentation here about the behaviour.
| * timer frequency to us at build time, in Hertz */ | ||
| uin64_t timer_frequency = CONFIG_TIMER_FREQUENCY; | ||
|
|
||
| /* FIXME: On ARM32 we need to pull in a 64-bit division helper; |
There was a problem hiding this comment.
Easy to check with a disassemble. But if the value is constant the division is probably replaced with shifts and multiplies. If frequency is a whole MHz, the calculation can be transformed back to duration_us multiplied by a constant. The maaxboard has frequency 8333333, that's probably a better test.
| void *tsc_freq_mhz_addr = (void *)tsc_freq_hdr + sizeof(seL4_BootInfoHeader); | ||
| uint32_t tsc_freq_mhz = *(uint32_t *)tsc_freq_mhz_addr; |
There was a problem hiding this comment.
| void *tsc_freq_mhz_addr = (void *)tsc_freq_hdr + sizeof(seL4_BootInfoHeader); | |
| uint32_t tsc_freq_mhz = *(uint32_t *)tsc_freq_mhz_addr; | |
| uint32_t tsc_freq_mhz = *(uint32_t *)&tsc_freq_hdr[1]; |
There was a problem hiding this comment.
I don't like this. Maybe I'm wrong here but it feels wrong to treat this like an array of headers.
There was a problem hiding this comment.
uint32_t tsc_freq_mhz = *(uint32_t *)(tsc_freq_hdr + 1); also works. And the +1 is exactly the same as + sizeof(seL4_BootInfoHeader), except you know for sure you didn't make a mistake with the type.
But feel free to leave it as it is, it's just a suggestion.
There was a problem hiding this comment.
I would prefer to leave as is. I don't like the +1 implicitly being strided by the size of the type.
| assert(!"unreachable"); | ||
| } | ||
|
|
||
| ZF_LOGD(" ticks: %lu", duration_ticks); |
There was a problem hiding this comment.
This looks weird. Is it for testing only or do you want to keep it? Can combine it with the previous debug print if you move that one to here.
There was a problem hiding this comment.
This only gets emitted if compiled with verbose logging. I can't combine it with a previous on one line because ZF_LOG functions (annoyingly) add a newline by default, probably because they prefix each line with the location.
I don't want to move the previous print here because then errors won't have printed out the data. However I guess I could make errors more informative (it just makes us_to_ticks more annoying because then I need to return if an error occurred).
There was a problem hiding this comment.
Why can't you just add it as extra output (on the same line) to:
ZF_LOGD(" Domain schedule entry[%lu]: domain %d duration: %llu %s",
i + spec->domainIndexShift, entry.domain, (unsigned long long)entry.duration,
(entry.kind == CDL_DomainSchedEntryKind_Us) ? "us" :
((entry.kind == CDL_DomainSchedEntryKind_Ticks) ? "ticks" : "[end marker]"));
print?
Moving that one down to here should be fine. Only error is the default case which should be unreachable anyway. And that us_to_ticks prints warnings before this print is fine too, it's clear enough from the output to which entry it belongs.
There was a problem hiding this comment.
us_to_ticks can fail. so moving it down won't print out the entry before hand if it does. I guess I could make us_to_ticks propagate an error but that's really annoying.
lsf37
left a comment
There was a problem hiding this comment.
I'm happy with this. I don't have a strong opinion on the C style questions either way. Happy for Julia to decide.
|
The main open question before merging this is whether we want units for each entry, like implemented here, or a global property on the schedule which chooses the time units, like proposed by @Furao seL4/microkit#445 (comment):
|
In the work on RFC-20 it was proposed that it would make sense
to remove the (micro)seconds-to-ticks conversions from seL4
and migrate all of (MCS) userspace APIs to be in ticks, to make dealing
with these calculations and their trickiness to instead be done
at user-level - and the tradeoffs one has to make to be no longer
kernel policy.
This PR demonstrates implements this for the capDL initialiser.
For other cases supported by the capDL specification, such
(MCS) budgets and periods currently specified in microseconds,
the capDL initialiser should be able to paper over the change
in kernel APIs without the users needing to deal with this
distinction.
These changes are motivated by the Domain schedules PR to the
seL4 Microkit, which creates a capDL specification for the
(Rust version) of the capDL initialiser to use. At the moment,
this PR is unable to be consistent about the values exposed
to the user for the units of the domain schedule: whilst we can
statically determine the us->ticks conversion for ARM/RISC-V
platforms, on x86 this is a runtime-known value only. Since
the spec is packed at system-build-time, we cannot therefore
specify x86 domain schedules in a consistent set of units.
The concrete specification changes from this PR is the addition
of a 'unit' suffix inside the domain schedule syntax. The
initialiser can then support either ticks (the default without
any units) or microseconds, and it performs a runtime conversion
of this value to ticks for the domain scheduler API.
Canonically, the end marker is left without units as (0, 0), but
we permit inputs of (0, 0 ticks) or (0, 0 us) for consistency.
Note: the meaning of a kernel tick is different between MCS and
non-MCS configurations.
One consequence of taking input in 'us' is that there are cases
where the input is unrepresentable in 'ticks'; and so this
introduces the possiblity of failure in the initialiser that
is unchecked at build-time.
For non-MCS, as kernel ticks are an integer multiple of 1 ms,
the conversion from us to ticks will fail if the conversion is
not exact. This is easy to avoid, one has to specify a 'us' value
that is a multiple of (CONFIG_TIMER_TICK_MS * 1000), something
which does not differ per platform.
For MCS, the kernel tick reflects the underlying timer frequency
(for the domain scheduler). Thus, there exists many frequencies
for which it is impossible to be exact in the conversion from
us to ticks. (those frequencies above 1 MHz, which is most
of them on modern 64-bit platforms). We guarantee a conversion
that is accurate to the nearest tick value, or failure otherwise.
This can be ±1 timer period. If you want to specify durations of
length close to the period between timer ticks, please use ticks
exactly. However, depending on kernel WCET domains of this
duration may not be schedulable.
Original PR description
This is partially an experiment, but to make this useful for userspace programs, it makes sense for the capDL specification to treat the domain schedule values as microseconds, instead of timer ticks.
In the work on RFC-20 it was proposed that it would make sense to remove this (micro)seconds-to-ticks conversions from seL4 and migrate all of userspace APIs to be in ticks, to make dealing with these calculations and their trickiness to instead be done at user-level.
This PR demonstrates that it should be possible to do this for at least the capDL initialiser. For other cases supported by the capDL specification, such as (MCS) budgets and periods currently specified in microseconds, the capDL initialiser should be able to paper over the change in kernel APIs without the users needing to deal with this distinction.
These changes are motivated by the Domain schedules PR to the seL4 Microkit, which creates a capDL specification for the (Rust version) of the capDL initialiser to use. At the moment, this PR is unable to be consistent about the values exposed to the user for the units of the domain schedule: whilst we can statically determine the us->ticks conversion for ARM/RISC-V platforms, on x86 this is a runtime-known value only. Since the spec is packed at system-build-time, we cannot therefore specify x86 domain schedules in a consistent set of units.
(Note, I haven't actually tested that this works, but I have tested a similar change to rust-seL4 capDL initialiser. This PR is mostly for discussion; this could likely also move to a formal RFC).