[PATCH RFC 0/2] tun: lockless xmit

[PATCH RFC 0/2] tun: lockless xmit

[Paolo Abeni]

This patch series try to remove the need for any lock in the tun device
xmit path, significantly improving the forwarding performance when multiple
processes are accessing the tun device (i.e. in a nic->bridge->tun->vm scenario).

The lockless xmit is obtained explicitly setting the NETIF_F_LLTX feature bit
and removing the default qdisc.

Unlikely most virtual devices, the tun driver has featured a default qdisc
for a long period, but it already lost such feature in linux 4.3.

Paolo Abeni (2):
  tun: don't require serialization lock on tx
  tun: don't set a default qdisc

 drivers/net/tun.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH RFC 1/2] tun: don't require serialization lock on tx

[Paolo Abeni]

The current tun_net_xmit() implementation don't need any external
lock since it relay on rcu protection for the tun data structure
and on socket queue lock for skb queuing.

This patch set the NETIF_F_LLTX feature bit in the tun device, so
that on xmit, in absence of qdisc, no serialization lock is acquired
by the caller.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/tun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index faf9297..42992dc 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1796,7 +1796,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
 				   TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX |
 				   NETIF_F_HW_VLAN_STAG_TX;
-		dev->features = dev->hw_features;
+		dev->features = dev->hw_features | NETIF_F_LLTX;
 		dev->vlan_features = dev->features &
 				     ~(NETIF_F_HW_VLAN_CTAG_TX |
 				       NETIF_F_HW_VLAN_STAG_TX);
-- 
1.8.3.1

[PATCH RFC 2/2] tun: don't set a default qdisc

[Paolo Abeni]

This patch disables the default qdisc by explicitly setting the
IFF_NO_QUEUE private flag so that now the tun xmit path do not
require any lock by default.

The default qdisc was first removed as a side effect of commit
f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/tun.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 42992dc..0a0b63c 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
 
 		break;
 	}
+	dev->priv_flags |= IFF_NO_QUEUE;
 }
 
 /* Character device part */
-- 
1.8.3.1

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Michael S. Tsirkin]

On Wed, Apr 13, 2016 at 11:04:46AM +0200, Paolo Abeni wrote:
> The current tun_net_xmit() implementation don't need any external
> lock since it relay on rcu protection for the tun data structure
> and on socket queue lock for skb queuing.
> 
> This patch set the NETIF_F_LLTX feature bit in the tun device, so
> that on xmit, in absence of qdisc, no serialization lock is acquired
> by the caller.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/tun.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index faf9297..42992dc 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1796,7 +1796,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
>  				   TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX |
>  				   NETIF_F_HW_VLAN_STAG_TX;
> -		dev->features = dev->hw_features;
> +		dev->features = dev->hw_features | NETIF_F_LLTX;

the documentation says:
        NETIF_F_LLTX_BIT,               /* LockLess TX - deprecated.  Please */
                                        /* do not use LLTX in new drivers */


git log gives this explanation:
    > 1) unnecessary;
    > 2) causes problems with AF_PACKET seeing things twice.



>  		dev->vlan_features = dev->features &
>  				     ~(NETIF_F_HW_VLAN_CTAG_TX |
>  				       NETIF_F_HW_VLAN_STAG_TX);
> -- 
> 1.8.3.1

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Hannes Frederic Sowa]

On 13.04.2016 11:41, Michael S. Tsirkin wrote:
> On Wed, Apr 13, 2016 at 11:04:46AM +0200, Paolo Abeni wrote:
>> The current tun_net_xmit() implementation don't need any external
>> lock since it relay on rcu protection for the tun data structure
>> and on socket queue lock for skb queuing.
>>
>> This patch set the NETIF_F_LLTX feature bit in the tun device, so
>> that on xmit, in absence of qdisc, no serialization lock is acquired
>> by the caller.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>>   drivers/net/tun.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index faf9297..42992dc 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1796,7 +1796,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>>   		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
>>   				   TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX |
>>   				   NETIF_F_HW_VLAN_STAG_TX;
>> -		dev->features = dev->hw_features;
>> +		dev->features = dev->hw_features | NETIF_F_LLTX;
>
> the documentation says:
>          NETIF_F_LLTX_BIT,               /* LockLess TX - deprecated.  Please */
>                                          /* do not use LLTX in new drivers */

In networking/netdev-features.txt

  * LLTX driver (deprecated for hardware drivers)

