diff mbox series

[for-next] RDMA/rxe: Cleanup rxe_init_packet()

Message ID 20220726195602.12368-1-rpearsonhpe@gmail.com (mailing list archive)
State Superseded
Headers show
Series [for-next] RDMA/rxe: Cleanup rxe_init_packet() | expand

Commit Message

Bob Pearson July 26, 2022, 7:56 p.m. UTC
Currently rxe_init_packet() recomputes ndev from the address
vector but struct rxe_dev already holds a reference to ndev.

Cleanup rxe_init_packet to use the value of ndev in rxe and drop
an unneeded parameter paylen since it is already carried in the
packet info struct.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_loc.h  |  2 +-
 drivers/infiniband/sw/rxe/rxe_net.c  | 49 +++++++++++-----------------
 drivers/infiniband/sw/rxe/rxe_req.c  |  2 +-
 drivers/infiniband/sw/rxe/rxe_resp.c |  3 +-
 4 files changed, 23 insertions(+), 33 deletions(-)

Comments

Haris Iqbal July 27, 2022, 10:23 a.m. UTC | #1
On Tue, Jul 26, 2022 at 9:56 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> Currently rxe_init_packet() recomputes ndev from the address
> vector but struct rxe_dev already holds a reference to ndev.
>
> Cleanup rxe_init_packet to use the value of ndev in rxe and drop
> an unneeded parameter paylen since it is already carried in the
> packet info struct.
>
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_loc.h  |  2 +-
>  drivers/infiniband/sw/rxe/rxe_net.c  | 49 +++++++++++-----------------
>  drivers/infiniband/sw/rxe/rxe_req.c  |  2 +-
>  drivers/infiniband/sw/rxe/rxe_resp.c |  3 +-
>  4 files changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index f9af30f582c6..7f98d296bc0d 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -93,7 +93,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
>
>  /* rxe_net.c */
>  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> -                               int paylen, struct rxe_pkt_info *pkt);
> +                               struct rxe_pkt_info *pkt);
>  int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
>                 struct sk_buff *skb);
>  int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index c53f4529f098..4a091f236dde 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -443,18 +443,22 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>         return err;
>  }
>
> +/**
> + * rxe_init_packet - allocate and initialize a new skb
> + * @rxe: rxe device
> + * @av: remote address vector
> + * @pkt: packet info
> + *
> + * Returns: an skb on success else NULL
> + */
>  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> -                               int paylen, struct rxe_pkt_info *pkt)
> +                               struct rxe_pkt_info *pkt)
>  {
>         unsigned int hdr_len;
>         struct sk_buff *skb = NULL;
> -       struct net_device *ndev;
> -       const struct ib_gid_attr *attr;
> +       struct net_device *ndev = rxe->ndev;
>         const int port_num = 1;
> -
> -       attr = rdma_get_gid_attr(&rxe->ib_dev, port_num, av->grh.sgid_index);
> -       if (IS_ERR(attr))
> -               return NULL;
> +       int skb_size;
>
>         if (av->network_type == RXE_NETWORK_TYPE_IPV4)
>                 hdr_len = ETH_HLEN + sizeof(struct udphdr) +
> @@ -463,26 +467,13 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>                 hdr_len = ETH_HLEN + sizeof(struct udphdr) +
>                         sizeof(struct ipv6hdr);
>
> -       rcu_read_lock();

Was removing this rcu lock intentional? If so, do we need a mention of
this in the commit message why its not needed anymore?

> -       ndev = rdma_read_gid_attr_ndev_rcu(attr);
> -       if (IS_ERR(ndev)) {
> -               rcu_read_unlock();
> -               goto out;
> -       }
> -       skb = alloc_skb(paylen + hdr_len + LL_RESERVED_SPACE(ndev),
> -                       GFP_ATOMIC);
> -
> -       if (unlikely(!skb)) {
> -               rcu_read_unlock();
> -               goto out;
> -       }
> -
> -       skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
> -
> -       /* FIXME: hold reference to this netdev until life of this skb. */
> -       skb->dev        = ndev;
> -       rcu_read_unlock();
> +       skb_size = LL_RESERVED_SPACE(ndev) + hdr_len + pkt->paylen;
> +       skb = alloc_skb(skb_size, GFP_ATOMIC);
> +       if (unlikely(!skb))
> +               return NULL;
>
> +       skb_reserve(skb, LL_RESERVED_SPACE(ndev) + hdr_len);
> +       skb->dev = ndev;
>         if (av->network_type == RXE_NETWORK_TYPE_IPV4)
>                 skb->protocol = htons(ETH_P_IP);
>         else
> @@ -490,11 +481,9 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>
>         pkt->rxe        = rxe;
>         pkt->port_num   = port_num;
> -       pkt->hdr        = skb_put(skb, paylen);
> -       pkt->mask       |= RXE_GRH_MASK;
> +       pkt->hdr        = skb_put(skb, pkt->paylen);
> +       pkt->mask      |= RXE_GRH_MASK;
>
> -out:
> -       rdma_put_gid_attr(attr);
>         return skb;
>  }
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 49e8f54db6f5..e72db960d7ea 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -397,7 +397,7 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp,
>         pkt->paylen = paylen;
>
>         /* init skb */
> -       skb = rxe_init_packet(rxe, av, paylen, pkt);
> +       skb = rxe_init_packet(rxe, av, pkt);
>         if (unlikely(!skb))
>                 return NULL;
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index b36ec5c4d5e0..02a5d4ebb45e 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -670,8 +670,9 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
>          */
>         pad = (-payload) & 0x3;
>         paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE;
> +       ack->paylen = paylen;

