diff mbox

[v2,3/4] IB/mlx5, ib_post_send(), IB_WR_REG_SIG_MR: Do not modify the 'wr' argument

Message ID 20180706230811.25720-4-bart.vanassche@wdc.com (mailing list archive)
State Superseded
Headers show

Commit Message

Bart Van Assche July 6, 2018, 11:08 p.m. UTC 
Since the next patch will constify the wr pointer, do not modify
the data that pointer points at.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/infiniband/hw/mlx5/qp.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

Leon Romanovsky July 8, 2018, 7:30 a.m. UTC | #1 
On Fri, Jul 06, 2018 at 04:08:10PM -0700, Bart Van Assche wrote:
> Since the next patch will constify the wr pointer, do not modify
> the data that pointer points at.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> ---
>  drivers/infiniband/hw/mlx5/qp.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> index ecfe96c4b1db..b55950bb4291 100644
> --- a/drivers/infiniband/hw/mlx5/qp.c
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -4316,10 +4316,10 @@ static void dump_wqe(struct mlx5_ib_qp *qp, int idx, int size_16)
>  	}
>  }
>
> -static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
> +static int __begin_wqe(struct mlx5_ib_qp *qp, void **seg,
>  		     struct mlx5_wqe_ctrl_seg **ctrl,
>  		     const struct ib_send_wr *wr, unsigned *idx,
> -		     int *size, int nreq)
> +		     int *size, int nreq, bool send_signaled, bool solicited)
>  {
>  	if (unlikely(mlx5_wq_overflow(&qp->sq, nreq, qp->ibqp.send_cq)))
>  		return -ENOMEM;
> @@ -4330,10 +4330,8 @@ static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
>  	*(uint32_t *)(*seg + 8) = 0;
>  	(*ctrl)->imm = send_ieth(wr);
>  	(*ctrl)->fm_ce_se = qp->sq_signal_bits |
> -		(wr->send_flags & IB_SEND_SIGNALED ?
> -		 MLX5_WQE_CTRL_CQ_UPDATE : 0) |
> -		(wr->send_flags & IB_SEND_SOLICITED ?
> -		 MLX5_WQE_CTRL_SOLICITED : 0);
> +		(send_signaled ? MLX5_WQE_CTRL_CQ_UPDATE : 0) |
> +		(solicited ? MLX5_WQE_CTRL_SOLICITED : 0);
>
>  	*seg += sizeof(**ctrl);
>  	*size = sizeof(**ctrl) / 16;
> @@ -4341,6 +4339,16 @@ static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
>  	return 0;
>  }
>
> +static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
> +		     struct mlx5_wqe_ctrl_seg **ctrl,
> +		     const struct ib_send_wr *wr, unsigned *idx,
> +		     int *size, int nreq)
> +{
> +	return __begin_wqe(qp, seg, ctrl, wr, idx, size, nreq,
> +			   wr->send_flags & IB_SEND_SIGNALED,
> +			   wr->send_flags & IB_SEND_SOLICITED);
> +}

It adds an extra call to data-path, most probably the effect of it will
be neglected, but it needs Ack from Max.


