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

[Jens Axboe <axboe@kernel.dk>]

On 11/26/19 11:08 AM, John Garry wrote:
> On 26/11/2019 17:25, Jens Axboe wrote:
>> On 11/26/19 10:23 AM, John Garry wrote:
>>> On 26/11/2019 17:11, Jens Axboe wrote:
>>>> On 11/26/19 9:54 AM, John Garry wrote:
>>>>> On 26/11/2019 15:14, Jens Axboe wrote:
>>>>>> On 11/26/19 2:14 AM, Hannes Reinecke wrote:
>>>>>>> Instead of allocating the tag bitmap in place we should be using a
>>>>>>> pointer. This is in preparation for shared host-wide bitmaps.
>>>>>>
>>>>>> Not a huge fan of this, it's an extra indirection in the hot path
>>>>>> of both submission and completion.
>>>>>
>>>>> Hi Jens,
>>>>>
>>>>> Thanks for having a look.
>>>>>
>>>>> I checked the disassembly for blk_mq_get_tag() as a sample - which I
>>>>> assume is one hot path function which you care about - and the cost of
>>>>> the indirection is a load instruction instead of an add, denoted by ***,
>>>>> below:
>>>>
>>>
>>> Hi Jens,
>>>
>>>> I'm not that worried about an extra instruction, my worry is the extra
>>>> load is from different memory. When it's embedded in the struct, we're
>>>> on the same cache line or adjacent.
>>>
>>> Right, so the earlier iteration of this series kept the embedded struct
>>> and we simply pointed at that, so I wouldn't expect a caching issue of
>>> different memory in that case.
>>
> 
> Hi Jens,
> 
>> That would be a much better solution for the common case, my concern
>> here is slowing down the fast path for device that don't need shared
>> tags.
>>
>> 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.

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. If if can be done
in a nice enough fashion and not sprinkled everywhere, in some fashion.

Should be testable, though.

-- 
Jens Axboe