Re: [PATCH net] net: fix races in netdev_tx_sent_queue()/dev_watchdog()

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Eric Dumazet <edumazet@google.com> writes:

> Some workloads hit the infamous dev_watchdog() message:
>
> "NETDEV WATCHDOG: eth0 (xxxx): transmit queue XX timed out"
>
> It seems possible to hit this even for perfectly normal
> BQL enabled drivers:
>
> 1) Assume a TX queue was idle for more than dev->watchdog_timeo
>    (5 seconds unless changed by the driver)
>
> 2) Assume a big packet is sent, exceeding current BQL limit.
>
> 3) Driver ndo_start_xmit() puts the packet in TX ring,
>    and netdev_tx_sent_queue() is called.
>
> 4) QUEUE_STATE_STACK_XOFF could be set from netdev_tx_sent_queue()
>    before txq->trans_start has been written.
>
> 5) txq->trans_start is written later, from netdev_start_xmit()
>
>     if (rc == NETDEV_TX_OK)
>           txq_trans_update(txq)
>
> dev_watchdog() running on another cpu could read the old
> txq->trans_start, and then see QUEUE_STATE_STACK_XOFF, because 5)
> did not happen yet.
>
> To solve the issue, write txq->trans_start right before one XOFF bit
> is set :
>
> - _QUEUE_STATE_DRV_XOFF from netif_tx_stop_queue()
> - __QUEUE_STATE_STACK_XOFF from netdev_tx_sent_queue()
>
> From dev_watchdog(), we have to read txq->state before txq->trans_start.
>
> Add memory barriers to enforce correct ordering.
>
> In the future, we could avoid writing over txq->trans_start for normal
> operations, and rename this field to txq->xoff_start_time.
>
> Fixes: bec251bc8b6a ("net: no longer stop all TX queues in dev_watchdog()")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>