On Tue, Jul 26, 2022 at 9:56 PM Bob Pearson <rpearsonhpe@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
>