NETIF_F_LLTX should be set in drivers that implement their own locking in
transmit path or don't need locking at all (e.g. software tunnels).
In ndo_start_xmit, it is recommended to use a try_lock and return
NETDEV_TX_LOCKED when the spin lock fails.  The locking should also properly
protect against other callbacks (the rules you need to find out).

Don't use it for new drivers.

I think this is documentation is correct and it is only deprecated for 
hardware drivers.

Bye,
Hannes

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[Michael S. Tsirkin]

On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
> This patch disables the default qdisc by explicitly setting the
> IFF_NO_QUEUE private flag so that now the tun xmit path do not
> require any lock by default.
> 
> The default qdisc was first removed as a side effect of commit
> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

I wonder about this back and forth.
Jason, do you see a workload where the default qdisc
is preferable?

> ---
>  drivers/net/tun.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 42992dc..0a0b63c 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
>  
>  		break;
>  	}
> +	dev->priv_flags |= IFF_NO_QUEUE;
>  }
>  
>  /* Character device part */
> -- 
> 1.8.3.1

Re: [PATCH RFC 0/2] tun: lockless xmit

[Michael S. Tsirkin]

On Wed, Apr 13, 2016 at 11:04:45AM +0200, Paolo Abeni wrote:
> This patch series try to remove the need for any lock in the tun device
> xmit path, significantly improving the forwarding performance when multiple
> processes are accessing the tun device (i.e. in a nic->bridge->tun->vm scenario).
> 
> The lockless xmit is obtained explicitly setting the NETIF_F_LLTX feature bit
> and removing the default qdisc.
> 
> Unlikely most virtual devices, the tun driver has featured a default qdisc
> for a long period, but it already lost such feature in linux 4.3.

Thanks -  I think it's a good idea to reduce the
lock contention there.

But I think it's unfortunate that it requires
bypassing the qdisc completely: this means
that anyone trying to do traffic shaping will
get back the contention.

Can we solve the lock contention for qdisc?
E.g. add a small lockless queue in front of it,
whoever has the qdisc lock would be
responsible for moving things from there to qdisc
proper.

Thoughts? Is there a chance this might work reasonably well?


> Paolo Abeni (2):
>   tun: don't require serialization lock on tx
>   tun: don't set a default qdisc
> 
>  drivers/net/tun.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> -- 
> 1.8.3.1

Re: [PATCH RFC 0/2] tun: lockless xmit

[Eric Dumazet]

On Wed, 2016-04-13 at 14:08 +0300, Michael S. Tsirkin wrote:
> On Wed, Apr 13, 2016 at 11:04:45AM +0200, Paolo Abeni wrote:
> > This patch series try to remove the need for any lock in the tun device
> > xmit path, significantly improving the forwarding performance when multiple
> > processes are accessing the tun device (i.e. in a nic->bridge->tun->vm scenario).
> > 
> > The lockless xmit is obtained explicitly setting the NETIF_F_LLTX feature bit
> > and removing the default qdisc.
> > 
> > Unlikely most virtual devices, the tun driver has featured a default qdisc
> > for a long period, but it already lost such feature in linux 4.3.
> 
> Thanks -  I think it's a good idea to reduce the
> lock contention there.
> 
> But I think it's unfortunate that it requires
> bypassing the qdisc completely: this means
> that anyone trying to do traffic shaping will
> get back the contention.
> 
> Can we solve the lock contention for qdisc?
> E.g. add a small lockless queue in front of it,
> whoever has the qdisc lock would be
> responsible for moving things from there to qdisc
> proper.
> 
> Thoughts? Is there a chance this might work reasonably well?

Adding any new queue in front of qdisc is problematic :
- Adds a new buffer, with extra latencies.
- If you want to implement priorities properly for X COS, you need X
queues.
- Who is going to service this extra buffer and feed the qdisc ?
- If the innocent guy is RT thread, maybe the extra latency will hurt.
- Adding another set of atomic ops.

We have such a schem here at Google (called holdq), but it was a
nightmare to tune.

We never tried to upstream this beast, it is kind of ugly, and were
expecting something better. Problem is : If you use HTB on a bonding
device, you want still to properly use MQ on the slaves.

HTB queue. 20 netperf generating UDP packets 
lpaa23:~# ./super_netperf 20 -H lpaa24 -t UDP_STREAM -l 3000 -- -m 100 &
[1] 181993


With the holdq feature turned on : about 1 Mpps

lpaa23:~# sar -n DEV 1 10|grep eth0|grep Average
Average:         eth0     28.50 999071.60      3.07 138542.64      0.00
0.00      0.60

