Userland should busy-wait briefly before using usleep() #4424
Conversation
ryao
changed the title
ryao
force-pushed
the
userland_timeout
branch
5 times, most recently
from
45bb07f to
3cd86f2
Compare
|
This functionality probably should be made a library function somewhere, but I am not sure where to put it. Maybe |
The userland code uses the `do {usleep()} while (unavaliable)` pattern
for closing races in asynchronous device initialization. However, the
normal case is that calling `usleep()` introduces an unnecessarily long
delay because the other side typically finishes what it was doing *much*
faster than the period in which we sleep. The periods in which we
`usleep()` are short, but they can add to a significant period of time
when scripts trigger the codepaths using `usleep()` in a loop.
`libzfs_load_module` uses the alternative pattern of `do {sched_yield()}
while (unavaliable)` for 10ms before falling back to the `do {usleep()}
while (unavaliable)` pattern, which is much better for latencies. As a
general principle, I would like to encourage contributors to avoid the
`do {usleep()} while (unavaliable)` pattern for closing asynchronous
races. Modifying the places that use it to do what `libzfs_load_module`
does should achieve that.
There are some minor behavior changes worth noting.
One is that reusing the exact code structure from `libzfs_load_module`
meant that `zpool_label_disk_wait()` will return the error code passed
by `stat64()`. I had considered altering it to maintaining the current
error behavior, but after looking at its consumers, I felt that they
would benefit from a more accurate error code.
Another is that while I used the same timeouts, time spent in the
kernel no longer can extend the timeouts, so the maximum timeouts should
be somewhat shorter. That should be more noticeable on a heavily loaded
system. One might consider the busy-wait to be a cause for concern
there, but the use of `sched_yield()` should lower the number of
iterations in which we busy-wait before falling back to `usleep()`.
The last is that I made a slight improvement to the original algorithm
where if we would timeout after returning from `usleep()`, we will now
skip the call and timeout early.
Signed-off-by: Richard Yao <[email protected]>
|
I was going to suggest the same thing since this is I believe the fourth place where we've needed to do this. The |
|
@ryao this functionality can be moved to a library function at a some point in the future. If your happy with the current patch I'll get it applied. |
|
Closing is favor of the patches in #4523. |
The userland code uses the
do {usleep()} while (unavaliable)patternfor closing races in asynchronous device initialization. However, the
normal case is that calling
usleep()introduces an unnecessarily longdelay because the other side typically finishes what it was doing much
faster than the period in which we sleep. The periods in which we
usleep()are short, but they can add to a significant period of timewhen scripts trigger the codepaths using
usleep()in a loop.libzfs_load_moduleuses the alternative pattern ofdo {sched_yield()} while (unavaliable)for 10ms before falling back to thedo {usleep()} while (unavaliable)pattern, which is much better for latencies. As ageneral principle, I would like to encourage contributors to avoid the
do {usleep()} while (unavaliable)pattern for closing asynchronousraces. Modifying the places that use it to do what
libzfs_load_moduledoes should achieve that.
There are some minor behavior changes worth noting.
One is that reusing the exact code structure from
libzfs_load_modulemeant that
zpool_label_disk_wait()will return the error code passedby
stat64(). I had considered altering it to maintaining the currenterror behavior, but after looking at its consumers, I felt that they
would benefit from a more accurate error code.
Another is that while I used the same timeouts, time spent in the
kernel no longer can extend the timeouts, so the maximum timeouts should
be somewhat shorter. That should be more noticeable on a heavily loaded
system. One might consider the busy-wait to be a cause for concern
there, but the use of
sched_yield()should lower the number ofiterations in which we busy-wait before falling back to
usleep().The last is that I made a slight improvement to the original algorithm
where if we would timeout after returning from
usleep(), we will nowskip the call and timeout early.
Signed-off-by: Richard Yao [email protected]