Maybe remove the old ack->paylen assignment which is done later in
this function?

>
> -       skb = rxe_init_packet(rxe, &qp->pri_av, paylen, ack);
> +       skb = rxe_init_packet(rxe, &qp->pri_av, ack);
>         if (!skb)
>                 return NULL;
>
> --
> 2.34.1
>
Bob Pearson July 27, 2022, 4:01 p.m. UTC | #2
On 7/27/22 05:23, Haris Iqbal wrote:
> On Tue, Jul 26, 2022 at 9:56 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>
>> Currently rxe_init_packet() recomputes ndev from the address
>> vector but struct rxe_dev already holds a reference to ndev.
>>
>> Cleanup rxe_init_packet to use the value of ndev in rxe and drop
>> an unneeded parameter paylen since it is already carried in the
>> packet info struct.
>>
>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>> ---
>>  drivers/infiniband/sw/rxe/rxe_loc.h  |  2 +-
>>  drivers/infiniband/sw/rxe/rxe_net.c  | 49 +++++++++++-----------------
>>  drivers/infiniband/sw/rxe/rxe_req.c  |  2 +-
>>  drivers/infiniband/sw/rxe/rxe_resp.c |  3 +-
>>  4 files changed, 23 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
>> index f9af30f582c6..7f98d296bc0d 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
>> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
>> @@ -93,7 +93,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
>>
>>  /* rxe_net.c */
>>  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>> -                               int paylen, struct rxe_pkt_info *pkt);
>> +                               struct rxe_pkt_info *pkt);
>>  int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
>>                 struct sk_buff *skb);
>>  int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
>> index c53f4529f098..4a091f236dde 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_net.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
>> @@ -443,18 +443,22 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>>         return err;
>>  }
>>
>> +/**
>> + * rxe_init_packet - allocate and initialize a new skb
>> + * @rxe: rxe device
>> + * @av: remote address vector
>> + * @pkt: packet info
>> + *
>> + * Returns: an skb on success else NULL
>> + */
>>  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>> -                               int paylen, struct rxe_pkt_info *pkt)
>> +                               struct rxe_pkt_info *pkt)
>>  {
>>         unsigned int hdr_len;
>>         struct sk_buff *skb = NULL;
>> -       struct net_device *ndev;
>> -       const struct ib_gid_attr *attr;
>> +       struct net_device *ndev = rxe->ndev;
>>         const int port_num = 1;
>> -
>> -       attr = rdma_get_gid_attr(&rxe->ib_dev, port_num, av->grh.sgid_index);
>> -       if (IS_ERR(attr))
>> -               return NULL;
>> +       int skb_size;
>>
>>         if (av->network_type == RXE_NETWORK_TYPE_IPV4)
>>                 hdr_len = ETH_HLEN + sizeof(struct udphdr) +
>> @@ -463,26 +467,13 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>>                 hdr_len = ETH_HLEN + sizeof(struct udphdr) +
>>                         sizeof(struct ipv6hdr);
>>
>> -       rcu_read_lock();
> 
> Was removing this rcu lock intentional? If so, do we need a mention of
> this in the commit message why its not needed anymore?
> 
The rcu lock was required to use the rdma_read_gid_attr_ndev_rcu() API.
For the rxe driver there is no way that the ndev is changing once the rxe device
is set up and ndev was passed in to rxe_newlink() and saved in the rxe_dev struct.
Not any good reason to do all this work to get ndev when we already know it.
>> -       ndev = rdma_read_gid_attr_ndev_rcu(attr);
>> -       if (IS_ERR(ndev)) {
>> -               rcu_read_unlock();
>> -               goto out;
>> -       }
>> -       skb = alloc_skb(paylen + hdr_len + LL_RESERVED_SPACE(ndev),
>> -                       GFP_ATOMIC);
>> -
>> -       if (unlikely(!skb)) {
>> -               rcu_read_unlock();
>> -               goto out;
>> -       }
>> -
>> -       skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
>> -
>> -       /* FIXME: hold reference to this netdev until life of this skb. */
>> -       skb->dev        = ndev;
>> -       rcu_read_unlock();
>> +       skb_size = LL_RESERVED_SPACE(ndev) + hdr_len + pkt->paylen;
>> +       skb = alloc_skb(skb_size, GFP_ATOMIC);
>> +       if (unlikely(!skb))
>> +               return NULL;
>>
>> +       skb_reserve(skb, LL_RESERVED_SPACE(ndev) + hdr_len);
>> +       skb->dev = ndev;
>>         if (av->network_type == RXE_NETWORK_TYPE_IPV4)
>>                 skb->protocol = htons(ETH_P_IP);
>>         else
>> @@ -490,11 +481,9 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>>
>>         pkt->rxe        = rxe;
>>         pkt->port_num   = port_num;
>> -       pkt->hdr        = skb_put(skb, paylen);
>> -       pkt->mask       |= RXE_GRH_MASK;
>> +       pkt->hdr        = skb_put(skb, pkt->paylen);
>> +       pkt->mask      |= RXE_GRH_MASK;
>>
>> -out:
>> -       rdma_put_gid_attr(attr);
>>         return skb;
>>  }
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
>> index 49e8f54db6f5..e72db960d7ea 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_req.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
>> @@ -397,7 +397,7 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp,
>>         pkt->paylen = paylen;
>>
>>         /* init skb */
>> -       skb = rxe_init_packet(rxe, av, paylen, pkt);
>> +       skb = rxe_init_packet(rxe, av, pkt);
>>         if (unlikely(!skb))
>>                 return NULL;
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index b36ec5c4d5e0..02a5d4ebb45e 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -670,8 +670,9 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
>>          */
>>         pad = (-payload) & 0x3;
>>         paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE;
>> +       ack->paylen = paylen;
> 
> Maybe remove the old ack->paylen assignment which is done later in
> this function?
> 
Agreed. Will send a v2 with this change.
>>
>> -       skb = rxe_init_packet(rxe, &qp->pri_av, paylen, ack);
>> +       skb = rxe_init_packet(rxe, &qp->pri_av, ack);
>>         if (!skb)
>>                 return NULL;
>>
>> --
>> 2.34.1
>>

