[PATCH net-next] net: bridge: add STP xstats

[PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
transition_fwd xstats counters to the bridge ports copied over via
netlink, providing useful information for STP.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 include/uapi/linux/if_bridge.h | 10 ++++++++++
 net/bridge/br_if.c             |  8 ++++++++
 net/bridge/br_netlink.c        |  9 +++++++++
 net/bridge/br_private.h        |  9 +++++++++
 net/bridge/br_stp.c            | 25 +++++++++++++++++++++++++
 net/bridge/br_stp_bpdu.c       | 12 ++++++++++++
 net/bridge/br_stp_if.c         | 27 +++++++++++++++++++++++++++
 7 files changed, 100 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 1b3c2b643a02..e7f2bb782006 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
 	__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+	__u64 transition_blk;
+	__u64 transition_fwd;
+	__u64 rx_bpdu;
+	__u64 tx_bpdu;
+	__u64 rx_tcn;
+	__u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  *     [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
 	BRIDGE_XSTATS_UNSPEC,
 	BRIDGE_XSTATS_VLAN,
 	BRIDGE_XSTATS_MCAST,
+	BRIDGE_XSTATS_STP,
 	BRIDGE_XSTATS_PAD,
 	__BRIDGE_XSTATS_MAX
 };
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 4fe30b182ee7..3eb214ef9763 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -250,6 +250,7 @@ static void release_nbp(struct kobject *kobj)
 {
 	struct net_bridge_port *p
 		= container_of(kobj, struct net_bridge_port, kobj);
+	free_percpu(p->stp_stats);
 	kfree(p);
 }
 
@@ -419,6 +420,12 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	if (p == NULL)
 		return ERR_PTR(-ENOMEM);
 
+	p->stp_stats = netdev_alloc_pcpu_stats(struct br_stp_stats);
+	if (!p->stp_stats) {
+		kfree(p);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	p->br = br;
 	dev_hold(dev);
 	p->dev = dev;
@@ -432,6 +439,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	err = br_multicast_add_port(p);
 	if (err) {
 		dev_put(dev);
+		free_percpu(p->stp_stats);
 		kfree(p);
 		p = ERR_PTR(err);
 	}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..03aced1f862b 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1597,6 +1597,15 @@ static int br_fill_linkxstats(struct sk_buff *skb,
 		}
 	}
 
+	if (p) {
+		struct bridge_stp_xstats xstats;
+
+		br_stp_get_xstats(p, &xstats);
+
+		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
+			goto nla_put_failure;
+	}
+
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	if (++vl_idx >= *prividx) {
 		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_MCAST,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 36b0367ca1e0..af5f28f0f2ef 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -95,6 +95,11 @@ struct br_vlan_stats {
 	struct u64_stats_sync syncp;
 };
 
+struct br_stp_stats {
+	struct bridge_stp_xstats xstats;
+	struct u64_stats_sync syncp;
+};
+
 struct br_tunnel_info {
 	__be64			tunnel_id;
 	struct metadata_dst	*tunnel_dst;
@@ -283,6 +288,8 @@ struct net_bridge_port {
 #endif
 	u16				group_fwd_mask;
 	u16				backup_redirected_cnt;
+
+	struct br_stp_stats		__percpu *stp_stats;
 };
 
 #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
@@ -1146,6 +1153,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *a);
 void br_stp_set_bridge_priority(struct net_bridge *br, u16 newprio);
 int br_stp_set_port_priority(struct net_bridge_port *p, unsigned long newprio);
 int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost);
+void br_stp_get_xstats(const struct net_bridge_port *p,
+		       struct bridge_stp_xstats *xstats);
 ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id);
 
 /* br_stp_bpdu.c */
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..8bcdab29442d 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,18 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
 		br_info(p->br, "port %u(%s) entered %s state\n",
 				(unsigned int) p->port_no, p->dev->name,
 				br_port_state_names[p->state]);
+
+	if (p->br->stp_enabled == BR_KERNEL_STP) {
+		struct br_stp_stats *stats;
+
+		stats = this_cpu_ptr(p->stp_stats);
+		u64_stats_update_begin(&stats->syncp);
+		if (p->state == BR_STATE_BLOCKING)
+			stats->xstats.transition_blk++;
+		else if (p->state == BR_STATE_FORWARDING)
+			stats->xstats.transition_fwd++;
+		u64_stats_update_end(&stats->syncp);
+	}
 }
 
 /* called under bridge lock */
