All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-10 21:20 ` Vivien Didelot
  0 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-10 21:20 UTC (permalink / raw)
  To: David S. Miller
  Cc: Roopa Prabhu, Nikolay Aleksandrov, netdev, bridge,
	Stephen Hemminger, 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_netlink.c        | 10 ++++++++++
 net/bridge/br_private.h        |  2 ++
 net/bridge/br_stp.c            | 15 +++++++++++++++
 net/bridge/br_stp_bpdu.c       |  4 ++++
 5 files changed, 41 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_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..d339cc314357 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1597,6 +1597,16 @@ static int br_fill_linkxstats(struct sk_buff *skb,
 		}
 	}
 
+	if (p) {
+		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
+					sizeof(p->stp_xstats),
+					BRIDGE_XSTATS_PAD);
+		if (!nla)
+			goto nla_put_failure;
+
+		memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
+	}
+
 #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..f540f3bdf294 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -283,6 +283,8 @@ struct net_bridge_port {
 #endif
 	u16				group_fwd_mask;
 	u16				backup_redirected_cnt;
+
+	struct bridge_stp_xstats	stp_xstats;
 };
 
 #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..6856a6d9282b 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,17 @@ 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) {
+		switch (p->state) {
+		case BR_STATE_BLOCKING:
+			p->stp_xstats.transition_blk++;
+			break;
+		case BR_STATE_FORWARDING:
+			p->stp_xstats.transition_fwd++;
+			break;
+		}
+	}
 }
 
 /* called under bridge lock */
@@ -484,6 +495,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
 	struct net_bridge *br;
 	int was_root;
 
+	p->stp_xstats.rx_bpdu++;
+
 	br = p->br;
 	was_root = br_is_root_bridge(br);
 
@@ -517,6 +530,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
 /* called under bridge lock */
 void br_received_tcn_bpdu(struct net_bridge_port *p)
 {
+	p->stp_xstats.rx_tcn++;
+
 	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..0e4572f31330 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -118,6 +118,8 @@ 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);
+
+	p->stp_xstats.tx_bpdu++;
 }
 
 /* called under bridge lock */
@@ -133,6 +135,8 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
 	buf[2] = 0;
 	buf[3] = BPDU_TYPE_TCN;
 	br_send_bpdu(p, buf, 4);
+
+	p->stp_xstats.tx_tcn++;
 }
 
 /*
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-10 21:20 ` Vivien Didelot
  0 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-10 21:20 UTC (permalink / raw)
  To: David S. Miller
  Cc: Nikolay Aleksandrov, netdev, Roopa Prabhu, bridge, 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_netlink.c        | 10 ++++++++++
 net/bridge/br_private.h        |  2 ++
 net/bridge/br_stp.c            | 15 +++++++++++++++
 net/bridge/br_stp_bpdu.c       |  4 ++++
 5 files changed, 41 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_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..d339cc314357 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1597,6 +1597,16 @@ static int br_fill_linkxstats(struct sk_buff *skb,
 		}
 	}
 
+	if (p) {
+		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
+					sizeof(p->stp_xstats),
+					BRIDGE_XSTATS_PAD);
+		if (!nla)
+			goto nla_put_failure;
+
+		memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
+	}
+
 #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..f540f3bdf294 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -283,6 +283,8 @@ struct net_bridge_port {
 #endif
 	u16				group_fwd_mask;
 	u16				backup_redirected_cnt;
+
+	struct bridge_stp_xstats	stp_xstats;
 };
 
 #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..6856a6d9282b 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,17 @@ 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) {
+		switch (p->state) {
+		case BR_STATE_BLOCKING:
+			p->stp_xstats.transition_blk++;
+			break;
+		case BR_STATE_FORWARDING:
+			p->stp_xstats.transition_fwd++;
+			break;
+		}
+	}
 }
 
 /* called under bridge lock */
@@ -484,6 +495,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
 	struct net_bridge *br;
 	int was_root;
 
+	p->stp_xstats.rx_bpdu++;
+
 	br = p->br;
 	was_root = br_is_root_bridge(br);
 
@@ -517,6 +530,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
 /* called under bridge lock */
 void br_received_tcn_bpdu(struct net_bridge_port *p)
 {
+	p->stp_xstats.rx_tcn++;
+
 	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..0e4572f31330 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -118,6 +118,8 @@ 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);
