Re: [PATCH 1/2] IB/isert: use unlikely macro in the fast path

[Max Gurtovoy <maxg@mellanox.com>]

On 8/5/2020 7:37 PM, Leon Romanovsky wrote:
> On Wed, Aug 05, 2020 at 07:28:50PM +0300, Max Gurtovoy wrote:
>> On 8/5/2020 7:06 PM, Leon Romanovsky wrote:
>>> On Wed, Aug 05, 2020 at 06:14:16PM +0300, Max Gurtovoy wrote:
>>>> On 8/5/2020 4:16 PM, Leon Romanovsky wrote:
>>>>> On Wed, Aug 05, 2020 at 03:12:30PM +0300, Max Gurtovoy wrote:
>>>>>> Add performance optimization that might slightly improve small IO sizes
>>>>>> benchmarks.
>>>>>>
>>>>>> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
>>>>>> ---
>>>>>>     drivers/infiniband/ulp/isert/ib_isert.c | 4 ++--
>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>> I find the expectation from "unlikely/likely" keywords to be overrated.
>>>>>
>>>>> When we introduced dissagregate post send verbs in rdma-core, we
>>>>> benchmarked likely/unlikely and didn't find any significant difference
>>>>> for code with and without such keywords.
>>>>>
>>>>> Thanks
>>>> Leon,
>>>>
>>>> We are using these small optimizations in all our ULPs and we saw benefit in
>>>> large scale and high loads (we did the same in NVMf/RDMA).
>>>>
>>>> These kind of optimizations might not be seen immediately but are
>>>> accumulated.
>>>>
>>>> I don't know why do you compare user-space benchmarks to storage drivers.
>>> Why not? It produces same asm code and both have same performance
>>> characteristic.
>>>
>>>> Can you please review the code ?
>>> There is nothing to review here, the patch is straightforward, I just
>>> don't believe in it.
>> Its ok.
>>
>> Just ignore it if you don't want to review it.
> OK, just because you asked.
>
> I reviewed this patch and didn't find any justification for performance
> claim, can you please provide us numbers before/after so we will be able
> to decide based on reliable data? It will help us to review our drivers
> and improve them even more.

As I said, these are incremental optimizations that probably won't be 
seen immediately with 1 or 2 changes. But accumulated small 
optimizations can reach to 3%-4%.

If you don't believe in this patch - ignore it and review others. I'm 
sure you have a lot. Let other maintainers review it.

You're also welcomed to remove the likely/unlikely macros from all Linux 
kernel and let's see what comments will it get from other maintainers.

>> The maintainers of iser target will review and decide if they believe in it
>> or not.
> Sure, I don't care who will provide numbers.

I'm not talking about providing numbers.

>
> Thanks
>
>>
>>>> Sagi,
>>>>
>>>> Can you send your comments as well ?
>>>>
>>>>