netdev - Re: Tun congestion/BQL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 10 Apr 2019 15:42:56 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Jason Wang <jasowang@...hat.com>,
        David Woodhouse <dwmw2@...radead.org>, netdev@...r.kernel.org
Subject: Re: Tun congestion/BQL

Jason Wang <jasowang@...hat.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