Bob
Haris Iqbal July 27, 2022, 4:34 p.m. UTC | #3
On Wed, Jul 27, 2022 at 6:01 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 7/27/22 05:23, Haris Iqbal wrote:
> > On Tue, Jul 26, 2022 at 9:56 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >>
> >> Currently rxe_init_packet() recomputes ndev from the address
> >> vector but struct rxe_dev already holds a reference to ndev.
> >>
> >> Cleanup rxe_init_packet to use the value of ndev in rxe and drop
> >> an unneeded parameter paylen since it is already carried in the
> >> packet info struct.
> >>
> >> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> >> ---
> >>  drivers/infiniband/sw/rxe/rxe_loc.h  |  2 +-
> >>  drivers/infiniband/sw/rxe/rxe_net.c  | 49 +++++++++++-----------------
> >>  drivers/infiniband/sw/rxe/rxe_req.c  |  2 +-
> >>  drivers/infiniband/sw/rxe/rxe_resp.c |  3 +-
> >>  4 files changed, 23 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> >> index f9af30f582c6..7f98d296bc0d 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> >> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> >> @@ -93,7 +93,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
> >>
> >>  /* rxe_net.c */
> >>  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >> -                               int paylen, struct rxe_pkt_info *pkt);
> >> +                               struct rxe_pkt_info *pkt);
> >>  int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
> >>                 struct sk_buff *skb);
> >>  int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> >> index c53f4529f098..4a091f236dde 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> >> @@ -443,18 +443,22 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> >>         return err;
> >>  }
> >>
> >> +/**
> >> + * rxe_init_packet - allocate and initialize a new skb
> >> + * @rxe: rxe device
> >> + * @av: remote address vector
> >> + * @pkt: packet info
> >> + *
> >> + * Returns: an skb on success else NULL
> >> + */
> >>  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >> -                               int paylen, struct rxe_pkt_info *pkt)
> >> +                               struct rxe_pkt_info *pkt)
> >>  {
> >>         unsigned int hdr_len;
> >>         struct sk_buff *skb = NULL;
> >> -       struct net_device *ndev;
> >> -       const struct ib_gid_attr *attr;
> >> +       struct net_device *ndev = rxe->ndev;
> >>         const int port_num = 1;
> >> -
> >> -       attr = rdma_get_gid_attr(&rxe->ib_dev, port_num, av->grh.sgid_index);
> >> -       if (IS_ERR(attr))
> >> -               return NULL;
> >> +       int skb_size;
> >>
> >>         if (av->network_type == RXE_NETWORK_TYPE_IPV4)
> >>                 hdr_len = ETH_HLEN + sizeof(struct udphdr) +
> >> @@ -463,26 +467,13 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >>                 hdr_len = ETH_HLEN + sizeof(struct udphdr) +
> >>                         sizeof(struct ipv6hdr);
> >>
> >> -       rcu_read_lock();
> >
> > Was removing this rcu lock intentional? If so, do we need a mention of
> > this in the commit message why its not needed anymore?
> >
> The rcu lock was required to use the rdma_read_gid_attr_ndev_rcu() API.
> For the rxe driver there is no way that the ndev is changing once the rxe device
> is set up and ndev was passed in to rxe_newlink() and saved in the rxe_dev struct.
> Not any good reason to do all this work to get ndev when we already know it.

I see. Thanks for the explanation.

One more question for my understanding.
There was a FIXME line (removed with this commit) mentioning that we
should hold a reference to the netdev. I assume it was talking about
dev_hold.
Now, while initing the rxe we call ib_device_set_netdev, which does
dev_hold. But I could not find rxe doing dev_hold anywhere directly. I
am assuming this is enough, or should rxe be also holding a reference
for this net device since technically it does use it directly at some
places?


> >> -       ndev = rdma_read_gid_attr_ndev_rcu(attr);
> >> -       if (IS_ERR(ndev)) {
> >> -               rcu_read_unlock();
> >> -               goto out;
> >> -       }
> >> -       skb = alloc_skb(paylen + hdr_len + LL_RESERVED_SPACE(ndev),
> >> -                       GFP_ATOMIC);
> >> -
> >> -       if (unlikely(!skb)) {
> >> -               rcu_read_unlock();
> >> -               goto out;
> >> -       }
> >> -
> >> -       skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
> >> -
> >> -       /* FIXME: hold reference to this netdev until life of this skb. */
> >> -       skb->dev        = ndev;
> >> -       rcu_read_unlock();
> >> +       skb_size = LL_RESERVED_SPACE(ndev) + hdr_len + pkt->paylen;
> >> +       skb = alloc_skb(skb_size, GFP_ATOMIC);
> >> +       if (unlikely(!skb))
> >> +               return NULL;
> >>
> >> +       skb_reserve(skb, LL_RESERVED_SPACE(ndev) + hdr_len);
> >> +       skb->dev = ndev;
> >>         if (av->network_type == RXE_NETWORK_TYPE_IPV4)
> >>                 skb->protocol = htons(ETH_P_IP);
> >>         else
> >> @@ -490,11 +481,9 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >>
> >>         pkt->rxe        = rxe;
> >>         pkt->port_num   = port_num;
> >> -       pkt->hdr        = skb_put(skb, paylen);
> >> -       pkt->mask       |= RXE_GRH_MASK;
> >> +       pkt->hdr        = skb_put(skb, pkt->paylen);
> >> +       pkt->mask      |= RXE_GRH_MASK;
> >>
> >> -out:
> >> -       rdma_put_gid_attr(attr);
> >>         return skb;
> >>  }
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> >> index 49e8f54db6f5..e72db960d7ea 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> >> @@ -397,7 +397,7 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp,
> >>         pkt->paylen = paylen;
> >>
> >>         /* init skb */
> >> -       skb = rxe_init_packet(rxe, av, paylen, pkt);
> >> +       skb = rxe_init_packet(rxe, av, pkt);
> >>         if (unlikely(!skb))
> >>                 return NULL;
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> >> index b36ec5c4d5e0..02a5d4ebb45e 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> >> @@ -670,8 +670,9 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
> >>          */
> >>         pad = (-payload) & 0x3;
> >>         paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE;
> >> +       ack->paylen = paylen;
> >
> > Maybe remove the old ack->paylen assignment which is done later in
> > this function?
> >
> Agreed. Will send a v2 with this change.
> >>
> >> -       skb = rxe_init_packet(rxe, &qp->pri_av, paylen, ack);
> >> +       skb = rxe_init_packet(rxe, &qp->pri_av, ack);
> >>         if (!skb)
> >>                 return NULL;
> >>
> >> --
> >> 2.34.1
> >>
>
> Bob
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index f9af30f582c6..7f98d296bc0d 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -93,7 +93,7 @@  void rxe_mw_cleanup(struct rxe_pool_elem *elem);
 
 /* rxe_net.c */
 struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