holdq turned off : about 620 Kpps

lpaa23:~# sar -n DEV 1 10|grep eth0|grep Average
Average:         eth0     39.00 617765.40      4.73  85667.42      0.00
0.00      0.90

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Eric Dumazet]

On Wed, 2016-04-13 at 11:04 +0200, Paolo Abeni wrote:
> The current tun_net_xmit() implementation don't need any external
> lock since it relay on rcu protection for the tun data structure
> and on socket queue lock for skb queuing.
> 
> This patch set the NETIF_F_LLTX feature bit in the tun device, so
> that on xmit, in absence of qdisc, no serialization lock is acquired
> by the caller.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/tun.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index faf9297..42992dc 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1796,7 +1796,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
>  				   TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX |
>  				   NETIF_F_HW_VLAN_STAG_TX;
> -		dev->features = dev->hw_features;
> +		dev->features = dev->hw_features | NETIF_F_LLTX;
>  		dev->vlan_features = dev->features &
>  				     ~(NETIF_F_HW_VLAN_CTAG_TX |
>  				       NETIF_F_HW_VLAN_STAG_TX);

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH RFC 0/2] tun: lockless xmit

[Michael S. Tsirkin]

On Wed, Apr 13, 2016 at 05:50:17AM -0700, Eric Dumazet wrote:
> On Wed, 2016-04-13 at 14:08 +0300, Michael S. Tsirkin wrote:
> > On Wed, Apr 13, 2016 at 11:04:45AM +0200, Paolo Abeni wrote:
> > > This patch series try to remove the need for any lock in the tun device
> > > xmit path, significantly improving the forwarding performance when multiple
> > > processes are accessing the tun device (i.e. in a nic->bridge->tun->vm scenario).
> > > 
> > > The lockless xmit is obtained explicitly setting the NETIF_F_LLTX feature bit
> > > and removing the default qdisc.
> > > 
> > > Unlikely most virtual devices, the tun driver has featured a default qdisc
> > > for a long period, but it already lost such feature in linux 4.3.
> > 
> > Thanks -  I think it's a good idea to reduce the
> > lock contention there.
> > 
> > But I think it's unfortunate that it requires
> > bypassing the qdisc completely: this means
> > that anyone trying to do traffic shaping will
> > get back the contention.
> > 
> > Can we solve the lock contention for qdisc?
> > E.g. add a small lockless queue in front of it,
> > whoever has the qdisc lock would be
> > responsible for moving things from there to qdisc
> > proper.
> > 
> > Thoughts? Is there a chance this might work reasonably well?
> 
> Adding any new queue in front of qdisc is problematic :
> - Adds a new buffer, with extra latencies.

Only where lock contention would previously occur, right?

> - If you want to implement priorities properly for X COS, you need X
> queues.

This definitely needs thought.

> - Who is going to service this extra buffer and feed the qdisc ?

The way I see it - whoever has the lock, at unlock time.

> - If the innocent guy is RT thread, maybe the extra latency will hurt.

Again - more than a lock?

> - Adding another set of atomic ops.

That's likely true. Use some per-cpu trick instead?

> We have such a schem here at Google (called holdq), but it was a
> nightmare to tune.
> 
> We never tried to upstream this beast, it is kind of ugly, and were
> expecting something better. Problem is : If you use HTB on a bonding
> device, you want still to properly use MQ on the slaves.
> 
> HTB queue. 20 netperf generating UDP packets 
> lpaa23:~# ./super_netperf 20 -H lpaa24 -t UDP_STREAM -l 3000 -- -m 100 &
> [1] 181993
> 
> 
> With the holdq feature turned on : about 1 Mpps
> 
> lpaa23:~# sar -n DEV 1 10|grep eth0|grep Average
> Average:         eth0     28.50 999071.60      3.07 138542.64      0.00
> 0.00      0.60
> 
> holdq turned off : about 620 Kpps
> 
> lpaa23:~# sar -n DEV 1 10|grep eth0|grep Average
> Average:         eth0     39.00 617765.40      4.73  85667.42      0.00
> 0.00      0.90
> 

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Michael S. Tsirkin]

