* [PATCH RFC 0/2] tun: lockless xmit
@ 2016-04-13 9:04 Paolo Abeni
2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni
` (2 more replies)
0 siblings, 3 replies; 29+ messages in thread
From: Paolo Abeni @ 2016-04-13 9:04 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Michael S. Tsirkin, Hannes Frederic Sowa,
Eric W. Biederman, Greg Kurz, Jason Wang
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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 9:04 [PATCH RFC 0/2] tun: lockless xmit Paolo Abeni @ 2016-04-13 9:04 ` Paolo Abeni 2016-04-13 9:41 ` Michael S. Tsirkin ` (4 more replies) 2016-04-13 9:04 ` [PATCH RFC 2/2] tun: don't set a default qdisc Paolo Abeni 2016-04-13 11:08 ` [PATCH RFC 0/2] tun: lockless xmit Michael S. Tsirkin 2 siblings, 5 replies; 29+ messages in thread From: Paolo Abeni @ 2016-04-13 9:04 UTC (permalink / raw) To: netdev Cc: David S. Miller, Michael S. Tsirkin, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni @ 2016-04-13 9:41 ` Michael S. Tsirkin 2016-04-13 9:48 ` Hannes Frederic Sowa 2016-04-13 12:52 ` Eric Dumazet ` (3 subsequent siblings) 4 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-13 9:41 UTC (permalink / raw) To: Paolo Abeni Cc: netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang, Christian Borntraeger, Herbert Xu 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 9:41 ` Michael S. Tsirkin @ 2016-04-13 9:48 ` Hannes Frederic Sowa 2016-04-13 12:57 ` Michael S. Tsirkin 0 siblings, 1 reply; 29+ messages in thread From: Hannes Frederic Sowa @ 2016-04-13 9:48 UTC (permalink / raw) To: Michael S. Tsirkin, Paolo Abeni Cc: netdev, David S. Miller, Eric W. Biederman, Greg Kurz, Jason Wang, Christian Borntraeger, Herbert Xu 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 9:48 ` Hannes Frederic Sowa @ 2016-04-13 12:57 ` Michael S. Tsirkin 2016-04-13 13:27 ` Eric Dumazet 0 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-13 12:57 UTC (permalink / raw) To: Hannes Frederic Sowa Cc: Paolo Abeni, netdev, David S. Miller, Eric W. Biederman, Greg Kurz, Jason Wang, Christian Borntraeger, Herbert Xu 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 12:57 ` Michael S. Tsirkin @ 2016-04-13 13:27 ` Eric Dumazet 2016-04-13 13:54 ` Michael S. Tsirkin 0 siblings, 1 reply; 29+ messages in thread From: Eric Dumazet @ 2016-04-13 13:27 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Hannes Frederic Sowa, Paolo Abeni, netdev, David S. Miller, Eric W. Biederman, Greg Kurz, Jason Wang, Christian Borntraeger, Herbert Xu 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 13:27 ` Eric Dumazet @ 2016-04-13 13:54 ` Michael S. Tsirkin 2016-04-13 14:39 ` Eric Dumazet 0 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-13 13:54 UTC (permalink / raw) To: Eric Dumazet Cc: Hannes Frederic Sowa, Paolo Abeni, netdev, David S. Miller, Eric W. Biederman, Greg Kurz, Jason Wang, Christian Borntraeger, Herbert Xu 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 13:54 ` Michael S. Tsirkin @ 2016-04-13 14:39 ` Eric Dumazet 0 siblings, 0 replies; 29+ messages in thread From: Eric Dumazet @ 2016-04-13 14:39 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Hannes Frederic Sowa, Paolo Abeni, netdev, David S. Miller, Eric W. Biederman, Greg Kurz, Jason Wang, Christian Borntraeger, Herbert Xu 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 :( ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni 2016-04-13 9:41 ` Michael S. Tsirkin @ 2016-04-13 12:52 ` Eric Dumazet 2016-04-13 14:26 ` Sergei Shtylyov ` (2 subsequent siblings) 4 siblings, 0 replies; 29+ messages in thread From: Eric Dumazet @ 2016-04-13 12:52 UTC (permalink / raw) To: Paolo Abeni Cc: netdev, David S. Miller, Michael S. Tsirkin, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni 2016-04-13 9:41 ` Michael S. Tsirkin 2016-04-13 12:52 ` Eric Dumazet @ 2016-04-13 14:26 ` Sergei Shtylyov 2016-04-14 6:50 ` Jason Wang 2016-04-14 10:27 ` Hannes Frederic Sowa 4 siblings, 0 replies; 29+ messages in thread From: Sergei Shtylyov @ 2016-04-13 14:26 UTC (permalink / raw) To: Paolo Abeni, netdev Cc: David S. Miller, Michael S. Tsirkin, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni ` (2 preceding siblings ...) 2016-04-13 14:26 ` Sergei Shtylyov @ 2016-04-14 6:50 ` Jason Wang 2016-04-14 10:27 ` Hannes Frederic Sowa 4 siblings, 0 replies; 29+ messages in thread From: Jason Wang @ 2016-04-14 6:50 UTC (permalink / raw) To: Paolo Abeni, netdev Cc: David S. Miller, Michael S. Tsirkin, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 1/2] tun: don't require serialization lock on tx 2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni ` (3 preceding siblings ...) 2016-04-14 6:50 ` Jason Wang @ 2016-04-14 10:27 ` Hannes Frederic Sowa 4 siblings, 0 replies; 29+ messages in thread From: Hannes Frederic Sowa @ 2016-04-14 10:27 UTC (permalink / raw) To: Paolo Abeni, netdev Cc: David S. Miller, Michael S. Tsirkin, Eric W. Biederman, Greg Kurz, Jason Wang 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-13 9:04 [PATCH RFC 0/2] tun: lockless xmit Paolo Abeni 2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni @ 2016-04-13 9:04 ` Paolo Abeni 2016-04-13 10:26 ` Michael S. Tsirkin 2016-04-13 11:08 ` [PATCH RFC 0/2] tun: lockless xmit Michael S. Tsirkin 2 siblings, 1 reply; 29+ messages in thread From: Paolo Abeni @ 2016-04-13 9:04 UTC (permalink / raw) To: netdev Cc: David S. Miller, Michael S. Tsirkin, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-13 9:04 ` [PATCH RFC 2/2] tun: don't set a default qdisc Paolo Abeni @ 2016-04-13 10:26 ` Michael S. Tsirkin 2016-04-13 15:22 ` David Miller 2016-04-14 6:49 ` Jason Wang 0 siblings, 2 replies; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-13 10:26 UTC (permalink / raw) To: Paolo Abeni Cc: netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-13 10:26 ` Michael S. Tsirkin @ 2016-04-13 15:22 ` David Miller 2016-04-14 6:49 ` Jason Wang 1 sibling, 0 replies; 29+ messages in thread From: David Miller @ 2016-04-13 15:22 UTC (permalink / raw) To: mst; +Cc: pabeni, netdev, hannes, ebiederm, gkurz, jasowang 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-13 10:26 ` Michael S. Tsirkin 2016-04-13 15:22 ` David Miller @ 2016-04-14 6:49 ` Jason Wang 2016-04-14 9:05 ` Michael S. Tsirkin 1 sibling, 1 reply; 29+ messages in thread From: Jason Wang @ 2016-04-14 6:49 UTC (permalink / raw) To: Michael S. Tsirkin, Paolo Abeni Cc: netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-14 6:49 ` Jason Wang @ 2016-04-14 9:05 ` Michael S. Tsirkin 2016-04-14 9:07 ` Jason Wang 0 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-14 9:05 UTC (permalink / raw) To: Jason Wang Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-14 9:05 ` Michael S. Tsirkin @ 2016-04-14 9:07 ` Jason Wang 2016-04-14 9:10 ` Michael S. Tsirkin 0 siblings, 1 reply; 29+ messages in thread From: Jason Wang @ 2016-04-14 9:07 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-14 9:07 ` Jason Wang @ 2016-04-14 9:10 ` Michael S. Tsirkin 2016-04-14 9:21 ` Jason Wang 0 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-14 9:10 UTC (permalink / raw) To: Jason Wang Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-14 9:10 ` Michael S. Tsirkin @ 2016-04-14 9:21 ` Jason Wang 2016-04-14 10:01 ` Michael S. Tsirkin 0 siblings, 1 reply; 29+ messages in thread From: Jason Wang @ 2016-04-14 9:21 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz 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). ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-14 9:21 ` Jason Wang @ 2016-04-14 10:01 ` Michael S. Tsirkin 2016-04-14 10:09 ` Hannes Frederic Sowa 0 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-14 10:01 UTC (permalink / raw) To: Jason Wang Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 2/2] tun: don't set a default qdisc 2016-04-14 10:01 ` Michael S. Tsirkin @ 2016-04-14 10:09 ` Hannes Frederic Sowa 0 siblings, 0 replies; 29+ messages in thread From: Hannes Frederic Sowa @ 2016-04-14 10:09 UTC (permalink / raw) To: Michael S. Tsirkin, Jason Wang Cc: Paolo Abeni, netdev, David S. Miller, Eric W. Biederman, Greg Kurz 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 0/2] tun: lockless xmit 2016-04-13 9:04 [PATCH RFC 0/2] tun: lockless xmit Paolo Abeni 2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni 2016-04-13 9:04 ` [PATCH RFC 2/2] tun: don't set a default qdisc Paolo Abeni @ 2016-04-13 11:08 ` Michael S. Tsirkin 2016-04-13 12:50 ` Eric Dumazet 2 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-13 11:08 UTC (permalink / raw) To: Paolo Abeni Cc: netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 0/2] tun: lockless xmit 2016-04-13 11:08 ` [PATCH RFC 0/2] tun: lockless xmit Michael S. Tsirkin @ 2016-04-13 12:50 ` Eric Dumazet 2016-04-13 12:56 ` Michael S. Tsirkin 0 siblings, 1 reply; 29+ messages in thread From: Eric Dumazet @ 2016-04-13 12:50 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 0/2] tun: lockless xmit 2016-04-13 12:50 ` Eric Dumazet @ 2016-04-13 12:56 ` Michael S. Tsirkin 2016-04-13 13:09 ` Eric Dumazet 0 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-13 12:56 UTC (permalink / raw) To: Eric Dumazet Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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 > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 0/2] tun: lockless xmit 2016-04-13 12:56 ` Michael S. Tsirkin @ 2016-04-13 13:09 ` Eric Dumazet 2016-04-13 13:17 ` Michael S. Tsirkin 0 siblings, 1 reply; 29+ messages in thread From: Eric Dumazet @ 2016-04-13 13:09 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 0/2] tun: lockless xmit 2016-04-13 13:09 ` Eric Dumazet @ 2016-04-13 13:17 ` Michael S. Tsirkin 2016-04-13 13:43 ` Eric Dumazet 0 siblings, 1 reply; 29+ messages in thread From: Michael S. Tsirkin @ 2016-04-13 13:17 UTC (permalink / raw) To: Eric Dumazet Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 0/2] tun: lockless xmit 2016-04-13 13:17 ` Michael S. Tsirkin @ 2016-04-13 13:43 ` Eric Dumazet 2016-04-13 16:42 ` Eric Dumazet 0 siblings, 1 reply; 29+ messages in thread From: Eric Dumazet @ 2016-04-13 13:43 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH RFC 0/2] tun: lockless xmit 2016-04-13 13:43 ` Eric Dumazet @ 2016-04-13 16:42 ` Eric Dumazet 0 siblings, 0 replies; 29+ messages in thread From: Eric Dumazet @ 2016-04-13 16:42 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Abeni, netdev, David S. Miller, Hannes Frederic Sowa, Eric W. Biederman, Greg Kurz, Jason Wang 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-04-14 10:27 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-13 9:04 [PATCH RFC 0/2] tun: lockless xmit Paolo Abeni 2016-04-13 9:04 ` [PATCH RFC 1/2] tun: don't require serialization lock on tx Paolo Abeni 2016-04-13 9:41 ` Michael S. Tsirkin 2016-04-13 9:48 ` Hannes Frederic Sowa 2016-04-13 12:57 ` Michael S. Tsirkin 2016-04-13 13:27 ` Eric Dumazet 2016-04-13 13:54 ` Michael S. Tsirkin 2016-04-13 14:39 ` Eric Dumazet 2016-04-13 12:52 ` Eric Dumazet 2016-04-13 14:26 ` Sergei Shtylyov 2016-04-14 6:50 ` Jason Wang 2016-04-14 10:27 ` Hannes Frederic Sowa 2016-04-13 9:04 ` [PATCH RFC 2/2] tun: don't set a default qdisc Paolo Abeni 2016-04-13 10:26 ` Michael S. Tsirkin 2016-04-13 15:22 ` David Miller 2016-04-14 6:49 ` Jason Wang 2016-04-14 9:05 ` Michael S. Tsirkin 2016-04-14 9:07 ` Jason Wang 2016-04-14 9:10 ` Michael S. Tsirkin 2016-04-14 9:21 ` Jason Wang 2016-04-14 10:01 ` Michael S. Tsirkin 2016-04-14 10:09 ` Hannes Frederic Sowa 2016-04-13 11:08 ` [PATCH RFC 0/2] tun: lockless xmit Michael S. Tsirkin 2016-04-13 12:50 ` Eric Dumazet 2016-04-13 12:56 ` Michael S. Tsirkin 2016-04-13 13:09 ` Eric Dumazet 2016-04-13 13:17 ` Michael S. Tsirkin 2016-04-13 13:43 ` Eric Dumazet 2016-04-13 16:42 ` Eric Dumazet
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.