tun: lockless xmit

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 <[email protected]>

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

drivers/net/tun.c +0 -0

index faf9297..42992dc 100644

drivers/net/tun.c +0 -0

 1) unnecessary;

2) causes problems with AF_PACKET seeing things twice.

drivers/net/tun.c +0 -0

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

Signature · 1 line

1.8.3.1

Quoted 13 lines · 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 <[email protected]>

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

drivers/net/tun.c static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) · line 1799

Quoted 10 lines

drivers/net/tun.c +1 -1

index faf9297..42992dc 100644
@@ -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 */

Quoted 14 lines · Michael S. Tsirkin

Quoted 13 lines · 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 <[email protected]>

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

Quoted 14 lines

drivers/net/tun.c static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) · line 1799

Quoted 10 lines

drivers/net/tun.c +1 -1

index faf9297..42992dc 100644
@@ -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?

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

Quoted 2 lines

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?

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 <[email protected]>

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

drivers/net/tun.c +0 -0

index faf9297..42992dc 100644

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 <[email protected]>

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 <[email protected]>

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

drivers/net/tun.c +0 -0

index faf9297..42992dc 100644

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 <[email protected]>

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 <[email protected]>

---
 drivers/net/tun.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/tun.c +1 -0

index 42992dc..0a0b63c 100644
@@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
 
 		break;
 	}
+	dev->priv_flags |= IFF_NO_QUEUE;
 }
 
 /* Character device part */

Signature · 1 line

1.8.3.1

Quoted 8 lines

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")

Quoted 1 line

Signed-off-by Paolo Abeni <[email protected]>

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

Quoted 4 lines · 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.

Quoted 5 lines

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 <[email protected]>

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

Quoted 19 lines

---
 drivers/net/tun.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/tun.c +1 -0

index 42992dc..0a0b63c 100644
@@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
 
 		break;
 	}
+	dev->priv_flags |= IFF_NO_QUEUE;
 }
 
 /* Character device part */

Signature · 1 line

1.8.3.1

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · 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.

Quoted 8 lines

Quoted 5 lines

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 <[email protected]>

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

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

Quoted 19 lines

Quoted 19 lines

---
 drivers/net/tun.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/tun.c +1 -0

index 42992dc..0a0b63c 100644
@@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
 
 		break;
 	}
+	dev->priv_flags |= IFF_NO_QUEUE;
 }
 
 /* Character device part */

Signature · 1 line

1.8.3.1

Quoted 17 lines · Jason Wang

Quoted 17 lines · Michael S. Tsirkin

Quoted 13 lines

Quoted 13 lines · Paolo Abeni

Quoted 9 lines

Quoted 9 lines

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 <[email protected]>

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 2 lines

Quoted 2 lines

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?

Quoted 32 lines · Michael S. Tsirkin

Quoted 17 lines · Jason Wang

Quoted 17 lines · Michael S. Tsirkin

Quoted 13 lines

Quoted 13 lines · Paolo Abeni

Quoted 9 lines

Quoted 9 lines

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 <[email protected]>

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 31 lines · Jason Wang

Quoted 29 lines · Michael S. Tsirkin

Quoted 15 lines · Jason Wang

Quoted 15 lines · Michael S. Tsirkin

Quoted 13 lines

Quoted 13 lines · Paolo Abeni

Quoted 9 lines

Quoted 9 lines

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 <[email protected]>

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

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?

Quoted 34 lines · Michael S. Tsirkin

Quoted 33 lines · Jason Wang

Quoted 31 lines · Michael S. Tsirkin

Quoted 15 lines · Jason Wang

Quoted 15 lines · Michael S. Tsirkin

Quoted 13 lines

Quoted 13 lines · Paolo Abeni

Quoted 9 lines

Quoted 9 lines

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 <[email protected]>

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 2 lines

Quoted 2 lines

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

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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).

Quoted 40 lines · Jason Wang

Quoted 37 lines · Michael S. Tsirkin

Quoted 36 lines · Jason Wang

Quoted 29 lines · Michael S. Tsirkin

Quoted 15 lines · Jason Wang

Quoted 15 lines · Michael S. Tsirkin

Quoted 13 lines

Quoted 13 lines · Paolo Abeni

Quoted 9 lines

Quoted 9 lines

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 <[email protected]>

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

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?

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(-)

Signature · 1 line

1.8.3.1

Quoted 9 lines · 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.

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?

Quoted 25 lines · Michael S. Tsirkin

Quoted 9 lines · 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.

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.

Quoted 1 line

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

Quoted 26 lines · Eric Dumazet

Quoted 25 lines · Michael S. Tsirkin

Quoted 9 lines · 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.

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.

Quoted 4 lines

Quoted 1 line

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?

Quoted 2 lines

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

This definitely needs thought.

Quoted 1 line

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

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

Quoted 1 line

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

Again - more than a lock?

Quoted 2 lines

- Adding another set of atomic ops.

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

You really need to convince John Fastabend to work full time on the real thing

Quoted 2 lines · Eric Dumazet

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 ...

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Eric Dumazet

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.