Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Kernel Timeout API #21305

Closed
andyross opened this issue Dec 11, 2019 · 15 comments
Closed

New Kernel Timeout API #21305

andyross opened this issue Dec 11, 2019 · 15 comments
Assignees
Labels
area: API Changes to public APIs area: Kernel RFC Request For Comments: want input from the community

Comments

@andyross
Copy link
Contributor

andyross commented Dec 11, 2019

New Kernel Timeout API

Opaque timeout type

A new type will be added, k_timeout_t, which represents an abstracted expiration time interpreted by the kernel. This will not have user-visible fields. It is intended to be initialized only via a new macro API.

  1. The data type stored in the timeout will be exposed to the user as an unsigned k_ticks_t typedef. The size of this type (and thus the internal precision of the kernel timeout tracking) will be selectable via a CONFIG_SYS_TIMEOUT_64BIT kconfig. Systems with long uptimes and rapid tick rates will be able to select a 64 timeout type to prevent rollover.

  2. K_TIMEOUT_{MS,US,NS,CYC,TICKS}(val) macros will be added which evaluate to a static initializer for a k_timeout_t representing an expiration in the relevant units from the current tick.

  3. K_NO_WAIT and K_FOREVER will become values of type k_timeout_t, but otherwise retain their current behavior. They will no longer be usable as arithmetic rvalues, even to test them for equality with other timeouts.

  4. K_TIMEOUT_EQ(a, b) will be added as a macro predicate to compare the values of two k_timeout_t objects for equality.

API use of new timeouts

The following public functions, which currently take a numeric delay in milliseconds, will have that argument modified to accept a k_timeout_t instead.

  • k_delayed_work_submit()
  • k_delayed_work_submit_to_queue()
  • k_fifo_get()
  • k_futex_wait()
  • k_lifo_get()
  • k_mbox_data_block_get()
  • k_mbox_get()
  • k_mbox_put()
  • k_mem_pool_alloc()
  • k_mem_slab_alloc()
  • k_msgq_get()
  • k_msgq_put()
  • k_mutex_lock()
  • k_pipe_get()
  • k_pipe_put()
  • k_poll()
  • k_queue_get()
  • k_sem_take()
  • k_sleep()
  • k_stack_pop()
  • k_timer_start()
  • k_thread_create()
  • sys_mutex_lock()
  • sys_sem_take()

New APIs

New APIs are added to permit the inspection of the expiration time of running timeout handles in tick units (to allow sophisticated applications to minimize unit conversion error) and as an absolute/uptime value (to avoid race conditions).

  • k_delayed_work_end_ticks() - end time in ticks of a delayed work item
  • k_delayed_work_remaining_ticks() - time remaining in ticks of a delayed work item
  • k_thread_timeout_end_ticks() - end time in ticks of a sleeping/pending thread
  • k_thread_timeout_remaining_ticks() - time remaining in ticks of a sleeping/pending thread
  • k_timer_remaining_ticks() - time remaining in ticks of a running timer

Also some convenience additions:

  • k_msleep() - Identical to current k_sleep(), takes s32_t count of milliseconds
  • k_uptime_ticks() - Exposes the internal tick counter to users who need it

Legacy Mode

These changes represent source-incompatible changes to public APIs. For apps requiring source compatibility, a "legacy" mode is provided by the CONFIG_SYS_TIMEOUT_LEGACY_API kconfig. In this mode, both k_ticks_t and k_timeout_t become typedefs for a 32 bit integer and are interpreted by all kernels APIs as a count of milliseconds, with the K_NO_WAIT and K_FOREVER tokens having the values they do in current code. Legacy mode is, obviously, not compatible with 64 bit timeouts.

Legacy will be the default in the initial version. It is not expected that all subsystems will work with new timeouts immediately.

Absolute Timeout Expiration

The ability will be added to set a timeout to expire at an absolute time value, in the same space as k_uptime_get(). That is, a timeout specified at an uptime of "1234 ms" will expire on the tick after the internal tick counter passes 1234 ms.

  • K_TIMEOUT_ABSOLUTE_{TICKS,CYC,MS,US,NS}(val) macros will be added which evaluate to a static initializer for a k_timeout_t representing an expiration in the relevant units when the system uptime reaches the specified value.

