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

[Hannes Reinecke <hare@suse.de>]

On 11/27/19 3:44 PM, John Garry wrote:
> On 27/11/2019 14:21, Jens Axboe wrote:
>> On 11/27/19 6:05 AM, 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.
>>
> 
> Hi Jens,
> 
>> How can it be the same? The approach in the patchset needs to load
>> *tags->ptr, this one needs tags->ptr. That's the big difference.
>>
> 
> In the patch for this thread, we have:
> 
> @@ -121,10 +121,10 @@ unsigned int blk_mq_get_tag(struct 
> blk_mq_alloc_data *data)
>               WARN_ON_ONCE(1);
>               return BLK_MQ_TAG_FAIL;
>           }
> -        bt = &tags->breserved_tags;
> +        bt = tags->breserved_tags;
>           tag_offset = 0;
>       } else {
> -        bt = &tags->bitmap_tags;
> +        bt = tags->bitmap_tags;
>           tag_offset = tags->nr_reserved_tags;
>       }
> 
> 
> So current code gets bt pointer by simply offsetting a certain distance 
> from tags pointer - that is the add I mention.
> 
> With the change in this patch, we need to load memory at address 
> &tags->bitmap_tags to get bt - this is the load I mention.
> 
> So for this:
> 
> if (tags->ptr == &tags->__default)
> 
> We load &tags->ptr to get the pointer value for comparison vs 
> &tags->__default.
> 
> There must be something I'm missing...
> 
The point here was that the load might refer to _other_ memory locations 
(as it's being allocated separately), thus incurring a cache miss.
With embedded tag bitmaps we'll load from the same cache line 
(hopefully), and won't get a performance hit.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)