* Re: [PATCH net-next] net: core: Expose number of link up/down transitions
2018-01-17 23:06 [PATCH net-next] net: core: Expose number of link up/down transitions Florian Fainelli
@ 2018-01-17 23:17 ` Jakub Kicinski
2018-01-17 23:49 ` Andrei Vagin
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2018-01-17 23:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, edumazet, andrew, cphealy, David Decotigny,
David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Daniel Borkmann, Nikolay Aleksandrov, Alexei Starovoitov,
Roopa Prabhu, Mahesh Bandewar, Vlad Yasevich, Jonas Bonn,
stephen hemminger, Hans Liljestrand, Reshetova, Elena,
Kirill Tkhai, Andrey Vagin, Florian Westphal, David Ahern,
Xin Long, Matthias Schiffer, Jiri Benc, open list
On Wed, 17 Jan 2018 15:06:57 -0800, Florian Fainelli wrote:
> +What: /sys/class/net/<iface/carrier_changes
nit: s/<iface/<iface>/ ?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] net: core: Expose number of link up/down transitions
2018-01-17 23:06 [PATCH net-next] net: core: Expose number of link up/down transitions Florian Fainelli
2018-01-17 23:17 ` Jakub Kicinski
@ 2018-01-17 23:49 ` Andrei Vagin
2018-01-18 0:06 ` Andrew Lunn
2018-01-17 23:52 ` Jiri Pirko
2018-01-18 0:28 ` David Ahern
3 siblings, 1 reply; 8+ messages in thread
From: Andrei Vagin @ 2018-01-17 23:49 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, edumazet, andrew, cphealy, David Decotigny,
David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Daniel Borkmann, Nikolay Aleksandrov, Alexei Starovoitov,
Roopa Prabhu, Mahesh Bandewar, Vlad Yasevich, Jakub Kicinski,
Jonas Bonn, stephen hemminger, Hans Liljestrand, Reshetova, Elena,
Kirill Tkhai, Andrey Vagin, Florian Westphal, David Ahern,
Xin Long, Matthias Schiffer, Jiri Benc, open list
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] net: core: Expose number of link up/down transitions
2018-01-17 23:49 ` Andrei Vagin
@ 2018-01-18 0:06 ` Andrew Lunn
2018-01-18 0:23 ` Andrei Vagin
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2018-01-18 0:06 UTC (permalink / raw)
To: Andrei Vagin
Cc: Florian Fainelli, netdev, edumazet, cphealy, David Decotigny,
David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Daniel Borkmann, Nikolay Aleksandrov, Alexei Starovoitov,
Roopa Prabhu, Mahesh Bandewar, Vlad Yasevich, Jakub Kicinski,
Jonas Bonn, stephen hemminger, Hans Liljestrand, Reshetova, Elena,
Kirill Tkhai, Andrey Vagin, Florian Westphal, David Ahern,
Xin Long, Matthias Schiffer, Jiri Benc, open list
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: core: Expose number of link up/down transitions
2018-01-18 0:06 ` Andrew Lunn
@ 2018-01-18 0:23 ` Andrei Vagin
0 siblings, 0 replies; 8+ messages in thread
From: Andrei Vagin @ 2018-01-18 0:23 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, netdev, edumazet, cphealy, David Decotigny,
David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Daniel Borkmann, Nikolay Aleksandrov, Alexei Starovoitov,
Roopa Prabhu, Mahesh Bandewar, Vlad Yasevich, Jakub Kicinski,
Jonas Bonn, stephen hemminger, Hans Liljestrand, Reshetova, Elena,
Kirill Tkhai, Andrey Vagin, Florian Westphal, David Ahern,
Xin Long, Matthias Schiffer, Jiri Benc, open list
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: core: Expose number of link up/down transitions
2018-01-17 23:06 [PATCH net-next] net: core: Expose number of link up/down transitions Florian Fainelli
2018-01-17 23:17 ` Jakub Kicinski
2018-01-17 23:49 ` Andrei Vagin
@ 2018-01-17 23:52 ` Jiri Pirko
2018-01-18 0:17 ` David Ahern
2018-01-18 0:28 ` David Ahern
3 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2018-01-17 23:52 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, edumazet, andrew, cphealy, David Decotigny,
David S. Miller, Jamal Hadi Salim, Cong Wang, Daniel Borkmann,
Nikolay Aleksandrov, Alexei Starovoitov, Roopa Prabhu,
Mahesh Bandewar, Vlad Yasevich, Jakub Kicinski, Jonas Bonn,
stephen hemminger, Hans Liljestrand, Reshetova, Elena,
Kirill Tkhai, Andrey Vagin, Florian Westphal, David Ahern,
Xin Long, Matthias Schiffer, Jiri Benc, open list
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
> };
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] net: core: Expose number of link up/down transitions
2018-01-17 23:52 ` Jiri Pirko
@ 2018-01-18 0:17 ` David Ahern
0 siblings, 0 replies; 8+ messages in thread
From: David Ahern @ 2018-01-18 0:17 UTC (permalink / raw)
To: Jiri Pirko, Florian Fainelli
Cc: netdev, edumazet, andrew, cphealy, David Decotigny,
David S. Miller, Jamal Hadi Salim, Cong Wang, Daniel Borkmann,
Nikolay Aleksandrov, Alexei Starovoitov, Roopa Prabhu,
Mahesh Bandewar, Vlad Yasevich, Jakub Kicinski, Jonas Bonn,
stephen hemminger, Hans Liljestrand, Reshetova, Elena,
Kirill Tkhai, Andrey Vagin, Florian Westphal, Xin Long,
Matthias Schiffer, Jiri Benc, open list
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: core: Expose number of link up/down transitions
2018-01-17 23:06 [PATCH net-next] net: core: Expose number of link up/down transitions Florian Fainelli
` (2 preceding siblings ...)
2018-01-17 23:52 ` Jiri Pirko
@ 2018-01-18 0:28 ` David Ahern
3 siblings, 0 replies; 8+ messages in thread
From: David Ahern @ 2018-01-18 0:28 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: edumazet, andrew, cphealy, David Decotigny, David S. Miller,
Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Nikolay Aleksandrov, Alexei Starovoitov, Roopa Prabhu,
Mahesh Bandewar, Vlad Yasevich, Jakub Kicinski, Jonas Bonn,
stephen hemminger, Hans Liljestrand, Reshetova, Elena,
Kirill Tkhai, Andrey Vagin, Florian Westphal, Xin Long,
Matthias Schiffer, Jiri Benc, open list
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.
^ permalink raw reply [flat|nested] 8+ messages in thread