kernel: deprecate and replace k_uptime_get_32()

[pabigot]

This function does not return the low 32 bits of k_uptime_get() as
suggested by it's documentation; it returns the number of milliseconds
represented by the low 32-bits of the underlying system clock. The
truncation before translation results in discontinuities at every point
where the system clock increments bit 33.

Refine the documentation to describe what it actually does, mark it
deprecated, and replace all in-tree uses with an expression that is
slower but does the right thing.

Closes #18739.

Signed-off-by: Peter Bigot [email protected]

[pfalcon]

This function does not return the low 32 bits of k_uptime_get() as
suggested by it's documentation

Ok, so that's where the bug lies, and what needs fixing, instead of making a statue of the incorrect implementation in stone and telling not to use it.

[pabigot]

Ok, so that's where the bug lies, and what needs fixing, instead of making a statue of the incorrect implementation in stone and telling not to use it.

We could re-implement k_uptime_get_32() as (u32_t)k_uptime_get(), but then the whole "value" of the _32 variant is lost (have to lock and use 64-bit arithmetic) so it might as well be deprecated and replaced. IMO it's more useful to be explicit that we're using a high-cost function, so alternative approaches can be taken (such as switching to track all time values and spans in ticks).

I'm happy to instead take the re-implement path, with or without deprecating, if that's preferred. But this API appears to be used in the kernel to measure timeouts: durations are measured by subtracting values returned by k_uptime_get_32() and when the timestamps cross a multiple of 2^32 the duration can be very wrong, so something has to be done.

PR #18744 takes the alternative approach.

[carlescufi]

This function does not return the low 32 bits of k_uptime_get() as
suggested by it's documentation

Ok, so that's where the bug lies, and what needs fixing, instead of making a statue of the incorrect implementation in stone and telling not to use it.

Not sure I get what you mean here @pfalcon. The current implementation is being marked as deprecated in this PR because what it does now does not reflect what a user might infer from the function name. Do you suggest that we should keep the function and cast to uint32 inside the new implementation or that we keep the current implementation but rename it?

[pizi-nordic]

Ok, so that's where the bug lies, and what needs fixing, instead of making a statue of the incorrect implementation in stone and telling not to use it.

This way we would have k_uptime_get_32() and k_uptime_get() with no difference on performance, which so far justified existence of k_uptime_get_32().

[alexanderwachter]

@pizi-nordic since TICKLESS_KERNEL is the default, k_uptime_get_32() does not even have better performance.

[pizi-nordic]

@alexanderwachter: So there is no doubts that this PR should go in...

[pfalcon]

Not sure I get what you mean here @pfalcon.

Let me explain by example. In #18708, it was found that pthread_cond_timedwait(), pthread_mutex_timedlock() functions don't do what's described in the documentation. However, I don't deprecate them with a desire to make users find workarounds or introduce new names for the same functions. I just fix incorrect implementation.

This way we would have k_uptime_get_32() and k_uptime_get() with no difference on performance, which so far justified existence of k_uptime_get_32().

Really? I thought it exists just because it's convenient API to use. And don't let me start on possible implications on register pressure when returning 64bit, instead of 32bits, on all architectures and compilers, past, present, and future, with LTO and not, with a case when ticks and millisecond perfectly align and not, for the case of no userspace separation and with it (reading comments on #18064 was insightful, even though it talks about 64-bit arguments, not returns).

Let me just point that fixing k_uptime_get_32() has a very obvious benefit of making unnecessary patches rampaging thru 47 files of the codebase (2 days before the planned release date, just by pure coincidence, surely nobody thinks about merging it into this release).

[andyross]

Seems like #17155 already addresses the issues here, no?

[pabigot]

Withdrawn, prefer #18744