[PATCH v1] net-sysfs: expose number of link up/down transitions

[PATCH v1] net-sysfs: expose number of link up/down transitions

[David Decotigny]

Tested:
  grep . /sys/class/net/*/count_link_*
  + ip link set dev X down/up

Signed-off-by: David Decotigny <decot@googlers.com>
---
 include/linux/netdevice.h |  4 ++++
 net/core/net-sysfs.c      | 18 ++++++++++++++++++
 net/sched/sch_generic.c   |  2 ++
 3 files changed, 24 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4b6d12c..cf52869 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1315,6 +1315,10 @@ struct net_device {
 					     * Do not use this in drivers.
 					     */
 
+	/* Stats to monitor link on/off, flapping */
+	atomic_t		count_link_up;
+	atomic_t		count_link_down;
+
 #ifdef CONFIG_WIRELESS_EXT
 	/* List of functions to handle Wireless Extensions (instead of ioctl).
 	 * See <net/iw_handler.h> for details. Jean II */
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index daed9a6..a65ac54 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -253,6 +253,22 @@ static ssize_t operstate_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(operstate);
 
+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 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_up);
+static DEVICE_ATTR_RO(count_link_down);
+
 /* read-write attributes */
 
 static int change_mtu(struct net_device *net, unsigned long new_mtu)
@@ -386,6 +402,8 @@ static struct attribute *net_class_attrs[] = {
 	&dev_attr_duplex.attr,
 	&dev_attr_dormant.attr,
 	&dev_attr_operstate.attr,
+	&dev_attr_count_link_up.attr,
+	&dev_attr_count_link_down.attr,
 	&dev_attr_ifalias.attr,
 	&dev_attr_carrier.attr,
 	&dev_attr_mtu.attr,
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index e82e43b..2d06943 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -310,6 +310,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->count_link_up);
 		linkwatch_fire_event(dev);
 		if (netif_running(dev))
 			__netdev_watchdog_up(dev);
@@ -328,6 +329,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->count_link_down);
 		linkwatch_fire_event(dev);
 	}
 }
-- 
1.9.1.423.g4596e3a

Re: [PATCH v1] net-sysfs: expose number of link up/down transitions

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

Hi David,

On Thu, 27 Mar 2014 21:35:00 -0700 David Decotigny <decot@googlers.com> wrote:
>
> Tested:
>   grep . /sys/class/net/*/count_link_*
>   + ip link set dev X down/up
> 
> Signed-off-by: David Decotigny <decot@googlers.com>

It is not a very enlightening commit message.  You should tell us why we
should add this to the kernel.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v1] net-sysfs: expose number of link up/down transitions

[Florian Fainelli]

2014-03-27 22:17 GMT-07:00 Stephen Rothwell <sfr@canb.auug.org.au>:
> Hi David,
>
> On Thu, 27 Mar 2014 21:35:00 -0700 David Decotigny <decot@googlers.com> wrote:
>>
>> Tested:
>>   grep . /sys/class/net/*/count_link_*
>>   + ip link set dev X down/up
>>
>> Signed-off-by: David Decotigny <decot@googlers.com>
>
> It is not a very enlightening commit message.  You should tell us why we
> should add this to the kernel.

This might be useful to detect unstable links for instance, by
measuring how many link transitions you get, which could indicate that
either your NIC or your link partner is at fault.

If possible this should come with a Documentation/ABI/testing/ entry,
quite a lot of network sysfs attributes are lacking one at the moment,
but there is no reason why this should continue.
--
Florian

Re: [PATCH v1] net-sysfs: expose number of link up/down transitions

[Jiri Pirko]

Fri, Mar 28, 2014 at 05:35:00AM CET, decot@googlers.com wrote:
>Tested:
>  grep . /sys/class/net/*/count_link_*
>  + ip link set dev X down/up
>
>Signed-off-by: David Decotigny <decot@googlers.com>
>---
> include/linux/netdevice.h |  4 ++++
> net/core/net-sysfs.c      | 18 ++++++++++++++++++
> net/sched/sch_generic.c   |  2 ++
> 3 files changed, 24 insertions(+)
>
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index 4b6d12c..cf52869 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -1315,6 +1315,10 @@ struct net_device {
> 					     * Do not use this in drivers.
> 					     */
> 
>+	/* Stats to monitor link on/off, flapping */
>+	atomic_t		count_link_up;
>+	atomic_t		count_link_down;
>+
> #ifdef CONFIG_WIRELESS_EXT
> 	/* List of functions to handle Wireless Extensions (instead of ioctl).
> 	 * See <net/iw_handler.h> for details. Jean II */
>diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>index daed9a6..a65ac54 100644
>--- a/net/core/net-sysfs.c
>+++ b/net/core/net-sysfs.c
>@@ -253,6 +253,22 @@ static ssize_t operstate_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(operstate);
> 
>+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 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_up);
>+static DEVICE_ATTR_RO(count_link_down);
>+


