>-----Original Message----- >From: Leon Romanovsky [mailto:leon@xxxxxxxxxx] >Sent: Wednesday, November 28, 2018 1:58 PM >To: Dalessandro, Dennis <dennis.dalessandro@xxxxxxxxx> >Cc: jgg@xxxxxxxx; dledford@xxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; Ruhl, >Michael J <michael.j.ruhl@xxxxxxxxx>; Marciniszyn, Mike ><mike.marciniszyn@xxxxxxxxx> >Subject: Re: [PATCH for-next 1/4] IB/{hfi1,qib}: Cleanup open coded sge sizing > >On Wed, Nov 28, 2018 at 10:35:33AM -0800, Dennis Dalessandro wrote: >> From: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx> >> >> Sge sizing is done in several places using an open coded method. >> >> This can cause maintenance issues. The open coded method is >> encapsulated in a helper routine. The helper was introduced with >> commit: >> >> 1198fcea8a78 ("IB/hfi1, rdmavt: Move SGE state helper routines into >> rdmavt") >> >> Update all call sites that have the open coded path with the helper >> routine. >> >> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx> >> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx> >> --- >> drivers/infiniband/hw/hfi1/verbs.c | 12 ++---------- >> drivers/infiniband/hw/qib/qib_ud.c | 6 +----- >> drivers/infiniband/hw/qib/qib_verbs.c | 18 +++--------------- >> 3 files changed, 6 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/infiniband/hw/hfi1/verbs.c >b/drivers/infiniband/hw/hfi1/verbs.c >> index 630f38e..55ec4db 100644 >> --- a/drivers/infiniband/hw/hfi1/verbs.c >> +++ b/drivers/infiniband/hw/hfi1/verbs.c >> @@ -553,11 +553,7 @@ static noinline int build_verbs_ulp_payload( >> int ret = 0; >> >> while (length) { >> - len = ss->sge.length; >> - if (len > length) >> - len = length; >> - if (len > ss->sge.sge_length) >> - len = ss->sge.sge_length; >> + len = rvt_get_sge_length(&ss->sge, length); >> WARN_ON_ONCE(len == 0); >> ret = sdma_txadd_kvaddr( >> sde->dd, >> @@ -914,12 +910,8 @@ int hfi1_verbs_send_pio(struct rvt_qp *qp, struct >hfi1_pkt_state *ps, >> if (ss) { >> while (len) { >> void *addr = ss->sge.vaddr; >> - u32 slen = ss->sge.length; >> + u32 slen = rvt_get_sge_length(&ss->sge, len); >> >> - if (slen > len) >> - slen = len; >> - if (slen > ss->sge.sge_length) >> - slen = ss->sge.sge_length; >> rvt_update_sge(ss, slen, false); >> seg_pio_copy_mid(pbuf, addr, slen); >> len -= slen; >> diff --git a/drivers/infiniband/hw/qib/qib_ud.c >b/drivers/infiniband/hw/qib/qib_ud.c >> index 4d4c31e..8dab667 100644 >> --- a/drivers/infiniband/hw/qib/qib_ud.c >> +++ b/drivers/infiniband/hw/qib/qib_ud.c >> @@ -172,12 +172,8 @@ static void qib_ud_loopback(struct rvt_qp *sqp, struct >rvt_swqe *swqe) >> ssge.num_sge = swqe->wr.num_sge; >> sge = &ssge.sge; >> while (length) { >> - u32 len = sge->length; >> + u32 len = rvt_get_sge_length(sge, length); >> >> - if (len > length) >> - len = length; >> - if (len > sge->sge_length) >> - len = sge->sge_length; >> BUG_ON(len == 0); > > ^^^^^^, above you are using WARN_ON_ONCE and below BUG_ON >I assume that everything can be safely removed. I was focused mainly on patch footprint rather than other issues and have not focused on the need for the BUG_ON(). I will add this to my list to follow up on this with a different patch. Mike >> rvt_copy_sge(qp, &qp->r_sge, sge->vaddr, len, true, false); >> sge->vaddr += len; >> diff --git a/drivers/infiniband/hw/qib/qib_verbs.c >b/drivers/infiniband/hw/qib/qib_verbs.c >> index 4b0f576..9cbc644 100644 >> --- a/drivers/infiniband/hw/qib/qib_verbs.c >> +++ b/drivers/infiniband/hw/qib/qib_verbs.c >> @@ -144,12 +144,8 @@ static u32 qib_count_sge(struct rvt_sge_state *ss, u32 >length) >> u32 ndesc = 1; /* count the header */ >> >> while (length) { >> - u32 len = sge.length; >> + u32 len = rvt_get_sge_length(&sge, length); >> >> - if (len > length) >> - len = length; >> - if (len > sge.sge_length) >> - len = sge.sge_length; >> BUG_ON(len == 0); >> if (((long) sge.vaddr & (sizeof(u32) - 1)) || >> (len != length && (len & (sizeof(u32) - 1)))) { >> @@ -187,12 +183,8 @@ static void qib_copy_from_sge(void *data, struct >rvt_sge_state *ss, u32 length) >> struct rvt_sge *sge = &ss->sge; >> >> while (length) { >> - u32 len = sge->length; >> + u32 len = rvt_get_sge_length(sge, length); >> >> - if (len > length) >> - len = length; >> - if (len > sge->sge_length) >> - len = sge->sge_length; >> BUG_ON(len == 0); >> memcpy(data, sge->vaddr, len); >> sge->vaddr += len; >> @@ -442,13 +434,9 @@ static void copy_io(u32 __iomem *piobuf, struct >rvt_sge_state *ss, >> u32 last; >> >> while (1) { >> - u32 len = ss->sge.length; >> + u32 len = rvt_get_sge_length(&ss->sge, length); >> u32 off; >> >> - if (len > length) >> - len = length; >> - if (len > ss->sge.sge_length) >> - len = ss->sge.sge_length; >> BUG_ON(len == 0); >> /* If the source address is not aligned, try to align it. */ >> off = (unsigned long)ss->sge.vaddr & (sizeof(u32) - 1); >>
