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

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

[Max Gurtovoy]

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(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index b7df38ee8ae0..c818eebe6538 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -847,7 +847,7 @@ isert_post_recv(struct isert_conn *isert_conn, struct iser_rx_desc *rx_desc)
 	rx_wr.next = NULL;
 
 	ret = ib_post_recv(isert_conn->qp, &rx_wr, NULL);
-	if (ret)
+	if (unlikely(ret))
 		isert_err("ib_post_recv() failed with ret: %d\n", ret);
 
 	return ret;
@@ -1831,7 +1831,7 @@ isert_post_response(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd)
 	}
 
 	ret = ib_post_send(isert_conn->qp, &isert_cmd->tx_desc.send_wr, NULL);
-	if (ret) {
+	if (unlikely(ret)) {
 		isert_err("ib_post_send failed with %d\n", ret);
 		return ret;
 	}
-- 
2.18.1

[PATCH 2/2] IB/isert: remove duplicated error prints

[Max Gurtovoy]

The isert_post_recv function prints an error in case of failures, so no
need for the callers to add another print.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index c818eebe6538..58c2a6ca5a5e 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -1243,12 +1243,7 @@ isert_handle_iscsi_dataout(struct isert_conn *isert_conn,
 	 * multiple data-outs on the same command can arrive -
 	 * so post the buffer before hand
 	 */
-	rc = isert_post_recv(isert_conn, rx_desc);
-	if (rc) {
-		isert_err("ib_post_recv failed with %d\n", rc);
-		return rc;
-	}
-	return 0;
+	return isert_post_recv(isert_conn, rx_desc);
 }
 
 static int
@@ -1825,10 +1820,8 @@ isert_post_response(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd)
 	int ret;
 
 	ret = isert_post_recv(isert_conn, isert_cmd->rx_desc);
-	if (ret) {
-		isert_err("ib_post_recv failed with %d\n", ret);
+	if (ret)
 		return ret;
-	}
 
 	ret = ib_post_send(isert_conn->qp, &isert_cmd->tx_desc.send_wr, NULL);
 	if (unlikely(ret)) {
@@ -2200,10 +2193,8 @@ isert_put_datain(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 				   &isert_cmd->tx_desc.send_wr);
 
 		rc = isert_post_recv(isert_conn, isert_cmd->rx_desc);
-		if (rc) {
-			isert_err("ib_post_recv failed with %d\n", rc);
+		if (rc)
 			return rc;
-		}
 
 		chain_wr = &isert_cmd->tx_desc.send_wr;
 	}
-- 
2.18.1

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

[Leon Romanovsky]

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

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

[Max Gurtovoy]

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.

Can you please review the code ?

Sagi,

Can you send your comments as well ?

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

[Leon Romanovsky]

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.

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

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

[Max Gurtovoy]

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.

The maintainers of iser target will review and decide if they believe in 
it or not.


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

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

[Leon Romanovsky]

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.

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

Thanks

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

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

[Max Gurtovoy]

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

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

[Sagi Grimberg]

Looks fine,

Acked-by: Sagi Grimberg <sagi@grimberg.me>

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

[Sagi Grimberg]

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

I don't see any reason to find evidence in justification here. It's a
fastpath call, which is unlikely to fail, and these macros are
considered common practice.

There is no reason to make Max to go and quantify a micro-optimization.

Re: [PATCH 2/2] IB/isert: remove duplicated error prints

[Sagi Grimberg]

Acked-by: Sagi Grimberg <sagi@grimberg.me>

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

[Leon Romanovsky]

On Thu, Aug 06, 2020 at 12:51:15PM -0700, Sagi Grimberg wrote:
>
> > 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.
>
> I don't see any reason to find evidence in justification here. It's a
> fastpath call, which is unlikely to fail, and these macros are
> considered common practice.
>
> There is no reason to make Max to go and quantify a micro-optimization.

Unfortunately Max didn't try to see if these likely/unlikely macros
change something, but I did.

Simple objdump -d before and after shows that GCC 9 generates same
ISERT code before and after this patch. It is expected and there are a lot
of reasons for that, but all of them can be reduced to two:
* First, GCC is awesome in building profiled code with right predictions for
standard flows.
* Second, likely/unlikely is intended to be used when input/output is random
from GCC point of view.

So as a summary, there is no optimization here, just misuse of unlikely macro.

BTW, old GCCs behave the same and kernel full of wrong copy/paste.

Thanks

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

[Sagi Grimberg]

>>> 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.
>>
>> I don't see any reason to find evidence in justification here. It's a
>> fastpath call, which is unlikely to fail, and these macros are
>> considered common practice.
>>
>> There is no reason to make Max to go and quantify a micro-optimization.
> 
> Unfortunately Max didn't try to see if these likely/unlikely macros
> change something, but I did.
> 
> Simple objdump -d before and after shows that GCC 9 generates same
> ISERT code before and after this patch. It is expected and there are a lot
> of reasons for that, but all of them can be reduced to two:
> * First, GCC is awesome in building profiled code with right predictions for
> standard flows.
> * Second, likely/unlikely is intended to be used when input/output is random
> from GCC point of view.
> 
> So as a summary, there is no optimization here, just misuse of unlikely macro.
> 
> BTW, old GCCs behave the same and kernel full of wrong copy/paste.

if that is the case, then we can drop this patch. Thanks for checking.

Re: [PATCH 2/2] IB/isert: remove duplicated error prints

[Jason Gunthorpe]

On Wed, Aug 05, 2020 at 03:12:31PM +0300, Max Gurtovoy wrote:
> The isert_post_recv function prints an error in case of failures, so no
> need for the callers to add another print.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Acked-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/infiniband/ulp/isert/ib_isert.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)

Applied to for-next

I dropped patch 1/1 about the unlikelys

Jason