If this is exposed in sysfs, this needs to be exposed via RT netlink as
well.


> /* read-write attributes */
> 
> static int change_mtu(struct net_device *net, unsigned long new_mtu)
>@@ -386,6 +402,8 @@ static struct attribute *net_class_attrs[] = {
> 	&dev_attr_duplex.attr,
> 	&dev_attr_dormant.attr,
> 	&dev_attr_operstate.attr,
>+	&dev_attr_count_link_up.attr,
>+	&dev_attr_count_link_down.attr,
> 	&dev_attr_ifalias.attr,
> 	&dev_attr_carrier.attr,
> 	&dev_attr_mtu.attr,
>diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>index e82e43b..2d06943 100644
>--- a/net/sched/sch_generic.c
>+++ b/net/sched/sch_generic.c
>@@ -310,6 +310,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->count_link_up);
> 		linkwatch_fire_event(dev);
> 		if (netif_running(dev))
> 			__netdev_watchdog_up(dev);
>@@ -328,6 +329,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->count_link_down);
> 		linkwatch_fire_event(dev);
> 	}
> }
>-- 
>1.9.1.423.g4596e3a
>

Re: [PATCH v1] net-sysfs: expose number of link up/down transitions

[Florian Fainelli]

2014-03-27 23:25 GMT-07:00 Jiri Pirko <jiri@resnulli.us>:
> Fri, Mar 28, 2014 at 05:35:00AM CET, decot@googlers.com wrote:
>>Tested:
>>  grep . /sys/class/net/*/count_link_*
>>  + ip link set dev X down/up
>>
>>Signed-off-by: David Decotigny <decot@googlers.com>
>>---
>> include/linux/netdevice.h |  4 ++++
>> net/core/net-sysfs.c      | 18 ++++++++++++++++++
>> net/sched/sch_generic.c   |  2 ++
>> 3 files changed, 24 insertions(+)
>>
>>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>index 4b6d12c..cf52869 100644
>>--- a/include/linux/netdevice.h
>>+++ b/include/linux/netdevice.h
>>@@ -1315,6 +1315,10 @@ struct net_device {
>>                                            * Do not use this in drivers.
>>                                            */
>>
>>+      /* Stats to monitor link on/off, flapping */
>>+      atomic_t                count_link_up;
>>+      atomic_t                count_link_down;
>>+
>> #ifdef CONFIG_WIRELESS_EXT
>>       /* List of functions to handle Wireless Extensions (instead of ioctl).
>>        * See <net/iw_handler.h> for details. Jean II */
>>diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>>index daed9a6..a65ac54 100644
>>--- a/net/core/net-sysfs.c
>>+++ b/net/core/net-sysfs.c
>>@@ -253,6 +253,22 @@ static ssize_t operstate_show(struct device *dev,
>> }
>> static DEVICE_ATTR_RO(operstate);
>>
>>+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 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_up);
>>+static DEVICE_ATTR_RO(count_link_down);
>>+
>
>
> If this is exposed in sysfs, this needs to be exposed via RT netlink as
> well.

This makes me wonder if this really needs to be a kernel-maintained
pair of counters. Since this seems to be useful for monitoring e.g:
servers for link flapping, I would assume that some user-space
script/program does read these sysfs entries periodically to report a
healthy link. If that's the case, what prevents the same
script/program from listening to netlink events and count the link
UP/DOWN events from there and report that?

>
>
>> /* read-write attributes */
>>
>> static int change_mtu(struct net_device *net, unsigned long new_mtu)
>>@@ -386,6 +402,8 @@ static struct attribute *net_class_attrs[] = {
>>       &dev_attr_duplex.attr,
>>       &dev_attr_dormant.attr,
>>       &dev_attr_operstate.attr,
>>+      &dev_attr_count_link_up.attr,
>>+      &dev_attr_count_link_down.attr,
>>       &dev_attr_ifalias.attr,
>>       &dev_attr_carrier.attr,
>>       &dev_attr_mtu.attr,
>>diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>>index e82e43b..2d06943 100644
>>--- a/net/sched/sch_generic.c
>>+++ b/net/sched/sch_generic.c
>>@@ -310,6 +310,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->count_link_up);
>>               linkwatch_fire_event(dev);
>>               if (netif_running(dev))
>>                       __netdev_watchdog_up(dev);
>>@@ -328,6 +329,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->count_link_down);
>>               linkwatch_fire_event(dev);
>>       }
>> }
>>--
>>1.9.1.423.g4596e3a
>>
> --
> 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



-- 
Florian

