diff mbox series

[for-next,v5,5/6] RDMA/rxe: Lookup kernel AH from ah index in UD WQEs

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

Commit Message

Bob Pearson Oct. 6, 2021, 1:58 a.m. UTC
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(-)

Comments

Zhu Yanjun Oct. 6, 2021, 11:55 a.m. UTC | #1
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
>
Pearson, Robert B Oct. 6, 2021, 2:42 p.m. UTC | #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
>
Zhu Yanjun Oct. 7, 2021, 3:12 a.m. UTC | #3
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 mbox series

Patch

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;