Re: Tun congestion/BQL

[Jason Wang <jasowang@redhat.com>]

On 2019/4/10 下午9:42, Toke Høiland-Jørgensen wrote:
> Jason Wang <jasowang@redhat.com> writes:
>
>> On 2019/4/10 下午9:01, David Woodhouse wrote:
>>> On Wed, 2019-04-10 at 15:01 +0300, David Woodhouse wrote:
>>>> --- a/drivers/net/tun.c
>>>> +++ b/drivers/net/tun.c
>>>> @@ -1125,7 +1128,9 @@ static netdev_tx_t tun_net_xmit(struct sk_buff
>>>> *skb, struct net_device *dev)
>>>>           if (tfile->flags & TUN_FASYNC)
>>>>                   kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
>>>>           tfile->socket.sk->sk_data_ready(tfile->socket.sk);
>>>>    
>>>> +       if (!ptr_ring_empty(&tfile->tx_ring))
>>>> +               netif_stop_queue(tun->dev);
>>>>           rcu_read_unlock();
>>>>           return NETDEV_TX_OK;
>>>>    
>>>>
>>> Hm, that should be using ptr_ring_full() shouldn't it? So...
>>>
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -1121,6 +1121,9 @@ static netdev_tx_t tun_net_xmit(struct s
>>>    	if (ptr_ring_produce(&tfile->tx_ring, skb))
>>>    		goto drop;
>>>    
>>> +	if (ptr_ring_full(&tfile->tx_ring))
>>> +		netif_stop_queue(tun->dev);
>>> +
>>>    	/* Notify and wake up reader process */
>>>    	if (tfile->flags & TUN_FASYNC)
>>>    		kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
>>> @@ -2229,6 +2232,7 @@ static ssize_t tun_do_read(struct tun_st
>>>    			consume_skb(skb);
>>>    	}
>>>    
>>> +	netif_wake_queue(tun->dev);
>>>    	return ret;
>>>    }
>>>    
>>>
>>> That doesn't seem to make much difference at all; it's still dropping a
>>> lot of packets because ptr_ring_produce() is returning non-zero.
>>
>> I think you need try to stop the queue just in this case? Ideally we may
>> want to stop the queue when the queue is about to full, but we don't
>> have such helper currently.
> Ideally we want to react when the queue starts building rather than when
> it starts getting full; by pushing back on upper layers (or, if
> forwarding, dropping packets to signal congestion).
>
> In practice, this means tuning the TX ring to the *minimum* size it can
> be without starving (this is basically what BQL does for Ethernet), and
> keeping packets queued in the qdisc layer instead, where it can be
> managed...
>
> -Toke


Yes, but we do have user that don't use qdisc at all.

Thanks