Re: [PATCH v1] net-sysfs: expose number of link up/down transitions

[Eric Dumazet]

On Fri, 2014-03-28 at 09:51 -0700, Florian Fainelli wrote:
> 2014-03-27 23:25 GMT-07:00 Jiri Pirko <jiri@resnulli.us>:
> >
> > If this is exposed in sysfs, this needs to be exposed via RT netlink as
> > well.
> 
> This makes me wonder if this really needs to be a kernel-maintained
> pair of counters. Since this seems to be useful for monitoring e.g:
> servers for link flapping, I would assume that some user-space
> script/program does read these sysfs entries periodically to report a
> healthy link. If that's the case, what prevents the same
> script/program from listening to netlink events and count the link
> UP/DOWN events from there and report that?

The same would be true for any counter in networking stack...

I do not feel we need to have a daemon running to 'count' events.

I like being able to run "ip -s link" , "ifconfig -a" or "nstat -a"
without having to connect to a daemon. This daemon will likely have
different api / constraints from distro to distro.

To me, first thing is a counter, then eventually events for daemons.

drop_monitor could eventually disappear if we had counters everywhere we
need them.

Re: [PATCH v1] net-sysfs: expose number of link up/down transitions

[Florian Fainelli]

2014-03-28 10:46 GMT-07:00 Eric Dumazet <eric.dumazet@gmail.com>:
> On Fri, 2014-03-28 at 09:51 -0700, Florian Fainelli wrote:
>> 2014-03-27 23:25 GMT-07:00 Jiri Pirko <jiri@resnulli.us>:
>> >
>> > If this is exposed in sysfs, this needs to be exposed via RT netlink as
>> > well.
>>
>> This makes me wonder if this really needs to be a kernel-maintained
>> pair of counters. Since this seems to be useful for monitoring e.g:
>> servers for link flapping, I would assume that some user-space
>> script/program does read these sysfs entries periodically to report a
>> healthy link. If that's the case, what prevents the same
>> script/program from listening to netlink events and count the link
>> UP/DOWN events from there and report that?
>
> The same would be true for any counter in networking stack...
>
> I do not feel we need to have a daemon running to 'count' events.
>
> I like being able to run "ip -s link" , "ifconfig -a" or "nstat -a"
> without having to connect to a daemon. This daemon will likely have
> different api / constraints from distro to distro.
>
> To me, first thing is a counter, then eventually events for daemons.

What I meant is that this sort of information most likely is already
part of some sort of machine health check that runs periodically,
which probably collects gazillions of other metrics. Whether the link
flapping information comes from sysfs or was maintained by a small
script which counts link UP/DOWN events from 'ip monitor link' and
outputs this into a file boils down to the same thing: the information
is available to some degree.

That said, this is a cheap pair of a counter in a slow path, so maybe
it is fine to have it without requiring any user-space dependency to
be read...

>
> drop_monitor could eventually disappear if we had counters everywhere we
> need them.
>
>
>



-- 
Florian

Re: [PATCH v1] net-sysfs: expose number of link up/down transitions

[Eric Dumazet]

On Fri, 2014-03-28 at 10:59 -0700, Florian Fainelli wrote:

> What I meant is that this sort of information most likely is already
> part of some sort of machine health check that runs periodically,
> which probably collects gazillions of other metrics. Whether the link
> flapping information comes from sysfs or was maintained by a small
> script which counts link UP/DOWN events from 'ip monitor link' and
> outputs this into a file boils down to the same thing: the information
> is available to some degree.

Yeah, I am not sure such a daemon runs on my netgear/OpenWrt router ;)

Re: [PATCH v1] net-sysfs: expose number of link up/down transitions

[David Decotigny]

Hi all,

Thank you all for your feedback. Indeed, these counters are meant to
detect link flapping situations, and we prefer to accumulate in the
kernel rather than doing it in user space based on the count of
up/down transitions. How about I update the patch with a new
IFLA_LINK_HEALTH that exposes the 2 counters as a new struct? Or: is
it safe/recommended for userspace backward-compatibility to append new
fields to structs like rtnl_link_statsX (I have the impression that
iproute2 would be ok with it)?

On Fri, Mar 28, 2014 at 11:08 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2014-03-28 at 10:59 -0700, Florian Fainelli wrote:
>
>> What I meant is that this sort of information most likely is already
>> part of some sort of machine health check that runs periodically,
>> which probably collects gazillions of other metrics. Whether the link
>> flapping information comes from sysfs or was maintained by a small
>> script which counts link UP/DOWN events from 'ip monitor link' and
>> outputs this into a file boils down to the same thing: the information
>> is available to some degree.
>
> Yeah, I am not sure such a daemon runs on my netgear/OpenWrt router ;)
>
>
>