Static Thread Delay Argument

K_THREAD_DEFINE() has a final argument that currently takes a delay in milliseconds, but cannot use the new timeout mechanism for technical reasons (it has to be precomputed by the compiler and can't be the result of an inlined C function, even one with constant arguments). Existing code will typically pass K_NO_WAIT and K_FOREVER as values for this argument, and that will have to change as timeouts are non-integral values. Instead, a new K_THREAD_NO_START token will be defined to allow for the behavior currently captured by K_FOREVER. Existing usage of K_NO_WAIT can simply specify zero milliseconds without ambiguity.

Obviously, apps built in legacy mode will continue to work with the existing tokens without change.

Note: Longer term, it would be nicer to evolved the k_thread_create() API to remove the delay parameter (which is very rarely used, and somewhat odd when compared with other OSes) in favor of encouraging apps to use a k_timer to do this themselves. Then "no start" could simply be a flag passed in the options parameter. This would be a good opportunity to remove two of the three parameter words, which again is an odd API and almost entirely unused. But that's for a different pull request.

@andyross andyross added the RFC Request For Comments: want input from the community label Dec 11, 2019
@andyross
Copy link
Contributor Author

OK, as promised, here's a more formal RFC for the timeout changes that were submitted back in July. There are no surprises here, it's exactly the same stuff.

I've tried to make it as minimal as possible to preserve the fundamental simplicity of what is happening, detailing just the API changes themselves and not the internals or (especially) the form of the patch series.

@nordic-krch
Copy link
Contributor

The data type stored in the timeout will be exposed to the user as a k_ticks_t typedef

I think it's worth mentioning that 32 bit type will be unsigned.

What about k_timer_start? As one of the goals of this change is to get higher precision timeouts then k_timer is a primary goal.

During one of the reviews comment was raised that k_timeout_t name is not always precise. I was wondering then if it shouldn't get other name (for example k_time_t since it is a representation of time as understand by the kernel).

K_TIMEOUT_EQ(a, b) will be added

there might be a need for K_TIMEOUT_LESS(a,b) and K_TIMEOUT_LEQ(a,b) as well

It would be also good that we settle our goals regarding switch to opaque struct as default instead of having legacy mode. Imo, this should happen as soon as tree gets stabilized after initial structure introduction gets in (ideally before 2.2).

@aescolar aescolar added area: API Changes to public APIs area: Kernel labels Dec 12, 2019
@andyross
Copy link
Contributor Author

What about k_timer_start?

Added (it was, of course, in the pull request from back in July). Also k_thread_create() was missing, and I added entries for k_fifo/lifo_get(), which are macros defined in terms of k_queue_get() but were historically functions and act like separate APIs.

Also added the unsigned note.

During one of the reviews comment was raised that k_timeout_t name is not always precise. I was wondering then if it shouldn't get other name (for example k_time_t since it is a representation of time as understand by the kernel).

It's not, really. That's what k_ticks_t is for. k_timeout_t is a representation of the requested time a timeout will expire. It's deliberately not an arithmetic type, because it can be expressed in different spaces. That's the same reason we don't do comparison on it -- if the user wants to do math in arithmetic units, they can. The reason for K_TIMEOUT_EQ() is that we have special, non-arithmetic tokens like K_FOREVER and K_NO_WAIT that existing code tests vs. timeouts already.

settle our goals regarding switch to opaque struct as default

No. My blind and naive acquiescence to "make legacy default" last time is the closest thing to a technical reason for this PR having failed. I'm going to submit a minimal change with just the core subsystems first, and we can do subsystems one by one over time (ideally collaboratively). It is abundantly clear that the project will never accept a change from me of the size the last PR grew to, and I'm out of patience to try.

And note that legacy mode is a firm requirement anyway. Without it, we're making source-incompatible changes to existing APIs, which is forbidden.

@nordic-krch
Copy link
Contributor

Once absolute timeouts are added there might be a need for functions returning target or deadline, something like: k_ticks_t k_timer_target().

@andyross
Copy link
Contributor Author

They're in there, under the "New APIs" section. User-visible objects that include timeouts are k_timer and k_delayed_work, both of which currently have "remaining" predicates (which return ms) but in this scheme get new "end" predicates returning an absolute time in ticks (note that this works regardless of how the timeout was registered). And threads that are pended on a timeout (e.g. in sleep, or in a IPC call that takes a timeout) now have remaining and get predicates too.

@nordic-krch
Copy link
Contributor

right, missed as an absolute/uptime value part there. good.

@nordic-krch
Copy link
Contributor

@andyross can you also clarify what happens with K_THREAD_DEFINE delay argument? If it is staying as milliseconds then it cannot accept K_NO_WAIT anymore.

@andyross
Copy link
Contributor Author

Excellent point. Added a section at the bottom about that macro.

@nordic-krch
Copy link
Contributor

@andyross thanks for adding section about delay in thread starting.

Note: Longer term, it would be nicer to evolved the k_thread_create() API to remove the delay parameter

Agree, it can be one of the steps. It isn't widely used.

@nordic-krch
Copy link
Contributor

@andyross from my side RFC looks ok and i see no other issues.

@nashif nashif removed their assignment Dec 18, 2019
@andrewboie
Copy link
Contributor

Note: Longer term, it would be nicer to evolved the k_thread_create() API to remove the delay parameter

I had been using the delay parameter for some things related to user mode. Typically:

  1. Create a thread, but set K_FOREVER as the delay. The thread object is now in an initialized state, but not yet running.
  2. Assign the thread to memory domains or adjust its permissions to other kernel objects.
  3. Call k_thread_start() to start it.

Assignment of kernel object permissions to threads works even if the object isn't initialized, but not memory domains. If we decide to drop the delay parameter we should take this under consideration -- in essence, user mode has additional knobs to twiddle for threads, and since we don't have these directly in the k_thread_create() API the current method is to start it with K_FOREVER and then make additional configuration. I haven't thought of alternatives but I'm sure we can come up with something.

I haven't been following the timeout discussions nearly as much as others, but the RFC looks good to me +1.

@pabigot pabigot removed their assignment Mar 13, 2020
@sslupsky
Copy link
Contributor

sslupsky commented Apr 10, 2020

@andyross The new API looks nice.

I ran into a minor issue today with sys pm policy:

enum power_states sys_pm_policy_next_state(s32_t ticks)

Should the type of the argument for this function now be k_ticks_t instead of s32_t?

Here is a copy of the compiler warning:

[146/159] Building CXX object CMakeFiles/app.dir/src/power.cpp.obj
../src/power.cpp: In function 'power_states sys_pm_policy_next_state(s32_t)':
../src/power.cpp:80:13: warning: comparison of integer expressions of different signedness: 's32_t' {aka 'int'} and 'k_ticks_t' {aka 'unsigned int'} [-Wsign-compare]
  if ((ticks != K_FOREVER) && (ticks < pm_min_residency[0])) {
../src/power.cpp:91:14: warning: comparison of integer expressions of different signedness: 's32_t' {aka 'int'} and 'k_ticks_t' {aka 'unsigned int'} [-Wsign-compare]
   if ((ticks == K_FOREVER) ||

@andyross
Copy link
Contributor Author

andyross commented Apr 10, 2020

@sslupsky (you might want to ask this in a separate issue instead of here in a closed PR): what is the code in question? The API for manipulating timeouts has changed a little, but if I'm reading correctly that code was never correct to begin with -- timeouts were always milliseconds, so comparing a value in ticks to K_FOREVER implies that you're doing something other than a kernel timeout. K_FOREVER is now an opaque type and can't be used like that.

@andyross
Copy link
Contributor Author

Editting: OK, I see now you're in a suspend handler. The integer token for "forever" passed to timer drivers and system suspend handlers is now K_TICKS_FOREVER. This was actually an API abuse internal to the kernel (as mentioned, K_FOREVER is supposed to be milliseconds, not ticks) that got corrected as part of this PR. Drivers have their own constant.

I'm a little worried this didn't get caught in CI, though. We apparently don't have good coverage for PM stuff.

@carlescufi
Copy link
Member

The kernel side of things is done. There's still a discussion ongoing about how best to convert the subsystems, but this issue doesn't cover that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Kernel RFC Request For Comments: want input from the community
Projects
None yet
Development

No branches or pull requests

8 participants