On 1/12/22 7:38 AM, Andy Shevchenko wrote:
> On Wed, Jan 12, 2022 at 12:51:13PM +0000, John Garry wrote:
>> On 12/01/2022 12:30, Andy Shevchenko wrote:
>>>>>> + if (test_bit(QUEUE_FLAG_HCTX_ACTIVE, &q->queue_flags) ||
>>>>>> + test_and_set_bit(QUEUE_FLAG_HCTX_ACTIVE, &q->queue_flags)) {
>>>>> Whoever wrote this code did too much defensive programming, because the first
>>>>> conditional doesn't make much sense here. Am I right?
>>>>>
>>>> I think because this judgement is in the general IO process, there are also
>>>> some performance considerations here.
>>> I didn't buy this. Is there any better argument why you need redundant
>>> test_bit() call?
>>
>> I think that the idea is that test_bit() is fast and test_and_set_bit() is
>> slow; as such, if we generally expect the bit to be set, then there is no
>> need to do the slower test_and_set_bit() always.
>
> It doesn't sound thought through solution, the bit can be flipped in
> between, so what is this all about? Maybe missing proper serialization
> somewhere else?
You need to work on your communication a bit - if there's a piece of
code you don't understand, maybe try being a bit less aggressive about
it? Otherwise people tend to just ignore you rather than explain it.
test_bit() is a lot faster than a test_and_set_bit(), and there's no
need to run the latter if the former returns true. This is a pretty
common optimization, particularly if the majority of the calls end up
having the bit set already.
Can the bit be flipped right after? Certainly! Can that happen if just
test_and_set_bit() is used? Of course! This isn't a critical section
with a lock, it's a piece of state. And guarding the RMW operation with
a test first doesn't change that one bit.
--
Jens Axboe
