Reducing the number of IB interrupts caused by the SRP initiator

Reducing the number of IB interrupts caused by the SRP initiator

[Bart Van Assche]

Hello,

Once I noticed that the SRP initiator triggers two IB interrupts per I/O (more
precise, per srp_queuecommand() call) I started looking at reducing the number
of IB interrupts. The patch below didn't help. Does this mean that the patch
below isn't complete or is this because of the hw driver used during the test
(mlx4) ?

Bart.

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 54c8fe2..1f674b8 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -241,7 +241,7 @@ static int srp_create_target_ib(struct srp_target_port *target)
 	init_attr->cap.max_recv_wr     = SRP_RQ_SIZE;
 	init_attr->cap.max_recv_sge    = 1;
 	init_attr->cap.max_send_sge    = 1;
-	init_attr->sq_sig_type         = IB_SIGNAL_ALL_WR;
+	init_attr->sq_sig_type         = IB_SIGNAL_REQ_WR;
 	init_attr->qp_type             = IB_QPT_RC;
 	init_attr->send_cq             = target->cq;
 	init_attr->recv_cq             = target->cq;
@@ -1001,7 +1001,8 @@ static int __srp_post_send(struct srp_target_port *target,
 	wr.sg_list    = &list;
 	wr.num_sge    = 1;
 	wr.opcode     = IB_WR_SEND;
-	wr.send_flags = IB_SEND_SIGNALED;
+	wr.send_flags = target->tx_head - target->tx_tail == SRP_SQ_SIZE - 1
+		? IB_SEND_SIGNALED : 0;
 
 	ret = ib_post_send(target->qp, &wr, &bad_wr);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Reducing the number of IB interrupts caused by the SRP initiator

[Eli Cohen]

On Sun, Jan 17, 2010 at 9:08 PM, Bart Van Assche
<bart.vanassche-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hello,
>
> Once I noticed that the SRP initiator triggers two IB interrupts per I/O (more
> precise, per srp_queuecommand() call) I started looking at reducing the number
> of IB interrupts. The patch below didn't help. Does this mean that the patch
> below isn't complete or is this because of the hw driver used during the test
> (mlx4) ?
When you say that the patch did not help, do you mean that it did not
reduce the rate of interrupts?  I also think that you have a race here
since the head and tail operations are not protected (I don't know the
SRP driver I think locking should be done here). It would be better if
you'd choose a value that is a fraction of the queue size to request a
completion so that you don't get to a situation where your queue is
full and you the sender is waiting for it to be cleared. Also, you did
not touch the completion handler. Seems to me you should increment the
tail for each completion by the number of completions you skipped.
Failing to do so will keep cause the completion to be requested each
post after the first time it skipped queue len completions.


>
> Bart.
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 54c8fe2..1f674b8 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -241,7 +241,7 @@ static int srp_create_target_ib(struct srp_target_port *target)
>        init_attr->cap.max_recv_wr     = SRP_RQ_SIZE;
>        init_attr->cap.max_recv_sge    = 1;
>        init_attr->cap.max_send_sge    = 1;
> -       init_attr->sq_sig_type         = IB_SIGNAL_ALL_WR;
> +       init_attr->sq_sig_type         = IB_SIGNAL_REQ_WR;
>        init_attr->qp_type             = IB_QPT_RC;
>        init_attr->send_cq             = target->cq;
>        init_attr->recv_cq             = target->cq;
> @@ -1001,7 +1001,8 @@ static int __srp_post_send(struct srp_target_port *target,
>        wr.sg_list    = &list;
>        wr.num_sge    = 1;
>        wr.opcode     = IB_WR_SEND;
> -       wr.send_flags = IB_SEND_SIGNALED;
> +       wr.send_flags = target->tx_head - target->tx_tail == SRP_SQ_SIZE - 1
> +               ? IB_SEND_SIGNALED : 0;
>
>        ret = ib_post_send(target->qp, &wr, &bad_wr);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Reducing the number of IB interrupts caused by the SRP initiator

[Bart Van Assche]

On Mon, Jan 18, 2010 at 12:29 PM, Eli Cohen <eli-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
>
> On Sun, Jan 17, 2010 at 9:08 PM, Bart Van Assche
> <bart.vanassche-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Once I noticed that the SRP initiator triggers two IB interrupts per I/O (more
> > precise, per srp_queuecommand() call) I started looking at reducing the number
> > of IB interrupts. The patch below didn't help. Does this mean that the patch
> > below isn't complete or is this because of the hw driver used during the test
> > (mlx4) ?
>
> When you say that the patch did not help, do you mean that it did not
> reduce the rate of interrupts?

Yes.

> I also think that you have a race here
> since the head and tail operations are not protected (I don't know the
> SRP driver I think locking should be done here).

The race was already present in the original SRP driver: tx_tail is
not consistently protected by target->scsi_host->host_lock. As far as
my analysis of the tx_tail access patterns is correct this race
doesn't hurt in the original SRP driver. But you are right, I should
carefully analyze whether this race is acceptable.

> It would be better if
> you'd choose a value that is a fraction of the queue size to request a
> completion so that you don't get to a situation where your queue is
> full and you the sender is waiting for it to be cleared. Also, you did
> not touch the completion handler. Seems to me you should increment the
> tail for each completion by the number of completions you skipped.
> Failing to do so will keep cause the completion to be requested each
> post after the first time it skipped queue len completions.

Thanks -- will have a look at this.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html