| 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 |
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 >
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
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 --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;
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(-)