On Wed, Apr 13, 2016 at 11:48:09AM +0200, Hannes Frederic Sowa wrote:
> On 13.04.2016 11:41, Michael S. Tsirkin wrote:
> >On Wed, Apr 13, 2016 at 11:04:46AM +0200, Paolo Abeni wrote:
> >>The current tun_net_xmit() implementation don't need any external
> >>lock since it relay on rcu protection for the tun data structure
> >>and on socket queue lock for skb queuing.
> >>
> >>This patch set the NETIF_F_LLTX feature bit in the tun device, so
> >>that on xmit, in absence of qdisc, no serialization lock is acquired
> >>by the caller.
> >>
> >>Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> >>---
> >>  drivers/net/tun.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >>index faf9297..42992dc 100644
> >>--- a/drivers/net/tun.c
> >>+++ b/drivers/net/tun.c
> >>@@ -1796,7 +1796,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> >>  		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
> >>  				   TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX |
> >>  				   NETIF_F_HW_VLAN_STAG_TX;
> >>-		dev->features = dev->hw_features;
> >>+		dev->features = dev->hw_features | NETIF_F_LLTX;
> >
> >the documentation says:
> >         NETIF_F_LLTX_BIT,               /* LockLess TX - deprecated.  Please */
> >                                         /* do not use LLTX in new drivers */
> 
> In networking/netdev-features.txt
> 
>  * LLTX driver (deprecated for hardware drivers)
> 
> NETIF_F_LLTX should be set in drivers that implement their own locking in
> transmit path or don't need locking at all (e.g. software tunnels).
> In ndo_start_xmit, it is recommended to use a try_lock and return
> NETDEV_TX_LOCKED when the spin lock fails.  The locking should also properly
> protect against other callbacks (the rules you need to find out).
> 
> Don't use it for new drivers.
> 
> I think this is documentation is correct and it is only deprecated for
> hardware drivers.
> 
> Bye,
> Hannes

Fine, but what's the AF_PACKET duplication that Herbert Xu
reported with NETIF_F_LLTX? Does anyone remember?

-- 
MST

Re: [PATCH RFC 0/2] tun: lockless xmit

[Eric Dumazet]

On Wed, 2016-04-13 at 15:56 +0300, Michael S. Tsirkin wrote:
> On Wed, Apr 13, 2016 at 05:50:17AM -0700, Eric Dumazet wrote:
> > On Wed, 2016-04-13 at 14:08 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Apr 13, 2016 at 11:04:45AM +0200, Paolo Abeni wrote:
> > > > This patch series try to remove the need for any lock in the tun device
> > > > xmit path, significantly improving the forwarding performance when multiple
> > > > processes are accessing the tun device (i.e. in a nic->bridge->tun->vm scenario).
> > > > 
> > > > The lockless xmit is obtained explicitly setting the NETIF_F_LLTX feature bit
> > > > and removing the default qdisc.
> > > > 
> > > > Unlikely most virtual devices, the tun driver has featured a default qdisc
> > > > for a long period, but it already lost such feature in linux 4.3.
> > > 
> > > Thanks -  I think it's a good idea to reduce the
> > > lock contention there.
> > > 
> > > But I think it's unfortunate that it requires
> > > bypassing the qdisc completely: this means
> > > that anyone trying to do traffic shaping will
> > > get back the contention.
> > > 
> > > Can we solve the lock contention for qdisc?
> > > E.g. add a small lockless queue in front of it,
> > > whoever has the qdisc lock would be
> > > responsible for moving things from there to qdisc
> > > proper.
> > > 
> > > Thoughts? Is there a chance this might work reasonably well?
> > 
> > Adding any new queue in front of qdisc is problematic :
> > - Adds a new buffer, with extra latencies.
> 
> Only where lock contention would previously occur, right?
> 
> > - If you want to implement priorities properly for X COS, you need X
> > queues.
> 
> This definitely needs thought.
> 
> > - Who is going to service this extra buffer and feed the qdisc ?
> 
> The way I see it - whoever has the lock, at unlock time.
> 
> > - If the innocent guy is RT thread, maybe the extra latency will hurt.
> 
> Again - more than a lock?

Way more. HTB is slow as hell.

Remember the qdisc dequeue is already a big problem in itself.
Adding another layer can practically double the latencies.

> 
> > - Adding another set of atomic ops.
> 
> That's likely true. Use some per-cpu trick instead?

We tried that, and we got miserable production incidents...

You really need to convince John Fastabend to work full time on the real
thing, not on another queue in front of the existing qdisc.

Re: [PATCH RFC 0/2] tun: lockless xmit

[Michael S. Tsirkin]

On Wed, Apr 13, 2016 at 06:09:26AM -0700, Eric Dumazet wrote:
> You really need to convince John Fastabend to work full time on the real
> thing

