Re: [PATCH 3/8] blk-mq: Use a pointer for sbitmap

[Jens Axboe <axboe@kernel.dk>]

On 11/27/19 6:12 AM, Hannes Reinecke wrote:
> On 11/27/19 2:05 PM, John Garry wrote:
>> On 27/11/2019 01:46, Jens Axboe wrote:
>>>>> Would be interesting to check the generated code for that, ideally we'd
>>>>> get rid of the extra load for that case, even if it is in the same
>>>>> cacheline.
>>>>>
>>>> I checked the disassembly and we still have the load instead of the add.
>>>>
>>>> This is not surprising, as the compiler would not know for certain that
>>>> we point to a field within the same struct. But at least we still should
>>>> point to a close memory.
>>>>
>>>> Note that the pointer could be dropped, which would remove the load, but
>>>> then we have many if-elses which could be slower, not to mention that
>>>> the blk-mq-tag code deals in bitmap pointers anyway.
>>
>> Hi Jens,
>>
>>> It might still be worthwhile to do:
>>>
>>> if (tags->ptr == &tags->__default)
>>>      foo(&tags->__default);
>>>
>>> to make it clear, as that branch will predict easily.
>>
>> Not sure. So this code does produce the same assembly, as we still need
>> to do the tags->ptr load for the comparison.
>>
>> And then if you consider blk_mq_get_tags() as an example, there is no
>> other hot value available to indicate whether the shared tags are used
>> to decide whether to use  &tags->__default.
>>
>> I'll consider it more.
>>
> After talking to our compiler folks I guess it should be possible to
> resurrect your original patch (with both the embedded bitmap and the
> point to the bitmap), linking the pointer to the embedded bitmap in the
> non-shared case.
> 
> Then we can access the pointer in all cases, but in the non-shared case
> that will then point to the embedded one, thus avoiding the possible
> cache miss.
> 
> I'll see how that goes.

That's exactly what I suggested yesterday, the discussion now is on how
to make that work in the cleanest way.

-- 
Jens Axboe