Re: [PATCH net-next v8 4/4] gve: Add support for raw addressing in the tx path

[Eric Dumazet]

On 11/25/20 6:38 PM, David Awogbemila wrote:
> From: Catherine Sullivan <csu...@google.com>
> 



> +     if (tx->raw_addressing)
> +             nsegs = gve_tx_add_skb_no_copy(priv, tx, skb);
> +     else
> +             nsegs = gve_tx_add_skb_copy(priv, tx, skb);
> +
> +     /* If the packet is getting sent, we need to update the skb */
> +     if (nsegs) {
> +             netdev_tx_sent_queue(tx->netdev_txq, skb->len);
> +             skb_tx_timestamp(skb);
> +
> +             /* Give packets to NIC. Even if this packet failed to send the 
> doorbell
> +              * might need to be rung because of xmit_more.
> +              */
> +             tx->req += nsegs;
>  
> -     if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
> -             return NETDEV_TX_OK;
> +             if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
> +                     return NETDEV_TX_OK;
>  
> -     gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
> +             gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
> +     }
>       return NETDEV_TX_OK;
>  }
>

Code does not match the comment (Give packets to NIC.
    Even if this packet failed to send the doorbell
    might need to be rung because of xmit_more.)

You probably meant :

    if (nsegs) {
        netdev_tx_sent_queue(tx->netdev_txq, skb->len);
        skb_tx_timestamp(skb);
        tx->req += nsegs;
        if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
            return NETDEV_TX_OK;
    }
    gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
    return NETDEV_TX_OK;


Or

    if (nsegs) {
        netdev_tx_sent_queue(tx->netdev_txq, skb->len);
        skb_tx_timestamp(skb);
        tx->req += nsegs;
    }
    if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
        return NETDEV_TX_OK;

    gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
    return NETDEV_TX_OK;

Also you forgot to free the skb if it was not sent (in case of mapping error in 
gve_tx_add_skb_no_copy())

So you probably need to call dev_kfree_skb_any(), or risk leaking memory and 
freeze sockets.

    if (nsegs) {
        netdev_tx_sent_queue(tx->netdev_txq, skb->len);
        skb_tx_timestamp(skb);
        tx->req += nsegs;
    } else {
        dev_kfree_skb_any(skb);
    }
    if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
        return NETDEV_TX_OK;

    gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
    return NETDEV_TX_OK;