Re: [PATCH net v2 3/3] net: nixge: fix tx queue handling

From: Zhang Changzhong
Date: Wed Nov 16 2022 - 07:00:49 EST


On 2022/11/16 18:27, Francois Romieu wrote:
> Zhang Changzhong <zhangchangzhong@xxxxxxxxxx> :
>> On 2022/11/16 7:04, Francois Romieu wrote:
>>> Zhang Changzhong <zhangchangzhong@xxxxxxxxxx> :
> [...]
>>>> 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