Re: [PATCH 4/9] sbitmap: test bit before calling test_and_set_bit()

[Jens Axboe <axboe@kernel.dk>]

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@kernel.dk>
>> ---
>>   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