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 for Android: free password hash cracker in your pocket

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

Message-ID: <40173c9d-845b-6163-5e27-17ef54df0c23@redhat.com>
Date:   Fri, 12 Apr 2019 12:23:08 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     David Woodhouse <dwmw2@...radead.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        netdev@...r.kernel.org
Cc:     "Michael S. Tsirkin" <mst@...hat.com>
Subject: Re: Tun congestion/BQL


On 2019/4/11 下午5:16, David Woodhouse wrote:
> On Thu, 2019-04-11 at 17:04 +0800, Jason Wang wrote:
>> Btw, forget to mention, I modify your patch to use
>> netif_stop/wake_subqueue() instead.
> Hm...
>
>
> --- /usr/src/debug/kernel-5.0.fc29/linux-5.0.5-
> 200.fc29.x86_64/drivers/net/tun.c2019-03-03 23:21:29.000000000 +0000
> +++ /home/fedora/tun/tun.c	2019-04-11 09:11:20.781683956 +0000
> @@ -1118,8 +1118,14 @@ static netdev_tx_t tun_net_xmit(struct s
>   
>   	nf_reset(skb);
>   
> -	if (ptr_ring_produce(&tfile->tx_ring, skb))
> +	if (ptr_ring_produce(&tfile->tx_ring, skb)) {
> +		netif_stop_subqueue(tun->dev, txq);
>   		goto drop;
> +	}
> +
> +	if (ptr_ring_full(&tfile->tx_ring)) {
> +		netif_stop_subqueue(tun->dev, txq);
> +	}
>   
>   	/* Notify and wake up reader process */
>   	if (tfile->flags & TUN_FASYNC)
> @@ -2229,6 +2235,8 @@ static ssize_t tun_do_read(struct tun_st
>   			consume_skb(skb);
>   	}
>   
> +	netif_wake_subqueue(tun->dev, tfile->queue_index);
> +
>   	return ret;
>   }
>   
>
> That gives me
>
> Socket  Message  Elapsed      Messages
> Size    Size     Time         Okay Errors   Throughput
> bytes   bytes    secs            #      #   10^6bits/sec
>
> 212992    1400   10.90     2277474      0    2340.95
> 212992           10.90     1174728           1207.47
>
> Without the first netif_stop_subqueue() call in the case where we
> actually *do* drop the packet (which AFAICT should never happen?) it
> looks like this:


Looks like subqueue wakeup could be done after subqueue stop, so you 
probably need to check !ptr_ring_full() before waking up the queue.

But it may still have issues:

- still racy between tun_net_xmit() and tun_do_read() to solve this need 
to protect the ring full check and subqueue status changing with 
producer lock

- still racy when there're multiple tun_net_xmit()

- but probing producer index may lead cacheline bounce which is kinda 
conflict with the design of ptr_ring which don't want consumer to peek 
producer index


If we really want to go this way, need more careful thought e.g need 
bring back the IFF_ONE_QUEUE behaviour and clear SOCK_ZEROCOPY when 
IFF_ONE_QUEUE is set.


>
> Socket  Message  Elapsed      Messages
> Size    Size     Time         Okay Errors   Throughput
> bytes   bytes    secs            #      #   10^6bits/sec
>
> 212992    1400   10.00     7675563      0    8596.61
> 212992           10.00     1441186           1614.13
>
> And without the patch at all, it was still faster when I just let it
> drop packets:
>
> Socket  Message  Elapsed      Messages
> Size    Size     Time         Okay Errors   Throughput
> bytes   bytes    secs            #      #   10^6bits/sec
>
> 212992    1400   10.00     8143709      0    9120.93
> 212992           10.00     1545035           1730.44
>
>

I think this is expected, the patch brings some overheads.

Thanks