Page MenuHomeFreeBSD

bitset: implement BIT_TEST_CLR_ATOMIC & BIT_TEST_SET_ATOMIC
ClosedPublic

Authored by rlibby on Dec 6 2019, 9:46 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 12 2024, 3:38 AM
Unknown Object (File)
Dec 22 2023, 10:14 PM
Unknown Object (File)
Nov 28 2023, 10:29 AM
Unknown Object (File)
Nov 27 2023, 12:33 PM
Unknown Object (File)
Nov 25 2023, 5:12 PM
Unknown Object (File)
Nov 23 2023, 7:00 PM
Unknown Object (File)
Nov 23 2023, 3:05 PM
Unknown Object (File)
Nov 23 2023, 4:46 AM
Subscribers

Details

Summary

That is, provide wrappers around the atomic_testandclear and
atomic_testandset primitives.

Submitted by: jeff

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28013
Build 26171: arc lint + arc unit

Event Timeline

I do not like that n is evaluated twice, but this is inherited from other macros in the file.

This revision is now accepted and ready to land.Dec 6 2019, 1:16 PM
share/man/man9/bitset.9
213

Really, it's returning whether the bit was set.

sys/sys/bitset.h
198

Is this note worth having in the man page instead?

share/man/man9/bitset.9
213

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.

markj added inline comments.
share/man/man9/bitset.9
213

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.

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.

cem added inline comments.
share/man/man9/bitset.9
27

Don’t forget to bump .Dd on commit.

Review feedback, man page date & wording, comment wording

This revision now requires review to proceed.Dec 6 2019, 9:25 PM
This revision is now accepted and ready to land.Dec 6 2019, 9:29 PM

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?

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?

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.
The atomic(9) API says that it operates on val % (NBBY * sizeof(long)). Is this a vote for restoring the comment?

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.

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.