| Message ID | 425e8f69ffd8fe315ef9d3cc678519c7060fb2e0.1460393493.git.pabeni@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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>
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?
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
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?
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
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 :(
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>
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>
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);
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(-)