Meaning making all qdiscs themselves lockless? With complex policies
like codel I can see how that might be challenging ...

-- 
MST

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Eric Dumazet]

     I. On Wed, 2016-04-13 at 15:57 +0300, Michael S. Tsirkin wrote:

> Fine, but what's the AF_PACKET duplication that Herbert Xu
> reported with NETIF_F_LLTX? Does anyone remember?

Really a lot of virtual drivers use NETIF_F_LLTX these days.

Duplication is more likely to happen with a qdisc, when a packet is
requeued if a driver returns NETDEV_TX_BUSY

Re: [PATCH RFC 0/2] tun: lockless xmit

[Eric Dumazet]

On Wed, 2016-04-13 at 16:17 +0300, Michael S. Tsirkin wrote:
> On Wed, Apr 13, 2016 at 06:09:26AM -0700, Eric Dumazet wrote:
> > You really need to convince John Fastabend to work full time on the real
> > thing
> 
> Meaning making all qdiscs themselves lockless? With complex policies
> like codel I can see how that might be challenging ...

Codel is a fifo, plus some droping capabilities at dequeue time.

It totally can be made lockless.

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Michael S. Tsirkin]

On Wed, Apr 13, 2016 at 06:27:08AM -0700, Eric Dumazet wrote:
>      I. On Wed, 2016-04-13 at 15:57 +0300, Michael S. Tsirkin wrote:
> 
> > Fine, but what's the AF_PACKET duplication that Herbert Xu
> > reported with NETIF_F_LLTX? Does anyone remember?
> 
> Really a lot of virtual drivers use NETIF_F_LLTX these days.
> 
> Duplication is more likely to happen with a qdisc, when a packet is
> requeued if a driver returns NETDEV_TX_BUSY

OK, now I understand what the duplication is about.
What about NETDEV_TX_LOCKED? Looks like it might have
the same effect?

This might be worth documenting in include/linux/netdevice.h,
might it not?


-- 
MST

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Sergei Shtylyov]

Hello.

On 4/13/2016 12:04 PM, Paolo Abeni wrote:

> The current tun_net_xmit() implementation don't need any external
> lock since it relay on rcu protection for the tun data structure

    s/relay/relies/?

> and on socket queue lock for skb queuing.
>
> This patch set the NETIF_F_LLTX feature bit in the tun device, so
> that on xmit, in absence of qdisc, no serialization lock is acquired
> by the caller.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
[...]

MBR, Sergei

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Eric Dumazet]

On Wed, 2016-04-13 at 16:54 +0300, Michael S. Tsirkin wrote:

> OK, now I understand what the duplication is about.
> What about NETDEV_TX_LOCKED? Looks like it might have
> the same effect?
> 
> This might be worth documenting in include/linux/netdevice.h,
> might it not?

I believe the intent is to get rid of NETDEV_TX_LOCKED at some point.

Some drivers seem to abuse it.

For example, drivers/infiniband/hw/nes/nes_nic.c is clearly completely
buggy :(

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 13 Apr 2016 13:26:51 +0300

> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>> This patch disables the default qdisc by explicitly setting the
>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>> require any lock by default.
>> 
>> The default qdisc was first removed as a side effect of commit
>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>> 
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> I wonder about this back and forth.
> Jason, do you see a workload where the default qdisc
> is preferable?

I don't think you should change this, putting the default back was a
bug fix.

I think this series is taking way too many liberties in order to
achieve locklessness, in my opinion.

If you have proper multi-queue, the qdisc lock and the netdev transmit
lock are almost completely free.

Re: [PATCH RFC 0/2] tun: lockless xmit

[Eric Dumazet]

On Wed, 2016-04-13 at 06:43 -0700, Eric Dumazet wrote:
> On Wed, 2016-04-13 at 16:17 +0300, Michael S. Tsirkin wrote:
> > On Wed, Apr 13, 2016 at 06:09:26AM -0700, Eric Dumazet wrote:
> > > You really need to convince John Fastabend to work full time on the real
> > > thing
> > 
> > Meaning making all qdiscs themselves lockless? With complex policies
> > like codel I can see how that might be challenging ...
> 
> Codel is a fifo, plus some droping capabilities at dequeue time.
> 
> It totally can be made lockless.

By lockless, I really meant decouple the enqueue() and dequeue() phases.

Both sides could use a separate exclusion mechanism.

So when qdisc_run() is dequeuing a bunch of packets (owning
__QDISC___STATE_RUNNING), other cpus would still be able to queue
additional packets.

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[Jason Wang]

On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>> This patch disables the default qdisc by explicitly setting the
>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>> require any lock by default.
>>
>> The default qdisc was first removed as a side effect of commit
>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> I wonder about this back and forth.
> Jason, do you see a workload where the default qdisc
> is preferable?

I don't know, but we used to behave like this so we'd better keep it.

An interesting thing is I vaguely remember that you have some concern
when I propose IFF_NO_QUEUE for macvtap[1] :)

