| Message ID | 20180117230704.21949-1-f.fainelli@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: core: Expose number of link up/down transitions | expand |
On Wed, 17 Jan 2018 15:06:57 -0800, Florian Fainelli wrote:
> +What: /sys/class/net/<iface/carrier_changes
nit: s/<iface/<iface>/ ?
On Wed, Jan 17, 2018 at 03:06:57PM -0800, Florian Fainelli wrote: > From: David Decotigny <decot@googlers.com> > > Expose the number of times the link has been going UP or DOWN, and > update the "carrier_changes" counter to be the sum of these two events. > While at it, also update the sysfs-class-net documentation to cover: > carrier_changes (3.15), count_link_up (4.16) and count_link_down (4.16) What is the idea to have two separate counters? Can a delta between them be a bigger than 1? > > Signed-off-by: David Decotigny <decot@googlers.com> > [Florian: > * rebase > * add documentation > * merge carrier_changes with up/down counters] > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > Documentation/ABI/testing/sysfs-class-net | 24 ++++++++++++++++++++++++ > include/linux/netdevice.h | 6 ++++-- > include/uapi/linux/if_link.h | 2 ++ > net/core/net-sysfs.c | 23 ++++++++++++++++++++++- > net/core/rtnetlink.c | 13 +++++++++++-- > net/sched/sch_generic.c | 4 ++-- > 6 files changed, 65 insertions(+), 7 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-net b/Documentation/ABI/testing/sysfs-class-net > index 6856da99b6f7..e4b0d5157305 100644 > --- a/Documentation/ABI/testing/sysfs-class-net > +++ b/Documentation/ABI/testing/sysfs-class-net > @@ -259,3 +259,27 @@ Contact: netdev@vger.kernel.org > Description: > Symbolic link to the PHY device this network device is attached > to. > + > +What: /sys/class/net/<iface/carrier_changes > +Date: Mar 2014 > +KernelVersion: 3.15 > +Contact: netdev@vger.kernel.org > +Description: > + 32-bit unsigned integer counting the number of times the link has > + seen a change from UP to DOWN and vice versa > + > +What: /sys/class/net/<iface/count_link_up > +Date: Jan 2018 > +KernelVersion: 4.16 > +Contact: netdev@vger.kernel.org > +Description: > + 32-bit unsigned integer counting the number of times the link has > + been up > + > +What: /sys/class/net/<iface/count_link_down > +Date: Jan 2018 > +KernelVersion: 4.16 > +Contact: netdev@vger.kernel.org > +Description: > + 32-bit unsigned integer counting the number of times the link has > + been down > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index ed0799a12bf2..28f68f7513d0 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1680,8 +1680,6 @@ struct net_device { > unsigned long base_addr; > int irq; > > - atomic_t carrier_changes; > - > /* > * Some hardware also needs these fields (state,dev_list, > * napi_list,unreg_list,close_list) but they are not > @@ -1719,6 +1717,10 @@ struct net_device { > atomic_long_t tx_dropped; > atomic_long_t rx_nohandler; > > + /* Stats to monitor link on/off, flapping */ > + atomic_t count_link_up; > + atomic_t count_link_down; > + > #ifdef CONFIG_WIRELESS_EXT > const struct iw_handler_def *wireless_handlers; > struct iw_public_data *wireless_data; > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > index f8f04fed6186..6e44b0674ba4 100644 > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -161,6 +161,8 @@ enum { > IFLA_EVENT, > IFLA_NEW_NETNSID, > IFLA_IF_NETNSID, > + IFLA_COUNT_LINK_UP, > + IFLA_COUNT_LINK_DOWN, > __IFLA_MAX > }; > > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c > index 7bf8b85ade16..9f732c3dc2ce 100644 > --- a/net/core/net-sysfs.c > +++ b/net/core/net-sysfs.c > @@ -295,10 +295,29 @@ static ssize_t carrier_changes_show(struct device *dev, > struct net_device *netdev = to_net_dev(dev); > > return sprintf(buf, fmt_dec, > - atomic_read(&netdev->carrier_changes)); > + atomic_read(&netdev->count_link_up) + > + atomic_read(&netdev->count_link_down)); > } > static DEVICE_ATTR_RO(carrier_changes); > > +static ssize_t count_link_up_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct net_device *netdev = to_net_dev(dev); > + > + return sprintf(buf, fmt_dec, atomic_read(&netdev->count_link_up)); > +} > +static DEVICE_ATTR_RO(count_link_up); > + > +static ssize_t count_link_down_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct net_device *netdev = to_net_dev(dev); > + > + return sprintf(buf, fmt_dec, atomic_read(&netdev->count_link_down)); > +} > +static DEVICE_ATTR_RO(count_link_down); > + > /* read-write attributes */ > > static int change_mtu(struct net_device *dev, unsigned long new_mtu) > @@ -547,6 +566,8 @@ static struct attribute *net_class_attrs[] __ro_after_init = { > &dev_attr_phys_port_name.attr, > &dev_attr_phys_switch_id.attr, > &dev_attr_proto_down.attr, > + &dev_attr_count_link_up.attr, > + &dev_attr_count_link_down.attr, > NULL, > }; > ATTRIBUTE_GROUPS(net_class); > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 16d644a4f974..68f69a956713 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -990,6 +990,8 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev, > + nla_total_size(4) /* IFLA_NEW_NETNSID */ > + nla_total_size(1) /* IFLA_PROTO_DOWN */ > + nla_total_size(4) /* IFLA_IF_NETNSID */ > + + nla_total_size(4) /* IFLA_COUNT_LINK_UP */ > + + nla_total_size(4) /* IFLA_COUNT_LINK_DOWN */ > + 0; > } > > @@ -1551,8 +1553,13 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, > nla_put_string(skb, IFLA_QDISC, dev->qdisc->ops->id)) || > nla_put_ifalias(skb, dev) || > nla_put_u32(skb, IFLA_CARRIER_CHANGES, > - atomic_read(&dev->carrier_changes)) || > - nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down)) > + atomic_read(&dev->count_link_up) + > + atomic_read(&dev->count_link_down)) || > + nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down) || > + nla_put_u32(skb, IFLA_COUNT_LINK_UP, > + atomic_read(&dev->count_link_up)) || > + nla_put_u32(skb, IFLA_COUNT_LINK_DOWN, > + atomic_read(&dev->count_link_down))) > goto nla_put_failure; > > if (event != IFLA_EVENT_NONE) { > @@ -1656,6 +1663,8 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = { > [IFLA_EVENT] = { .type = NLA_U32 }, > [IFLA_GROUP] = { .type = NLA_U32 }, > [IFLA_IF_NETNSID] = { .type = NLA_S32 }, > + [IFLA_COUNT_LINK_UP] = { .type = NLA_U32 }, > + [IFLA_COUNT_LINK_DOWN] = { .type = NLA_U32 }, > }; > > static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = { > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > index ef8b4ecde2ac..28941636afa3 100644 > --- a/net/sched/sch_generic.c > +++ b/net/sched/sch_generic.c > @@ -510,7 +510,7 @@ void netif_carrier_on(struct net_device *dev) > if (test_and_clear_bit(__LINK_STATE_NOCARRIER, &dev->state)) { > if (dev->reg_state == NETREG_UNINITIALIZED) > return; > - atomic_inc(&dev->carrier_changes); > + atomic_inc(&dev->count_link_up); > linkwatch_fire_event(dev); > if (netif_running(dev)) > __netdev_watchdog_up(dev); > @@ -529,7 +529,7 @@ void netif_carrier_off(struct net_device *dev) > if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) { > if (dev->reg_state == NETREG_UNINITIALIZED) > return; > - atomic_inc(&dev->carrier_changes); > + atomic_inc(&dev->count_link_down); > linkwatch_fire_event(dev); > } > } > -- > 2.14.1 >
Thu, Jan 18, 2018 at 12:06:57AM CET, f.fainelli@gmail.com wrote: >From: David Decotigny <decot@googlers.com> > >Expose the number of times the link has been going UP or DOWN, and >update the "carrier_changes" counter to be the sum of these two events. >While at it, also update the sysfs-class-net documentation to cover: >carrier_changes (3.15), count_link_up (4.16) and count_link_down (4.16) > >Signed-off-by: David Decotigny <decot@googlers.com> [...] >@@ -161,6 +161,8 @@ enum { > IFLA_EVENT, > IFLA_NEW_NETNSID, > IFLA_IF_NETNSID, >+ IFLA_COUNT_LINK_UP, >+ IFLA_COUNT_LINK_DOWN, IFLA_LINK_UP_COUNT, IFLA_LINK_DOWN_COUNT, would sound a bit nicer to me. > __IFLA_MAX > };
> What is the idea to have two separate counters? Can a delta between them > be a bigger than 1? Yes, it can. These counters are incremented in netif_carrier_on() / netif_carrier_off(). They are not always called in pairs and they can be called multiple times for the same event. The phylib will call them when it notices the PHY saying the link is down/up, and the MAC driver sometimes also calls them. Andrew
On 1/17/18 3:52 PM, Jiri Pirko wrote: > Thu, Jan 18, 2018 at 12:06:57AM CET, f.fainelli@gmail.com wrote: >> From: David Decotigny <decot@googlers.com> >> >> Expose the number of times the link has been going UP or DOWN, and >> update the "carrier_changes" counter to be the sum of these two events. >> While at it, also update the sysfs-class-net documentation to cover: >> carrier_changes (3.15), count_link_up (4.16) and count_link_down (4.16) >> >> Signed-off-by: David Decotigny <decot@googlers.com> > > [...] > > >> @@ -161,6 +161,8 @@ enum { >> IFLA_EVENT, >> IFLA_NEW_NETNSID, >> IFLA_IF_NETNSID, >> + IFLA_COUNT_LINK_UP, >> + IFLA_COUNT_LINK_DOWN, > > > IFLA_LINK_UP_COUNT, > IFLA_LINK_DOWN_COUNT, > > would sound a bit nicer to me. given existing IFLA_LINK_* attributes how about IFLA_CARRIER_{UP,DOWN}_COUNT?
On Thu, Jan 18, 2018 at 01:06:52AM +0100, Andrew Lunn wrote: > > What is the idea to have two separate counters? Can a delta between them > > be a bigger than 1? > > Yes, it can. > > These counters are incremented in netif_carrier_on() / > netif_carrier_off(). They are not always called in pairs and they can > be called multiple times for the same event. The phylib will call them > when it notices the PHY saying the link is down/up, and the MAC driver > sometimes also calls them. We check the __LINK_STATE_NOCARRIER bit before changing these counters, so if we call netif_carrier_on() twice, the counter will be incrimented only by one, will it not? void netif_carrier_on(struct net_device *dev) if (test_and_clear_bit(__LINK_STATE_NOCARRIER, &dev->state)) { atomic_inc(&dev->carrier_changes); ... void netif_carrier_off(struct net_device *dev) if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) { atomic_inc(&dev->carrier_changes); > > Andrew
On 1/17/18 3:06 PM, Florian Fainelli wrote: > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c > index 7bf8b85ade16..9f732c3dc2ce 100644 > --- a/net/core/net-sysfs.c > +++ b/net/core/net-sysfs.c > @@ -295,10 +295,29 @@ static ssize_t carrier_changes_show(struct device *dev, > struct net_device *netdev = to_net_dev(dev); > > return sprintf(buf, fmt_dec, > - atomic_read(&netdev->carrier_changes)); > + atomic_read(&netdev->count_link_up) + > + atomic_read(&netdev->count_link_down)); > } > static DEVICE_ATTR_RO(carrier_changes); > > +static ssize_t count_link_up_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct net_device *netdev = to_net_dev(dev); > + > + return sprintf(buf, fmt_dec, atomic_read(&netdev->count_link_up)); > +} > +static DEVICE_ATTR_RO(count_link_up); > + > +static ssize_t count_link_down_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct net_device *netdev = to_net_dev(dev); > + > + return sprintf(buf, fmt_dec, atomic_read(&netdev->count_link_down)); > +} > +static DEVICE_ATTR_RO(count_link_down); > + > /* read-write attributes */ > > static int change_mtu(struct net_device *dev, unsigned long new_mtu) > @@ -547,6 +566,8 @@ static struct attribute *net_class_attrs[] __ro_after_init = { > &dev_attr_phys_port_name.attr, > &dev_attr_phys_switch_id.attr, > &dev_attr_proto_down.attr, > + &dev_attr_count_link_up.attr, > + &dev_attr_count_link_down.attr, > NULL, > }; > ATTRIBUTE_GROUPS(net_class); Personally, I do not like adding any more sysfs files. As discussed in the past sysfs is a huge contributor to the overhead of netdevs.
diff --git a/Documentation/ABI/testing/sysfs-class-net b/Documentation/ABI/testing/sysfs-class-net index 6856da99b6f7..e4b0d5157305 100644 --- a/Documentation/ABI/testing/sysfs-class-net +++ b/Documentation/ABI/testing/sysfs-class-net @@ -259,3 +259,27 @@ Contact: netdev@vger.kernel.org Description: Symbolic link to the PHY device this network device is attached to. + +What: /sys/class/net/<iface/carrier_changes +Date: Mar 2014 +KernelVersion: 3.15 +Contact: netdev@vger.kernel.org +Description: + 32-bit unsigned integer counting the number of times the link has + seen a change from UP to DOWN and vice versa + +What: /sys/class/net/<iface/count_link_up +Date: Jan 2018 +KernelVersion: 4.16 +Contact: netdev@vger.kernel.org +Description: + 32-bit unsigned integer counting the number of times the link has + been up + +What: /sys/class/net/<iface/count_link_down +Date: Jan 2018 +KernelVersion: 4.16 +Contact: netdev@vger.kernel.org +Description: + 32-bit unsigned integer counting the number of times the link has + been down diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index ed0799a12bf2..28f68f7513d0 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1680,8 +1680,6 @@ struct net_device { unsigned long base_addr; int irq; - atomic_t carrier_changes; - /* * Some hardware also needs these fields (state,dev_list, * napi_list,unreg_list,close_list) but they are not @@ -1719,6 +1717,10 @@ struct net_device { atomic_long_t tx_dropped; atomic_long_t rx_nohandler; + /* Stats to monitor link on/off, flapping */ + atomic_t count_link_up; + atomic_t count_link_down; + #ifdef CONFIG_WIRELESS_EXT const struct iw_handler_def *wireless_handlers; struct iw_public_data *wireless_data; diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index f8f04fed6186..6e44b0674ba4 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -161,6 +161,8 @@ enum { IFLA_EVENT, IFLA_NEW_NETNSID, IFLA_IF_NETNSID, + IFLA_COUNT_LINK_UP, + IFLA_COUNT_LINK_DOWN, __IFLA_MAX }; diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 7bf8b85ade16..9f732c3dc2ce 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -295,10 +295,29 @@ static ssize_t carrier_changes_show(struct device *dev, struct net_device *netdev = to_net_dev(dev); return sprintf(buf, fmt_dec, - atomic_read(&netdev->carrier_changes)); + atomic_read(&netdev->count_link_up) + + atomic_read(&netdev->count_link_down)); } static DEVICE_ATTR_RO(carrier_changes); +static ssize_t count_link_up_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct net_device *netdev = to_net_dev(dev); + + return sprintf(buf, fmt_dec, atomic_read(&netdev->count_link_up)); +} +static DEVICE_ATTR_RO(count_link_up); + +static ssize_t count_link_down_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct net_device *netdev = to_net_dev(dev); + + return sprintf(buf, fmt_dec, atomic_read(&netdev->count_link_down)); +} +static DEVICE_ATTR_RO(count_link_down); + /* read-write attributes */ static int change_mtu(struct net_device *dev, unsigned long new_mtu) @@ -547,6 +566,8 @@ static struct attribute *net_class_attrs[] __ro_after_init = { &dev_attr_phys_port_name.attr, &dev_attr_phys_switch_id.attr, &dev_attr_proto_down.attr, + &dev_attr_count_link_up.attr, + &dev_attr_count_link_down.attr, NULL, }; ATTRIBUTE_GROUPS(net_class); diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 16d644a4f974..68f69a956713 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -990,6 +990,8 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev, + nla_total_size(4) /* IFLA_NEW_NETNSID */ + nla_total_size(1) /* IFLA_PROTO_DOWN */ + nla_total_size(4) /* IFLA_IF_NETNSID */ + + nla_total_size(4) /* IFLA_COUNT_LINK_UP */ + + nla_total_size(4) /* IFLA_COUNT_LINK_DOWN */ + 0; } @@ -1551,8 +1553,13 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, nla_put_string(skb, IFLA_QDISC, dev->qdisc->ops->id)) || nla_put_ifalias(skb, dev) || nla_put_u32(skb, IFLA_CARRIER_CHANGES, - atomic_read(&dev->carrier_changes)) || - nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down)) + atomic_read(&dev->count_link_up) + + atomic_read(&dev->count_link_down)) || + nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down) || + nla_put_u32(skb, IFLA_COUNT_LINK_UP, + atomic_read(&dev->count_link_up)) || + nla_put_u32(skb, IFLA_COUNT_LINK_DOWN, + atomic_read(&dev->count_link_down))) goto nla_put_failure; if (event != IFLA_EVENT_NONE) { @@ -1656,6 +1663,8 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = { [IFLA_EVENT] = { .type = NLA_U32 }, [IFLA_GROUP] = { .type = NLA_U32 }, [IFLA_IF_NETNSID] = { .type = NLA_S32 }, + [IFLA_COUNT_LINK_UP] = { .type = NLA_U32 }, + [IFLA_COUNT_LINK_DOWN] = { .type = NLA_U32 }, }; static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = { diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index ef8b4ecde2ac..28941636afa3 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -510,7 +510,7 @@ void netif_carrier_on(struct net_device *dev) if (test_and_clear_bit(__LINK_STATE_NOCARRIER, &dev->state)) { if (dev->reg_state == NETREG_UNINITIALIZED) return; - atomic_inc(&dev->carrier_changes); + atomic_inc(&dev->count_link_up); linkwatch_fire_event(dev); if (netif_running(dev)) __netdev_watchdog_up(dev); @@ -529,7 +529,7 @@ void netif_carrier_off(struct net_device *dev) if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) { if (dev->reg_state == NETREG_UNINITIALIZED) return; - atomic_inc(&dev->carrier_changes); + atomic_inc(&dev->count_link_down); linkwatch_fire_event(dev); } }