On 8/29/19 4:18 PM, Jakub Kicinski wrote:
On Thu, 29 Aug 2019 11:27:16 -0700, Shannon Nelson wrote:+netdev_tx_t ionic_start_xmit(struct sk_buff *skb, struct net_device *netdev) +{ + u16 queue_index = skb_get_queue_mapping(skb); + struct ionic_lif *lif = netdev_priv(netdev); + struct ionic_queue *q; + int ndescs; + int err; + + if (unlikely(!test_bit(IONIC_LIF_UP, lif->state))) { + dev_kfree_skb(skb); + return NETDEV_TX_OK; + } + + if (likely(lif_to_txqcq(lif, queue_index))) + q = lif_to_txq(lif, queue_index); + else + q = lif_to_txq(lif, 0); + + ndescs = ionic_tx_descs_needed(q, skb); + if (ndescs < 0) + goto err_out_drop; + + if (!ionic_q_has_space(q, ndescs)) {You should stop the queue in advance, whenever you can't ensure that a max size frame can be placed on the ring. Requeuing is very expensive so modern drivers should try to never return NETDEV_TX_BUSY
Yes, I see how that's been done in nfp - good idea.
+ netif_stop_subqueue(netdev, queue_index); + q->stop++; + + /* Might race with ionic_tx_clean, check again */ + smp_rmb(); + if (ionic_q_has_space(q, ndescs)) { + netif_wake_subqueue(netdev, queue_index); + q->wake++; + } else { + return NETDEV_TX_BUSY; + } + } + + if (skb_is_gso(skb)) + err = ionic_tx_tso(q, skb); + else + err = ionic_tx(q, skb); + + if (err) + goto err_out_drop; + + return NETDEV_TX_OK; + +err_out_drop: + netif_stop_subqueue(netdev, queue_index);This stopping of the queue is suspicious, if ionic_tx() fails there's no guarantee the queue will ever be woken up, no?
Yes, that does look odd. If there isn't a new descriptor with an skb in the queue, it won't get cleaned and reenabled in the Tx clean.
sln
+ q->stop++; + q->drop++; + dev_kfree_skb(skb); + return NETDEV_TX_OK; +}
