WT-12331 API review #10562
WT-12331 API review #10562
Conversation
src/include/wtfutex.h
Outdated
| * | ||
| * \retval -EINVAL Not a futex, or pointer to [val] is null. | ||
| */ | ||
| int __wt_fetch(struct __wt_futex *ftx, uint32_t *val); |
There was a problem hiding this comment.
Is there a reason not to call this __wt_futex_fetch? I'm trying to figure out if fetch/store consciously skip the futex prefix.
I'm not that familiar with futexes, but the interface seems sensible to me as is.
There was a problem hiding this comment.
No: entirely inadvertent. Fixed -- thanks.
src/include/wtfutex.h
Outdated
| /* | ||
| * Get the value associated with the futex. | ||
| * | ||
| * \retval -EINVAL Not a futex, or pointer to [val] is null. |
There was a problem hiding this comment.
I think WT normally returns positive, not negative error codes?
There was a problem hiding this comment.
Happy to change, this is an old device driver habit. My experience has been so far that the majority of WT calls set errno and return -1. Happy to bring it into expectations.
src/include/wtfutex.h
Outdated
| /* | ||
| * Wait on a futex. | ||
| * | ||
| * \param expected Expected current value of futex. |
There was a problem hiding this comment.
What is the expected return value if the current value of futex is not the same as the expected value?
There was a problem hiding this comment.
It is treated as a wakeup.
src/include/wtfutex.h
Outdated
| * \param expected Expected current value of futex. | ||
| * | ||
| * \retval -EINVAL Not a futex, or [timeout_us] <= 0. | ||
| * \retval -ETIMEDOUT Timeout expired before being awoken. |
There was a problem hiding this comment.
What would this function do if the system call gets interrupted and fails with EINTR? In that case, would the function wait again, or would it fail - and if so, with which return value?
There was a problem hiding this comment.
If the futex call is interrupted, a new timeout is calculated and it goes back to waiting.
src/include/wtfutex.h
Outdated
| * | ||
| * \retval -EINVAL Not a futex. | ||
| */ | ||
| int __wt_futex_store(struct __wt_futex *ftx, uint32_t val); |
There was a problem hiding this comment.
Would you expect a need to add atomic operations on futexes in the future, such as increment, decrement, or compare-and-swap? If so, is it worth defining them now, or we'd just add them as needed?
There was a problem hiding this comment.
The idea here originally was to provide some kind of wrapper for a basic use: with the idea of trying to create a usable abstract mechanism. However you raise a good point: this is a low-level mechanism and we will likely want to vary how we modify the associated 32-bit value. So I have pared the API to a portable "futex ops" API.
There was a problem hiding this comment.
I started a review, got interrupted, ate lunch, came back... and found that Peter had stolen my comment about negative error returns! :-)
I'm not sure whether the header name is intended to be part of the API up for review here, but I'd call this os_futex.h since it's abstracting functionality across multiple operating systems, or possibly just place this content in os.h.
I'll LGTM and follow along on Peter's comments.
Rather than try to abstract out the futex, provide a minimal porting interface as originally envisaged. Reformat in the WT style.
This is and should be a very thin wrapper around OS implementations. Higher level constructs can utilze session context as appropriate. Cast to volatile in the parameter.
Adds unit test.
|
Test coverage is too low, this change probably shouldn't be merged as-is.
|
Add the header only for API review.