Re: [PATCH for-next v3 05/10] RDMA/rxe: Extend rxe_icrc.c to support frags

[Zhu Yanjun <yanjun.zhu@xxxxxxxxx>]

在 2023/7/28 22:49, Bob Pearson 写道:
> On 7/28/23 09:20, Zhu Yanjun wrote:
> > 在 2023/7/28 4:01, Bob Pearson 写道:
> > > Extend the subroutines rxe_icrc_generate() and rxe_icrc_check()
> > > to support skb frags.
> > >
> > > Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
> > > ---
> > >    drivers/infiniband/sw/rxe/rxe_icrc.c | 65 ++++++++++++++++++++++++----
> > >    drivers/infiniband/sw/rxe/rxe_net.c  | 51 +++++++++++++++++-----
> > >    drivers/infiniband/sw/rxe/rxe_recv.c |  1 +
> > >    3 files changed, 98 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
> > > index c9aa0995e900..393391863350 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_icrc.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
> > > @@ -63,7 +63,7 @@ static __be32 rxe_crc32(struct rxe_dev *rxe, __be32 crc, void *next, size_t len)
> > >      /**
> > >     * rxe_icrc_hdr() - Compute the partial ICRC for the network and transport
> > > - *          headers of a packet.
> > > + *            headers of a packet.
> > >     * @skb: packet buffer
> > >     * @pkt: packet information
> > >     *
> > > @@ -129,6 +129,56 @@ static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> > >        return crc;
> > >    }
> > >    +/**
> > > + * rxe_icrc_payload() - Compute the ICRC for a packet payload and also
> > > + *            compute the address of the icrc in the packet.
> > > + * @skb: packet buffer
> > > + * @pkt: packet information
> > > + * @icrc: current icrc i.e. including headers
> > > + * @icrcp: returned pointer to icrc in skb
> > > + *
> > > + * Return: 0 if the values match else an error
> > > + */
> > > +static __be32 rxe_icrc_payload(struct sk_buff *skb, struct rxe_pkt_info *pkt,
> > > +                   __be32 icrc, __be32 **icrcp)
> > > +{
> > > +    struct skb_shared_info *shinfo = skb_shinfo(skb);
> > > +    skb_frag_t *frag;
> > > +    u8 *addr;
> > > +    int hdr_len;
> > > +    int len;
> > > +    int i;
> > > +
> > > +    /* handle any payload left in the linear buffer */
> > > +    hdr_len = rxe_opcode[pkt->opcode].length;
> > > +    addr = pkt->hdr + hdr_len;
> > > +    len = skb_tail_pointer(skb) - skb_transport_header(skb)
> > > +        - sizeof(struct udphdr) - hdr_len;
> > > +    if (!shinfo->nr_frags) {
> > > +        len -= RXE_ICRC_SIZE;
> > > +        *icrcp = (__be32 *)(addr + len);
> > > +    }
> > > +    if (len > 0)
> > > +        icrc = rxe_crc32(pkt->rxe, icrc, payload_addr(pkt), len);
> > > +    WARN_ON(len < 0);
> > > +
> > > +    /* handle any payload in frags */
> > > +    for (i = 0; i < shinfo->nr_frags; i++) {
> > > +        frag = &shinfo->frags[i];
> > > +        addr = page_to_virt(frag->bv_page) + frag->bv_offset;
> > > +        len = frag->bv_len;
> > > +        if (i == shinfo->nr_frags - 1) {
> > > +            len -= RXE_ICRC_SIZE;
> > > +            *icrcp = (__be32 *)(addr + len);
> > > +        }
> > > +        if (len > 0)
> > > +            icrc = rxe_crc32(pkt->rxe, icrc, addr, len);
> > > +        WARN_ON(len < 0);
> > > +    }
> > > +
> > > +    return icrc;
> > > +}
> > > +
> > >    /**
> > >     * rxe_icrc_check() - Compute ICRC for a packet and compare to the ICRC
> > >     *              delivered in the packet.
> > > @@ -143,13 +193,11 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> > >        __be32 pkt_icrc;
> > >        __be32 icrc;
> > >    -    icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> > > -    pkt_icrc = *icrcp;
> > > -
> > >        icrc = rxe_icrc_hdr(skb, pkt);
> > > -    icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> > > -                payload_size(pkt) + pkt->pad);
> > > +    icrc = rxe_icrc_payload(skb, pkt, icrc, &icrcp);
> > > +
> > >        icrc = ~icrc;
> > > +    pkt_icrc = *icrcp;
> > >          if (unlikely(icrc != pkt_icrc))
> > >            return -EINVAL;
> > > @@ -167,9 +215,8 @@ void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> > >        __be32 *icrcp;
> > >        __be32 icrc;
> > >    -    icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> > >        icrc = rxe_icrc_hdr(skb, pkt);
> > > -    icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> > > -                payload_size(pkt) + pkt->pad);
> > > +    icrc = rxe_icrc_payload(skb, pkt, icrc, &icrcp);
> > > +
> > >        *icrcp = ~icrc;
> > >    }
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> > > index c44ef39010f1..c43f9dd3ae6e 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_net.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> > > @@ -148,33 +148,53 @@ static int rxe_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > >        struct udphdr *udph;
> > >        struct rxe_dev *rxe;
> > >        struct rxe_pkt_info *pkt = SKB_TO_PKT(skb);
> > > +    u8 opcode;
> > > +    u8 buf[1];
> > > +    u8 *p;
> > opcode and *p duplicate.
> > You can use only one variable.
> opcode is u8 p is *u8.


I mean that you can use one variable, (for example,opcode) can complete 
the same functionality with the 2 variables (p and opcode).

Zhu Yanjun


> > u8 *opcode;
> > >          /* takes a reference on rxe->ib_dev
> > >         * drop when skb is freed
> > >         */
> > >        rxe = get_rxe_from_skb(skb);
> > >        if (!rxe)
> > > -        goto drop;
> > > +        goto err_drop;
> > >    -    if (skb_linearize(skb)) {
> > > -        ib_device_put(&rxe->ib_dev);
> > > -        goto drop;
> > > +    /* Get bth opcode out of skb, it may be in a fragment */
> > > +    p = skb_header_pointer(skb, sizeof(struct udphdr), 1, buf);
> > > +    if (!p)
> > > +        goto err_device_put;
> > > +    opcode = *p;
> >
> >      opcode = skb_header_pointer(skb, sizeof(struct udphdr), 1, buf);
> >      if (!opcode)
> >          goto err_device_put;
> > ;
> > > +
> > > +    /* If using fragmented skbs make sure roce headers
> > > +     * are in linear buffer else make skb linear
> > > +     */
> > > +    if (rxe_use_sg && skb_is_nonlinear(skb)) {
> > > +        int delta = rxe_opcode[opcode].length -
> >          int delta = rxe_opcode[(*opcode)].length -
> >
> > > +            (skb_headlen(skb) - sizeof(struct udphdr));
> > > +
> > > +        if (delta > 0 && !__pskb_pull_tail(skb, delta))
> > > +            goto err_device_put;
> > > +    } else {
> > > +        if (skb_linearize(skb))
> > > +            goto err_device_put;
> > >        }
> > >          udph = udp_hdr(skb);
> > >        pkt->rxe = rxe;
> > >        pkt->port_num = 1;
> > >        pkt->hdr = (u8 *)(udph + 1);
> > > -    pkt->mask = RXE_GRH_MASK;
> > > +    pkt->mask = rxe_opcode[opcode].mask | RXE_GRH_MASK;
> > <..>
> >
> > Zhu Yanjun
> > >        pkt->paylen = be16_to_cpu(udph->len) - sizeof(*udph);
> > >    -    /* remove udp header */
> > >        skb_pull(skb, sizeof(struct udphdr));
> > >          rxe_rcv(skb);
> > >          return 0;
> > > -drop:
> > > +
> > > +err_device_put:
> > > +    ib_device_put(&rxe->ib_dev);
> > > +err_drop:
> > >        kfree_skb(skb);
> > >          return 0;
> > > @@ -446,24 +466,35 @@ static int rxe_send(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> > >     */
> > >    static int rxe_loopback(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> > >    {
> > > -    memcpy(SKB_TO_PKT(skb), pkt, sizeof(*pkt));
> > > +    struct rxe_pkt_info *newpkt;
> > > +    int err;
> > >    +    /* make loopback line up with rxe_udp_encap_recv */
> > >        if (skb->protocol == htons(ETH_P_IP))
> > >            skb_pull(skb, sizeof(struct iphdr));
> > >        else
> > >            skb_pull(skb, sizeof(struct ipv6hdr));
> > > +    skb_reset_transport_header(skb);
> > > +
> > > +    newpkt = SKB_TO_PKT(skb);
> > > +    memcpy(newpkt, pkt, sizeof(*newpkt));
> > > +    newpkt->hdr = skb_transport_header(skb) + sizeof(struct udphdr);
> > >          if (WARN_ON(!ib_device_try_get(&pkt->rxe->ib_dev))) {
> > >            kfree_skb(skb);
> > > -        return -EIO;
> > > +        err = -EINVAL;
> > > +        goto drop;
> > >        }
> > >          /* remove udp header */
> > >        skb_pull(skb, sizeof(struct udphdr));
> > >          rxe_rcv(skb);
> > > -
> > >        return 0;
> > > +
> > > +drop:
> > > +    kfree_skb(skb);
> > > +    return err;
> > >    }
> > >      int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
> > > index f912a913f89a..940197199252 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_recv.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
> > > @@ -338,6 +338,7 @@ void rxe_rcv(struct sk_buff *skb)
> > >        if (unlikely(err))
> > >            goto drop;
> > >    +    /* skb->data points at UDP header */
> > >        err = rxe_icrc_check(skb, pkt);
> > >        if (unlikely(err))
> > >            goto drop;