I think this could be done by management or more safe by introducing a
new tun flag (TUN_NO_QUEUE).

[1] https://lkml.org/lkml/2015/8/24/147

>
>> ---
>>  drivers/net/tun.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 42992dc..0a0b63c 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
>>  
>>  		break;
>>  	}
>> +	dev->priv_flags |= IFF_NO_QUEUE;
>>  }
>>  
>>  /* Character device part */
>> -- 
>> 1.8.3.1

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Jason Wang]

On 04/13/2016 05:04 PM, Paolo Abeni wrote:
> The current tun_net_xmit() implementation don't need any external
> lock since it relay on rcu protection for the tun data structure
> and on socket queue lock for skb queuing.
>
> This patch set the NETIF_F_LLTX feature bit in the tun device, so
> that on xmit, in absence of qdisc, no serialization lock is acquired
> by the caller.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/tun.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index faf9297..42992dc 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1796,7 +1796,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
>  				   TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX |
>  				   NETIF_F_HW_VLAN_STAG_TX;
> -		dev->features = dev->hw_features;
> +		dev->features = dev->hw_features | NETIF_F_LLTX;
>  		dev->vlan_features = dev->features &
>  				     ~(NETIF_F_HW_VLAN_CTAG_TX |
>  				       NETIF_F_HW_VLAN_STAG_TX);

Acked-by: Jason Wang <jasowang@redhat.com>

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[Michael S. Tsirkin]

On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
> 
> 
> On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
> > On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
> >> This patch disables the default qdisc by explicitly setting the
> >> IFF_NO_QUEUE private flag so that now the tun xmit path do not
> >> require any lock by default.
> >>
> >> The default qdisc was first removed as a side effect of commit
> >> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
> >> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
> >>
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > I wonder about this back and forth.
> > Jason, do you see a workload where the default qdisc
> > is preferable?
> 
> I don't know, but we used to behave like this so we'd better keep it.
> 
> An interesting thing is I vaguely remember that you have some concern
> when I propose IFF_NO_QUEUE for macvtap[1] :)
> 
> [1] https://lkml.org/lkml/2015/8/24/147

It's the same concern - that we aren't fully addressing
the problem, so if user configures a qdisc, we are back to square 1.
It's especially annoying that IIUC in this setup, if one
does configured a non default qdisc, there's no way to go back.
It doesn't necessarily mean we must not do it as an intermediate step,
though.

> 
> I think this could be done by management or more safe by introducing a
> new tun flag (TUN_NO_QUEUE).

What exactly does this flag do/mean?

> >
> >> ---
> >>  drivers/net/tun.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >> index 42992dc..0a0b63c 100644
> >> --- a/drivers/net/tun.c
> >> +++ b/drivers/net/tun.c
> >> @@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
> >>  
> >>  		break;
> >>  	}
> >> +	dev->priv_flags |= IFF_NO_QUEUE;
> >>  }
> >>  
> >>  /* Character device part */
> >> -- 
> >> 1.8.3.1

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[Jason Wang]

On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
> On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
>> > 
>> > 
>> > On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
>>> > > On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>>>> > >> This patch disables the default qdisc by explicitly setting the
>>>> > >> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>>>> > >> require any lock by default.
>>>> > >>
>>>> > >> The default qdisc was first removed as a side effect of commit
>>>> > >> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>>>> > >> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>>>> > >>
>>>> > >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> > > I wonder about this back and forth.
>>> > > Jason, do you see a workload where the default qdisc
>>> > > is preferable?
>> > 
>> > I don't know, but we used to behave like this so we'd better keep it.
>> > 
>> > An interesting thing is I vaguely remember that you have some concern
>> > when I propose IFF_NO_QUEUE for macvtap[1] :)
>> > 
>> > [1] https://lkml.org/lkml/2015/8/24/147
> It's the same concern - that we aren't fully addressing
> the problem, so if user configures a qdisc, we are back to square 1.
> It's especially annoying that IIUC in this setup, if one
> does configured a non default qdisc, there's no way to go back.
> It doesn't necessarily mean we must not do it as an intermediate step,
> though.
>
>> > 
>> > I think this could be done by management or more safe by introducing a
>> > new tun flag (TUN_NO_QUEUE).
> What exactly does this flag do/mean?