+
+	p->stp_xstats.tx_bpdu++;
 }
 
 /* called under bridge lock */
@@ -133,6 +135,8 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
 	buf[2] = 0;
 	buf[3] = BPDU_TYPE_TCN;
 	br_send_bpdu(p, buf, 4);
+
+	p->stp_xstats.tx_tcn++;
 }
 
 /*
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH iproute2 v3] iplink: add support for STP xstats
  2019-12-10 21:20 ` [Bridge] " Vivien Didelot
@ 2019-12-10 21:20   ` Vivien Didelot
  -1 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-10 21:20 UTC (permalink / raw)
  To: David S. Miller
  Cc: Roopa Prabhu, Nikolay Aleksandrov, netdev, bridge,
	Stephen Hemminger, Vivien Didelot

Add support for the BRIDGE_XSTATS_STP xstats, as follow:

    # ip link xstats type bridge_slave dev lan4 stp
    lan4
                        STP BPDU:  RX: 0 TX: 61
                        STP TCN:   RX: 0 TX: 0
                        STP Transitions: Blocked: 2 Forwarding: 1

Or below as JSON:

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

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 include/uapi/linux/if_bridge.h | 10 ++++++++++
 ip/iplink_bridge.c             | 26 ++++++++++++++++++++++++++
 2 files changed, 36 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..bbd6f3a8 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,29 @@ 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:  ", "");
+			print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu ",
+				  sstats->rx_bpdu);
+			print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+				  sstats->tx_bpdu);
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP TCN:   ", "");
+			print_u64(PRINT_ANY, "rx_tcn", "RX: %llu ",
+				  sstats->rx_tcn);
+			print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+				  sstats->tx_tcn);
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP Transitions: ", "");
+			print_u64(PRINT_ANY, "transition_blk", "Blocked: %llu ",
+				  sstats->transition_blk);
+			print_u64(PRINT_ANY, "transition_fwd", "Forwarding: %llu\n",
+				  sstats->transition_fwd);
+			close_json_object();
+			break;
 		}
 	}
 	close_json_object();
@@ -843,6 +867,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


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Bridge] [PATCH iproute2 v3] iplink: add support for STP xstats
@ 2019-12-10 21:20   ` Vivien Didelot
  0 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-10 21:20 UTC (permalink / raw)
  To: David S. Miller
  Cc: Nikolay Aleksandrov, netdev, Roopa Prabhu, bridge, Vivien Didelot

Add support for the BRIDGE_XSTATS_STP xstats, as follow:

    # ip link xstats type bridge_slave dev lan4 stp
    lan4
                        STP BPDU:  RX: 0 TX: 61
                        STP TCN:   RX: 0 TX: 0
                        STP Transitions: Blocked: 2 Forwarding: 1

Or below as JSON:

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

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 include/uapi/linux/if_bridge.h | 10 ++++++++++
 ip/iplink_bridge.c             | 26 ++++++++++++++++++++++++++
 2 files changed, 36 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..bbd6f3a8 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,29 @@ 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:  ", "");
+			print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu ",
+				  sstats->rx_bpdu);
+			print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+				  sstats->tx_bpdu);
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP TCN:   ", "");
+			print_u64(PRINT_ANY, "rx_tcn", "RX: %llu ",
+				  sstats->rx_tcn);
+			print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+				  sstats->tx_tcn);
+			print_string(PRINT_FP, NULL,
+				     "%-16s    STP Transitions: ", "");
+			print_u64(PRINT_ANY, "transition_blk", "Blocked: %llu ",
+				  sstats->transition_blk);
+			print_u64(PRINT_ANY, "transition_fwd", "Forwarding: %llu\n",
+				  sstats->transition_fwd);
+			close_json_object();
+			break;
 		}
 	}
 	close_json_object();
@@ -843,6 +867,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


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-10 21:20 ` [Bridge] " Vivien Didelot
@ 2019-12-10 21:45   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 24+ messages in thread
From: Nikolay Aleksandrov @ 2019-12-10 21:45 UTC (permalink / raw)
  To: Vivien Didelot, David S. Miller
  Cc: Roopa Prabhu, netdev, bridge, Stephen Hemminger

On 10/12/2019 23:20, 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_netlink.c        | 10 ++++++++++
>  net/bridge/br_private.h        |  2 ++
>  net/bridge/br_stp.c            | 15 +++++++++++++++
>  net/bridge/br_stp_bpdu.c       |  4 ++++
>  5 files changed, 41 insertions(+)
> 

Hi,
I like it! Unfortunately there is one issue still, more 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_netlink.c b/net/bridge/br_netlink.c
> index a0a54482aabc..d339cc314357 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1597,6 +1597,16 @@ static int br_fill_linkxstats(struct sk_buff *skb,
>  		}
>  	}
>  
> +	if (p) {
> +		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
> +					sizeof(p->stp_xstats),
> +					BRIDGE_XSTATS_PAD);
> +		if (!nla)
> +			goto nla_put_failure;
> +
> +		memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));

You need to take the STP lock here to get a proper snapshot of the values.

> +	}
> +
>  #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..f540f3bdf294 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -283,6 +283,8 @@ struct net_bridge_port {
>  #endif
>  	u16				group_fwd_mask;
>  	u16				backup_redirected_cnt;
> +
> +	struct bridge_stp_xstats	stp_xstats;
>  };
>  
>  #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 1f1410f8d312..6856a6d9282b 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -45,6 +45,17 @@ 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) {
> +		switch (p->state) {
> +		case BR_STATE_BLOCKING:
> +			p->stp_xstats.transition_blk++;
> +			break;
> +		case BR_STATE_FORWARDING:
> +			p->stp_xstats.transition_fwd++;
> +			break;
> +		}
> +	}
>  }
>  
>  /* called under bridge lock */
> @@ -484,6 +495,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
>  	struct net_bridge *br;
>  	int was_root;
>  
> +	p->stp_xstats.rx_bpdu++;
> +
>  	br = p->br;
>  	was_root = br_is_root_bridge(br);
>  
> @@ -517,6 +530,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
>  /* called under bridge lock */
>  void br_received_tcn_bpdu(struct net_bridge_port *p)
>  {
> +	p->stp_xstats.rx_tcn++;
> +
>  	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..0e4572f31330 100644
> --- a/net/bridge/br_stp_bpdu.c
> +++ b/net/bridge/br_stp_bpdu.c
> @@ -118,6 +118,8 @@ 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);
> +
> +	p->stp_xstats.tx_bpdu++;
>  }
>  
>  /* called under bridge lock */
> @@ -133,6 +135,8 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
>  	buf[2] = 0;
>  	buf[3] = BPDU_TYPE_TCN;
>  	br_send_bpdu(p, buf, 4);
> +
> +	p->stp_xstats.tx_tcn++;
>  }
>  
>  /*
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-10 21:45   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 24+ messages in thread
From: Nikolay Aleksandrov @ 2019-12-10 21:45 UTC (permalink / raw)
  To: Vivien Didelot, David S. Miller; +Cc: netdev, Roopa Prabhu, bridge

On 10/12/2019 23:20, 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_netlink.c        | 10 ++++++++++
>  net/bridge/br_private.h        |  2 ++
>  net/bridge/br_stp.c            | 15 +++++++++++++++
>  net/bridge/br_stp_bpdu.c       |  4 ++++
>  5 files changed, 41 insertions(+)
> 

Hi,
I like it! Unfortunately there is one issue still, more 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_netlink.c b/net/bridge/br_netlink.c
> index a0a54482aabc..d339cc314357 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1597,6 +1597,16 @@ static int br_fill_linkxstats(struct sk_buff *skb,
>  		}
>  	}
>  
> +	if (p) {
> +		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
> +					sizeof(p->stp_xstats),
> +					BRIDGE_XSTATS_PAD);
> +		if (!nla)
> +			goto nla_put_failure;
> +
> +		memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));

You need to take the STP lock here to get a proper snapshot of the values.

> +	}
> +
>  #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..f540f3bdf294 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -283,6 +283,8 @@ struct net_bridge_port {
>  #endif
>  	u16				group_fwd_mask;
>  	u16				backup_redirected_cnt;
> +
> +	struct bridge_stp_xstats	stp_xstats;
>  };
>  
>  #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 1f1410f8d312..6856a6d9282b 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -45,6 +45,17 @@ 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) {
> +		switch (p->state) {
> +		case BR_STATE_BLOCKING:
> +			p->stp_xstats.transition_blk++;
> +			break;
> +		case BR_STATE_FORWARDING:
> +			p->stp_xstats.transition_fwd++;
> +			break;
> +		}
> +	}
>  }
>  
>  /* called under bridge lock */
> @@ -484,6 +495,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
>  	struct net_bridge *br;
>  	int was_root;
>  
> +	p->stp_xstats.rx_bpdu++;
> +
>  	br = p->br;
>  	was_root = br_is_root_bridge(br);
>  
> @@ -517,6 +530,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
>  /* called under bridge lock */
>  void br_received_tcn_bpdu(struct net_bridge_port *p)
>  {
> +	p->stp_xstats.rx_tcn++;
> +
>  	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..0e4572f31330 100644
> --- a/net/bridge/br_stp_bpdu.c
> +++ b/net/bridge/br_stp_bpdu.c
> @@ -118,6 +118,8 @@ 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);
> +
> +	p->stp_xstats.tx_bpdu++;
>  }
>  
>  /* called under bridge lock */
> @@ -133,6 +135,8 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
>  	buf[2] = 0;
>  	buf[3] = BPDU_TYPE_TCN;
>  	br_send_bpdu(p, buf, 4);
> +
> +	p->stp_xstats.tx_tcn++;
>  }
>  
>  /*
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-10 21:45   ` [Bridge] " Nikolay Aleksandrov
@ 2019-12-11  2:02     ` Vivien Didelot
  -1 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-11  2:02 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: David S. Miller, Roopa Prabhu, netdev, bridge, Stephen Hemminger

Hi Nikolay,

On Tue, 10 Dec 2019 23:45:13 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> > +	if (p) {
> > +		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
> > +					sizeof(p->stp_xstats),
> > +					BRIDGE_XSTATS_PAD);
> > +		if (!nla)
> > +			goto nla_put_failure;
> > +
> > +		memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
> 
> You need to take the STP lock here to get a proper snapshot of the values.

Good catch! I see a br->multicast_lock but no br->stp_lock. Is this what
you expect?

    spin_lock_bh(&br->lock);
    memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
    spin_unlock_bh(&br->lock);


Thanks,

	Vivien

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-11  2:02     ` Vivien Didelot
  0 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-11  2:02 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, Roopa Prabhu, bridge, David S. Miller

Hi Nikolay,

On Tue, 10 Dec 2019 23:45:13 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> > +	if (p) {
> > +		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
> > +					sizeof(p->stp_xstats),
> > +					BRIDGE_XSTATS_PAD);
> > +		if (!nla)
> > +			goto nla_put_failure;
> > +
> > +		memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
> 
> You need to take the STP lock here to get a proper snapshot of the values.

Good catch! I see a br->multicast_lock but no br->stp_lock. Is this what
you expect?

    spin_lock_bh(&br->lock);
    memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
    spin_unlock_bh(&br->lock);


Thanks,

	Vivien

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-11  2:02     ` [Bridge] " Vivien Didelot
@ 2019-12-11  9:40       ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 24+ messages in thread
From: Nikolay Aleksandrov @ 2019-12-11  9:40 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: David S. Miller, Roopa Prabhu, netdev, bridge, Stephen Hemminger

On 11/12/2019 04:02, Vivien Didelot wrote:
> Hi Nikolay,
> 
> On Tue, 10 Dec 2019 23:45:13 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>> +	if (p) {
>>> +		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
>>> +					sizeof(p->stp_xstats),
>>> +					BRIDGE_XSTATS_PAD);
>>> +		if (!nla)
>>> +			goto nla_put_failure;
>>> +
>>> +		memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
>>
>> You need to take the STP lock here to get a proper snapshot of the values.
> 
> Good catch! I see a br->multicast_lock but no br->stp_lock. Is this what
> you expect?
> 
>     spin_lock_bh(&br->lock);
>     memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
>     spin_unlock_bh(&br->lock);
> 

Yeah, this is a very old lock (pre-git) which needs some attention. :)
That is the lock and the above looks good to me.

> 
> Thanks,
> 
> 	Vivien
> 

Cheers,
 Nik

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-11  9:40       ` Nikolay Aleksandrov
  0 siblings, 0 replies; 24+ messages in thread
From: Nikolay Aleksandrov @ 2019-12-11  9:40 UTC (permalink / raw)
  To: Vivien Didelot; +Cc: netdev, Roopa Prabhu, bridge, David S. Miller

On 11/12/2019 04:02, Vivien Didelot wrote:
> Hi Nikolay,
> 
> On Tue, 10 Dec 2019 23:45:13 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>> +	if (p) {
>>> +		nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
>>> +					sizeof(p->stp_xstats),
>>> +					BRIDGE_XSTATS_PAD);
>>> +		if (!nla)
>>> +			goto nla_put_failure;
>>> +
>>> +		memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
>>
>> You need to take the STP lock here to get a proper snapshot of the values.
> 
> Good catch! I see a br->multicast_lock but no br->stp_lock. Is this what
> you expect?
> 
>     spin_lock_bh(&br->lock);
>     memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
>     spin_unlock_bh(&br->lock);
> 

Yeah, this is a very old lock (pre-git) which needs some attention. :)
That is the lock and the above looks good to me.

> 
> Thanks,
> 
> 	Vivien
> 

Cheers,
 Nik

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-10 21:20 ` [Bridge] " Vivien Didelot
@ 2019-12-11 15:38   ` David Ahern
  -1 siblings, 0 replies; 24+ messages in thread
From: David Ahern @ 2019-12-11 15:38 UTC (permalink / raw)
  To: Vivien Didelot, David S. Miller
  Cc: Roopa Prabhu, Nikolay Aleksandrov, netdev, bridge, Stephen Hemminger

On 12/10/19 2:20 PM, Vivien Didelot wrote:
> 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
>  };

Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-11 15:38   ` David Ahern
  0 siblings, 0 replies; 24+ messages in thread
From: David Ahern @ 2019-12-11 15:38 UTC (permalink / raw)
  To: Vivien Didelot, David S. Miller
  Cc: Nikolay Aleksandrov, netdev, Roopa Prabhu, bridge

On 12/10/19 2:20 PM, Vivien Didelot wrote:
> 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
>  };

Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-11 15:38   ` [Bridge] " David Ahern
@ 2019-12-11 15:42     ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 24+ messages in thread
From: Nikolay Aleksandrov @ 2019-12-11 15:42 UTC (permalink / raw)
  To: David Ahern, Vivien Didelot, David S. Miller
  Cc: Roopa Prabhu, netdev, bridge, Stephen Hemminger

On 11/12/2019 17:38, David Ahern wrote:
> On 12/10/19 2:20 PM, Vivien Didelot wrote:
>> 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
>>  };
> 
> Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> 

Oh yes, good catch. That has to be fixed, too.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-11 15:42     ` Nikolay Aleksandrov
  0 siblings, 0 replies; 24+ messages in thread
From: Nikolay Aleksandrov @ 2019-12-11 15:42 UTC (permalink / raw)
  To: David Ahern, Vivien Didelot, David S. Miller; +Cc: netdev, Roopa Prabhu, bridge

On 11/12/2019 17:38, David Ahern wrote:
> On 12/10/19 2:20 PM, Vivien Didelot wrote:
>> 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
>>  };
> 
> Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> 

Oh yes, good catch. That has to be fixed, too.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-11 15:42     ` [Bridge] " Nikolay Aleksandrov
@ 2019-12-11 18:41       ` Vivien Didelot
  -1 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-11 18:41 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: David Ahern, David S. Miller, Roopa Prabhu, netdev, bridge,
	Stephen Hemminger

Hi David, Nikolay,

On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >>  /* 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
> >>  };
> > 
> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> > 
> 
> Oh yes, good catch. That has to be fixed, too.
> 

This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
and __BRIDGE_XSTATS_MAX?


Thanks,

	Vivien

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-11 18:41       ` Vivien Didelot
  0 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-11 18:41 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: netdev, Roopa Prabhu, bridge, David Ahern, David S. Miller

Hi David, Nikolay,

On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >>  /* 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
> >>  };
> > 
> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> > 
> 
> Oh yes, good catch. That has to be fixed, too.
> 

This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
and __BRIDGE_XSTATS_MAX?


Thanks,

	Vivien

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-11 18:41       ` [Bridge] " Vivien Didelot
@ 2019-12-11 20:01         ` David Miller
  -1 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2019-12-11 20:01 UTC (permalink / raw)
  To: vivien.didelot; +Cc: nikolay, dsahern, roopa, netdev, bridge, stephen

From: Vivien Didelot <vivien.didelot@gmail.com>
Date: Wed, 11 Dec 2019 13:41:33 -0500

> Hi David, Nikolay,
> 
> On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>> >>  /* 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
>> >>  };
>> > 
>> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
>> > 
>> 
>> Oh yes, good catch. That has to be fixed, too.
>> 
> 
> This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
> and __BRIDGE_XSTATS_MAX?

Because, just like any other attribute value, BRIDGE_XSTATS_PAD is an
API and fixed in stone.  You can't add things before it which change
it's value.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-11 20:01         ` David Miller
  0 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2019-12-11 20:01 UTC (permalink / raw)
  To: vivien.didelot; +Cc: nikolay, netdev, roopa, bridge, dsahern

From: Vivien Didelot <vivien.didelot@gmail.com>
Date: Wed, 11 Dec 2019 13:41:33 -0500

> Hi David, Nikolay,
> 
> On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>> >>  /* 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
>> >>  };
>> > 
>> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
>> > 
>> 
>> Oh yes, good catch. That has to be fixed, too.
>> 
> 
> This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
> and __BRIDGE_XSTATS_MAX?

Because, just like any other attribute value, BRIDGE_XSTATS_PAD is an
API and fixed in stone.  You can't add things before it which change
it's value.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-11 20:01         ` [Bridge] " David Miller
@ 2019-12-11 21:47           ` Vivien Didelot
  -1 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-11 21:47 UTC (permalink / raw)
  To: David Miller; +Cc: nikolay, dsahern, roopa, netdev, bridge, stephen

Hi David,

On Wed, 11 Dec 2019 12:01:20 -0800 (PST), David Miller <davem@davemloft.net> wrote:
> >> >>  /* 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
> >> >>  };
> >> > 
> >> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> >> > 
> >> 
> >> Oh yes, good catch. That has to be fixed, too.
> >> 
> > 
> > This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
> > and __BRIDGE_XSTATS_MAX?
> 
> Because, just like any other attribute value, BRIDGE_XSTATS_PAD is an
> API and fixed in stone.  You can't add things before it which change
> it's value.

I thought the whole point of using enums was to avoid caring about fixed
numeric values, but well. To be more precise, what I don't get is that when
I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
xstats don't show up anymore in iproute2.


Thanks,

	Vivien

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-11 21:47           ` Vivien Didelot
  0 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-11 21:47 UTC (permalink / raw)
  To: David Miller; +Cc: nikolay, netdev, roopa, bridge, dsahern

Hi David,

On Wed, 11 Dec 2019 12:01:20 -0800 (PST), David Miller <davem@davemloft.net> wrote:
> >> >>  /* 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
> >> >>  };
> >> > 
> >> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> >> > 
> >> 
> >> Oh yes, good catch. That has to be fixed, too.
> >> 
> > 
> > This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
> > and __BRIDGE_XSTATS_MAX?
> 
> Because, just like any other attribute value, BRIDGE_XSTATS_PAD is an
> API and fixed in stone.  You can't add things before it which change
> it's value.

I thought the whole point of using enums was to avoid caring about fixed
numeric values, but well. To be more precise, what I don't get is that when
I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
xstats don't show up anymore in iproute2.


Thanks,

	Vivien

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-11 21:47           ` [Bridge] " Vivien Didelot
@ 2019-12-11 22:16             ` David Miller
  -1 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2019-12-11 22:16 UTC (permalink / raw)
  To: vivien.didelot; +Cc: nikolay, dsahern, roopa, netdev, bridge, stephen

From: Vivien Didelot <vivien.didelot@gmail.com>
Date: Wed, 11 Dec 2019 16:47:54 -0500

> I thought the whole point of using enums was to avoid caring about fixed
> numeric values, but well.

I don't get where you got that idea from.

Each and every netlink attribute value is like IPPROTO_TCP in an ipv4
header, the exact values matter, and therefore you cannot make changes
that modify existing values.

Therefore, you must add new attributes to the end of the enumberation,
right before the __MAX value.

> To be more precise, what I don't get is that when
> I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
> xstats don't show up anymore in iproute2.

Because you ahve to recompile iproute2 so that it uses the corrected value
in the kernel header, did you do that?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-11 22:16             ` David Miller
  0 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2019-12-11 22:16 UTC (permalink / raw)
  To: vivien.didelot; +Cc: nikolay, netdev, roopa, bridge, dsahern

From: Vivien Didelot <vivien.didelot@gmail.com>
Date: Wed, 11 Dec 2019 16:47:54 -0500

> I thought the whole point of using enums was to avoid caring about fixed
> numeric values, but well.

I don't get where you got that idea from.

Each and every netlink attribute value is like IPPROTO_TCP in an ipv4
header, the exact values matter, and therefore you cannot make changes
that modify existing values.

Therefore, you must add new attributes to the end of the enumberation,
right before the __MAX value.

> To be more precise, what I don't get is that when
> I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
> xstats don't show up anymore in iproute2.

Because you ahve to recompile iproute2 so that it uses the corrected value
in the kernel header, did you do that?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next v2] net: bridge: add STP xstats
  2019-12-11 22:16             ` [Bridge] " David Miller
@ 2019-12-12  1:02               ` Vivien Didelot
  -1 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-12  1:02 UTC (permalink / raw)
  To: David Miller; +Cc: nikolay, dsahern, roopa, netdev, bridge, stephen

Hi David,

On Wed, 11 Dec 2019 14:16:58 -0800 (PST), David Miller <davem@davemloft.net> wrote:
> > To be more precise, what I don't get is that when
> > I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
> > xstats don't show up anymore in iproute2.
> 
> Because you ahve to recompile iproute2 so that it uses the corrected value
> in the kernel header, did you do that?

Meh you were correct, my rebuild didn't pick up the header change :-/

I also moved the STP xstats copy below the mcast xstats copy to be consistent
with the order. I'll respin right away.


Thanks,

	Vivien

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
@ 2019-12-12  1:02               ` Vivien Didelot
  0 siblings, 0 replies; 24+ messages in thread
From: Vivien Didelot @ 2019-12-12  1:02 UTC (permalink / raw)
  To: David Miller; +Cc: nikolay, netdev, roopa, bridge, dsahern

Hi David,

On Wed, 11 Dec 2019 14:16:58 -0800 (PST), David Miller <davem@davemloft.net> wrote:
> > To be more precise, what I don't get is that when
> > I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
> > xstats don't show up anymore in iproute2.
> 
> Because you ahve to recompile iproute2 so that it uses the corrected value
> in the kernel header, did you do that?

Meh you were correct, my rebuild didn't pick up the header change :-/

I also moved the STP xstats copy below the mcast xstats copy to be consistent
with the order. I'll respin right away.


Thanks,

	Vivien

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2019-12-12  1:02 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10 21:20 [PATCH net-next v2] net: bridge: add STP xstats Vivien Didelot
2019-12-10 21:20 ` [Bridge] " Vivien Didelot
2019-12-10 21:20 ` [PATCH iproute2 v3] iplink: add support for " Vivien Didelot
2019-12-10 21:20   ` [Bridge] " Vivien Didelot
2019-12-10 21:45 ` [PATCH net-next v2] net: bridge: add " Nikolay Aleksandrov
2019-12-10 21:45   ` [Bridge] " Nikolay Aleksandrov
2019-12-11  2:02   ` Vivien Didelot
2019-12-11  2:02     ` [Bridge] " Vivien Didelot
2019-12-11  9:40     ` Nikolay Aleksandrov
2019-12-11  9:40       ` [Bridge] " Nikolay Aleksandrov
2019-12-11 15:38 ` David Ahern
2019-12-11 15:38   ` [Bridge] " David Ahern
2019-12-11 15:42   ` Nikolay Aleksandrov
2019-12-11 15:42     ` [Bridge] " Nikolay Aleksandrov
2019-12-11 18:41     ` Vivien Didelot
2019-12-11 18:41       ` [Bridge] " Vivien Didelot
2019-12-11 20:01       ` David Miller
2019-12-11 20:01         ` [Bridge] " David Miller
2019-12-11 21:47         ` Vivien Didelot
2019-12-11 21:47           ` [Bridge] " Vivien Didelot
2019-12-11 22:16           ` David Miller
2019-12-11 22:16             ` [Bridge] " David Miller
2019-12-12  1:02             ` Vivien Didelot
2019-12-12  1:02               ` [Bridge] " Vivien Didelot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.