Page MenuHomeFreeBSD

bitset: implement BIT_TEST_CLR_ATOMIC & BIT_TEST_SET_ATOMIC
AcceptedPublic

Authored by rlibby on Dec 6 2019, 9:46 AM.

Details

Reviewers
jeff
markj
kib
cem
Group Reviewers
docs
Summary

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

Submitted by: jeff

Diff Detail

Lint
Lint OK
Unit
No Unit 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
212

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

sys/sys/bitset.h
197

Is this note worth having in the man page instead?

share/man/man9/bitset.9
212

Okay, how about "atomically clears/sets the bit and returns whether it was set", here and below?

sys/sys/bitset.h
197

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
212

Sounds good to me.

sys/sys/bitset.h
197

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
197

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
197

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.