| Message ID | 20200525172212.14413-5-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Four SRP initiator and target patches | expand |
On Mon, May 25, 2020 at 10:22:12AM -0700, Bart Van Assche wrote: > The ib_srpt driver limits max_send_sge to 16. Since that is a workaround > for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do > not use the value advertised by the driver (32) since that causes QP's > to transition to the error status. Bart, How are you avoiding mlx4 bug in this patch? Isn't "attrs->max_send_sge" come from driver as is? Thanks > > See also commit f95ccffc715b ("IB/mlx4: Use 4K pages for kernel QP's WQE > buffer"). > > Cc: Laurence Oberman <loberman@redhat.com> > Cc: Kamal Heib <kamalheib1@gmail.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/infiniband/ulp/srpt/ib_srpt.c | 3 +-- > drivers/infiniband/ulp/srpt/ib_srpt.h | 5 ----- > 2 files changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > index 1ad3cc7c553a..86e4c87e7ec2 100644 > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > @@ -1816,8 +1816,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch) > */ > qp_init->cap.max_send_wr = min(sq_size / 2, attrs->max_qp_wr); > qp_init->cap.max_rdma_ctxs = sq_size / 2; > - qp_init->cap.max_send_sge = min(attrs->max_send_sge, > - SRPT_MAX_SG_PER_WQE); > + qp_init->cap.max_send_sge = attrs->max_send_sge; > qp_init->cap.max_recv_sge = 1; > qp_init->port_num = ch->sport->port; > if (sdev->use_srq) > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h > index 2e1a69840857..f31c349d07a1 100644 > --- a/drivers/infiniband/ulp/srpt/ib_srpt.h > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.h > @@ -105,11 +105,6 @@ enum { > SRP_CMD_ACA = 0x4, > > SRPT_DEF_SG_TABLESIZE = 128, > - /* > - * An experimentally determined value that avoids that QP creation > - * fails due to "swiotlb buffer is full" on systems using the swiotlb. > - */ > - SRPT_MAX_SG_PER_WQE = 16, > > MIN_SRPT_SQ_SIZE = 16, > DEF_SRPT_SQ_SIZE = 4096,
On 2020-05-25 10:51, Leon Romanovsky wrote: > On Mon, May 25, 2020 at 10:22:12AM -0700, Bart Van Assche wrote: >> The ib_srpt driver limits max_send_sge to 16. Since that is a workaround >> for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do >> not use the value advertised by the driver (32) since that causes QP's >> to transition to the error status. > > How are you avoiding mlx4 bug in this patch? > Isn't "attrs->max_send_sge" come from driver as is? Hi Leon, Development of this patch started considerable time ago - before the ib_srpt driver was converted to the RDMA R/W API. Before that conversion the ib_srpt driver was using attrs->max_send_sge limit for both RDMA writes and RDMA reads, which is wrong. Hence the need for the "max_sge_delta" parameter (max_send_sge = 32 and max_sge_rd = 30 for mlx4). The following code from drivers/infiniband/core/rw.c selects the proper limit: u32 max_sge = dir == DMA_TO_DEVICE ? qp->max_write_sge : qp->max_read_sge; The following code in drivers/infiniband/core/verbs.c sets these limits: qp->max_write_sge = qp_init_attr->cap.max_send_sge; qp->max_read_sge = min_t(u32, qp_init_attr->cap.max_send_sge, device->attrs.max_sge_rd); Bart.
On 2020-05-25 12:15, Bart Van Assche wrote: > On 2020-05-25 10:51, Leon Romanovsky wrote: >> On Mon, May 25, 2020 at 10:22:12AM -0700, Bart Van Assche wrote: >>> The ib_srpt driver limits max_send_sge to 16. Since that is a workaround >>> for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do >>> not use the value advertised by the driver (32) since that causes QP's >>> to transition to the error status. >> >> How are you avoiding mlx4 bug in this patch? >> Isn't "attrs->max_send_sge" come from driver as is? > > Hi Leon, > > Development of this patch started considerable time ago - before the > ib_srpt driver was converted to the RDMA R/W API. Before that conversion > the ib_srpt driver was using attrs->max_send_sge limit for both RDMA > writes and RDMA reads, which is wrong. Hence the need for the > "max_sge_delta" parameter (max_send_sge = 32 and max_sge_rd = 30 for > mlx4). The following code from drivers/infiniband/core/rw.c selects the > proper limit: > > u32 max_sge = dir == DMA_TO_DEVICE ? qp->max_write_sge : > qp->max_read_sge; > > The following code in drivers/infiniband/core/verbs.c sets these limits: > > qp->max_write_sge = qp_init_attr->cap.max_send_sge; > qp->max_read_sge = min_t(u32, qp_init_attr->cap.max_send_sge, > device->attrs.max_sge_rd); The commit message should be shortened to the following: "The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. See also commit f95ccffc715b ("IB/mlx4: Use 4K pages for kernel QP's WQE buffer")." Bart.
On Mon, May 25, 2020 at 02:51:25PM -0700, Bart Van Assche wrote: > On 2020-05-25 12:15, Bart Van Assche wrote: > > On 2020-05-25 10:51, Leon Romanovsky wrote: > >> On Mon, May 25, 2020 at 10:22:12AM -0700, Bart Van Assche wrote: > >>> The ib_srpt driver limits max_send_sge to 16. Since that is a workaround > >>> for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do > >>> not use the value advertised by the driver (32) since that causes QP's > >>> to transition to the error status. > >> > >> How are you avoiding mlx4 bug in this patch? > >> Isn't "attrs->max_send_sge" come from driver as is? > > > > Hi Leon, > > > > Development of this patch started considerable time ago - before the > > ib_srpt driver was converted to the RDMA R/W API. Before that conversion > > the ib_srpt driver was using attrs->max_send_sge limit for both RDMA > > writes and RDMA reads, which is wrong. Hence the need for the > > "max_sge_delta" parameter (max_send_sge = 32 and max_sge_rd = 30 for > > mlx4). The following code from drivers/infiniband/core/rw.c selects the > > proper limit: > > > > u32 max_sge = dir == DMA_TO_DEVICE ? qp->max_write_sge : > > qp->max_read_sge; > > > > The following code in drivers/infiniband/core/verbs.c sets these limits: > > > > qp->max_write_sge = qp_init_attr->cap.max_send_sge; > > qp->max_read_sge = min_t(u32, qp_init_attr->cap.max_send_sge, > > device->attrs.max_sge_rd); > > The commit message should be shortened to the following: "The ib_srpt > driver limits max_send_sge to 16. Since that is a workaround > for an mlx4 bug that has been fixed, increase max_send_sge. See also > commit f95ccffc715b ("IB/mlx4: Use 4K pages for kernel QP's WQE buffer")." Yes, please. The proposed commit message describes better the change. Thanks > > Bart.
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c index 1ad3cc7c553a..86e4c87e7ec2 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.c +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c @@ -1816,8 +1816,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch) */ qp_init->cap.max_send_wr = min(sq_size / 2, attrs->max_qp_wr); qp_init->cap.max_rdma_ctxs = sq_size / 2; - qp_init->cap.max_send_sge = min(attrs->max_send_sge, - SRPT_MAX_SG_PER_WQE); + qp_init->cap.max_send_sge = attrs->max_send_sge; qp_init->cap.max_recv_sge = 1; qp_init->port_num = ch->sport->port; if (sdev->use_srq) diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h index 2e1a69840857..f31c349d07a1 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.h +++ b/drivers/infiniband/ulp/srpt/ib_srpt.h @@ -105,11 +105,6 @@ enum { SRP_CMD_ACA = 0x4, SRPT_DEF_SG_TABLESIZE = 128, - /* - * An experimentally determined value that avoids that QP creation - * fails due to "swiotlb buffer is full" on systems using the swiotlb. - */ - SRPT_MAX_SG_PER_WQE = 16, MIN_SRPT_SQ_SIZE = 16, DEF_SRPT_SQ_SIZE = 4096,
The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do not use the value advertised by the driver (32) since that causes QP's to transition to the error status. See also commit f95ccffc715b ("IB/mlx4: Use 4K pages for kernel QP's WQE buffer"). Cc: Laurence Oberman <loberman@redhat.com> Cc: Kamal Heib <kamalheib1@gmail.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/infiniband/ulp/srpt/ib_srpt.c | 3 +-- drivers/infiniband/ulp/srpt/ib_srpt.h | 5 ----- 2 files changed, 1 insertion(+), 7 deletions(-)