On 10/14/21 1:20 AM, Hannes Reinecke wrote: > On 10/13/21 6:54 PM, Jens Axboe wrote: >> If we come across bits that are already set, then it's quicker to test >> that first and gate the test_and_set_bit() operation on the result of >> the bit test. >> >> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx> >> --- >> lib/sbitmap.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/sbitmap.c b/lib/sbitmap.c >> index c6e2f1f2c4d2..11b244a8d00f 100644 >> --- a/lib/sbitmap.c >> +++ b/lib/sbitmap.c >> @@ -166,7 +166,7 @@ static int __sbitmap_get_word(unsigned long *word, unsigned long depth, >> return -1; >> } >> >> - if (!test_and_set_bit_lock(nr, word)) >> + if (!test_bit(nr, word) && !test_and_set_bit_lock(nr, word)) >> break; >> >> hint = nr + 1; >> > Hah. Finally something to latch on. > > I've seen this coding pattern quite a lot in the block layer, and, of > course, mostly in the hot path. > (Kinda the point, I guess :-) > > However, my question is this: > > While 'test_and_set_bit()' is atomic, the combination of > 'test_bit && !test_and_set_bit()' is not. > > IE this change moves an atomic operation into a non-atomic one. > So how can we be sure that this patch doesn't introduce a race condition? > And if it doesn't, should we add some comment above the code why this is > safe to do here? If test_bit() returns true, we don't bother with the atomic test_and_set. That's to avoid re-dirtying a cacheline when the operation would be pointless. There's no race in this case, we just skip this bit. The sequence of !test_bit && !test_and_set_bit is very much still atomic, it's just gated on the fact that our optimistic first check said that the bit is currently clear. It obviously has to be. -- Jens Axboe
