Page MenuHomeFreeBSD

Improve timeout precision of pthread_cond_timedwait()
ClosedPublic

Authored by mav on Feb 3 2022, 10:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Sep 17, 2:15 AM
Unknown Object (File)
Mon, Sep 16, 8:57 PM
Unknown Object (File)
Sep 15 2024, 6:16 PM
Unknown Object (File)
Sep 9 2024, 3:27 PM
Unknown Object (File)
Sep 4 2024, 9:23 AM
Unknown Object (File)
Aug 29 2024, 4:11 PM
Unknown Object (File)
Aug 17 2024, 5:53 AM
Unknown Object (File)
Aug 6 2024, 3:57 AM
Subscribers

Details

Summary

This code was not touched when all other user-space sleep paths were switched to sbintime_t and decoupled from hardclock. When it is possible, convert supplied times into sbinuptime to supply directly to msleep_sbt() with C_ABSOLUTE.

Test Plan

Simple test tool shows reachable timeout precision down to few microseconds instead of few milliseconds.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mav requested review of this revision.Feb 3 2022, 10:07 PM
This revision is now accepted and ready to land.Feb 3 2022, 10:58 PM
sys/kern/kern_umtx.c
724

Can we have the explicit list there, instead of 'default'?

726

You initialize *flags in umtxq_sleep() anyway. I suggest to to *flags |= C_HARDCLOCK there instead (and remove *flags = 0 above), to make the interface somewhat more flexible.

I think e.g. about using C_ABSOLUTE or C_PRECALC there, since right now we do too many conversions for is_abs_real case.

sys/kern/kern_umtx.c
724

I suppose just to increase visibility? Because as I was able to find here should appear only 4 different values (see _pthread_condattr_setclock()). Even those values I listed already is a bit overkill.

726

You initialize *flags in umtxq_sleep() anyway. I suggest to to *flags |= C_HARDCLOCK there instead (and remove *flags = 0 above), to make the interface somewhat more flexible.

I don't feel like it would make it better. It would make result depending on previous calls, that while would change nothing is more dirty.

I think e.g. about using C_ABSOLUTE or C_PRECALC there, since right now we do too many conversions for is_abs_real case.

I thought about it briefly, but C_ABSOLUTE require having binuptime, while none of clocks use that. MONOTONIC and UPTIME are close, but require backward conversion from nanoseconds. REALTIME I guess would also require correction on getboottimebin()?

In case of hardclock I haven't actually added any more conversions, just not removed any. If we assume that the only clocks of that kind that may be there are profiles, then relation to hardclock is probably not required (statclock may be even closer for multi-threaded processes) and I could possibly just limit minimal time instead, but unlike the other cases in case of profiles the code has no current binuptime at all, so it can pass only relative sbintime.

sys/kern/kern_umtx.c
724

It is it make life easier for anybody reading the code after you, and for code clarity.
Ideally, there would be a default: case with __unreachable() statement.

For instance, it is not clear to me without a lot of code reading which cases go to C_HARDCLOCK. I would expect that at least everything wall-clock related with _PRECISE would, but apparently you did not do it this way.

726

You initialize *flags in umtxq_sleep() anyway. I suggest to to *flags |= C_HARDCLOCK there instead (and remove *flags = 0 above), to make the interface somewhat more flexible.

I don't feel like it would make it better. It would make result depending on previous calls, that while would change nothing is more dirty.

I think e.g. about using C_ABSOLUTE or C_PRECALC there, since right now we do too many conversions for is_abs_real case.

I thought about it briefly, but C_ABSOLUTE require having binuptime, while none of clocks use that. MONOTONIC and UPTIME are close, but require backward conversion from nanoseconds. REALTIME I guess would also require correction on getboottimebin()?

There is already a mechanism to reschedule umtx sleeps on abs time change. This what umtx_abs_time_update() stuff is about, see 9dbdf2a16998ee49011f874b94754e2d046045f3.

In case of hardclock I haven't actually added any more conversions, just not removed any. If we assume that the only clocks of that kind that may be there are profiles, then relation to hardclock is probably not required (statclock may be even closer for multi-threaded processes) and I could possibly just limit minimal time instead, but unlike the other cases in case of profiles the code has no current binuptime at all, so it can pass only relative sbintime.

I am not claiming that you added any new conversions, I am saying that we probably can remove some if this code is better structured.

mav edited the summary of this revision. (Show Details)

I've rewritten the patch to avoid clock reads when possible, converting most of supplied time values directly into sbinuptime for use with C_ABSOLUTE. For few other (profiling) clocks I don't have good implementation ideas, so left them as-is.

This revision now requires review to proceed.Feb 23 2022, 11:08 PM
mav marked 4 inline comments as done.Feb 23 2022, 11:09 PM

Remove unneeded assignment.

This revision is now accepted and ready to land.Mar 3 2022, 11:58 PM