[PATCH] iser-target: Disable TX completion interrupt coalescing

[PATCH] iser-target: Disable TX completion interrupt coalescing

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch explicitly disables TX completion interrupt coalescing logic
in isert_put_response() and isert_put_datain() that was originally added
as an efficiency optimization in commit 95b60f07.

It has been reported that this change can trigger ABORT_TASK timeouts
under certain small block workloads, where disabling coalescing was
required for stability.  According to Sagi, this doesn't impact
overall performance, so go ahead and disable it for now.

Reported-by: Moussa Ba <moussaba@micron.com>
Cc: Moussa Ba <moussaba@micron.com>
Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
Cc: <stable@vger.kernel.org> # 3.13+
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 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 40969b6..f719112 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -2183,7 +2183,7 @@ isert_put_response(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 		isert_cmd->tx_desc.num_sge = 2;
 	}
 
-	isert_init_send_wr(isert_conn, isert_cmd, send_wr, true);
+	isert_init_send_wr(isert_conn, isert_cmd, send_wr, false);
 
 	pr_debug("Posting SCSI Response IB_WR_SEND >>>>>>>>>>>>>>>>>>>>>>\n");
 
@@ -2882,7 +2882,7 @@ isert_put_datain(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 				     &isert_cmd->tx_desc.iscsi_header);
 		isert_init_tx_hdrs(isert_conn, &isert_cmd->tx_desc);
 		isert_init_send_wr(isert_conn, isert_cmd,
-				   &isert_cmd->tx_desc.send_wr, true);
+				   &isert_cmd->tx_desc.send_wr, false);
 		isert_cmd->rdma_wr.s_send_wr.next = &isert_cmd->tx_desc.send_wr;
 		wr->send_wr_num += 1;
 	}
-- 
1.7.10.4

Re: [PATCH] iser-target: Disable TX completion interrupt coalescing

[Sagi Grimberg]

On 10/6/2014 5:15 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch explicitly disables TX completion interrupt coalescing logic
> in isert_put_response() and isert_put_datain() that was originally added
> as an efficiency optimization in commit 95b60f07.
>
> It has been reported that this change can trigger ABORT_TASK timeouts
> under certain small block workloads, where disabling coalescing was
> required for stability.  According to Sagi, this doesn't impact
> overall performance, so go ahead and disable it for now.
>
> Reported-by: Moussa Ba <moussaba@micron.com>
> Cc: Moussa Ba <moussaba@micron.com>
> Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Cc: <stable@vger.kernel.org> # 3.13+
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   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 40969b6..f719112 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -2183,7 +2183,7 @@ isert_put_response(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
>   		isert_cmd->tx_desc.num_sge = 2;
>   	}
>
> -	isert_init_send_wr(isert_conn, isert_cmd, send_wr, true);
> +	isert_init_send_wr(isert_conn, isert_cmd, send_wr, false);

I Would like to avoid taking the conn_mutex in the IO path.
Care to respin this with avoiding taking the mutex? Or you want me to?
I have a couple of patches in the pipe anyway...

Sagi.

Re: [PATCH] iser-target: Disable TX completion interrupt coalescing

[Nicholas A. Bellinger]

On Tue, 2014-10-07 at 09:58 +0300, Sagi Grimberg wrote:
> On 10/6/2014 5:15 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch explicitly disables TX completion interrupt coalescing logic
> > in isert_put_response() and isert_put_datain() that was originally added
> > as an efficiency optimization in commit 95b60f07.
> >
> > It has been reported that this change can trigger ABORT_TASK timeouts
> > under certain small block workloads, where disabling coalescing was
> > required for stability.  According to Sagi, this doesn't impact
> > overall performance, so go ahead and disable it for now.
> >
> > Reported-by: Moussa Ba <moussaba@micron.com>
> > Cc: Moussa Ba <moussaba@micron.com>
> > Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > Cc: <stable@vger.kernel.org> # 3.13+
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   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 40969b6..f719112 100644
> > --- a/drivers/infiniband/ulp/isert/ib_isert.c
> > +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> > @@ -2183,7 +2183,7 @@ isert_put_response(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
> >   		isert_cmd->tx_desc.num_sge = 2;
> >   	}
> >
> > -	isert_init_send_wr(isert_conn, isert_cmd, send_wr, true);
> > +	isert_init_send_wr(isert_conn, isert_cmd, send_wr, false);
> 
> I Would like to avoid taking the conn_mutex in the IO path.

Yes, please.

> Care to respin this with avoiding taking the mutex? Or you want me to?

Let's keep this patch as is for -rc1 so it can be easily picked up for
stable, especially considering that it's been verified by Moussa for
some time.

> I have a couple of patches in the pipe anyway...
> 

Thank you Sagi.

--nab

Re: [PATCH] iser-target: Disable TX completion interrupt coalescing

[Sagi Grimberg]

On 10/8/2014 9:01 AM, Nicholas A. Bellinger wrote:
> On Tue, 2014-10-07 at 09:58 +0300, Sagi Grimberg wrote:
>> On 10/6/2014 5:15 AM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>>
>>> This patch explicitly disables TX completion interrupt coalescing logic
>>> in isert_put_response() and isert_put_datain() that was originally added
>>> as an efficiency optimization in commit 95b60f07.
>>>
>>> It has been reported that this change can trigger ABORT_TASK timeouts
>>> under certain small block workloads, where disabling coalescing was
>>> required for stability.  According to Sagi, this doesn't impact
>>> overall performance, so go ahead and disable it for now.
>>>
>>> Reported-by: Moussa Ba <moussaba@micron.com>
>>> Cc: Moussa Ba <moussaba@micron.com>
>>> Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
>>> Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
>>> Cc: <stable@vger.kernel.org> # 3.13+
>>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>>> ---
>>>    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 40969b6..f719112 100644
>>> --- a/drivers/infiniband/ulp/isert/ib_isert.c
>>> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
>>> @@ -2183,7 +2183,7 @@ isert_put_response(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
>>>    		isert_cmd->tx_desc.num_sge = 2;
>>>    	}
>>>
>>> -	isert_init_send_wr(isert_conn, isert_cmd, send_wr, true);
>>> +	isert_init_send_wr(isert_conn, isert_cmd, send_wr, false);
>>
>> I Would like to avoid taking the conn_mutex in the IO path.
>
> Yes, please.
>
>> Care to respin this with avoiding taking the mutex? Or you want me to?
>
> Let's keep this patch as is for -rc1 so it can be easily picked up for
> stable, especially considering that it's been verified by Moussa for
> some time.

OK we can do that.

But I do plan to remove this code altogether.
I have some fixes and optimizations I want to add
and it will be much easier without this code (given
its not active).

Once those are in, I think it would be easier to handle
TX completions coalescing.

You can add:
acked-by: Sagi Grimberg <sagig@dev.mellanox.co.il>

Sagi.