| Message ID | 20091110175647.615305929@vyatta.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi, On Tue, Nov 10, 2009 at 10:50:53AM -0800, Stephen Hemminger wrote: > On Tue, 10 Nov 2009 19:43:21 +0100 > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > Stephen Hemminger a écrit : > > > When showing device statistics use RCU rather than read_lock(&dev_base_lock) > > > Compile tested only. > > > > > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > > > > > --- a/net/decnet/dn_dev.c 2009-11-10 09:30:55.557376454 -0800 > > > +++ b/net/decnet/dn_dev.c 2009-11-10 09:40:03.847005394 -0800 > > > @@ -856,9 +856,7 @@ int dn_dev_bind_default(__le16 *addr) > > > dev = dn_dev_get_default(); > > > last_chance: > > > if (dev) { > > > - read_lock(&dev_base_lock); > > > rv = dn_dev_get_first(dev, addr); > > > - read_unlock(&dev_base_lock); > > > dev_put(dev); > > > if (rv == 0 || dev == init_net.loopback_dev) > > > return rv; > > > @@ -1323,18 +1321,18 @@ static inline int is_dn_dev(struct net_d > > > > > > I dont understand this part. Why previous locking can be avoided ? > > dn_dev_get_default acquires a reference on dev so the device can > not go away. > > It could be the original author meant to ensure the address list > doesn't change. If so, then rtnl_lock() should have been used. The original author has tried to remember what he was thinking when he wrote this code :-) I think you are right that it should be rtnl_lock() as we don't want the address list changing at this point. On the other hand I notice also that other bits of the code seem to be using dev_base_lock too. Maybe this is a hang over from another era? I'll have to refresh my memory some more before I can give a verdict on that I'm afraid, Steve. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Hemminger a écrit : > When showing device statistics use RCU rather than read_lock(&dev_base_lock) > Compile tested only. > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > --- a/net/decnet/dn_dev.c 2009-11-10 09:30:55.557376454 -0800 > +++ b/net/decnet/dn_dev.c 2009-11-10 09:40:03.847005394 -0800 > @@ -856,9 +856,7 @@ int dn_dev_bind_default(__le16 *addr) > dev = dn_dev_get_default(); > last_chance: > if (dev) { > - read_lock(&dev_base_lock); > rv = dn_dev_get_first(dev, addr); > - read_unlock(&dev_base_lock); > dev_put(dev); > if (rv == 0 || dev == init_net.loopback_dev) > return rv; > @@ -1323,18 +1321,18 @@ static inline int is_dn_dev(struct net_d I dont understand this part. Why previous locking can be avoided ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 10 Nov 2009 19:43:21 +0100 Eric Dumazet <eric.dumazet@gmail.com> wrote: > Stephen Hemminger a écrit : > > When showing device statistics use RCU rather than read_lock(&dev_base_lock) > > Compile tested only. > > > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > > > --- a/net/decnet/dn_dev.c 2009-11-10 09:30:55.557376454 -0800 > > +++ b/net/decnet/dn_dev.c 2009-11-10 09:40:03.847005394 -0800 > > @@ -856,9 +856,7 @@ int dn_dev_bind_default(__le16 *addr) > > dev = dn_dev_get_default(); > > last_chance: > > if (dev) { > > - read_lock(&dev_base_lock); > > rv = dn_dev_get_first(dev, addr); > > - read_unlock(&dev_base_lock); > > dev_put(dev); > > if (rv == 0 || dev == init_net.loopback_dev) > > return rv; > > @@ -1323,18 +1321,18 @@ static inline int is_dn_dev(struct net_d > > > I dont understand this part. Why previous locking can be avoided ? dn_dev_get_default acquires a reference on dev so the device can not go away. It could be the original author meant to ensure the address list doesn't change. If so, then rtnl_lock() should have been used. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Hemminger a écrit : > On Tue, 10 Nov 2009 19:43:21 +0100 > Eric Dumazet <eric.dumazet@gmail.com> wrote: > >> Stephen Hemminger a écrit : >>> When showing device statistics use RCU rather than read_lock(&dev_base_lock) >>> Compile tested only. >>> >>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> >>> >>> --- a/net/decnet/dn_dev.c 2009-11-10 09:30:55.557376454 -0800 >>> +++ b/net/decnet/dn_dev.c 2009-11-10 09:40:03.847005394 -0800 >>> @@ -856,9 +856,7 @@ int dn_dev_bind_default(__le16 *addr) >>> dev = dn_dev_get_default(); >>> last_chance: >>> if (dev) { >>> - read_lock(&dev_base_lock); >>> rv = dn_dev_get_first(dev, addr); >>> - read_unlock(&dev_base_lock); >>> dev_put(dev); >>> if (rv == 0 || dev == init_net.loopback_dev) >>> return rv; >>> @@ -1323,18 +1321,18 @@ static inline int is_dn_dev(struct net_d >> >> I dont understand this part. Why previous locking can be avoided ? > > dn_dev_get_default acquires a reference on dev so the device can > not go away. > > It could be the original author meant to ensure the address list > doesn't change. If so, then rtnl_lock() should have been used. Hmm... I spot some bugs during my previous round of patches, so maybe this is a real bug... We should double check. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Stephen Hemminger <shemminger@vyatta.com> Date: Tue, 10 Nov 2009 09:54:53 -0800 > When showing device statistics use RCU rather than read_lock(&dev_base_lock) > Compile tested only. > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/net/decnet/dn_dev.c 2009-11-10 09:30:55.557376454 -0800 +++ b/net/decnet/dn_dev.c 2009-11-10 09:40:03.847005394 -0800 @@ -856,9 +856,7 @@ int dn_dev_bind_default(__le16 *addr) dev = dn_dev_get_default(); last_chance: if (dev) { - read_lock(&dev_base_lock); rv = dn_dev_get_first(dev, addr); - read_unlock(&dev_base_lock); dev_put(dev); if (rv == 0 || dev == init_net.loopback_dev) return rv; @@ -1323,18 +1321,18 @@ static inline int is_dn_dev(struct net_d } static void *dn_dev_seq_start(struct seq_file *seq, loff_t *pos) - __acquires(&dev_base_lock) + __acquires(rcu) { int i; struct net_device *dev; - read_lock(&dev_base_lock); + rcu_read_lock(); if (*pos == 0) return SEQ_START_TOKEN; i = 1; - for_each_netdev(&init_net, dev) { + for_each_netdev_rcu(&init_net, dev) { if (!is_dn_dev(dev)) continue; @@ -1355,7 +1353,7 @@ static void *dn_dev_seq_next(struct seq_ if (v == SEQ_START_TOKEN) dev = net_device_entry(&init_net.dev_base_head); - for_each_netdev_continue(&init_net, dev) { + for_each_netdev_continue_rcu(&init_net, dev) { if (!is_dn_dev(dev)) continue; @@ -1366,9 +1364,9 @@ static void *dn_dev_seq_next(struct seq_ } static void dn_dev_seq_stop(struct seq_file *seq, void *v) - __releases(&dev_base_lock) + __releases(rcu) { - read_unlock(&dev_base_lock); + rcu_read_unlock(); } static char *dn_type2asc(char type)
When showing device statistics use RCU rather than read_lock(&dev_base_lock) Compile tested only. Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>