> +
>  static void finish_wqe(struct mlx5_ib_qp *qp,
>  		       struct mlx5_wqe_ctrl_seg *ctrl,
>  		       u8 size, unsigned idx, u64 wr_id,
> @@ -4499,10 +4507,8 @@ static int _mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  				 * SET_PSV WQEs are not signaled and solicited
>  				 * on error
>  				 */
> -				wr->send_flags &= ~IB_SEND_SIGNALED;
> -				wr->send_flags |= IB_SEND_SOLICITED;
> -				err = begin_wqe(qp, &seg, &ctrl, wr,
> -						&idx, &size, nreq);
> +				err = __begin_wqe(qp, &seg, &ctrl, wr, &idx,
> +						  &size, nreq, 0, 1);

1. 0 -> false, 1 -> true
2. There is another begin_wqe() in the case of IB_WR_REG_SIG_MR which
needs proper wr->send_flags.

>  				if (err) {
>  					mlx5_ib_warn(dev, "\n");
>  					err = -ENOMEM;
> --
> 2.18.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche July 9, 2018, 8:49 p.m. UTC | #2 
On Sun, 2018-07-08 at 10:30 +0300, Leon Romanovsky wrote:
> On Fri, Jul 06, 2018 at 04:08:10PM -0700, Bart Van Assche wrote:

> > +static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,

> > +		     struct mlx5_wqe_ctrl_seg **ctrl,

> > +		     const struct ib_send_wr *wr, unsigned *idx,

> > +		     int *size, int nreq)

> > +{

> > +	return __begin_wqe(qp, seg, ctrl, wr, idx, size, nreq,

> > +			   wr->send_flags & IB_SEND_SIGNALED,

> > +			   wr->send_flags & IB_SEND_SOLICITED);

> > +}

> 

> It adds an extra call to data-path, most probably the effect of it will

> be neglected, but it needs Ack from Max.


Hi Leon,

Since this patch changes begin_wqe() into a one-line function I expect that the
compiler will inline __begin_wqe() into begin_wqe().

> >  static void finish_wqe(struct mlx5_ib_qp *qp,

> >  		       struct mlx5_wqe_ctrl_seg *ctrl,

> >  		       u8 size, unsigned idx, u64 wr_id,

> > @@ -4499,10 +4507,8 @@ static int _mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,

> >  				 * SET_PSV WQEs are not signaled and solicited

> >  				 * on error

> >  				 */

> > -				wr->send_flags &= ~IB_SEND_SIGNALED;

> > -				wr->send_flags |= IB_SEND_SOLICITED;

> > -				err = begin_wqe(qp, &seg, &ctrl, wr,

> > -						&idx, &size, nreq);

> > +				err = __begin_wqe(qp, &seg, &ctrl, wr, &idx,

> > +						  &size, nreq, 0, 1);

> 

> 1. 0 -> false, 1 -> true

> 2. There is another begin_wqe() in the case of IB_WR_REG_SIG_MR which

> needs proper wr->send_flags.


I will use true and false instead of 1 and 0. But please note that the prototype
of begin_wqe() has not been changed so the begin_wqe() calls that do not modify
the work request send flags do not have to be modified.

Bart.
Leon Romanovsky July 10, 2018, 6 a.m. UTC | #3 
On Mon, Jul 09, 2018 at 08:49:38PM +0000, Bart Van Assche wrote:
> On Sun, 2018-07-08 at 10:30 +0300, Leon Romanovsky wrote:
> > On Fri, Jul 06, 2018 at 04:08:10PM -0700, Bart Van Assche wrote:
> > > +static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
> > > +		     struct mlx5_wqe_ctrl_seg **ctrl,
> > > +		     const struct ib_send_wr *wr, unsigned *idx,
> > > +		     int *size, int nreq)
> > > +{
> > > +	return __begin_wqe(qp, seg, ctrl, wr, idx, size, nreq,
> > > +			   wr->send_flags & IB_SEND_SIGNALED,
> > > +			   wr->send_flags & IB_SEND_SOLICITED);
> > > +}
> >
> > It adds an extra call to data-path, most probably the effect of it will
> > be neglected, but it needs Ack from Max.
>
> Hi Leon,
>
> Since this patch changes begin_wqe() into a one-line function I expect that the
> compiler will inline __begin_wqe() into begin_wqe().

Agree

>
> > >  static void finish_wqe(struct mlx5_ib_qp *qp,
> > >  		       struct mlx5_wqe_ctrl_seg *ctrl,
> > >  		       u8 size, unsigned idx, u64 wr_id,
> > > @@ -4499,10 +4507,8 @@ static int _mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
> > >  				 * SET_PSV WQEs are not signaled and solicited
> > >  				 * on error
> > >  				 */
> > > -				wr->send_flags &= ~IB_SEND_SIGNALED;
> > > -				wr->send_flags |= IB_SEND_SOLICITED;
> > > -				err = begin_wqe(qp, &seg, &ctrl, wr,
> > > -						&idx, &size, nreq);
> > > +				err = __begin_wqe(qp, &seg, &ctrl, wr, &idx,
> > > +						  &size, nreq, 0, 1);
> >
> > 1. 0 -> false, 1 -> true
> > 2. There is another begin_wqe() in the case of IB_WR_REG_SIG_MR which
> > needs proper wr->send_flags.
>
> I will use true and false instead of 1 and 0. But please note that the prototype
> of begin_wqe() has not been changed so the begin_wqe() calls that do not modify
> the work request send flags do not have to be modified.

Right, but you removed updates of "wr->send_flags" in the lines above,
and it will change behaviour of next begin_wqe() in that case.

>
> Bart.
>
>
>
>
Bart Van Assche July 10, 2018, 3:04 p.m. UTC | #4 
T24gVHVlLCAyMDE4LTA3LTEwIGF0IDA5OjAwICswMzAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6
DQo+IFJpZ2h0LCBidXQgeW91IHJlbW92ZWQgdXBkYXRlcyBvZiAid3ItPnNlbmRfZmxhZ3MiIGlu
IHRoZSBsaW5lcyBhYm92ZSwNCj4gYW5kIGl0IHdpbGwgY2hhbmdlIGJlaGF2aW91ciBvZiBuZXh0
IGJlZ2luX3dxZSgpIGluIHRoYXQgY2FzZS4NCg0KQWgsIHRoYW5rcywgdGhhdCdzIHNvbWV0aGlu
ZyBJIGhhZCBtaXNzZWQuIEkgd2lsbCB1cGRhdGUgcGF0Y2ggMy80Lg0KDQpCYXJ0Lg0KDQoNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index ecfe96c4b1db..b55950bb4291 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -4316,10 +4316,10 @@  static void dump_wqe(struct mlx5_ib_qp *qp, int idx, int size_16)
 	}
 }
 