@@ -481,9 +493,15 @@ static void br_topology_change_acknowledge(struct net_bridge_port *p)
 void br_received_config_bpdu(struct net_bridge_port *p,
 			     const struct br_config_bpdu *bpdu)
 {
+	struct br_stp_stats *stats;
 	struct net_bridge *br;
 	int was_root;
 
+	stats = this_cpu_ptr(p->stp_stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->xstats.rx_bpdu++;
+	u64_stats_update_end(&stats->syncp);
+
 	br = p->br;
 	was_root = br_is_root_bridge(br);
 
@@ -517,6 +535,13 @@ void br_received_config_bpdu(struct net_bridge_port *p,
 /* called under bridge lock */
 void br_received_tcn_bpdu(struct net_bridge_port *p)
 {
+	struct br_stp_stats *stats;
+
+	stats = this_cpu_ptr(p->stp_stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->xstats.rx_tcn++;
+	u64_stats_update_end(&stats->syncp);
+
 	if (br_is_designated_port(p)) {
 		br_info(p->br, "port %u(%s) received tcn bpdu\n",
 			(unsigned int) p->port_no, p->dev->name);
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 7796dd9d42d7..2dbd11e21f2a 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -78,6 +78,7 @@ static inline int br_get_ticks(const unsigned char *src)
 /* called under bridge lock */
 void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
 {
+	struct br_stp_stats *stats;
 	unsigned char buf[35];
 
 	if (p->br->stp_enabled != BR_KERNEL_STP)
@@ -118,11 +119,17 @@ void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
 	br_set_ticks(buf+33, bpdu->forward_delay);
 
 	br_send_bpdu(p, buf, 35);
+
+	stats = this_cpu_ptr(p->stp_stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->xstats.tx_bpdu++;
+	u64_stats_update_end(&stats->syncp);
 }
 
 /* called under bridge lock */
 void br_send_tcn_bpdu(struct net_bridge_port *p)
 {
+	struct br_stp_stats *stats;
 	unsigned char buf[4];
 
 	if (p->br->stp_enabled != BR_KERNEL_STP)
@@ -133,6 +140,11 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
 	buf[2] = 0;
 	buf[3] = BPDU_TYPE_TCN;
 	br_send_bpdu(p, buf, 4);
+
+	stats = this_cpu_ptr(p->stp_stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->xstats.tx_tcn++;
+	u64_stats_update_end(&stats->syncp);
 }
 
 /*
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index d174d3a566aa..cbce7d0e40b9 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -333,6 +333,33 @@ int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost)
 	return 0;
 }
 
+void br_stp_get_xstats(const struct net_bridge_port *p,
+		       struct bridge_stp_xstats *xstats)
+{
+	int i;
+
+	memset(xstats, 0, sizeof(*xstats));
+
+	for_each_possible_cpu(i) {
+		struct bridge_stp_xstats cpu_xstats;
+		struct br_stp_stats *stats;
+		unsigned int start;
+
+		stats = per_cpu_ptr(p->stp_stats, i);
+		do {
+			start = u64_stats_fetch_begin_irq(&stats->syncp);
+			memcpy(&cpu_xstats, &stats->xstats, sizeof(cpu_xstats));
+		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+
+		xstats->transition_blk += cpu_xstats.transition_blk;
+		xstats->transition_fwd += cpu_xstats.transition_fwd;
+		xstats->rx_bpdu += cpu_xstats.rx_bpdu;
+		xstats->tx_bpdu += cpu_xstats.tx_bpdu;
+		xstats->rx_tcn += cpu_xstats.rx_tcn;
+		xstats->tx_tcn += cpu_xstats.tx_tcn;
+	}
+}
+
 ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id)
 {
 	return sprintf(buf, "%.2x%.2x.%.2x%.2x%.2x%.2x%.2x%.2x\n",
-- 
2.24.0

[Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
transition_fwd xstats counters to the bridge ports copied over via
netlink, providing useful information for STP.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 include/uapi/linux/if_bridge.h | 10 ++++++++++
 net/bridge/br_if.c             |  8 ++++++++
 net/bridge/br_netlink.c        |  9 +++++++++
 net/bridge/br_private.h        |  9 +++++++++
 net/bridge/br_stp.c            | 25 +++++++++++++++++++++++++
 net/bridge/br_stp_bpdu.c       | 12 ++++++++++++
 net/bridge/br_stp_if.c         | 27 +++++++++++++++++++++++++++
 7 files changed, 100 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 1b3c2b643a02..e7f2bb782006 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
 	__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+	__u64 transition_blk;
+	__u64 transition_fwd;
+	__u64 rx_bpdu;
+	__u64 tx_bpdu;
+	__u64 rx_tcn;
+	__u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  *     [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
 	BRIDGE_XSTATS_UNSPEC,
 	BRIDGE_XSTATS_VLAN,
 	BRIDGE_XSTATS_MCAST,
+	BRIDGE_XSTATS_STP,
 	BRIDGE_XSTATS_PAD,
 	__BRIDGE_XSTATS_MAX
 };
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 4fe30b182ee7..3eb214ef9763 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -250,6 +250,7 @@ static void release_nbp(struct kobject *kobj)
 {
 	struct net_bridge_port *p
 		= container_of(kobj, struct net_bridge_port, kobj);
+	free_percpu(p->stp_stats);
 	kfree(p);
 }
 
@@ -419,6 +420,12 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	if (p == NULL)
 		return ERR_PTR(-ENOMEM);
 
+	p->stp_stats = netdev_alloc_pcpu_stats(struct br_stp_stats);
+	if (!p->stp_stats) {
+		kfree(p);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	p->br = br;
 	dev_hold(dev);
 	p->dev = dev;
@@ -432,6 +439,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	err = br_multicast_add_port(p);
 	if (err) {
 		dev_put(dev);
+		free_percpu(p->stp_stats);
 		kfree(p);
 		p = ERR_PTR(err);
 	}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..03aced1f862b 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1597,6 +1597,15 @@ static int br_fill_linkxstats(struct sk_buff *skb,
 		}
 	}
 
+	if (p) {
+		struct bridge_stp_xstats xstats;
+
+		br_stp_get_xstats(p, &xstats);
+
+		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
+			goto nla_put_failure;
+	}
+
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	if (++vl_idx >= *prividx) {
 		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_MCAST,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 36b0367ca1e0..af5f28f0f2ef 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -95,6 +95,11 @@ struct br_vlan_stats {
 	struct u64_stats_sync syncp;
 };
 
+struct br_stp_stats {
+	struct bridge_stp_xstats xstats;
+	struct u64_stats_sync syncp;
+};
+
 struct br_tunnel_info {
 	__be64			tunnel_id;
 	struct metadata_dst	*tunnel_dst;
@@ -283,6 +288,8 @@ struct net_bridge_port {
 #endif
 	u16				group_fwd_mask;
 	u16				backup_redirected_cnt;
+
+	struct br_stp_stats		__percpu *stp_stats;
 };
 
 #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
@@ -1146,6 +1153,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *a);
 void br_stp_set_bridge_priority(struct net_bridge *br, u16 newprio);
 int br_stp_set_port_priority(struct net_bridge_port *p, unsigned long newprio);
 int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost);
+void br_stp_get_xstats(const struct net_bridge_port *p,
+		       struct bridge_stp_xstats *xstats);
 ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id);
 
 /* br_stp_bpdu.c */
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..8bcdab29442d 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,18 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
 		br_info(p->br, "port %u(%s) entered %s state\n",
 				(unsigned int) p->port_no, p->dev->name,
 				br_port_state_names[p->state]);
+
+	if (p->br->stp_enabled == BR_KERNEL_STP) {
+		struct br_stp_stats *stats;
+
+		stats = this_cpu_ptr(p->stp_stats);
+		u64_stats_update_begin(&stats->syncp);
+		if (p->state == BR_STATE_BLOCKING)
+			stats->xstats.transition_blk++;
+		else if (p->state == BR_STATE_FORWARDING)
+			stats->xstats.transition_fwd++;
+		u64_stats_update_end(&stats->syncp);
+	}
 }
 
 /* called under bridge lock */
@@ -481,9 +493,15 @@ static void br_topology_change_acknowledge(struct net_bridge_port *p)
 void br_received_config_bpdu(struct net_bridge_port *p,
 			     const struct br_config_bpdu *bpdu)
 {
+	struct br_stp_stats *stats;
 	struct net_bridge *br;
 	int was_root;
 
+	stats = this_cpu_ptr(p->stp_stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->xstats.rx_bpdu++;
+	u64_stats_update_end(&stats->syncp);
+
 	br = p->br;
 	was_root = br_is_root_bridge(br);
 
@@ -517,6 +535,13 @@ void br_received_config_bpdu(struct net_bridge_port *p,
 /* called under bridge lock */
 void br_received_tcn_bpdu(struct net_bridge_port *p)
 {
+	struct br_stp_stats *stats;
+
+	stats = this_cpu_ptr(p->stp_stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->xstats.rx_tcn++;
+	u64_stats_update_end(&stats->syncp);
+
 	if (br_is_designated_port(p)) {
 		br_info(p->br, "port %u(%s) received tcn bpdu\n",
 			(unsigned int) p->port_no, p->dev->name);
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 7796dd9d42d7..2dbd11e21f2a 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -78,6 +78,7 @@ static inline int br_get_ticks(const unsigned char *src)
 /* called under bridge lock */
 void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
 {
+	struct br_stp_stats *stats;
 	unsigned char buf[35];
 
 	if (p->br->stp_enabled != BR_KERNEL_STP)
@@ -118,11 +119,17 @@ void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
 	br_set_ticks(buf+33, bpdu->forward_delay);
 
 	br_send_bpdu(p, buf, 35);
+
+	stats = this_cpu_ptr(p->stp_stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->xstats.tx_bpdu++;
+	u64_stats_update_end(&stats->syncp);
 }
 
 /* called under bridge lock */
 void br_send_tcn_bpdu(struct net_bridge_port *p)
 {
+	struct br_stp_stats *stats;
 	unsigned char buf[4];
 
 	if (p->br->stp_enabled != BR_KERNEL_STP)
@@ -133,6 +140,11 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
 	buf[2] = 0;
 	buf[3] = BPDU_TYPE_TCN;
 	br_send_bpdu(p, buf, 4);
+
+	stats = this_cpu_ptr(p->stp_stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->xstats.tx_tcn++;
+	u64_stats_update_end(&stats->syncp);
 }
 
 /*
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index d174d3a566aa..cbce7d0e40b9 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -333,6 +333,33 @@ int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost)
 	return 0;
 }
 
+void br_stp_get_xstats(const struct net_bridge_port *p,
+		       struct bridge_stp_xstats *xstats)
+{
+	int i;
+
+	memset(xstats, 0, sizeof(*xstats));
+
+	for_each_possible_cpu(i) {
+		struct bridge_stp_xstats cpu_xstats;
+		struct br_stp_stats *stats;
+		unsigned int start;
+
+		stats = per_cpu_ptr(p->stp_stats, i);
+		do {
+			start = u64_stats_fetch_begin_irq(&stats->syncp);
+			memcpy(&cpu_xstats, &stats->xstats, sizeof(cpu_xstats));
+		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+
+		xstats->transition_blk += cpu_xstats.transition_blk;
+		xstats->transition_fwd += cpu_xstats.transition_fwd;
+		xstats->rx_bpdu += cpu_xstats.rx_bpdu;
+		xstats->tx_bpdu += cpu_xstats.tx_bpdu;
+		xstats->rx_tcn += cpu_xstats.rx_tcn;
+		xstats->tx_tcn += cpu_xstats.tx_tcn;
+	}
+}
+
 ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id)
 {
 	return sprintf(buf, "%.2x%.2x.%.2x%.2x%.2x%.2x%.2x%.2x\n",
-- 
2.24.0

[PATCH iproute2 v2] iplink: add support for STP xstats

[Vivien Didelot]

Add support for the BRIDGE_XSTATS_STP xstats, as follow:

    # ip link xstats type bridge_slave dev lan5
                        STP BPDU:
                          RX: 0
                          TX: 39
                        STP TCN:
                          RX: 0
                          TX: 0
                        STP Transitions:
                          Blocked: 0
                          Forwarding: 1
                        IGMP queries:
                          RX: v1 0 v2 0 v3 0
                          TX: v1 0 v2 0 v3 0
    ...

Or below as JSON:

    # ip -j -p link xstats type bridge_slave dev lan0 | head
    [ {
            "ifname": "lan0",
            "stp": {
                "rx_bpdu": 0,
                "tx_bpdu": 500,
                "rx_tcn": 0,
                "tx_tcn": 0,
                "transition_blk": 0,
                "transition_fwd": 1
            },
    ...

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 include/uapi/linux/if_bridge.h | 10 ++++++++++
 ip/iplink_bridge.c             | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 31fc51bd..42f76697 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
 	__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+	__u64 transition_blk;
+	__u64 transition_fwd;
+	__u64 rx_bpdu;
+	__u64 tx_bpdu;
+	__u64 rx_tcn;
+	__u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  *     [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
 	BRIDGE_XSTATS_UNSPEC,
 	BRIDGE_XSTATS_VLAN,
 	BRIDGE_XSTATS_MCAST,
+	BRIDGE_XSTATS_STP,
 	BRIDGE_XSTATS_PAD,
 	__BRIDGE_XSTATS_MAX
 };
diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
index 06f736d4..92e051c8 100644
--- a/ip/iplink_bridge.c
+++ b/ip/iplink_bridge.c
@@ -688,6 +688,7 @@ static void bridge_print_xstats_help(struct link_util *lu, FILE *f)
 static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
 {
 	struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];
+	struct bridge_stp_xstats *sstats;
 	struct br_mcast_stats *mstats;
 	struct rtattr *i, *list;
 	const char *ifname = "";
@@ -807,6 +808,35 @@ static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
 				  mstats->mld_parse_errors);
 			close_json_object();
 			break;
+		case BRIDGE_XSTATS_STP:
+			sstats = RTA_DATA(i);
+			open_json_object("stp");
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP BPDU:\n", "");
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu\n",
+				  sstats->rx_bpdu);
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+				  sstats->tx_bpdu);
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP TCN:\n", "");
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "rx_tcn", "RX: %llu\n",
+				  sstats->rx_tcn);
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+				  sstats->tx_tcn);
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP Transitions:\n", "");
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "transition_blk", "Blocked: %llu\n",
+				  sstats->transition_blk);
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "transition_fwd", "Forwarding: %llu\n",
+				  sstats->transition_fwd);
+			close_json_object();
+			break;
 		}
 	}
 	close_json_object();
@@ -843,6 +873,8 @@ int bridge_parse_xstats(struct link_util *lu, int argc, char **argv)
 	while (argc > 0) {
 		if (strcmp(*argv, "igmp") == 0 || strcmp(*argv, "mcast") == 0) {
 			xstats_print_attr = BRIDGE_XSTATS_MCAST;
+		} else if (strcmp(*argv, "stp") == 0) {
+			xstats_print_attr = BRIDGE_XSTATS_STP;
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
 			filter_index = ll_name_to_index(*argv);
-- 
2.24.0

[Bridge] [PATCH iproute2 v2] iplink: add support for STP xstats

[Vivien Didelot]

Add support for the BRIDGE_XSTATS_STP xstats, as follow:

    # ip link xstats type bridge_slave dev lan5
                        STP BPDU:
                          RX: 0
                          TX: 39
                        STP TCN:
                          RX: 0
                          TX: 0
                        STP Transitions:
                          Blocked: 0
                          Forwarding: 1
                        IGMP queries:
                          RX: v1 0 v2 0 v3 0
                          TX: v1 0 v2 0 v3 0
    ...

Or below as JSON:

    # ip -j -p link xstats type bridge_slave dev lan0 | head
    [ {
            "ifname": "lan0",
            "stp": {
                "rx_bpdu": 0,
                "tx_bpdu": 500,
                "rx_tcn": 0,
                "tx_tcn": 0,
                "transition_blk": 0,
                "transition_fwd": 1
            },
    ...

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 include/uapi/linux/if_bridge.h | 10 ++++++++++
 ip/iplink_bridge.c             | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 31fc51bd..42f76697 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
 	__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+	__u64 transition_blk;
+	__u64 transition_fwd;
+	__u64 rx_bpdu;
+	__u64 tx_bpdu;
+	__u64 rx_tcn;
+	__u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  *     [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
 	BRIDGE_XSTATS_UNSPEC,
 	BRIDGE_XSTATS_VLAN,
 	BRIDGE_XSTATS_MCAST,
+	BRIDGE_XSTATS_STP,
 	BRIDGE_XSTATS_PAD,
 	__BRIDGE_XSTATS_MAX
 };
diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
index 06f736d4..92e051c8 100644
--- a/ip/iplink_bridge.c
+++ b/ip/iplink_bridge.c
@@ -688,6 +688,7 @@ static void bridge_print_xstats_help(struct link_util *lu, FILE *f)
 static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
 {
 	struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];
+	struct bridge_stp_xstats *sstats;
 	struct br_mcast_stats *mstats;
 	struct rtattr *i, *list;
 	const char *ifname = "";
@@ -807,6 +808,35 @@ static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
 				  mstats->mld_parse_errors);
 			close_json_object();
 			break;
+		case BRIDGE_XSTATS_STP:
+			sstats = RTA_DATA(i);
+			open_json_object("stp");
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP BPDU:\n", "");
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu\n",
+				  sstats->rx_bpdu);
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+				  sstats->tx_bpdu);
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP TCN:\n", "");
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "rx_tcn", "RX: %llu\n",
+				  sstats->rx_tcn);
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+				  sstats->tx_tcn);
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP Transitions:\n", "");
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "transition_blk", "Blocked: %llu\n",
+				  sstats->transition_blk);
+			print_string(PRINT_FP, NULL, "%-16s      ", "");
+			print_u64(PRINT_ANY, "transition_fwd", "Forwarding: %llu\n",
+				  sstats->transition_fwd);
+			close_json_object();
+			break;
 		}
 	}
 	close_json_object();
@@ -843,6 +873,8 @@ int bridge_parse_xstats(struct link_util *lu, int argc, char **argv)
 	while (argc > 0) {
 		if (strcmp(*argv, "igmp") == 0 || strcmp(*argv, "mcast") == 0) {
 			xstats_print_attr = BRIDGE_XSTATS_MCAST;
+		} else if (strcmp(*argv, "stp") == 0) {
+			xstats_print_attr = BRIDGE_XSTATS_STP;
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
 			filter_index = ll_name_to_index(*argv);
-- 
2.24.0

Re: [PATCH iproute2 v2] iplink: add support for STP xstats

[Stephen Hemminger]

On Mon,  9 Dec 2019 18:05:22 -0500
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> Add support for the BRIDGE_XSTATS_STP xstats, as follow:
> 
>     # ip link xstats type bridge_slave dev lan5
>                         STP BPDU:
>                           RX: 0
>                           TX: 39
>                         STP TCN:
>                           RX: 0
>                           TX: 0
>                         STP Transitions:
>                           Blocked: 0
>                           Forwarding: 1
>                         IGMP queries:
>                           RX: v1 0 v2 0 v3 0
>                           TX: v1 0 v2 0 v3 0
>     ...

Might I suggest a more concise format:
	STP BPDU:  RX: 0 TX: 39
	STP TCN:   RX: 0 TX:0
	STP Transitions: Blocked: 0 Forwarding: 1
...

Re: [Bridge] [PATCH iproute2 v2] iplink: add support for STP xstats

[Stephen Hemminger]

On Mon,  9 Dec 2019 18:05:22 -0500
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> Add support for the BRIDGE_XSTATS_STP xstats, as follow:
> 
>     # ip link xstats type bridge_slave dev lan5
>                         STP BPDU:
>                           RX: 0
>                           TX: 39
>                         STP TCN:
>                           RX: 0
>                           TX: 0
>                         STP Transitions:
>                           Blocked: 0
>                           Forwarding: 1
>                         IGMP queries:
>                           RX: v1 0 v2 0 v3 0
>                           TX: v1 0 v2 0 v3 0
>     ...

Might I suggest a more concise format:
	STP BPDU:  RX: 0 TX: 39
	STP TCN:   RX: 0 TX:0
	STP Transitions: Blocked: 0 Forwarding: 1
...

Re: [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 01:05, Vivien Didelot wrote:
> This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
> transition_fwd xstats counters to the bridge ports copied over via
> netlink, providing useful information for STP.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---
>  include/uapi/linux/if_bridge.h | 10 ++++++++++
>  net/bridge/br_if.c             |  8 ++++++++
>  net/bridge/br_netlink.c        |  9 +++++++++
>  net/bridge/br_private.h        |  9 +++++++++
>  net/bridge/br_stp.c            | 25 +++++++++++++++++++++++++
>  net/bridge/br_stp_bpdu.c       | 12 ++++++++++++
>  net/bridge/br_stp_if.c         | 27 +++++++++++++++++++++++++++
>  7 files changed, 100 insertions(+)
> 

Hi Vivien,
Why did you send the bridge patch again ? Does it have any changes ?

Why do you need percpu ? All of these seem to be incremented with the
bridge lock held. A few more comments below.

> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index 1b3c2b643a02..e7f2bb782006 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
>  	__u32 pad2;
>  };
>  
> +struct bridge_stp_xstats {
> +	__u64 transition_blk;
> +	__u64 transition_fwd;
> +	__u64 rx_bpdu;
> +	__u64 tx_bpdu;
> +	__u64 rx_tcn;
> +	__u64 tx_tcn;
> +};
> +
>  /* Bridge multicast database attributes
>   * [MDBA_MDB] = {
>   *     [MDBA_MDB_ENTRY] = {
> @@ -261,6 +270,7 @@ enum {
>  	BRIDGE_XSTATS_UNSPEC,
>  	BRIDGE_XSTATS_VLAN,
>  	BRIDGE_XSTATS_MCAST,
> +	BRIDGE_XSTATS_STP,
>  	BRIDGE_XSTATS_PAD,
>  	__BRIDGE_XSTATS_MAX
>  };
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 4fe30b182ee7..3eb214ef9763 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -250,6 +250,7 @@ static void release_nbp(struct kobject *kobj)
>  {
>  	struct net_bridge_port *p
>  		= container_of(kobj, struct net_bridge_port, kobj);
> +	free_percpu(p->stp_stats);

Please leave a new line between local var declaration and the code. I know
it was missing, but you can add it now. :)

>  	kfree(p);
>  }
>  
> @@ -419,6 +420,12 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>  	if (p == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> +	p->stp_stats = netdev_alloc_pcpu_stats(struct br_stp_stats);
> +	if (!p->stp_stats) {
> +		kfree(p);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	p->br = br;
>  	dev_hold(dev);
>  	p->dev = dev;
> @@ -432,6 +439,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>  	err = br_multicast_add_port(p);
>  	if (err) {
>  		dev_put(dev);
> +		free_percpu(p->stp_stats);
>  		kfree(p);
>  		p = ERR_PTR(err);
>  	}
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index a0a54482aabc..03aced1f862b 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1597,6 +1597,15 @@ static int br_fill_linkxstats(struct sk_buff *skb,
>  		}
>  	}
>  
> +	if (p) {
> +		struct bridge_stp_xstats xstats;

Please rename the local var here, using just xstats is misleading.
Maybe stp_xstats ?

> +
> +		br_stp_get_xstats(p, &xstats);
> +
> +		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
> +			goto nla_put_failure;

Could you please follow how mcast xstats are dumped and do something similar ?
It'd be nice to have similar code to audit.

> +	}
> +
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	if (++vl_idx >= *prividx) {
>  		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_MCAST,
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 36b0367ca1e0..af5f28f0f2ef 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -95,6 +95,11 @@ struct br_vlan_stats {
>  	struct u64_stats_sync syncp;
>  };
>  
> +struct br_stp_stats {
> +	struct bridge_stp_xstats xstats;
> +	struct u64_stats_sync syncp;
> +};
> +
>  struct br_tunnel_info {
>  	__be64			tunnel_id;
>  	struct metadata_dst	*tunnel_dst;
> @@ -283,6 +288,8 @@ struct net_bridge_port {
>  #endif
>  	u16				group_fwd_mask;
>  	u16				backup_redirected_cnt;
> +
> +	struct br_stp_stats		__percpu *stp_stats;
>  };
>  
>  #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
> @@ -1146,6 +1153,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *a);
>  void br_stp_set_bridge_priority(struct net_bridge *br, u16 newprio);
>  int br_stp_set_port_priority(struct net_bridge_port *p, unsigned long newprio);
>  int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost);
> +void br_stp_get_xstats(const struct net_bridge_port *p,
> +		       struct bridge_stp_xstats *xstats);
>  ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id);
>  
>  /* br_stp_bpdu.c */
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 1f1410f8d312..8bcdab29442d 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -45,6 +45,18 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
>  		br_info(p->br, "port %u(%s) entered %s state\n",
>  				(unsigned int) p->port_no, p->dev->name,
>  				br_port_state_names[p->state]);
> +
> +	if (p->br->stp_enabled == BR_KERNEL_STP) {
> +		struct br_stp_stats *stats;
> +
> +		stats = this_cpu_ptr(p->stp_stats);
> +		u64_stats_update_begin(&stats->syncp);
> +		if (p->state == BR_STATE_BLOCKING)
> +			stats->xstats.transition_blk++;
> +		else if (p->state == BR_STATE_FORWARDING)
> +			stats->xstats.transition_fwd++;
> +		u64_stats_update_end(&stats->syncp);
> +	}
>  }
>  
>  /* called under bridge lock */
> @@ -481,9 +493,15 @@ static void br_topology_change_acknowledge(struct net_bridge_port *p)
>  void br_received_config_bpdu(struct net_bridge_port *p,
>  			     const struct br_config_bpdu *bpdu)
>  {
> +	struct br_stp_stats *stats;
>  	struct net_bridge *br;
>  	int was_root;
>  
> +	stats = this_cpu_ptr(p->stp_stats);
> +	u64_stats_update_begin(&stats->syncp);
> +	stats->xstats.rx_bpdu++;
> +	u64_stats_update_end(&stats->syncp);
> +
>  	br = p->br;
>  	was_root = br_is_root_bridge(br);
>  
> @@ -517,6 +535,13 @@ void br_received_config_bpdu(struct net_bridge_port *p,
>  /* called under bridge lock */
>  void br_received_tcn_bpdu(struct net_bridge_port *p)
>  {
> +	struct br_stp_stats *stats;
> +
> +	stats = this_cpu_ptr(p->stp_stats);
> +	u64_stats_update_begin(&stats->syncp);
> +	stats->xstats.rx_tcn++;
> +	u64_stats_update_end(&stats->syncp);
> +
>  	if (br_is_designated_port(p)) {
>  		br_info(p->br, "port %u(%s) received tcn bpdu\n",
>  			(unsigned int) p->port_no, p->dev->name);
> diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
> index 7796dd9d42d7..2dbd11e21f2a 100644
> --- a/net/bridge/br_stp_bpdu.c
> +++ b/net/bridge/br_stp_bpdu.c
> @@ -78,6 +78,7 @@ static inline int br_get_ticks(const unsigned char *src)
>  /* called under bridge lock */
>  void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
>  {
> +	struct br_stp_stats *stats;
>  	unsigned char buf[35];
>  
>  	if (p->br->stp_enabled != BR_KERNEL_STP)
> @@ -118,11 +119,17 @@ void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
>  	br_set_ticks(buf+33, bpdu->forward_delay);
>  
>  	br_send_bpdu(p, buf, 35);
> +
> +	stats = this_cpu_ptr(p->stp_stats);
> +	u64_stats_update_begin(&stats->syncp);
> +	stats->xstats.tx_bpdu++;
> +	u64_stats_update_end(&stats->syncp);
>  }
>  
>  /* called under bridge lock */
>  void br_send_tcn_bpdu(struct net_bridge_port *p)
>  {
> +	struct br_stp_stats *stats;
>  	unsigned char buf[4];
>  
>  	if (p->br->stp_enabled != BR_KERNEL_STP)
> @@ -133,6 +140,11 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
>  	buf[2] = 0;
>  	buf[3] = BPDU_TYPE_TCN;
>  	br_send_bpdu(p, buf, 4);
> +
> +	stats = this_cpu_ptr(p->stp_stats);
> +	u64_stats_update_begin(&stats->syncp);
> +	stats->xstats.tx_tcn++;
> +	u64_stats_update_end(&stats->syncp);
>  }
>  
>  /*
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index d174d3a566aa..cbce7d0e40b9 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -333,6 +333,33 @@ int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost)
>  	return 0;
>  }
>  
> +void br_stp_get_xstats(const struct net_bridge_port *p,
> +		       struct bridge_stp_xstats *xstats)
> +{
> +	int i;
> +
> +	memset(xstats, 0, sizeof(*xstats));
> +
> +	for_each_possible_cpu(i) {
> +		struct bridge_stp_xstats cpu_xstats;
> +		struct br_stp_stats *stats;
> +		unsigned int start;
> +
> +		stats = per_cpu_ptr(p->stp_stats, i);
> +		do {
> +			start = u64_stats_fetch_begin_irq(&stats->syncp);
> +			memcpy(&cpu_xstats, &stats->xstats, sizeof(cpu_xstats));
> +		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
> +
> +		xstats->transition_blk += cpu_xstats.transition_blk;
> +		xstats->transition_fwd += cpu_xstats.transition_fwd;
> +		xstats->rx_bpdu += cpu_xstats.rx_bpdu;
> +		xstats->tx_bpdu += cpu_xstats.tx_bpdu;
> +		xstats->rx_tcn += cpu_xstats.rx_tcn;
> +		xstats->tx_tcn += cpu_xstats.tx_tcn;
> +	}
> +}
> +
>  ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id)
>  {
>  	return sprintf(buf, "%.2x%.2x.%.2x%.2x%.2x%.2x%.2x%.2x\n",
> 

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 01:05, Vivien Didelot wrote:
> This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
> transition_fwd xstats counters to the bridge ports copied over via
> netlink, providing useful information for STP.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---
>  include/uapi/linux/if_bridge.h | 10 ++++++++++
>  net/bridge/br_if.c             |  8 ++++++++
>  net/bridge/br_netlink.c        |  9 +++++++++
>  net/bridge/br_private.h        |  9 +++++++++
>  net/bridge/br_stp.c            | 25 +++++++++++++++++++++++++
>  net/bridge/br_stp_bpdu.c       | 12 ++++++++++++
>  net/bridge/br_stp_if.c         | 27 +++++++++++++++++++++++++++
>  7 files changed, 100 insertions(+)
> 

Hi Vivien,
Why did you send the bridge patch again ? Does it have any changes ?

Why do you need percpu ? All of these seem to be incremented with the
bridge lock held. A few more comments below.

> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index 1b3c2b643a02..e7f2bb782006 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
>  	__u32 pad2;
>  };
>  
> +struct bridge_stp_xstats {
> +	__u64 transition_blk;
> +	__u64 transition_fwd;
> +	__u64 rx_bpdu;
> +	__u64 tx_bpdu;
> +	__u64 rx_tcn;
> +	__u64 tx_tcn;
> +};
> +
>  /* Bridge multicast database attributes
>   * [MDBA_MDB] = {
>   *     [MDBA_MDB_ENTRY] = {
> @@ -261,6 +270,7 @@ enum {
>  	BRIDGE_XSTATS_UNSPEC,
>  	BRIDGE_XSTATS_VLAN,
>  	BRIDGE_XSTATS_MCAST,
> +	BRIDGE_XSTATS_STP,
>  	BRIDGE_XSTATS_PAD,
>  	__BRIDGE_XSTATS_MAX
>  };
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 4fe30b182ee7..3eb214ef9763 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -250,6 +250,7 @@ static void release_nbp(struct kobject *kobj)
>  {
>  	struct net_bridge_port *p
>  		= container_of(kobj, struct net_bridge_port, kobj);
> +	free_percpu(p->stp_stats);

Please leave a new line between local var declaration and the code. I know
it was missing, but you can add it now. :)

>  	kfree(p);
>  }
>  
> @@ -419,6 +420,12 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>  	if (p == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> +	p->stp_stats = netdev_alloc_pcpu_stats(struct br_stp_stats);
> +	if (!p->stp_stats) {
> +		kfree(p);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	p->br = br;
>  	dev_hold(dev);
>  	p->dev = dev;
> @@ -432,6 +439,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>  	err = br_multicast_add_port(p);
>  	if (err) {
>  		dev_put(dev);
> +		free_percpu(p->stp_stats);
>  		kfree(p);
>  		p = ERR_PTR(err);
>  	}
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index a0a54482aabc..03aced1f862b 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1597,6 +1597,15 @@ static int br_fill_linkxstats(struct sk_buff *skb,
>  		}
>  	}
>  
> +	if (p) {
> +		struct bridge_stp_xstats xstats;

Please rename the local var here, using just xstats is misleading.
Maybe stp_xstats ?

> +
> +		br_stp_get_xstats(p, &xstats);
> +
> +		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
> +			goto nla_put_failure;

Could you please follow how mcast xstats are dumped and do something similar ?
It'd be nice to have similar code to audit.

> +	}
> +
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	if (++vl_idx >= *prividx) {
>  		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_MCAST,
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 36b0367ca1e0..af5f28f0f2ef 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -95,6 +95,11 @@ struct br_vlan_stats {
>  	struct u64_stats_sync syncp;
>  };
>  
> +struct br_stp_stats {
> +	struct bridge_stp_xstats xstats;
> +	struct u64_stats_sync syncp;
> +};
> +
>  struct br_tunnel_info {
>  	__be64			tunnel_id;
>  	struct metadata_dst	*tunnel_dst;
> @@ -283,6 +288,8 @@ struct net_bridge_port {
>  #endif
>  	u16				group_fwd_mask;
>  	u16				backup_redirected_cnt;
> +
> +	struct br_stp_stats		__percpu *stp_stats;
>  };
>  
>  #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
> @@ -1146,6 +1153,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *a);
>  void br_stp_set_bridge_priority(struct net_bridge *br, u16 newprio);
>  int br_stp_set_port_priority(struct net_bridge_port *p, unsigned long newprio);
>  int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost);
> +void br_stp_get_xstats(const struct net_bridge_port *p,
> +		       struct bridge_stp_xstats *xstats);
>  ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id);
>  
>  /* br_stp_bpdu.c */
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 1f1410f8d312..8bcdab29442d 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -45,6 +45,18 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
>  		br_info(p->br, "port %u(%s) entered %s state\n",
>  				(unsigned int) p->port_no, p->dev->name,
>  				br_port_state_names[p->state]);
> +
> +	if (p->br->stp_enabled == BR_KERNEL_STP) {
> +		struct br_stp_stats *stats;
> +
> +		stats = this_cpu_ptr(p->stp_stats);
> +		u64_stats_update_begin(&stats->syncp);
> +		if (p->state == BR_STATE_BLOCKING)
> +			stats->xstats.transition_blk++;
> +		else if (p->state == BR_STATE_FORWARDING)
> +			stats->xstats.transition_fwd++;
> +		u64_stats_update_end(&stats->syncp);
> +	}
>  }
>  
>  /* called under bridge lock */
> @@ -481,9 +493,15 @@ static void br_topology_change_acknowledge(struct net_bridge_port *p)
>  void br_received_config_bpdu(struct net_bridge_port *p,
>  			     const struct br_config_bpdu *bpdu)
>  {
> +	struct br_stp_stats *stats;
>  	struct net_bridge *br;
>  	int was_root;
>  
> +	stats = this_cpu_ptr(p->stp_stats);
> +	u64_stats_update_begin(&stats->syncp);
> +	stats->xstats.rx_bpdu++;
> +	u64_stats_update_end(&stats->syncp);
> +
>  	br = p->br;
>  	was_root = br_is_root_bridge(br);
>  
> @@ -517,6 +535,13 @@ void br_received_config_bpdu(struct net_bridge_port *p,
>  /* called under bridge lock */
>  void br_received_tcn_bpdu(struct net_bridge_port *p)
>  {
> +	struct br_stp_stats *stats;
> +
> +	stats = this_cpu_ptr(p->stp_stats);
> +	u64_stats_update_begin(&stats->syncp);
> +	stats->xstats.rx_tcn++;
> +	u64_stats_update_end(&stats->syncp);
> +
>  	if (br_is_designated_port(p)) {
>  		br_info(p->br, "port %u(%s) received tcn bpdu\n",
>  			(unsigned int) p->port_no, p->dev->name);
> diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
> index 7796dd9d42d7..2dbd11e21f2a 100644
> --- a/net/bridge/br_stp_bpdu.c
> +++ b/net/bridge/br_stp_bpdu.c
> @@ -78,6 +78,7 @@ static inline int br_get_ticks(const unsigned char *src)
>  /* called under bridge lock */
>  void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
>  {
> +	struct br_stp_stats *stats;
>  	unsigned char buf[35];
>  
>  	if (p->br->stp_enabled != BR_KERNEL_STP)
> @@ -118,11 +119,17 @@ void br_send_config_bpdu(struct net_bridge_port *p, struct br_config_bpdu *bpdu)
>  	br_set_ticks(buf+33, bpdu->forward_delay);
>  
>  	br_send_bpdu(p, buf, 35);
> +
> +	stats = this_cpu_ptr(p->stp_stats);
> +	u64_stats_update_begin(&stats->syncp);
> +	stats->xstats.tx_bpdu++;
> +	u64_stats_update_end(&stats->syncp);
>  }
>  
>  /* called under bridge lock */
>  void br_send_tcn_bpdu(struct net_bridge_port *p)
>  {
> +	struct br_stp_stats *stats;
>  	unsigned char buf[4];
>  
>  	if (p->br->stp_enabled != BR_KERNEL_STP)
> @@ -133,6 +140,11 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
>  	buf[2] = 0;
>  	buf[3] = BPDU_TYPE_TCN;
>  	br_send_bpdu(p, buf, 4);
> +
> +	stats = this_cpu_ptr(p->stp_stats);
> +	u64_stats_update_begin(&stats->syncp);
> +	stats->xstats.tx_tcn++;
> +	u64_stats_update_end(&stats->syncp);
>  }
>  
>  /*
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index d174d3a566aa..cbce7d0e40b9 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -333,6 +333,33 @@ int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost)
>  	return 0;
>  }
>  
> +void br_stp_get_xstats(const struct net_bridge_port *p,
> +		       struct bridge_stp_xstats *xstats)
> +{
> +	int i;
> +
> +	memset(xstats, 0, sizeof(*xstats));
> +
> +	for_each_possible_cpu(i) {
> +		struct bridge_stp_xstats cpu_xstats;
> +		struct br_stp_stats *stats;
> +		unsigned int start;
> +
> +		stats = per_cpu_ptr(p->stp_stats, i);
> +		do {
> +			start = u64_stats_fetch_begin_irq(&stats->syncp);
> +			memcpy(&cpu_xstats, &stats->xstats, sizeof(cpu_xstats));
> +		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
> +
> +		xstats->transition_blk += cpu_xstats.transition_blk;
> +		xstats->transition_fwd += cpu_xstats.transition_fwd;
> +		xstats->rx_bpdu += cpu_xstats.rx_bpdu;
> +		xstats->tx_bpdu += cpu_xstats.tx_bpdu;
> +		xstats->rx_tcn += cpu_xstats.rx_tcn;
> +		xstats->tx_tcn += cpu_xstats.tx_tcn;
> +	}
> +}
> +
>  ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id)
>  {
>  	return sprintf(buf, "%.2x%.2x.%.2x%.2x%.2x%.2x%.2x%.2x\n",
> 

Re: [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 09:49, Nikolay Aleksandrov wrote:
> On 10/12/2019 01:05, Vivien Didelot wrote:
>> This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
>> transition_fwd xstats counters to the bridge ports copied over via
>> netlink, providing useful information for STP.
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
>> ---
>>  include/uapi/linux/if_bridge.h | 10 ++++++++++
>>  net/bridge/br_if.c             |  8 ++++++++
>>  net/bridge/br_netlink.c        |  9 +++++++++
>>  net/bridge/br_private.h        |  9 +++++++++
>>  net/bridge/br_stp.c            | 25 +++++++++++++++++++++++++
>>  net/bridge/br_stp_bpdu.c       | 12 ++++++++++++
>>  net/bridge/br_stp_if.c         | 27 +++++++++++++++++++++++++++
>>  7 files changed, 100 insertions(+)
>>
[snip]
>>  }
>>  
>> @@ -419,6 +420,12 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>>  	if (p == NULL)
>>  		return ERR_PTR(-ENOMEM);
>>  
>> +	p->stp_stats = netdev_alloc_pcpu_stats(struct br_stp_stats);
>> +	if (!p->stp_stats) {
>> +		kfree(p);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>>  	p->br = br;
>>  	dev_hold(dev);
>>  	p->dev = dev;
>> @@ -432,6 +439,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>>  	err = br_multicast_add_port(p);
>>  	if (err) {
>>  		dev_put(dev);
>> +		free_percpu(p->stp_stats);
>>  		kfree(p);
>>  		p = ERR_PTR(err);
>>  	}
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index a0a54482aabc..03aced1f862b 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -1597,6 +1597,15 @@ static int br_fill_linkxstats(struct sk_buff *skb,
>>  		}
>>  	}
>>  
>> +	if (p) {
>> +		struct bridge_stp_xstats xstats;
> 
> Please rename the local var here, using just xstats is misleading.
> Maybe stp_xstats ?
> 

Actually if this gets re-written to follow the mcast dump the local var
would disappear altogether.

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 09:49, Nikolay Aleksandrov wrote:
> On 10/12/2019 01:05, Vivien Didelot wrote:
>> This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
>> transition_fwd xstats counters to the bridge ports copied over via
>> netlink, providing useful information for STP.
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
>> ---
>>  include/uapi/linux/if_bridge.h | 10 ++++++++++
>>  net/bridge/br_if.c             |  8 ++++++++
>>  net/bridge/br_netlink.c        |  9 +++++++++
>>  net/bridge/br_private.h        |  9 +++++++++
>>  net/bridge/br_stp.c            | 25 +++++++++++++++++++++++++
>>  net/bridge/br_stp_bpdu.c       | 12 ++++++++++++
>>  net/bridge/br_stp_if.c         | 27 +++++++++++++++++++++++++++
>>  7 files changed, 100 insertions(+)
>>
[snip]
>>  }
>>  
>> @@ -419,6 +420,12 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>>  	if (p == NULL)
>>  		return ERR_PTR(-ENOMEM);
>>  
>> +	p->stp_stats = netdev_alloc_pcpu_stats(struct br_stp_stats);
>> +	if (!p->stp_stats) {
>> +		kfree(p);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>>  	p->br = br;
>>  	dev_hold(dev);
>>  	p->dev = dev;
>> @@ -432,6 +439,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>>  	err = br_multicast_add_port(p);
>>  	if (err) {
>>  		dev_put(dev);
>> +		free_percpu(p->stp_stats);
>>  		kfree(p);
>>  		p = ERR_PTR(err);
>>  	}
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index a0a54482aabc..03aced1f862b 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -1597,6 +1597,15 @@ static int br_fill_linkxstats(struct sk_buff *skb,
>>  		}
>>  	}
>>  
>> +	if (p) {
>> +		struct bridge_stp_xstats xstats;
> 
> Please rename the local var here, using just xstats is misleading.
> Maybe stp_xstats ?
> 

Actually if this gets re-written to follow the mcast dump the local var
would disappear altogether.

Re: [PATCH iproute2 v2] iplink: add support for STP xstats

[Vivien Didelot]

Hi Stephen,

On Mon, 9 Dec 2019 16:13:45 -0800, Stephen Hemminger <stephen@networkplumber.org> wrote:
> On Mon,  9 Dec 2019 18:05:22 -0500
> Vivien Didelot <vivien.didelot@gmail.com> wrote:
> 
> > Add support for the BRIDGE_XSTATS_STP xstats, as follow:
> > 
> >     # ip link xstats type bridge_slave dev lan5
> >                         STP BPDU:
> >                           RX: 0
> >                           TX: 39
> >                         STP TCN:
> >                           RX: 0
> >                           TX: 0
> >                         STP Transitions:
> >                           Blocked: 0
> >                           Forwarding: 1
> >                         IGMP queries:
> >                           RX: v1 0 v2 0 v3 0
> >                           TX: v1 0 v2 0 v3 0
> >     ...
> 
> Might I suggest a more concise format:
> 	STP BPDU:  RX: 0 TX: 39
> 	STP TCN:   RX: 0 TX:0
> 	STP Transitions: Blocked: 0 Forwarding: 1
> ...

I don't mind if you prefer this format ;-)

Re: [Bridge] [PATCH iproute2 v2] iplink: add support for STP xstats

[Vivien Didelot]

Hi Stephen,

On Mon, 9 Dec 2019 16:13:45 -0800, Stephen Hemminger <stephen@networkplumber.org> wrote:
> On Mon,  9 Dec 2019 18:05:22 -0500
> Vivien Didelot <vivien.didelot@gmail.com> wrote:
> 
> > Add support for the BRIDGE_XSTATS_STP xstats, as follow:
> > 
> >     # ip link xstats type bridge_slave dev lan5
> >                         STP BPDU:
> >                           RX: 0
> >                           TX: 39
> >                         STP TCN:
> >                           RX: 0
> >                           TX: 0
> >                         STP Transitions:
> >                           Blocked: 0
> >                           Forwarding: 1
> >                         IGMP queries:
> >                           RX: v1 0 v2 0 v3 0
> >                           TX: v1 0 v2 0 v3 0
> >     ...
> 
> Might I suggest a more concise format:
> 	STP BPDU:  RX: 0 TX: 39
> 	STP TCN:   RX: 0 TX:0
> 	STP Transitions: Blocked: 0 Forwarding: 1
> ...

I don't mind if you prefer this format ;-)

Re: [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

Hi Nikolay,

On Tue, 10 Dec 2019 09:49:59 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> Why did you send the bridge patch again ? Does it have any changes ?

The second iproute2 patch does not include the include guards update, but
I kept the bridge_stp_stats structure and the BRIDGE_XSTATS_STP definition
otherwise iproute2 wouldn't compile.

> 
> Why do you need percpu ? All of these seem to be incremented with the
> bridge lock held. A few more comments below.

All other xstats are incremented percpu, I simply followed the pattern.

> >  	struct net_bridge_port *p
> >  		= container_of(kobj, struct net_bridge_port, kobj);
> > +	free_percpu(p->stp_stats);
> 
> Please leave a new line between local var declaration and the code. I know
> it was missing, but you can add it now. :)

OK.

> > +	if (p) {
> > +		struct bridge_stp_xstats xstats;
> 
> Please rename the local var here, using just xstats is misleading.
> Maybe stp_xstats ?

This isn't misleading to me since its scope is limited to the current block
and not the entire function. The block above dumping the VLAN xstats is
using a local "struct br_vlan_stats stats" variable for example.

> 
> > +
> > +		br_stp_get_xstats(p, &xstats);
> > +
> > +		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
> > +			goto nla_put_failure;
> 
> Could you please follow how mcast xstats are dumped and do something similar ?
> It'd be nice to have similar code to audit.

Sure. I would also love to have easily auditable code in net/bridge. For
the bridge STP xstats I followed the VLAN xstats code above, which does:

    if (nla_put(skb, BRIDGE_XSTATS_VLAN, sizeof(vxi), &vxi))
        goto nla_put_failure;

But I can change the STP xstats code to the following:

    if (p) {
        nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
                                sizeof(struct bridge_stp_xstats),
                                BRIDGE_XSTATS_PAD);
        if (!nla)
            goto nla_put_failure;

        br_stp_get_xstats(p, nla_data(nla));
    }

Would that be preferred?


Thanks,

	Vivien

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

Hi Nikolay,

On Tue, 10 Dec 2019 09:49:59 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> Why did you send the bridge patch again ? Does it have any changes ?

The second iproute2 patch does not include the include guards update, but
I kept the bridge_stp_stats structure and the BRIDGE_XSTATS_STP definition
otherwise iproute2 wouldn't compile.

> 
> Why do you need percpu ? All of these seem to be incremented with the
> bridge lock held. A few more comments below.

All other xstats are incremented percpu, I simply followed the pattern.

> >  	struct net_bridge_port *p
> >  		= container_of(kobj, struct net_bridge_port, kobj);
> > +	free_percpu(p->stp_stats);
> 
> Please leave a new line between local var declaration and the code. I know
> it was missing, but you can add it now. :)

OK.

> > +	if (p) {
> > +		struct bridge_stp_xstats xstats;
> 
> Please rename the local var here, using just xstats is misleading.
> Maybe stp_xstats ?

This isn't misleading to me since its scope is limited to the current block
and not the entire function. The block above dumping the VLAN xstats is
using a local "struct br_vlan_stats stats" variable for example.

> 
> > +
> > +		br_stp_get_xstats(p, &xstats);
> > +
> > +		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
> > +			goto nla_put_failure;
> 
> Could you please follow how mcast xstats are dumped and do something similar ?
> It'd be nice to have similar code to audit.

Sure. I would also love to have easily auditable code in net/bridge. For
the bridge STP xstats I followed the VLAN xstats code above, which does:

    if (nla_put(skb, BRIDGE_XSTATS_VLAN, sizeof(vxi), &vxi))
        goto nla_put_failure;

But I can change the STP xstats code to the following:

    if (p) {
        nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
                                sizeof(struct bridge_stp_xstats),
                                BRIDGE_XSTATS_PAD);
        if (!nla)
            goto nla_put_failure;

        br_stp_get_xstats(p, nla_data(nla));
    }

Would that be preferred?


Thanks,

	Vivien

Re: [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 21:39, Vivien Didelot wrote:
> Hi Nikolay,
> 
> On Tue, 10 Dec 2019 09:49:59 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
>> Why did you send the bridge patch again ? Does it have any changes ?
> 
> The second iproute2 patch does not include the include guards update, but
> I kept the bridge_stp_stats structure and the BRIDGE_XSTATS_STP definition
> otherwise iproute2 wouldn't compile.
> >>
>> Why do you need percpu ? All of these seem to be incremented with the
>> bridge lock held. A few more comments below.
> 
> All other xstats are incremented percpu, I simply followed the pattern.
> 

We have already a lock, we can use it and avoid the whole per-cpu memory handling.
It seems to be acquired in all cases where these counters need to be changed.

>>>  	struct net_bridge_port *p
>>>  		= container_of(kobj, struct net_bridge_port, kobj);
>>> +	free_percpu(p->stp_stats);
>>
>> Please leave a new line between local var declaration and the code. I know
>> it was missing, but you can add it now. :)
> 
> OK.
> 
>>> +	if (p) {
>>> +		struct bridge_stp_xstats xstats;
>>
>> Please rename the local var here, using just xstats is misleading.
>> Maybe stp_xstats ?
> 
> This isn't misleading to me since its scope is limited to the current block
> and not the entire function. The block above dumping the VLAN xstats is
> using a local "struct br_vlan_stats stats" variable for example.
> 

Yep, as I answered to myself earlier, with the below change this goes away.

>>
>>> +
>>> +		br_stp_get_xstats(p, &xstats);
>>> +
>>> +		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
>>> +			goto nla_put_failure;
>>
>> Could you please follow how mcast xstats are dumped and do something similar ?
>> It'd be nice to have similar code to audit.
> 
> Sure. I would also love to have easily auditable code in net/bridge. For
> the bridge STP xstats I followed the VLAN xstats code above, which does:
> 
>     if (nla_put(skb, BRIDGE_XSTATS_VLAN, sizeof(vxi), &vxi))
>         goto nla_put_failure;
> 

Yeah, we can move that to a vlan-specific helper too, but that is an unrelated change.

> But I can change the STP xstats code to the following:
> 
>     if (p) {
>         nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
>                                 sizeof(struct bridge_stp_xstats),
>                                 BRIDGE_XSTATS_PAD);
>         if (!nla)
>             goto nla_put_failure;
> 
>         br_stp_get_xstats(p, nla_data(nla));
>     }
> 
> Would that be preferred?
> 
> 

Perfect, thanks!

> Thanks,
> 
> 	Vivien
> 

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 21:39, Vivien Didelot wrote:
> Hi Nikolay,
> 
> On Tue, 10 Dec 2019 09:49:59 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
>> Why did you send the bridge patch again ? Does it have any changes ?
> 
> The second iproute2 patch does not include the include guards update, but
> I kept the bridge_stp_stats structure and the BRIDGE_XSTATS_STP definition
> otherwise iproute2 wouldn't compile.
> >>
>> Why do you need percpu ? All of these seem to be incremented with the
>> bridge lock held. A few more comments below.
> 
> All other xstats are incremented percpu, I simply followed the pattern.
> 

We have already a lock, we can use it and avoid the whole per-cpu memory handling.
It seems to be acquired in all cases where these counters need to be changed.

>>>  	struct net_bridge_port *p
>>>  		= container_of(kobj, struct net_bridge_port, kobj);
>>> +	free_percpu(p->stp_stats);
>>
>> Please leave a new line between local var declaration and the code. I know
>> it was missing, but you can add it now. :)
> 
> OK.
> 
>>> +	if (p) {
>>> +		struct bridge_stp_xstats xstats;
>>
>> Please rename the local var here, using just xstats is misleading.
>> Maybe stp_xstats ?
> 
> This isn't misleading to me since its scope is limited to the current block
> and not the entire function. The block above dumping the VLAN xstats is
> using a local "struct br_vlan_stats stats" variable for example.
> 

Yep, as I answered to myself earlier, with the below change this goes away.

>>
>>> +
>>> +		br_stp_get_xstats(p, &xstats);
>>> +
>>> +		if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), &xstats))
>>> +			goto nla_put_failure;
>>
>> Could you please follow how mcast xstats are dumped and do something similar ?
>> It'd be nice to have similar code to audit.
> 
> Sure. I would also love to have easily auditable code in net/bridge. For
> the bridge STP xstats I followed the VLAN xstats code above, which does:
> 
>     if (nla_put(skb, BRIDGE_XSTATS_VLAN, sizeof(vxi), &vxi))
>         goto nla_put_failure;
> 

Yeah, we can move that to a vlan-specific helper too, but that is an unrelated change.

> But I can change the STP xstats code to the following:
> 
>     if (p) {
>         nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
>                                 sizeof(struct bridge_stp_xstats),
>                                 BRIDGE_XSTATS_PAD);
>         if (!nla)
>             goto nla_put_failure;
> 
>         br_stp_get_xstats(p, nla_data(nla));
>     }
> 
> Would that be preferred?
> 
> 

Perfect, thanks!

> Thanks,
> 
> 	Vivien
> 

Re: [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

Hi Nikolay,

On Tue, 10 Dec 2019 21:50:10 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >> Why do you need percpu ? All of these seem to be incremented with the
> >> bridge lock held. A few more comments below.
> > 
> > All other xstats are incremented percpu, I simply followed the pattern.
> > 
> 
> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
> It seems to be acquired in all cases where these counters need to be changed.

Since the other xstats counters are currently implemented this way, I prefer
to keep the code as is, until we eventually change them all if percpu is in
fact not needed anymore.

The new series is ready and I can submit it now if there's no objection.


Thanks,

	Vivien

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

Hi Nikolay,

On Tue, 10 Dec 2019 21:50:10 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >> Why do you need percpu ? All of these seem to be incremented with the
> >> bridge lock held. A few more comments below.
> > 
> > All other xstats are incremented percpu, I simply followed the pattern.
> > 
> 
> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
> It seems to be acquired in all cases where these counters need to be changed.

Since the other xstats counters are currently implemented this way, I prefer
to keep the code as is, until we eventually change them all if percpu is in
fact not needed anymore.

The new series is ready and I can submit it now if there's no objection.


Thanks,

	Vivien

Re: [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 22:10, Vivien Didelot wrote:
> Hi Nikolay,
> 
> On Tue, 10 Dec 2019 21:50:10 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>>> Why do you need percpu ? All of these seem to be incremented with the
>>>> bridge lock held. A few more comments below.
>>>
>>> All other xstats are incremented percpu, I simply followed the pattern.
>>>
>>
>> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
>> It seems to be acquired in all cases where these counters need to be changed.
> 
> Since the other xstats counters are currently implemented this way, I prefer
> to keep the code as is, until we eventually change them all if percpu is in
> fact not needed anymore.
> 
> The new series is ready and I can submit it now if there's no objection.
> 
> 
> Thanks,
> 
> 	Vivien
> 

There is a reason other counters use per-cpu - they're incremented without any locking from fast-path.
The bridge STP code already has a lock which is acquired in all of these paths and we don't need
this overhead and the per-cpu memory allocations. Unless you can find a STP codepath which actually
needs per-cpu, I'd prefer you drop it.

Thank you,
 Nik

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 22:10, Vivien Didelot wrote:
> Hi Nikolay,
> 
> On Tue, 10 Dec 2019 21:50:10 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>>> Why do you need percpu ? All of these seem to be incremented with the
>>>> bridge lock held. A few more comments below.
>>>
>>> All other xstats are incremented percpu, I simply followed the pattern.
>>>
>>
>> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
>> It seems to be acquired in all cases where these counters need to be changed.
> 
> Since the other xstats counters are currently implemented this way, I prefer
> to keep the code as is, until we eventually change them all if percpu is in
> fact not needed anymore.
> 
> The new series is ready and I can submit it now if there's no objection.
> 
> 
> Thanks,
> 
> 	Vivien
> 

There is a reason other counters use per-cpu - they're incremented without any locking from fast-path.
The bridge STP code already has a lock which is acquired in all of these paths and we don't need
this overhead and the per-cpu memory allocations. Unless you can find a STP codepath which actually
needs per-cpu, I'd prefer you drop it.

Thank you,
 Nik

Re: [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

On Tue, 10 Dec 2019 22:15:26 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >>>> Why do you need percpu ? All of these seem to be incremented with the
> >>>> bridge lock held. A few more comments below.
> >>>
> >>> All other xstats are incremented percpu, I simply followed the pattern.
> >>>
> >>
> >> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
> >> It seems to be acquired in all cases where these counters need to be changed.
> > 
> > Since the other xstats counters are currently implemented this way, I prefer
> > to keep the code as is, until we eventually change them all if percpu is in
> > fact not needed anymore.
> > 
> > The new series is ready and I can submit it now if there's no objection.
> 
> There is a reason other counters use per-cpu - they're incremented without any locking from fast-path.
> The bridge STP code already has a lock which is acquired in all of these paths and we don't need
> this overhead and the per-cpu memory allocations. Unless you can find a STP codepath which actually
> needs per-cpu, I'd prefer you drop it.

Ho ok I understand what you mean now. I'll drop the percpu attribute.


Thanks,

	Vivien

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

On Tue, 10 Dec 2019 22:15:26 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >>>> Why do you need percpu ? All of these seem to be incremented with the
> >>>> bridge lock held. A few more comments below.
> >>>
> >>> All other xstats are incremented percpu, I simply followed the pattern.
> >>>
> >>
> >> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
> >> It seems to be acquired in all cases where these counters need to be changed.
> > 
> > Since the other xstats counters are currently implemented this way, I prefer
> > to keep the code as is, until we eventually change them all if percpu is in
> > fact not needed anymore.
> > 
> > The new series is ready and I can submit it now if there's no objection.
> 
> There is a reason other counters use per-cpu - they're incremented without any locking from fast-path.
> The bridge STP code already has a lock which is acquired in all of these paths and we don't need
> this overhead and the per-cpu memory allocations. Unless you can find a STP codepath which actually
> needs per-cpu, I'd prefer you drop it.

Ho ok I understand what you mean now. I'll drop the percpu attribute.


Thanks,

	Vivien

Re: [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 22:34, Vivien Didelot wrote:
> On Tue, 10 Dec 2019 22:15:26 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>>>>> Why do you need percpu ? All of these seem to be incremented with the
>>>>>> bridge lock held. A few more comments below.
>>>>>
>>>>> All other xstats are incremented percpu, I simply followed the pattern.
>>>>>
>>>>
>>>> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
>>>> It seems to be acquired in all cases where these counters need to be changed.
>>>
>>> Since the other xstats counters are currently implemented this way, I prefer
>>> to keep the code as is, until we eventually change them all if percpu is in
>>> fact not needed anymore.
>>>
>>> The new series is ready and I can submit it now if there's no objection.
>>
>> There is a reason other counters use per-cpu - they're incremented without any locking from fast-path.
>> The bridge STP code already has a lock which is acquired in all of these paths and we don't need
>> this overhead and the per-cpu memory allocations. Unless you can find a STP codepath which actually
>> needs per-cpu, I'd prefer you drop it.
> 
> Ho ok I understand what you mean now. I'll drop the percpu attribute.
> 
> 
> Thanks,
> 
> 	Vivien
> 

Great, thanks again.
I think it's clear, but I'll add just in case to avoid extra work - you can drop
the dynamic memory allocation altogether and make the struct part of net_bridge_port.

Cheers,
 Nik

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Nikolay Aleksandrov]

On 10/12/2019 22:34, Vivien Didelot wrote:
> On Tue, 10 Dec 2019 22:15:26 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>>>>> Why do you need percpu ? All of these seem to be incremented with the
>>>>>> bridge lock held. A few more comments below.
>>>>>
>>>>> All other xstats are incremented percpu, I simply followed the pattern.
>>>>>
>>>>
>>>> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
>>>> It seems to be acquired in all cases where these counters need to be changed.
>>>
>>> Since the other xstats counters are currently implemented this way, I prefer
>>> to keep the code as is, until we eventually change them all if percpu is in
>>> fact not needed anymore.
>>>
>>> The new series is ready and I can submit it now if there's no objection.
>>
>> There is a reason other counters use per-cpu - they're incremented without any locking from fast-path.
>> The bridge STP code already has a lock which is acquired in all of these paths and we don't need
>> this overhead and the per-cpu memory allocations. Unless you can find a STP codepath which actually
>> needs per-cpu, I'd prefer you drop it.
> 
> Ho ok I understand what you mean now. I'll drop the percpu attribute.
> 
> 
> Thanks,
> 
> 	Vivien
> 

Great, thanks again.
I think it's clear, but I'll add just in case to avoid extra work - you can drop
the dynamic memory allocation altogether and make the struct part of net_bridge_port.

Cheers,
 Nik

Re: [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

On Tue, 10 Dec 2019 22:52:59 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >>>>>> Why do you need percpu ? All of these seem to be incremented with the
> >>>>>> bridge lock held. A few more comments below.
> >>>>>
> >>>>> All other xstats are incremented percpu, I simply followed the pattern.
> >>>>>
> >>>>
> >>>> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
> >>>> It seems to be acquired in all cases where these counters need to be changed.
> >>>
> >>> Since the other xstats counters are currently implemented this way, I prefer
> >>> to keep the code as is, until we eventually change them all if percpu is in
> >>> fact not needed anymore.
> >>>
> >>> The new series is ready and I can submit it now if there's no objection.
> >>
> >> There is a reason other counters use per-cpu - they're incremented without any locking from fast-path.
> >> The bridge STP code already has a lock which is acquired in all of these paths and we don't need
> >> this overhead and the per-cpu memory allocations. Unless you can find a STP codepath which actually
> >> needs per-cpu, I'd prefer you drop it.
> > 
> > Ho ok I understand what you mean now. I'll drop the percpu attribute.
> 
> Great, thanks again.
> I think it's clear, but I'll add just in case to avoid extra work - you can drop
> the dynamic memory allocation altogether and make the struct part of net_bridge_port.

Yup, that's what I've done and it makes the patch shamely small now ;)


Thanks,

	Vivien

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

On Tue, 10 Dec 2019 22:52:59 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >>>>>> Why do you need percpu ? All of these seem to be incremented with the
> >>>>>> bridge lock held. A few more comments below.
> >>>>>
> >>>>> All other xstats are incremented percpu, I simply followed the pattern.
> >>>>>
> >>>>
> >>>> We have already a lock, we can use it and avoid the whole per-cpu memory handling.
> >>>> It seems to be acquired in all cases where these counters need to be changed.
> >>>
> >>> Since the other xstats counters are currently implemented this way, I prefer
> >>> to keep the code as is, until we eventually change them all if percpu is in
> >>> fact not needed anymore.
> >>>
> >>> The new series is ready and I can submit it now if there's no objection.
> >>
> >> There is a reason other counters use per-cpu - they're incremented without any locking from fast-path.
> >> The bridge STP code already has a lock which is acquired in all of these paths and we don't need
> >> this overhead and the per-cpu memory allocations. Unless you can find a STP codepath which actually
> >> needs per-cpu, I'd prefer you drop it.
> > 
> > Ho ok I understand what you mean now. I'll drop the percpu attribute.
> 
> Great, thanks again.
> I think it's clear, but I'll add just in case to avoid extra work - you can drop
> the dynamic memory allocation altogether and make the struct part of net_bridge_port.

Yup, that's what I've done and it makes the patch shamely small now ;)


Thanks,

	Vivien

Re: [PATCH iproute2 v2] iplink: add support for STP xstats

[Stephen Hemminger]

On Tue, 10 Dec 2019 13:16:33 -0500
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> Hi Stephen,
> 
> On Mon, 9 Dec 2019 16:13:45 -0800, Stephen Hemminger <stephen@networkplumber.org> wrote:
> > On Mon,  9 Dec 2019 18:05:22 -0500
> > Vivien Didelot <vivien.didelot@gmail.com> wrote:
> >   
> > > Add support for the BRIDGE_XSTATS_STP xstats, as follow:
> > > 
> > >     # ip link xstats type bridge_slave dev lan5
> > >                         STP BPDU:
> > >                           RX: 0
> > >                           TX: 39
> > >                         STP TCN:
> > >                           RX: 0
> > >                           TX: 0
> > >                         STP Transitions:
> > >                           Blocked: 0
> > >                           Forwarding: 1
> > >                         IGMP queries:
> > >                           RX: v1 0 v2 0 v3 0
> > >                           TX: v1 0 v2 0 v3 0
> > >     ...  
> > 
> > Might I suggest a more concise format:
> > 	STP BPDU:  RX: 0 TX: 39
> > 	STP TCN:   RX: 0 TX:0
> > 	STP Transitions: Blocked: 0 Forwarding: 1
> > ...  
> 
> I don't mind if you prefer this format ;-)

Just trying to reduce the long output. Which is already too long for the xstat as it is.

Re: [Bridge] [PATCH iproute2 v2] iplink: add support for STP xstats

[Stephen Hemminger]

On Tue, 10 Dec 2019 13:16:33 -0500
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> Hi Stephen,
> 
> On Mon, 9 Dec 2019 16:13:45 -0800, Stephen Hemminger <stephen@networkplumber.org> wrote:
> > On Mon,  9 Dec 2019 18:05:22 -0500
> > Vivien Didelot <vivien.didelot@gmail.com> wrote:
> >   
> > > Add support for the BRIDGE_XSTATS_STP xstats, as follow:
> > > 
> > >     # ip link xstats type bridge_slave dev lan5
> > >                         STP BPDU:
> > >                           RX: 0
> > >                           TX: 39
> > >                         STP TCN:
> > >                           RX: 0
> > >                           TX: 0
> > >                         STP Transitions:
> > >                           Blocked: 0
> > >                           Forwarding: 1
> > >                         IGMP queries:
> > >                           RX: v1 0 v2 0 v3 0
> > >                           TX: v1 0 v2 0 v3 0
> > >     ...  
> > 
> > Might I suggest a more concise format:
> > 	STP BPDU:  RX: 0 TX: 39
> > 	STP TCN:   RX: 0 TX:0
> > 	STP Transitions: Blocked: 0 Forwarding: 1
> > ...  
> 
> I don't mind if you prefer this format ;-)

Just trying to reduce the long output. Which is already too long for the xstat as it is.