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

John Garry
2019-11-27T14:44:40Z

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...

Thanks,
John