It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
flag.

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[Michael S. Tsirkin]

On Thu, Apr 14, 2016 at 05:07:50PM +0800, Jason Wang wrote:
> 
> 
> On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
> > On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
> >> > 
> >> > 
> >> > On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
> >>> > > On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
> >>>> > >> This patch disables the default qdisc by explicitly setting the
> >>>> > >> IFF_NO_QUEUE private flag so that now the tun xmit path do not
> >>>> > >> require any lock by default.
> >>>> > >>
> >>>> > >> The default qdisc was first removed as a side effect of commit
> >>>> > >> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
> >>>> > >> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
> >>>> > >>
> >>>> > >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> >>> > > I wonder about this back and forth.
> >>> > > Jason, do you see a workload where the default qdisc
> >>> > > is preferable?
> >> > 
> >> > I don't know, but we used to behave like this so we'd better keep it.
> >> > 
> >> > An interesting thing is I vaguely remember that you have some concern
> >> > when I propose IFF_NO_QUEUE for macvtap[1] :)
> >> > 
> >> > [1] https://lkml.org/lkml/2015/8/24/147
> > It's the same concern - that we aren't fully addressing
> > the problem, so if user configures a qdisc, we are back to square 1.
> > It's especially annoying that IIUC in this setup, if one
> > does configured a non default qdisc, there's no way to go back.
> > It doesn't necessarily mean we must not do it as an intermediate step,
> > though.
> >
> >> > 
> >> > I think this could be done by management or more safe by introducing a
> >> > new tun flag (TUN_NO_QUEUE).
> > What exactly does this flag do/mean?
> 
> It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
> flag.

But what does it mean for the user? When to set it and when not to set
it?

-- 
MST

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[Jason Wang]

On 04/14/2016 05:10 PM, Michael S. Tsirkin wrote:
> On Thu, Apr 14, 2016 at 05:07:50PM +0800, Jason Wang wrote:
>>
>> On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
>>> On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
>>>>>
>>>>> On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>>>>>>>>> This patch disables the default qdisc by explicitly setting the
>>>>>>>>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>>>>>>>>> require any lock by default.
>>>>>>>>>
>>>>>>>>> The default qdisc was first removed as a side effect of commit
>>>>>>>>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>>>>>>>>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>>>>>>>>>
>>>>>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>>>>>> I wonder about this back and forth.
>>>>>>> Jason, do you see a workload where the default qdisc
>>>>>>> is preferable?
>>>>> I don't know, but we used to behave like this so we'd better keep it.
>>>>>
>>>>> An interesting thing is I vaguely remember that you have some concern
>>>>> when I propose IFF_NO_QUEUE for macvtap[1] :)
>>>>>
>>>>> [1] https://lkml.org/lkml/2015/8/24/147
>>> It's the same concern - that we aren't fully addressing
>>> the problem, so if user configures a qdisc, we are back to square 1.
>>> It's especially annoying that IIUC in this setup, if one
>>> does configured a non default qdisc, there's no way to go back.
>>> It doesn't necessarily mean we must not do it as an intermediate step,
>>> though.
>>>
>>>>> I think this could be done by management or more safe by introducing a
>>>>> new tun flag (TUN_NO_QUEUE).
>>> What exactly does this flag do/mean?
>> It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
>> flag.
> But what does it mean for the user? When to set it and when not to set
> it?

It was used for user that don't want qdisc (e.g for the user that only
cares about performance).

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[Michael S. Tsirkin]

