That is, provide wrappers around the atomic_testandclear and
atomic_testandset primitives.
Submitted by: jeff
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I do not like that n is evaluated twice, but this is inherited from other macros in the file.
| share/man/man9/bitset.9 | ||
|---|---|---|
| 216 | Okay, how about "atomically clears/sets the bit and returns whether it was set", here and below? | |
| sys/sys/bitset.h | ||
| 198 | My feeling is no. Which aspect were you thinking? The bitset API is consistent in taking a bit number and not a mask, but the atomic API is not. I feel like the phrasing being added to the man page above is clear about it being the bit number, in the context of the paragraph. Thoughts? The modulus bit was just intended to explain why there's no % _BITSET_BITS in the macro implementation. | |
| share/man/man9/bitset.9 | ||
|---|---|---|
| 216 | Sounds good to me. | |
| sys/sys/bitset.h | ||
| 198 | I see now, before I didn't quite understand what the comment was pointing out. I agree that it doesn't belong in the man page. Honestly, I'm not sure it is very useful: if you understand atomic(9), the note is obvious, and if you don't, then you should read atomic(9). At least, I think it should go before the definition of BIT_CLR_ATOMIC since it applies to the macros above too. | |
| sys/sys/bitset.h | ||
|---|---|---|
| 198 | Okay. I can shorten it to a reference to atomic(9) with a warning that the APIs are not the same. | |
Comment Actions
Mark’s suggested wording looks good to me (as does the original).
| sys/sys/bitset.h | ||
|---|---|---|
| 198 | I like that it justifies the difference between use of a mask in the BIT_SET_ATOMIC_ implementation above and use of a bit here. But it’s not a sticking point for me. | |
| share/man/man9/bitset.9 | ||
|---|---|---|
| 27 | Don’t forget to bump .Dd on commit. | |
Comment Actions
So I had the bright idea before committing to check if atomic_testandset/clear is actually implemented on all architectures, and it isn't (yet?). I would like to commit this patch anyway and just hold off on D22703. That way we won't break any builds (this patch alone is not yet connected to anything) but I can reduce my patch stack a little. Sound okay?
Comment Actions
No objection from me.
If any of the missing archs are sparc64, 32-bit mips, 32-bit arm v5 (and maybe v6...), don't feel obligated to add the support. They're all slated for death on 2020-01-01 anyway.
| sys/sys/bitset.h | ||
|---|---|---|
| 200 | Hm, should the (n) in the 2nd argument of atomic_testandset_long() be ((n) - __bitset_word((_s), (n)) * 8 * sizeof(long)) or something like that? It seems like the current structure won't work for any bitset_word other than the 0th one. | |
| sys/sys/bitset.h | ||
|---|---|---|
| 200 | Or more straightforwardly, ((n) % _BITSET_BITS). | |
| sys/sys/bitset.h | ||
|---|---|---|
| 200 | I think it works... This is what the last sentence of the original comment was about. | |
| sys/sys/bitset.h | ||
|---|---|---|
| 200 | Oh, I see. Yeah, and the implementation seem to have that behavior as well. It is a surprising API for me. I guess it might be a vote for that comment. | |
Comment Actions
I think r359311 / git ca0ec73c11a7bb9ed409466e514fa5c34b6c84b5 / D22963 (generic atomic_testand{set,clear}_long) satisfied the dependency I was worried about here so I'm going to go ahead with this. The only conflict in the rebase was the man doc date.