| Message ID | 20180706230811.25720-4-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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. > > > >
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 --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;
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(-)