-				int paylen, struct rxe_pkt_info *pkt);
+				struct rxe_pkt_info *pkt);
 int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
 		struct sk_buff *skb);
 int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index c53f4529f098..4a091f236dde 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -443,18 +443,22 @@  int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
 	return err;
 }
 
+/**
+ * rxe_init_packet - allocate and initialize a new skb
+ * @rxe: rxe device
+ * @av: remote address vector
+ * @pkt: packet info
+ *
+ * Returns: an skb on success else NULL
+ */
 struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
-				int paylen, struct rxe_pkt_info *pkt)
+				struct rxe_pkt_info *pkt)
 {
 	unsigned int hdr_len;
 	struct sk_buff *skb = NULL;
-	struct net_device *ndev;
-	const struct ib_gid_attr *attr;
+	struct net_device *ndev = rxe->ndev;
 	const int port_num = 1;
-
-	attr = rdma_get_gid_attr(&rxe->ib_dev, port_num, av->grh.sgid_index);
-	if (IS_ERR(attr))
-		return NULL;
+	int skb_size;
 
 	if (av->network_type == RXE_NETWORK_TYPE_IPV4)
 		hdr_len = ETH_HLEN + sizeof(struct udphdr) +
@@ -463,26 +467,13 @@  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
 		hdr_len = ETH_HLEN + sizeof(struct udphdr) +
 			sizeof(struct ipv6hdr);
 
-	rcu_read_lock();
-	ndev = rdma_read_gid_attr_ndev_rcu(attr);
-	if (IS_ERR(ndev)) {
-		rcu_read_unlock();
-		goto out;
-	}
-	skb = alloc_skb(paylen + hdr_len + LL_RESERVED_SPACE(ndev),
-			GFP_ATOMIC);
-
-	if (unlikely(!skb)) {
-		rcu_read_unlock();
-		goto out;
-	}
-
-	skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
-
-	/* FIXME: hold reference to this netdev until life of this skb. */
-	skb->dev	= ndev;
-	rcu_read_unlock();
+	skb_size = LL_RESERVED_SPACE(ndev) + hdr_len + pkt->paylen;
+	skb = alloc_skb(skb_size, GFP_ATOMIC);
+	if (unlikely(!skb))
+		return NULL;
 
+	skb_reserve(skb, LL_RESERVED_SPACE(ndev) + hdr_len);
+	skb->dev = ndev;
 	if (av->network_type == RXE_NETWORK_TYPE_IPV4)
 		skb->protocol = htons(ETH_P_IP);
 	else
@@ -490,11 +481,9 @@  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
 
 	pkt->rxe	= rxe;
 	pkt->port_num	= port_num;
-	pkt->hdr	= skb_put(skb, paylen);
-	pkt->mask	|= RXE_GRH_MASK;
+	pkt->hdr	= skb_put(skb, pkt->paylen);
+	pkt->mask      |= RXE_GRH_MASK;
 
-out:
-	rdma_put_gid_attr(attr);
 	return skb;
 }
 
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 49e8f54db6f5..e72db960d7ea 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -397,7 +397,7 @@  static struct sk_buff *init_req_packet(struct rxe_qp *qp,
 	pkt->paylen = paylen;
 
 	/* init skb */
-	skb = rxe_init_packet(rxe, av, paylen, pkt);
+	skb = rxe_init_packet(rxe, av, pkt);
 	if (unlikely(!skb))
 		return NULL;
 
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index b36ec5c4d5e0..02a5d4ebb45e 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -670,8 +670,9 @@  static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
 	 */
 	pad = (-payload) & 0x3;
 	paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE;
+	ack->paylen = paylen;
 
-	skb = rxe_init_packet(rxe, &qp->pri_av, paylen, ack);
+	skb = rxe_init_packet(rxe, &qp->pri_av, ack);
 	if (!skb)
 		return NULL;