Messages in this thread | | | Subject | Re: [PATCH net v2 3/3] net: nixge: fix tx queue handling | From | Zhang Changzhong <> | Date | Wed, 16 Nov 2022 19:52:34 +0800 |
| |
On 2022/11/16 18:27, Francois Romieu wrote: > Zhang Changzhong <zhangchangzhong@huawei.com> : >> On 2022/11/16 7:04, Francois Romieu wrote: >>> Zhang Changzhong <zhangchangzhong@huawei.com> : > [...] >>>> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c >>>> index 91b7ebc..3776a03 100644 >>>> --- a/drivers/net/ethernet/ni/nixge.c >>>> +++ b/drivers/net/ethernet/ni/nixge.c >>> [...] >>>> static netdev_tx_t nixge_start_xmit(struct sk_buff *skb, >>>> @@ -518,10 +523,15 @@ static netdev_tx_t nixge_start_xmit(struct sk_buff *skb, >>>> cur_p = &priv->tx_bd_v[priv->tx_bd_tail]; >>>> tx_skb = &priv->tx_skb[priv->tx_bd_tail]; >>>> >>>> - if (nixge_check_tx_bd_space(priv, num_frag + 1)) { >>>> - if (!netif_queue_stopped(ndev)) >>>> - netif_stop_queue(ndev); >>>> - return NETDEV_TX_OK; >>>> + if (unlikely(nixge_check_tx_bd_space(priv, num_frag + 1))) { >>>> + /* Should not happen as last start_xmit call should have >>>> + * checked for sufficient space and queue should only be >>>> + * woken when sufficient space is available. >>>> + */ >>> >>> Almost. IRQ triggering after nixge_start_xmit::netif_stop_queue and >>> before nixge_start_xmit::smp_mb may wrongly wake queue. >>> >> >> I don't know what you mean by "wronly wake queue". The queue is woken >> only when there is sufficient for next packet. > > Between nixge_start_xmit::netif_stop_queue and nixge_start_xmit::smp_mb, > "next" packet is current packet in hard_start_xmit. However said current > packet may not be accounted for in the IRQ context transmit completion > handler. > > [nixge_start_xmit] > > ++priv->tx_bd_tail; > priv->tx_bd_tail %= TX_BD_NUM; > > /* Stop queue if next transmit may not have space */ > if (nixge_check_tx_bd_space(priv, MAX_SKB_FRAGS + 1)) { > netif_stop_queue(ndev); > > Which value does [nixge_start_xmit_done] read as priv->tx_bd_tail at > this point ? The value set a few lines above or some older value ? > > /* Matches barrier in nixge_start_xmit_done */ > smp_mb(); > > /* Space might have just been freed - check again */ > if (!nixge_check_tx_bd_space(priv, MAX_SKB_FRAGS + 1)) > netif_wake_queue(ndev); > } >
Got it! Thanks a lot for your detailed explanation.
Best Regards, Changzhong
| |