| Message ID | 20181128183522.4599.52324.stgit@scvm10.sc.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IB/hfi1: Clean up and code improvements | expand |
On Wed, Nov 28, 2018 at 10:35:33AM -0800, Dennis Dalessandro wrote: > From: Michael J. Ruhl <michael.j.ruhl@intel.com> > > 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@intel.com> > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > --- > 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. > 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); >
>-----Original Message----- >From: Leon Romanovsky [mailto:leon@kernel.org] >Sent: Wednesday, November 28, 2018 1:58 PM >To: Dalessandro, Dennis <dennis.dalessandro@intel.com> >Cc: jgg@ziepe.ca; dledford@redhat.com; linux-rdma@vger.kernel.org; Ruhl, >Michael J <michael.j.ruhl@intel.com>; Marciniszyn, Mike ><mike.marciniszyn@intel.com> >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@intel.com> >> >> 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@intel.com> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> >> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> >> --- >> 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); >>
On 11/28/2018 2:02 PM, Ruhl, Michael J wrote: >> 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. We have a handful of BUG_ON calls in qib code. I'm ambivalent towards them. Don't think it's worth anyone's time to mess with since they've been there for 10+ years. On the other hand I would happily Ack a patch that removes them. -Denny
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); 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);