| Message ID | 20211006015815.28350-6-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Replace AV by AH in UD sends | expand |
On Wed, Oct 6, 2021 at 9:58 AM Bob Pearson <rpearsonhpe@gmail.com> wrote: > > Add code to rxe_get_av in rxe_av.c to use the AH index in UD send WQEs > to lookup the kernel AH. For old user providers continue to use the AV > passed in WQEs. Move setting pkt->rxe to before the call to rxe_get_av() > to get access to the AH pool. > > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> > --- > drivers/infiniband/sw/rxe/rxe_av.c | 20 +++++++++++++++++++- > drivers/infiniband/sw/rxe/rxe_req.c | 8 +++++--- > 2 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_av.c b/drivers/infiniband/sw/rxe/rxe_av.c > index 85580ea5eed0..38c7b6fb39d7 100644 > --- a/drivers/infiniband/sw/rxe/rxe_av.c > +++ b/drivers/infiniband/sw/rxe/rxe_av.c > @@ -101,11 +101,29 @@ void rxe_av_fill_ip_info(struct rxe_av *av, struct rdma_ah_attr *attr) > > struct rxe_av *rxe_get_av(struct rxe_pkt_info *pkt) > { > + struct rxe_ah *ah; > + u32 ah_num; > + > if (!pkt || !pkt->qp) > return NULL; > > if (qp_type(pkt->qp) == IB_QPT_RC || qp_type(pkt->qp) == IB_QPT_UC) > return &pkt->qp->pri_av; > > - return (pkt->wqe) ? &pkt->wqe->wr.wr.ud.av : NULL; > + if (!pkt->wqe) > + return NULL; > + > + ah_num = pkt->wqe->wr.wr.ud.ah_num; > + if (ah_num) { > + /* only new user provider or kernel client */ struct rxe_ah *ah; ah is only used in this snippet. Is it better to move to here? It is only a trivial problem. Zhu Yanjun > + ah = rxe_pool_get_index(&pkt->rxe->ah_pool, ah_num); > + if (!ah || ah->ah_num != ah_num || rxe_ah_pd(ah) != pkt->qp->pd) { > + pr_warn("Unable to find AH matching ah_num\n"); > + return NULL; > + } > + return &ah->av; > + } > + > + /* only old user provider for UD sends*/ > + return &pkt->wqe->wr.wr.ud.av; > } > diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c > index fe275fcaffbd..0c9d2af15f3d 100644 > --- a/drivers/infiniband/sw/rxe/rxe_req.c > +++ b/drivers/infiniband/sw/rxe/rxe_req.c > @@ -379,9 +379,8 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp, > /* length from start of bth to end of icrc */ > paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE; > > - /* pkt->hdr, rxe, port_num and mask are initialized in ifc > - * layer > - */ > + /* pkt->hdr, port_num and mask are initialized in ifc layer */ > + pkt->rxe = rxe; > pkt->opcode = opcode; > pkt->qp = qp; > pkt->psn = qp->req.psn; > @@ -391,6 +390,9 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp, > > /* init skb */ > av = rxe_get_av(pkt); > + if (!av) > + return NULL; > + > skb = rxe_init_packet(rxe, av, paylen, pkt); > if (unlikely(!skb)) > return NULL; > -- > 2.30.2 >
Zhu, It's a matter of preference. I find that for me always putting all the local variables at the top of a subroutine saves time and reduces bugs. I know where to look. They're always there. And there are no tricky scope issues to think about. If you can't see them because they are off the screen the subroutine is probably too big. BTW do you have a new email address? I just saw one go by. Bob -----Original Message----- From: Zhu Yanjun <zyjzyj2000@gmail.com> Sent: Wednesday, October 6, 2021 6:56 AM To: Bob Pearson <rpearsonhpe@gmail.com> Cc: Jason Gunthorpe <jgg@nvidia.com>; RDMA mailing list <linux-rdma@vger.kernel.org> Subject: Re: [PATCH for-next v5 5/6] RDMA/rxe: Lookup kernel AH from ah index in UD WQEs On Wed, Oct 6, 2021 at 9:58 AM Bob Pearson <rpearsonhpe@gmail.com> wrote: > > Add code to rxe_get_av in rxe_av.c to use the AH index in UD send WQEs > to lookup the kernel AH. For old user providers continue to use the AV > passed in WQEs. Move setting pkt->rxe to before the call to > rxe_get_av() to get access to the AH pool. > > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> > --- > drivers/infiniband/sw/rxe/rxe_av.c | 20 +++++++++++++++++++- > drivers/infiniband/sw/rxe/rxe_req.c | 8 +++++--- > 2 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_av.c > b/drivers/infiniband/sw/rxe/rxe_av.c > index 85580ea5eed0..38c7b6fb39d7 100644 > --- a/drivers/infiniband/sw/rxe/rxe_av.c > +++ b/drivers/infiniband/sw/rxe/rxe_av.c > @@ -101,11 +101,29 @@ void rxe_av_fill_ip_info(struct rxe_av *av, > struct rdma_ah_attr *attr) > > struct rxe_av *rxe_get_av(struct rxe_pkt_info *pkt) { > + struct rxe_ah *ah; > + u32 ah_num; > + > if (!pkt || !pkt->qp) > return NULL; > > if (qp_type(pkt->qp) == IB_QPT_RC || qp_type(pkt->qp) == IB_QPT_UC) > return &pkt->qp->pri_av; > > - return (pkt->wqe) ? &pkt->wqe->wr.wr.ud.av : NULL; > + if (!pkt->wqe) > + return NULL; > + > + ah_num = pkt->wqe->wr.wr.ud.ah_num; > + if (ah_num) { > + /* only new user provider or kernel client */ struct rxe_ah *ah; ah is only used in this snippet. Is it better to move to here? It is only a trivial problem. Zhu Yanjun > + ah = rxe_pool_get_index(&pkt->rxe->ah_pool, ah_num); > + if (!ah || ah->ah_num != ah_num || rxe_ah_pd(ah) != pkt->qp->pd) { > + pr_warn("Unable to find AH matching ah_num\n"); > + return NULL; > + } > + return &ah->av; > + } > + > + /* only old user provider for UD sends*/ > + return &pkt->wqe->wr.wr.ud.av; > } > diff --git a/drivers/infiniband/sw/rxe/rxe_req.c > b/drivers/infiniband/sw/rxe/rxe_req.c > index fe275fcaffbd..0c9d2af15f3d 100644 > --- a/drivers/infiniband/sw/rxe/rxe_req.c > +++ b/drivers/infiniband/sw/rxe/rxe_req.c > @@ -379,9 +379,8 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp, > /* length from start of bth to end of icrc */ > paylen = rxe_opcode[opcode].length + payload + pad + > RXE_ICRC_SIZE; > > - /* pkt->hdr, rxe, port_num and mask are initialized in ifc > - * layer > - */ > + /* pkt->hdr, port_num and mask are initialized in ifc layer */ > + pkt->rxe = rxe; > pkt->opcode = opcode; > pkt->qp = qp; > pkt->psn = qp->req.psn; > @@ -391,6 +390,9 @@ static struct sk_buff *init_req_packet(struct > rxe_qp *qp, > > /* init skb */ > av = rxe_get_av(pkt); > + if (!av) > + return NULL; > + > skb = rxe_init_packet(rxe, av, paylen, pkt); > if (unlikely(!skb)) > return NULL; > -- > 2.30.2 >
On Wed, Oct 6, 2021 at 10:42 PM Pearson, Robert B <robert.pearson2@hpe.com> wrote: > > Zhu, > > It's a matter of preference. I find that for me always putting all the local variables at the top of a subroutine saves time and reduces bugs. I know where to look. They're always there. And there are no tricky scope issues to think about. If you can't see them because they are off the screen the subroutine is probably too big. > Yeah. It is a matter of preference. I like to put all the variables near where they are used. Do not worry. I am fine with your preference. Zhu Yanjun > BTW do you have a new email address? I just saw one go by. > > Bob > > -----Original Message----- > From: Zhu Yanjun <zyjzyj2000@gmail.com> > Sent: Wednesday, October 6, 2021 6:56 AM > To: Bob Pearson <rpearsonhpe@gmail.com> > Cc: Jason Gunthorpe <jgg@nvidia.com>; RDMA mailing list <linux-rdma@vger.kernel.org> > Subject: Re: [PATCH for-next v5 5/6] RDMA/rxe: Lookup kernel AH from ah index in UD WQEs > > On Wed, Oct 6, 2021 at 9:58 AM Bob Pearson <rpearsonhpe@gmail.com> wrote: > > > > Add code to rxe_get_av in rxe_av.c to use the AH index in UD send WQEs > > to lookup the kernel AH. For old user providers continue to use the AV > > passed in WQEs. Move setting pkt->rxe to before the call to > > rxe_get_av() to get access to the AH pool. > > > > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> > > --- > > drivers/infiniband/sw/rxe/rxe_av.c | 20 +++++++++++++++++++- > > drivers/infiniband/sw/rxe/rxe_req.c | 8 +++++--- > > 2 files changed, 24 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_av.c > > b/drivers/infiniband/sw/rxe/rxe_av.c > > index 85580ea5eed0..38c7b6fb39d7 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_av.c > > +++ b/drivers/infiniband/sw/rxe/rxe_av.c > > @@ -101,11 +101,29 @@ void rxe_av_fill_ip_info(struct rxe_av *av, > > struct rdma_ah_attr *attr) > > > > struct rxe_av *rxe_get_av(struct rxe_pkt_info *pkt) { > > + struct rxe_ah *ah; > > + u32 ah_num; > > + > > if (!pkt || !pkt->qp) > > return NULL; > > > > if (qp_type(pkt->qp) == IB_QPT_RC || qp_type(pkt->qp) == IB_QPT_UC) > > return &pkt->qp->pri_av; > > > > - return (pkt->wqe) ? &pkt->wqe->wr.wr.ud.av : NULL; > > + if (!pkt->wqe) > > + return NULL; > > + > > + ah_num = pkt->wqe->wr.wr.ud.ah_num; > > + if (ah_num) { > > + /* only new user provider or kernel client */ > > struct rxe_ah *ah; > ah is only used in this snippet. Is it better to move to here? > It is only a trivial problem. > > Zhu Yanjun > > + ah = rxe_pool_get_index(&pkt->rxe->ah_pool, ah_num); > > + if (!ah || ah->ah_num != ah_num || rxe_ah_pd(ah) != pkt->qp->pd) { > > + pr_warn("Unable to find AH matching ah_num\n"); > > + return NULL; > > + } > > + return &ah->av; > > + } > > + > > + /* only old user provider for UD sends*/ > > + return &pkt->wqe->wr.wr.ud.av; > > } > > diff --git a/drivers/infiniband/sw/rxe/rxe_req.c > > b/drivers/infiniband/sw/rxe/rxe_req.c > > index fe275fcaffbd..0c9d2af15f3d 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_req.c > > +++ b/drivers/infiniband/sw/rxe/rxe_req.c > > @@ -379,9 +379,8 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp, > > /* length from start of bth to end of icrc */ > > paylen = rxe_opcode[opcode].length + payload + pad + > > RXE_ICRC_SIZE; > > > > - /* pkt->hdr, rxe, port_num and mask are initialized in ifc > > - * layer > > - */ > > + /* pkt->hdr, port_num and mask are initialized in ifc layer */ > > + pkt->rxe = rxe; > > pkt->opcode = opcode; > > pkt->qp = qp; > > pkt->psn = qp->req.psn; > > @@ -391,6 +390,9 @@ static struct sk_buff *init_req_packet(struct > > rxe_qp *qp, > > > > /* init skb */ > > av = rxe_get_av(pkt); > > + if (!av) > > + return NULL; > > + > > skb = rxe_init_packet(rxe, av, paylen, pkt); > > if (unlikely(!skb)) > > return NULL; > > -- > > 2.30.2 > >
diff --git a/drivers/infiniband/sw/rxe/rxe_av.c b/drivers/infiniband/sw/rxe/rxe_av.c index 85580ea5eed0..38c7b6fb39d7 100644 --- a/drivers/infiniband/sw/rxe/rxe_av.c +++ b/drivers/infiniband/sw/rxe/rxe_av.c @@ -101,11 +101,29 @@ void rxe_av_fill_ip_info(struct rxe_av *av, struct rdma_ah_attr *attr) struct rxe_av *rxe_get_av(struct rxe_pkt_info *pkt) { + struct rxe_ah *ah; + u32 ah_num; + if (!pkt || !pkt->qp) return NULL; if (qp_type(pkt->qp) == IB_QPT_RC || qp_type(pkt->qp) == IB_QPT_UC) return &pkt->qp->pri_av; - return (pkt->wqe) ? &pkt->wqe->wr.wr.ud.av : NULL; + if (!pkt->wqe) + return NULL; + + ah_num = pkt->wqe->wr.wr.ud.ah_num; + if (ah_num) { + /* only new user provider or kernel client */ + ah = rxe_pool_get_index(&pkt->rxe->ah_pool, ah_num); + if (!ah || ah->ah_num != ah_num || rxe_ah_pd(ah) != pkt->qp->pd) { + pr_warn("Unable to find AH matching ah_num\n"); + return NULL; + } + return &ah->av; + } + + /* only old user provider for UD sends*/ + return &pkt->wqe->wr.wr.ud.av; } diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c index fe275fcaffbd..0c9d2af15f3d 100644 --- a/drivers/infiniband/sw/rxe/rxe_req.c +++ b/drivers/infiniband/sw/rxe/rxe_req.c @@ -379,9 +379,8 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp, /* length from start of bth to end of icrc */ paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE; - /* pkt->hdr, rxe, port_num and mask are initialized in ifc - * layer - */ + /* pkt->hdr, port_num and mask are initialized in ifc layer */ + pkt->rxe = rxe; pkt->opcode = opcode; pkt->qp = qp; pkt->psn = qp->req.psn; @@ -391,6 +390,9 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp, /* init skb */ av = rxe_get_av(pkt); + if (!av) + return NULL; + skb = rxe_init_packet(rxe, av, paylen, pkt); if (unlikely(!skb)) return NULL;
Add code to rxe_get_av in rxe_av.c to use the AH index in UD send WQEs to lookup the kernel AH. For old user providers continue to use the AV passed in WQEs. Move setting pkt->rxe to before the call to rxe_get_av() to get access to the AH pool. Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> --- drivers/infiniband/sw/rxe/rxe_av.c | 20 +++++++++++++++++++- drivers/infiniband/sw/rxe/rxe_req.c | 8 +++++--- 2 files changed, 24 insertions(+), 4 deletions(-)