On Thu, Apr 14, 2016 at 05:21:40PM +0800, Jason Wang wrote:
> 
> 
> On 04/14/2016 05:10 PM, Michael S. Tsirkin wrote:
> > On Thu, Apr 14, 2016 at 05:07:50PM +0800, Jason Wang wrote:
> >>
> >> On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
> >>> On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
> >>>>>
> >>>>> On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
> >>>>>>> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
> >>>>>>>>> This patch disables the default qdisc by explicitly setting the
> >>>>>>>>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
> >>>>>>>>> require any lock by default.
> >>>>>>>>>
> >>>>>>>>> The default qdisc was first removed as a side effect of commit
> >>>>>>>>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
> >>>>>>>>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> >>>>>>> I wonder about this back and forth.
> >>>>>>> Jason, do you see a workload where the default qdisc
> >>>>>>> is preferable?
> >>>>> I don't know, but we used to behave like this so we'd better keep it.
> >>>>>
> >>>>> An interesting thing is I vaguely remember that you have some concern
> >>>>> when I propose IFF_NO_QUEUE for macvtap[1] :)
> >>>>>
> >>>>> [1] https://lkml.org/lkml/2015/8/24/147
> >>> It's the same concern - that we aren't fully addressing
> >>> the problem, so if user configures a qdisc, we are back to square 1.
> >>> It's especially annoying that IIUC in this setup, if one
> >>> does configured a non default qdisc, there's no way to go back.
> >>> It doesn't necessarily mean we must not do it as an intermediate step,
> >>> though.
> >>>
> >>>>> I think this could be done by management or more safe by introducing a
> >>>>> new tun flag (TUN_NO_QUEUE).
> >>> What exactly does this flag do/mean?
> >> It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
> >> flag.
> > But what does it mean for the user? When to set it and when not to set
> > it?
> 
> It was used for user that don't want qdisc (e.g for the user that only
> cares about performance).

However
- for tun, how is a fifo qdisc functionally different?
- it doesn't prevent configuring a qdisc, does it?

-- 
MST

Re: [PATCH RFC 2/2] tun: don't set a default qdisc

[Hannes Frederic Sowa]

On 14.04.2016 12:01, Michael S. Tsirkin wrote:
> On Thu, Apr 14, 2016 at 05:21:40PM +0800, Jason Wang wrote:
>>
>>
>> On 04/14/2016 05:10 PM, Michael S. Tsirkin wrote:
>>> On Thu, Apr 14, 2016 at 05:07:50PM +0800, Jason Wang wrote:
>>>>
>>>> On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
>>>>>>>
>>>>>>> On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
>>>>>>>>> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>>>>>>>>>>> This patch disables the default qdisc by explicitly setting the
>>>>>>>>>>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>>>>>>>>>>> require any lock by default.
>>>>>>>>>>>
>>>>>>>>>>> The default qdisc was first removed as a side effect of commit
>>>>>>>>>>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>>>>>>>>>>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>>>>>>>> I wonder about this back and forth.
>>>>>>>>> Jason, do you see a workload where the default qdisc
>>>>>>>>> is preferable?
>>>>>>> I don't know, but we used to behave like this so we'd better keep it.
>>>>>>>
>>>>>>> An interesting thing is I vaguely remember that you have some concern
>>>>>>> when I propose IFF_NO_QUEUE for macvtap[1] :)
>>>>>>>
>>>>>>> [1] https://lkml.org/lkml/2015/8/24/147
>>>>> It's the same concern - that we aren't fully addressing
>>>>> the problem, so if user configures a qdisc, we are back to square 1.
>>>>> It's especially annoying that IIUC in this setup, if one
>>>>> does configured a non default qdisc, there's no way to go back.
>>>>> It doesn't necessarily mean we must not do it as an intermediate step,
>>>>> though.
>>>>>
>>>>>>> I think this could be done by management or more safe by introducing a
>>>>>>> new tun flag (TUN_NO_QUEUE).
>>>>> What exactly does this flag do/mean?
>>>> It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
>>>> flag.
>>> But what does it mean for the user? When to set it and when not to set
>>> it?
>>
>> It was used for user that don't want qdisc (e.g for the user that only
>> cares about performance).
>
> However
> - for tun, how is a fifo qdisc functionally different?

pfifo wouldn't matter match, but the pfifo_qdisc classifies packets into 
different bands and does alter the way ip packets are transmitted 
outside. With the LLTX change I don't see how in any case a queue should 
build up, so I don't really see this as a problem.

The only reason why this change could do harm is scripting and API 
compatibility, e.g. tools expect after creation a qdisc to be present to 
modify its behavior.

> - it doesn't prevent configuring a qdisc, does it?

Correct, you can still add qdiscs later on and switch back to no_queue, 
also. We are just talking about changing the default.

Bye,
Hannes

Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx

[Hannes Frederic Sowa]

On 13.04.2016 11:04, Paolo Abeni wrote:
> The current tun_net_xmit() implementation don't need any external
> lock since it relay on rcu protection for the tun data structure
> and on socket queue lock for skb queuing.
>
> This patch set the NETIF_F_LLTX feature bit in the tun device, so
> that on xmit, in absence of qdisc, no serialization lock is acquired
> by the caller.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>