-static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
+static int __begin_wqe(struct mlx5_ib_qp *qp, void **seg,
 		     struct mlx5_wqe_ctrl_seg **ctrl,
 		     const struct ib_send_wr *wr, unsigned *idx,
-		     int *size, int nreq)
+		     int *size, int nreq, bool send_signaled, bool solicited)
 {
 	if (unlikely(mlx5_wq_overflow(&qp->sq, nreq, qp->ibqp.send_cq)))
 		return -ENOMEM;
@@ -4330,10 +4330,8 @@  static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
 	*(uint32_t *)(*seg + 8) = 0;
 	(*ctrl)->imm = send_ieth(wr);
 	(*ctrl)->fm_ce_se = qp->sq_signal_bits |
-		(wr->send_flags & IB_SEND_SIGNALED ?
-		 MLX5_WQE_CTRL_CQ_UPDATE : 0) |
-		(wr->send_flags & IB_SEND_SOLICITED ?
-		 MLX5_WQE_CTRL_SOLICITED : 0);
+		(send_signaled ? MLX5_WQE_CTRL_CQ_UPDATE : 0) |
+		(solicited ? MLX5_WQE_CTRL_SOLICITED : 0);
 
 	*seg += sizeof(**ctrl);
 	*size = sizeof(**ctrl) / 16;
@@ -4341,6 +4339,16 @@  static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
 	return 0;
 }
 
+static int begin_wqe(struct mlx5_ib_qp *qp, void **seg,
+		     struct mlx5_wqe_ctrl_seg **ctrl,
+		     const struct ib_send_wr *wr, unsigned *idx,
+		     int *size, int nreq)
+{
+	return __begin_wqe(qp, seg, ctrl, wr, idx, size, nreq,
+			   wr->send_flags & IB_SEND_SIGNALED,
+			   wr->send_flags & IB_SEND_SOLICITED);
+}
+
 static void finish_wqe(struct mlx5_ib_qp *qp,
 		       struct mlx5_wqe_ctrl_seg *ctrl,
 		       u8 size, unsigned idx, u64 wr_id,
@@ -4499,10 +4507,8 @@  static int _mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 				 * SET_PSV WQEs are not signaled and solicited
 				 * on error
 				 */
-				wr->send_flags &= ~IB_SEND_SIGNALED;
-				wr->send_flags |= IB_SEND_SOLICITED;
-				err = begin_wqe(qp, &seg, &ctrl, wr,
-						&idx, &size, nreq);
+				err = __begin_wqe(qp, &seg, &ctrl, wr, &idx,
+						  &size, nreq, 0, 1);
 				if (err) {
 					mlx5_ib_warn(dev, "\n");
 					err = -ENOMEM;