On 5/22/23 11:17, Ming Lei wrote:
> On Mon, May 22, 2023 at 10:29:05AM +0900, Damien Le Moal wrote:
>> On 5/22/23 10:23, Ming Lei wrote:
>>> On Mon, May 22, 2023 at 10:15:22AM +0900, Damien Le Moal wrote:
>>>> On 5/22/23 09:43, Tian Lan wrote:
>>>>> From: Tian Lan <tian.lan@xxxxxxxxxxxx>
>>>>>
>>>>> If multiple CPUs are sharing the same hardware queue, it can
>>>>> cause leak in the active queue counter tracking when __blk_mq_tag_busy()
>>>>> is executed simultaneously.
>>>>>
>>>>> Fixes: ee78ec1077d3 ("blk-mq: blk_mq_tag_busy is no need to return a value")
>>>>> Signed-off-by: Tian Lan <tian.lan@xxxxxxxxxxxx>
>>>>> ---
>>>>> block/blk-mq-tag.c | 10 ++++++----
>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
>>>>> index d6af9d431dc6..07372032238a 100644
>>>>> --- a/block/blk-mq-tag.c
>>>>> +++ b/block/blk-mq-tag.c
>>>>> @@ -42,13 +42,15 @@ void __blk_mq_tag_busy(struct blk_mq_hw_ctx *hctx)
>>>>> if (blk_mq_is_shared_tags(hctx->flags)) {
>>>>> struct request_queue *q = hctx->queue;
>>>>>
>>>>> - if (test_bit(QUEUE_FLAG_HCTX_ACTIVE, &q->queue_flags))
>>>>> + if (test_bit(QUEUE_FLAG_HCTX_ACTIVE, &q->queue_flags) ||
>>>>> + test_and_set_bit(QUEUE_FLAG_HCTX_ACTIVE, &q->queue_flags)) {
>>>>
>>>> This is weird. test_and_set_bit() returns the bit old value, so shouldn't this be:
>>>>
>>>> if (test_and_set_bit(QUEUE_FLAG_HCTX_ACTIVE, &q->queue_flags))
>>>> return;
>>>>
>>>> ?
>>>
>>> It is one micro optimization since test_and_set_bit is much heavier than
>>> test_bit, so test_and_set_bit() is just needed in the 1st time.
>>
>> But having the 2 calls test_bit + test_and_set_bit creates a race, doesn't it ?
>> What if another cpu clears the bit between these 2 calls ?
>
> If test_bit() returns 0, there isn't such race since both sides are atomic OP.
>
> If test_bit() returns 1:
>
> 1) __blk_mq_tag_busy() vs. __blk_mq_tag_busy()
> - both does nothing
>
> 2) __blk_mq_tag_busy() vs. __blk_mq_tag_idle()
> - hctx_may_queue() is always following __blk_mq_tag_busy()
> - hctx_may_queue() returns true in case that this flag is cleared
> - current __blk_mq_tag_busy() does nothing, the following allocation
> works fine given hctx_may_queue() returns true
> - new __blk_mq_tag_busy() will setup everything as fine
OK. Thanks. It would be nice to update the function comment (which is not very
clear due to grammar issues) to document this unusual use of the bit functions,
including the fact that it is an optimization to avoid dirtying cache lines.
>
>
> Thanks,
> Ming
>
--
Damien Le Moal
Western Digital Research
